launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20108
Re: [Merge] lp:~thomir/launchpad/devel-add-read-ff into lp:launchpad
Review: Needs Fixing code
Diff comments:
>
No-op changes.
> === modified file 'database/schema/security.cfg'
>
The garbo jobs were removed a couple of branches ago, so I think there's a mismerge here.
> === modified file 'lib/lp/registry/browser/tests/test_gpgkey.py'
>
This test doesn't seem like it should need gpgservice.
> === 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"/>
It doesn't really make sense to allow those to be set directly, and I think you've replaced the code that used to do that.
> + </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'
This test's intent is to verify that a key can be retrieved once it's added. This removes that check. Is there a corresponding unit test that replaces it?
>
> === 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']
Does active still need to be settable?
> +
> + @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.
is_new is always derived from the LP DB, even if the read flag is set.
> @@ -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
The owner_id calculation cannot be guarded with the read FF. It's critical that we always write with the correct owner ID, and the intent of the read flag is just to prevent application behaviour from changing based on the gpgservice, not to prevent reading from it at all.
Setting is_new only if the read FF is set is important, though.
Additionally, we need to verify that the logged in user is authorised to act as owner_id. Don't want them reactivating somebody else's key!
> + 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)
If there's no existing owner then there's no existing key, so we don't need to deactivate it, right?
> 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']
key_data is a list of keys here, but a single key in the previous method.
> + gpg_keys.extend(
> + [GPGServiceKey(d) for d in key_data if d['enabled'] == active])
> + return gpg_keys
This drops the check below that there's no current LoginToken to reactivate the key. That check should probably be factored out of this method, or simply duplicated in this branch. I'd prefer the former, though. There's also no sort applied in this branch.
> 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]
>
It's still important that write-only work. So you might have to duplicate the test once again...
> === 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):
"LPGPGClient"?
> """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))
There are no tests of cross-OpenID activations, reactivations or deactivations.
>
> === 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='-_')
Indentation.
> + 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
