launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/services/gpg/handler.py'
> --- lib/lp/services/gpg/handler.py	2016-05-19 04:37:33 +0000
> +++ lib/lp/services/gpg/handler.py	2016-06-16 05:38:44 +0000
> @@ -640,9 +641,28 @@
>  
>      def __init__(self):
>          super(LPGPGClient, self).__init__(bypass_proxy=True)
> +        self.action = None
>  
>      def get_endpoint(self):
>          return config.gpgservice.api_endpoint
>  
>      def get_timeout(self):
>          return 30.0
> +
> +    def on_request_start(self, method, path, data=None,
> +                         headers=dict()):
> +        assert self.action is None, "Error: Overlapping requests to gpgservice"
> +        timeline = get_request_timeline(
> +            get_current_browser_request())
> +        if data:
> +            data_summary = '%d byte body' % len(data)
> +        else:
> +            data_summary = 'no body'
> +        self.action = timeline.start(
> +            "gpgservice-%s" % method,
> +            ' '.join((path, data_summary, json.dumps(headers)))

The headers are liable to in the future include things like credentials that should not be logged to the timeline. Can we sensibly filter the headers to reasonable set?

> +        )
> +
> +    def on_request_end(self):
> +        self.action.finish()
> +        self.action = None
> 
> === modified file 'lib/lp/services/gpg/tests/test_gpghandler.py'
> --- lib/lp/services/gpg/tests/test_gpghandler.py	2016-03-23 17:55:39 +0000
> +++ lib/lp/services/gpg/tests/test_gpghandler.py	2016-06-16 05:38:44 +0000
> @@ -493,3 +496,67 @@
>              keyset.activate,
>              person2, key, key.can_encrypt
>          )
> +
> +    def assert_last_timeline_action(self, expected_method, expected_url):
> +        timeline = get_request_timeline(get_current_browser_request())
> +        action = timeline.actions[-1]
> +        self.assertEqual("gpgservice-%s" % expected_method, action.category)
> +        self.assertEqual(expected_url, action.detail.split(" ", 1)[0])
> +
> +    def test_get_keys_for_owner_has_timeline_support(self):
> +        client = getUtility(IGPGClient)
> +        user = self.get_random_owner_id_string()
> +        client.getKeysForOwner(user)
> +
> +        self.assert_last_timeline_action(
> +            'get', construct_url("/users/{owner_id}/keys", user))

Is the method as sent by the client lowercase? That seems weird.

> +
> +    def test_add_key_for_owner_has_timeline_support(self):
> +        self.useFixture(KeyServerTac())
> +        client = getUtility(IGPGClient)
> +        user = self.get_random_owner_id_string()
> +        fingerprint = 'A419AE861E88BC9E04B9C26FBA2B9389DFD20543'
> +        client.addKeyForOwner(user, fingerprint)
> +
> +        self.assert_last_timeline_action(
> +            'post', construct_url("/users/{owner_id}/keys", user))
> +
> +    def test_disable_key_for_owner_has_timeline_support(self):
> +        self.useFixture(KeyServerTac())
> +        client = getUtility(IGPGClient)
> +        user = self.get_random_owner_id_string()
> +        fingerprint = 'A419AE861E88BC9E04B9C26FBA2B9389DFD20543'
> +        client.addKeyForOwner(user, fingerprint)
> +        client.disableKeyForOwner(user, fingerprint)
> +
> +        self.assert_last_timeline_action(
> +            'delete',
> +            construct_url(
> +                "/users/{owner_id}/keys/{fingerprint}", user, fingerprint))
> +
> +    def test_get_key_by_fingerprint_has_timeline_support(self):
> +        client = getUtility(IGPGClient)
> +        fingerprint = 'A419AE861E88BC9E04B9C26FBA2B9389DFD20543'
> +        client.getKeyByFingerprint(fingerprint)
> +
> +        self.assert_last_timeline_action(
> +            'get',
> +            construct_url("/keys/{fingerprint}", fingerprint=fingerprint))
> +
> +    def test_get_keys_by_fingerprints_has_timeline_support(self):
> +        client = getUtility(IGPGClient)
> +        fingerprints = [
> +            'A419AE861E88BC9E04B9C26FBA2B9389DFD20543',
> +            'B439AF863EDEFC9E04FAB26FBA2B7289DF324545',
> +        ]
> +        client.getKeysByFingerprints(fingerprints)
> +
> +        self.assert_last_timeline_action(
> +            'get',
> +            construct_url(
> +                "/keys/{fingerprint}", fingerprint=','.join(fingerprints)))
> +
> +
> +def construct_url(template, owner_id='', fingerprint=''):
> +    owner_id = base64.b64encode(owner_id, altchars='-_')
> +    return template.format(owner_id=owner_id, fingerprint=fingerprint)


-- 
https://code.launchpad.net/~thomir/launchpad/devel-add-gpg-client-timeline-support/+merge/297589
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.