launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20126
Re: [Merge] lp:~thomir/launchpad/devel-add-read-ff into lp:launchpad
Thanks for the review.
All of your comments have been addressed, and I left some comments in the diff as well. Let me know if you have any concerns.
Thanks.
Diff comments:
>
Fixed.
> === modified file 'database/schema/security.cfg'
>
Fixed, I think. The public.sourcepackagerelease change is still required, I believe?
> === modified file 'lib/lp/registry/browser/tests/test_gpgkey.py'
>
Fixed, as per IRC.
> === modified file 'lib/lp/registry/configure.zcml'
> --- lib/lp/registry/configure.zcml 2016-02-28 19:13:17 +0000
> +++ lib/lp/registry/configure.zcml 2016-03-17 23:47:34 +0000
> @@ -1284,6 +1284,14 @@
> permission="launchpad.Edit"
> set_attributes="active can_encrypt"/>
> </class>
> + <class
> + class="lp.registry.model.gpgkey.GPGServiceKey">
> + <allow
> + interface="lp.registry.interfaces.gpg.IGPGKey"/>
> + <require
> + permission="launchpad.Edit"
> + set_attributes="active can_encrypt"/>
Good point - fixed.
> + </class>
>
> <!-- GPGKeySet -->
>
>
> === modified file 'lib/lp/registry/doc/gpgkey.txt'
> --- lib/lp/registry/doc/gpgkey.txt 2016-03-01 12:46:32 +0000
> +++ lib/lp/registry/doc/gpgkey.txt 2016-03-17 23:47:34 +0000
> @@ -25,9 +30,9 @@
> ... GPGKeyAlgorithm.G)
> <GPGKey...>
>
> -As is pulling it based on the key ID:
> +As is pulling it based on the key fingerprint:
>
> >>> key = gpgkeyset.getByFingerprint(
> - ... 'DEADBEEF12345678DEADBEEF12345678DEADBEEF')
> + ... 'ABCDEF0123456789ABCDDCBA0000111112345678')
> >>> key.owner.name
> - u'name12'
> + u'name16'
OK. the doctest has been deleted, and I added three tests to test_gpgkey.py. I'm not sure that's the correct place for them, but I don't see any other spot in the codebase that explicitly covers IGPGKeySet. Let me know if that wasn't the correct thing to do.
>
> === modified file 'lib/lp/registry/model/gpgkey.py'
> --- lib/lp/registry/model/gpgkey.py 2016-03-16 00:32:05 +0000
> +++ lib/lp/registry/model/gpgkey.py 2016-03-17 23:47:34 +0000
> @@ -65,6 +68,53 @@
> return '%s%s/%s' % (self.keysize, self.algorithm.title, self.keyid)
>
>
> +@implementer(IGPGKey)
> +class GPGServiceKey:
> +
> + def __init__(self, key_data):
> + self._key_data = key_data
> + self.active = key_data['enabled']
No, fixed.
> +
> + @property
> + def keysize(self):
> + return self._key_data['size']
> +
> + @property
> + def algorithm(self):
> + return GPGKeyAlgorithm.items[self._key_data['algorithm']]
> +
> + @property
> + def keyid(self):
> + return self._key_data['id']
> +
> + @property
> + def fingerprint(self):
> + return self._key_data['fingerprint']
> +
> + @property
> + def displayname(self):
> + return '%s%s/%s' % (self.keysize, self.algorithm.title, self.keyid)
> +
> + @property
> + def keyserverURL(self):
> + return getUtility(
> + IGPGHandler).getURLForKeyInServer(self.fingerprint, public=True)
> +
> + @property
> + def can_encrypt(self):
> + return self._key_data['can_encrypt']
> +
> + @property
> + def owner(self):
> + return getUtility(IPersonSet).getByOpenIDIdentifier(
> + self._key_data['owner'])
> +
> + @property
> + def ownerID(self):
> + return self.owner.id
> +
> +
> +
> @implementer(IGPGKeySet)
> class GPGKeySet:
>
> @@ -79,7 +129,11 @@
> def activate(self, requester, key, can_encrypt):
> """See `IGPGKeySet`."""
> fingerprint = key.fingerprint
> - lp_key = self.getByFingerprint(fingerprint)
> + # XXX: This is a little ugly - we can't use getByFingerprint here since
> + # if the READ_FROM_GPGSERVICE FF is set we'll get a GPGServiceKey object
> + # instead of a GPGKey object, and we need to change the database
> + # representation in all cases.
> + lp_key = GPGKey.selectOneBy(fingerprint=fingerprint)
> if lp_key:
> is_new = False
> # Then the key already exists, so let's reactivate it.
Fixed, as discussed on IRC.
> @@ -95,50 +149,99 @@
> ownerID, keyid, fingerprint, keysize, algorithm,
> can_encrypt=can_encrypt)
> if getFeatureFlag(GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG):
> + # XXX: Further to the comment above, if READ_FROM_GPGSERVICE FF is
> + # set then we need to duplicate the block above but reading from
> + # the gpgservice instead of the database:
> client = getUtility(IGPGClient)
> - openid_identifier = self.getOwnerIdForPerson(lp_key.owner)
> - client.addKeyForOwner(openid_identifier, key.fingerprint)
> + owner_id = self.getOwnerIdForPerson(requester)
> + if getFeatureFlag(GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG):
> + # Users with more than one openid identifier may be re-activating
> + # a key that was previously deactivated with their non-default
> + # openid identifier. If that's the case, use the same openid
> + # identifier rather than the default one:
> + key_data = client.getKeyByFingerprint(fingerprint)
> + if key_data:
> + is_new = False
> + owner_id = key_data['owner']
> + else:
> + is_new = True
Done, Done, and Done with a new test as well (note that the old code allowed this as well).
> + gpgservice_key = GPGServiceKey(client.addKeyForOwner(owner_id, key.fingerprint))
> + if getFeatureFlag(GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG):
> + lp_key = gpgservice_key
> return lp_key, is_new
>
> def deactivate(self, key):
> key.active = False
> if getFeatureFlag(GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG):
> + # Users with more than one openid identifier may be deactivating
> + # a key that is associated with their non-default openid identifier.
> + # If that's the case, use the same openid identifier rather than
> + # the default one:
> client = getUtility(IGPGClient)
> - openid_identifier = self.getOwnerIdForPerson(key.owner)
> + key_data = client.getKeyByFingerprint(key.fingerprint)
> + if key_data:
> + openid_identifier = key_data['owner']
> + else:
> + openid_identifier = self.getOwnerIdForPerson(key.owner)
Fixed - it's now a no-op. Perhaps this should raise an AssertionError?
> client.disableKeyForOwner(openid_identifier, key.fingerprint)
>
> def getByFingerprint(self, fingerprint, default=None):
> """See `IGPGKeySet`"""
> - result = GPGKey.selectOneBy(fingerprint=fingerprint)
> - if result is None:
> - return default
> - return result
> + if getFeatureFlag(GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG):
> + key_data = getUtility(IGPGClient).getKeyByFingerprint(fingerprint)
> + return GPGServiceKey(key_data) if key_data else default
> + else:
> + result = GPGKey.selectOneBy(fingerprint=fingerprint)
> + if result is None:
> + return default
> + return result
>
> def getByFingerprints(self, fingerprints):
> """See `IGPGKeySet`"""
> - return IStore(GPGKey).find(
> - GPGKey, GPGKey.fingerprint.is_in(fingerprints))
> + fingerprints = list(fingerprints)
> + if getFeatureFlag(GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG):
> + client = getUtility(IGPGClient)
> + return [GPGServiceKey(key_data) for key_data in client.getKeysByFingerprints(fingerprints)]
> + else:
> + return list(IStore(GPGKey).find(
> + GPGKey, GPGKey.fingerprint.is_in(fingerprints)))
>
> def getGPGKeysForPerson(self, owner, active=True):
> - if active is False:
> - query = """
> - active = false
> - AND fingerprint NOT IN
> - (SELECT fingerprint FROM LoginToken
> - WHERE fingerprint IS NOT NULL
> - AND requester = %s
> - AND date_consumed is NULL
> - )
> - """ % sqlvalues(owner.id)
> + if getFeatureFlag(GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG):
> + client = getUtility(IGPGClient)
> + owner_ids = self._getAllOwnerIdsForPerson(owner)
> + if not owner_ids:
> + return []
> + gpg_keys = []
> + for owner_id in owner_ids:
> + key_data = client.getKeysForOwner(owner_id)['keys']
Fixed.
> + gpg_keys.extend(
> + [GPGServiceKey(d) for d in key_data if d['enabled'] == active])
> + return gpg_keys
Hmmm. The existing check does everything in a single query, which is quite nice. I've implemented the check slightly differently here. I also added a test to verify the fix.
> else:
> - query = 'active=true'
> -
> - query += ' AND owner=%s' % sqlvalues(owner.id)
> -
> - return list(GPGKey.select(query, orderBy='id'))
> + if active is False:
> + query = """
> + active = false
> + AND fingerprint NOT IN
> + (SELECT fingerprint FROM LoginToken
> + WHERE fingerprint IS NOT NULL
> + AND requester = %s
> + AND date_consumed is NULL
> + )
> + """ % sqlvalues(owner.id)
> + else:
> + query = 'active=true'
> + query += ' AND owner=%s' % sqlvalues(owner.id)
> + return list(GPGKey.select(query, orderBy='id'))
>
> def getOwnerIdForPerson(self, owner):
> """See IGPGKeySet."""
> url = IOpenIDPersistentIdentity(owner).openid_identity_url
> assert url is not None
> return url
> +
> + def _getAllOwnerIdsForPerson(self, owner):
> + identifiers = IStore(OpenIdIdentifier).find(
> + OpenIdIdentifier, account=owner.account)
> + openid_provider_root = config.launchpad.openid_provider_root
> + return [openid_provider_root + '+id/' + i.identifier.encode('ascii') for i in identifiers]
>
Good point, will do.
> === modified file 'lib/lp/registry/tests/test_doc.py'
>
> === modified file 'lib/lp/services/gpg/handler.py'
> --- lib/lp/services/gpg/handler.py 2016-03-16 00:34:56 +0000
> +++ lib/lp/services/gpg/handler.py 2016-03-17 23:47:34 +0000
> @@ -638,132 +639,12 @@
> return fingerprint
>
>
> -def sanitize_fingerprint_or_raise(fingerprint):
> - """Check the sanity of 'fingerprint'.
> -
> - If 'fingerprint' is a valid fingerprint, the sanitised version will be
> - returned (see sanitize_fingerprint).
> -
> - Otherwise, a ValueError will be raised.
> - """
> - sane_fingerprint = sanitize_fingerprint(fingerprint)
> - if sane_fingerprint is None:
> - raise ValueError("Invalid fingerprint: %r." % fingerprint)
> - return sane_fingerprint
> -
> -
> @implementer(IGPGClient)
> -class GPGClient:
> +class GPGClientImpl(GPGClient):
Done.
> """See IGPGClient."""
>
> - def __init__(self):
> - self.write_hooks = set()
> -
> - def getKeysForOwner(self, owner_id):
> - """See IGPGClient."""
> - path = '/users/%s/keys' % self._encode_owner_id(owner_id)
> - resp = self._request('get', path)
> - if resp.status_code != http_codes['OK']:
> - self.raise_for_error(resp)
> - return resp.json()
> -
> - def addKeyForOwner(self, owner_id, fingerprint):
> - """See IGPGClient."""
> - fingerprint = sanitize_fingerprint_or_raise(fingerprint)
> - path = '/users/%s/keys' % self._encode_owner_id(owner_id)
> - data = dict(fingerprint=fingerprint)
> - resp = self._request('post', path, data)
> - if resp.status_code == http_codes['CREATED']:
> - self._notify_writes()
> - # status 200 (OK) is returned if the key was already enabled:
> - elif resp.status_code != http_codes['OK']:
> - self.raise_for_error(resp)
> -
> - def disableKeyForOwner(self, owner_id, fingerprint):
> - """See IGPGClient."""
> - fingerprint = sanitize_fingerprint_or_raise(fingerprint)
> - path = '/users/%s/keys/%s' % (self._encode_owner_id(owner_id), fingerprint)
> - resp = self._request('delete', path)
> - if resp.status_code == http_codes['OK']:
> - self._notify_writes()
> - else:
> - self.raise_for_error(resp)
> -
> - def getKeyByFingerprint(self, fingerprint):
> - fingerprint = sanitize_fingerprint_or_raise(fingerprint)
> - path = '/keys/%s' % fingerprint
> - resp = self._request('get', path)
> - if resp.status_code == http_codes['OK']:
> - return resp.json()
> - elif resp.status_code == http_codes['NOT_FOUND']:
> - return None
> - else:
> - self.raise_for_error(resp)
> -
> - def registerWriteHook(self, hook_callable):
> - """See IGPGClient."""
> - if not callable(hook_callable):
> - raise TypeError("'hook_callable' parameter must be a callable.")
> - self.write_hooks.add(hook_callable)
> -
> - def unregisterWriteHook(self, hook_callable):
> - """See IGPGClient."""
> - if hook_callable not in self.write_hooks:
> - raise ValueError("%r not registered.")
> - self.write_hooks.remove(hook_callable)
> -
> - def addKeyForTest(self, owner_id, keyid, fingerprint, keysize, algorithm, enabled,
> - can_encrypt):
> - """See IGPGClient."""
> - document = {'keys': [{
> - 'owner': owner_id,
> - 'id': keyid,
> - 'fingerprint': fingerprint,
> - 'size': keysize,
> - 'algorithm': algorithm,
> - 'enabled': enabled,
> - 'can_encrypt': can_encrypt}]}
> - path = '/test/add_keys'
> - resp = self._request('post', path, document)
> - if resp.status_code == http_codes['NOT_FOUND']:
> - raise RuntimeError(
> - "gpgservice was not configured with test endpoints enabled.")
> - elif resp.status_code != http_codes['OK']:
> - self.raise_for_error(resp)
> -
> - def _notify_writes(self):
> - errors = []
> - for hook in self.write_hooks:
> - try:
> - hook()
> - except Exception as e:
> - errors.append(str(e))
> - if errors:
> - raise Exception("The operation succeeded, but one or more write"
> - " hooks failed: %s" % ', '.join(errors))
> -
> - def _encode_owner_id(self, owner_id):
> - return base64.b64encode(owner_id, altchars='-_')
> -
> - def raise_for_error(self, response):
> - """Raise GPGServiceException based on what's in 'response'."""
> - if response.headers['Content-Type'] == 'application/json':
> - message = response.json()['status']
> - else:
> - message = "Unhandled service error. HTTP Status: %d HTTP Body: %s" % (
> - response.status_code, response.content)
> - raise GPGServiceException(message)
> -
> - @property
> - def timeout(self):
> - # Perhaps this should be from config?
> + def get_endpoint(self):
> + return "http://{}".format(config.gpgservice.api_endpoint)
> +
> + def get_timeout(self):
> return 30.0
> -
> - @property
> - def endpoint(self):
> - return "http://{}".format(config.gpgservice.api_endpoint)
> -
> - def _request(self, method, path, data=None, **kwargs):
> - response = getattr(requests, method)(
> - urljoin(self.endpoint, path), json=data, timeout=self.timeout, **kwargs)
> - return response
>
> === modified file 'lib/lp/services/gpg/tests/test_gpghandler.py'
> --- lib/lp/services/gpg/tests/test_gpghandler.py 2016-03-17 04:05:36 +0000
> +++ lib/lp/services/gpg/tests/test_gpghandler.py 2016-03-17 23:47:34 +0000
> @@ -386,15 +392,40 @@
> raises(ValueError))
>
> def test_can_add_IGPGKey_to_test_enabled_gpgservice(self):
> + self.useFixture(
> + FeatureFixture({'gpg.write_to_gpgservice': True}))
> client = getUtility(IGPGClient)
> person = self.factory.makePerson()
> + # With the feature flag enabled, the following creates a
> + # gpg key on the gpgservice.
> gpgkey = self.factory.makeGPGKey(person)
> - user = self.get_random_owner_id_string()
> - client.addKeyForTest(user, gpgkey.keyid, gpgkey.fingerprint,
> - gpgkey.keysize, gpgkey.algorithm.name,
> - gpgkey.active, gpgkey.can_encrypt)
> -
> + user = getUtility(IGPGKeySet).getOwnerIdForPerson(person)
> key = client.getKeyByFingerprint(gpgkey.fingerprint)
> self.assertThat(
> key, ContainsDict({'owner': Equals(user),
> 'fingerprint': Equals(gpgkey.fingerprint)}))
> +
> + def makePersonWithMultipleGPGKeysInDifferentOpenids(self):
> + """Make a person with multiple GPG keys owned by
> + different openid identifiers. This happens as a result
> + of an account merge.
> +
> + :returns: an IPerson instance with two keys under
> + different openid identifiers.
> + """
> + person = self.factory.makePerson()
> + self.factory.makeGPGKey(person)
> + # Create a second openid identifier from 30 days ago.
> + # This simulates the account merge:
> + identifier = OpenIdIdentifier()
> + identifier.account = person.account
> + identifier.identifier = u'openid_identifier'
> + identifier.date_created = THIRTY_DAYS_AGO
> + IMasterStore(OpenIdIdentifier).add(identifier)
> + self.factory.makeGPGKey(person)
> + return person
> +
> + def test_can_retrieve_keys_for_all_openid_identifiers(self):
> + person = self.makePersonWithMultipleGPGKeysInDifferentOpenids()
> + keys = getUtility(IGPGKeySet).getGPGKeysForPerson(person)
> + self.assertThat(keys, HasLength(2))
Added.
>
> === modified file 'lib/lp/testing/gpgservice/tests/test_fixture.py'
> --- lib/lp/testing/gpgservice/tests/test_fixture.py 2016-03-16 20:41:49 +0000
> +++ lib/lp/testing/gpgservice/tests/test_fixture.py 2016-03-17 23:47:34 +0000
> @@ -47,9 +47,9 @@
> def test_fixture_can_create_test_data(self):
> fixture = self.useFixture(GPGKeyServiceFixture())
> conn = httplib.HTTPConnection(fixture.bind_address)
> - conn.request(
> - 'GET',
> - '/users/%s/keys' % base64.b64encode('name16_oid', altchars='-_'))
> + user = base64.b64encode(
> + config.launchpad.openid_provider_root + '+id/name16_oid', altchars='-_')
Fixed.
> + conn.request('GET', '/users/%s/keys' % user)
> resp = conn.getresponse()
> self.assertEqual(200, resp.status)
> data = json.loads(resp.read())
--
https://code.launchpad.net/~thomir/launchpad/devel-add-read-ff/+merge/288989
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
