launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/registry/model/gpgkey.py'
> --- lib/lp/registry/model/gpgkey.py	2016-03-03 16:16:16 +0000
> +++ lib/lp/registry/model/gpgkey.py	2016-03-14 19:42:05 +0000
> @@ -66,28 +71,42 @@
>      def new(self, ownerID, keyid, fingerprint, keysize,
>              algorithm, active=True, can_encrypt=False):
>          """See `IGPGKeySet`"""
> -        return GPGKey(owner=ownerID, keyid=keyid,
> +        key = GPGKey(owner=ownerID, keyid=keyid,
>                        fingerprint=fingerprint, keysize=keysize,
>                        algorithm=algorithm, active=active,
>                        can_encrypt=can_encrypt)
> +        return key

This can be reverted now the service bits are split.

>  
>      def activate(self, requester, key, can_encrypt):
>          """See `IGPGKeySet`."""
>          fingerprint = key.fingerprint
>          lp_key = self.getByFingerprint(fingerprint)
>          if lp_key:
> +            is_new = False
>              # Then the key already exists, so let's reactivate it.
>              lp_key.active = True
>              lp_key.can_encrypt = can_encrypt
> -            return lp_key, False
> -        ownerID = requester.id
> -        keyid = key.keyid
> -        keysize = key.keysize
> -        algorithm = GPGKeyAlgorithm.items[key.algorithm]
> -        lp_key = self.new(
> -            ownerID, keyid, fingerprint, keysize, algorithm,
> -            can_encrypt=can_encrypt)
> -        return lp_key, True
> +        else:
> +            is_new = True
> +            ownerID = requester.id
> +            keyid = key.keyid
> +            keysize = key.keysize
> +            algorithm = GPGKeyAlgorithm.items[key.algorithm]
> +            lp_key = self.new(
> +                ownerID, keyid, fingerprint, keysize, algorithm,
> +                can_encrypt=can_encrypt)
> +        if getFeatureFlag(GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG):
> +            client = getUtility(IGPGClient)
> +            openid_identifier = self.getOwnerIdForPerson(lp_key.owner)
> +            client.addKeyForOwner(openid_identifier, key.fingerprint)
> +        return lp_key, is_new
> +
> +    def deactivate(self, key):
> +        key.active = False
> +        if getFeatureFlag(GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG):
> +            client = getUtility(IGPGClient)
> +            openid_identifier = self.getOwnerIdForPerson(key.owner)
> +            client.disableKeyForOwner(openid_identifier, key.fingerprint)
>  
>      def getByFingerprint(self, fingerprint, default=None):
>          """See `IGPGKeySet`"""
> @@ -118,3 +137,7 @@
>          query += ' AND owner=%s' % sqlvalues(owner.id)
>  
>          return list(GPGKey.select(query, orderBy='id'))
> +
> +    def getOwnerIdForPerson(self, owner):
> +        """See IGPGKeySet."""
> +        return IOpenIDPersistentIdentity(owner).openid_identity_url

I'd raise an AssertionError if the identity URL is None. We hope it never will on production, but we want it to blow up safely if it does.

> 
> === modified file 'lib/lp/services/gpg/handler.py'
> --- lib/lp/services/gpg/handler.py	2016-03-14 19:42:04 +0000
> +++ lib/lp/services/gpg/handler.py	2016-03-14 19:42:05 +0000
> @@ -674,7 +675,7 @@
>          resp = self._request('post', path, data)
>          if resp.status_code == http_codes['CREATED']:
>              self._notify_writes()
> -        else:
> +        elif resp.status_code != http_codes['OK']:

This might deserve a comment, specifically hinting that a 201 Created is returned not just in the case, but the reenable case too. 200 OK means it was already enabled.

>              self.raise_for_error(resp)
>  
>      def disableKeyForOwner(self, owner_id, fingerprint):
> 
> === modified file 'lib/lp/services/gpg/interfaces.py'
> --- lib/lp/services/gpg/interfaces.py	2016-03-14 19:42:04 +0000
> +++ lib/lp/services/gpg/interfaces.py	2016-03-14 19:42:05 +0000
> @@ -459,6 +461,12 @@
>          :raises socket.error" on socket-level errors (connection timeouts etc)
>          """
>  
> +    def getKeyByFingerprint(fingerprint):
> +        """Get a GPG key by it's fingerprint.

its

> +
> +        :raises ValueError: if the fingerprint isn't valid.
> +        """
> +
>      def registerWriteHook(hook_callable):
>          """Register a write hook.
>  


-- 
https://code.launchpad.net/~thomir/launchpad/devel-start-integration/+merge/286972
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.