launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20083
[Merge] lp:~thomir/launchpad/devel-add-read-ff into lp:launchpad
Thomi Richards has proposed merging lp:~thomir/launchpad/devel-add-read-ff into lp:launchpad with lp:~thomir/launchpad/devel-start-integration as a prerequisite.
Commit message:
Add feature flag to read GPG keys from gpgservice.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~thomir/launchpad/devel-add-read-ff/+merge/288989
Add feature flag to read GPG keys from gpgservice.
This branch lists ~thomir/launchpad/devel-make-diff-sensible-again as the pre-requisite branch simply to make the diff sensible. The actual pre-requisite branch is: ~thomir/launchpad/devel-start-integration
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thomir/launchpad/devel-add-read-ff into lp:launchpad.
=== modified file 'database/sampledata/current.sql'
--- database/sampledata/current.sql 2016-02-24 06:08:16 +0000
+++ database/sampledata/current.sql 2016-03-14 22:23:19 +0000
@@ -3744,8 +3744,6 @@
ALTER TABLE featureflag DISABLE TRIGGER ALL;
-
-
ALTER TABLE featureflag ENABLE TRIGGER ALL;
@@ -11356,5 +11354,3 @@
ALTER TABLE xref ENABLE TRIGGER ALL;
-
-
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2016-03-14 22:23:19 +0000
+++ database/schema/security.cfg 2016-03-14 22:23:19 +0000
@@ -2280,6 +2280,7 @@
public.accesspolicygrant = SELECT, DELETE
public.account = SELECT, DELETE
public.answercontact = SELECT, DELETE
+public.archive = SELECT, UPDATE
public.branch = SELECT, UPDATE
public.branchjob = SELECT, DELETE
public.binarypackagename = SELECT
@@ -2321,6 +2322,7 @@
public.milestonetag = SELECT
public.openidconsumerassociation = SELECT, DELETE
public.openidconsumernonce = SELECT, DELETE
+public.packageupload = SELECT, UPDATE
public.person = SELECT, DELETE
public.personsettings = SELECT, UPDATE
public.product = SELECT, UPDATE
@@ -2330,9 +2332,10 @@
public.previewdiff = SELECT, DELETE
public.revisionauthor = SELECT, UPDATE
public.revisioncache = SELECT, DELETE
+public.signedcodeofconduct = SELECT, UPDATE
public.snapfile = SELECT, DELETE
public.sourcepackagename = SELECT
-public.sourcepackagerelease = SELECT
+public.sourcepackagerelease = SELECT, UPDATE
public.sourcepackagepublishinghistory = SELECT, UPDATE
public.suggestivepotemplate = INSERT, DELETE
public.teammembership = SELECT, DELETE
=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py 2015-10-26 14:54:43 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py 2016-03-14 22:23:19 +0000
@@ -27,7 +27,10 @@
record_two_runs,
TestCaseWithFactory,
)
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+ DatabaseFunctionalLayer,
+ LaunchpadFunctionalLayer,
+ )
from lp.testing.matchers import HasQueryCount
from lp.testing.pages import (
LaunchpadWebServiceCaller,
@@ -72,7 +75,7 @@
class TestPersonAccountStatus(TestCaseWithFactory):
- layer = DatabaseFunctionalLayer
+ layer = LaunchpadFunctionalLayer
def test_account_status_history_restricted(self):
person = self.factory.makePerson()
=== 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-14 22:23:19 +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"/>
+ </class>
<!-- GPGKeySet -->
=== modified file 'lib/lp/registry/interfaces/gpg.py'
--- lib/lp/registry/interfaces/gpg.py 2016-03-14 22:23:19 +0000
+++ lib/lp/registry/interfaces/gpg.py 2016-03-14 22:23:19 +0000
@@ -84,6 +84,9 @@
inactive ones.
"""
+ def getOwnerIdForPerson(person):
+ """return an owner id string suitable for sending to gpgservice."""
+
def getByFingerprints(fingerprints):
"""Get multiple OpenPGP keys by their fingerprints."""
=== modified file 'lib/lp/registry/model/gpgkey.py'
--- lib/lp/registry/model/gpgkey.py 2016-03-14 22:23:19 +0000
+++ lib/lp/registry/model/gpgkey.py 2016-03-14 22:23:19 +0000
@@ -18,6 +18,7 @@
IGPGKey,
IGPGKeySet,
)
+from lp.registry.interfaces.person import IPersonSet
from lp.services.database.enumcol import EnumCol
from lp.services.database.interfaces import IStore
from lp.services.database.sqlbase import (
@@ -27,6 +28,7 @@
from lp.services.features import getFeatureFlag
from lp.services.gpg.interfaces import (
GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG,
+ GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG,
GPGKeyAlgorithm,
IGPGClient,
IGPGHandler,
@@ -65,6 +67,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']
+
+ @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:
@@ -80,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.
@@ -96,9 +149,17 @@
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)
+ if getFeatureFlag(GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG):
+ lp_key = self.getByFingerprint(key.fingerprint)
+ is_new = lp_key is None
+ # TODO: make addKeyForOwner return the newly added key?
+ # TODO: Can we assume that getOwnerIdForPerson won't return None here?
+ client.addKeyForOwner(self.getOwnerIdForPerson(requester), key.fingerprint)
+ lp_key = self.getByFingerprint(key.fingerprint)
return lp_key, is_new
def deactivate(self, key):
@@ -110,33 +171,50 @@
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 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_id = self.getOwnerIdForPerson(owner)
+ if owner_id is None:
+ return []
+ keys = client.getKeysForOwner(owner_id)['keys']
+ return [GPGServiceKey(d) for d in keys if d['enabled'] == active]
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."""
=== modified file 'lib/lp/services/gpg/handler.py'
--- lib/lp/services/gpg/handler.py 2016-03-14 22:23:19 +0000
+++ lib/lp/services/gpg/handler.py 2016-03-14 22:23:19 +0000
@@ -699,6 +699,20 @@
else:
self.raise_for_error(resp)
+ def getKeysByFingerprints(self, fingerprints):
+ if len(fingerprints) == 1:
+ return [self.getKeyByFingerprint(fingerprints[0])]
+ fingerprint = ','.join(
+ [sanitize_fingerprint_or_raise(f) for f in fingerprints])
+ path = '/keys/%s' % fingerprint
+ resp = self._request('get', path)
+ if resp.status_code == http_codes['OK']:
+ return resp.json()['keys']
+ elif resp.status_code == http_codes['NOT_FOUND']:
+ return []
+ else:
+ self.raise_for_error(resp)
+
def registerWriteHook(self, hook_callable):
"""See IGPGClient."""
if not callable(hook_callable):
=== modified file 'lib/lp/services/gpg/interfaces.py'
--- lib/lp/services/gpg/interfaces.py 2016-03-14 22:23:19 +0000
+++ lib/lp/services/gpg/interfaces.py 2016-03-14 22:23:19 +0000
@@ -3,6 +3,7 @@
__all__ = [
'GPG_DATABASE_READONLY_FEATURE_FLAG',
+ 'GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG',
'GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG',
'GPGKeyAlgorithm',
'GPGKeyDoesNotExistOnServer',
@@ -52,6 +53,7 @@
GPG_DATABASE_READONLY_FEATURE_FLAG = u"gpg.database_read_only"
GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG = u"gpg.write_to_gpgservice"
+GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG = u"gpg.read_from_gpgservice"
def valid_fingerprint(fingerprint):
@@ -467,6 +469,13 @@
:raises ValueError: if the fingerprint isn't valid.
"""
+ def getKeysByFingerprints(fingerprints):
+ """Bulk retrieve GPG keys by a list of fingerprints.
+
+ :param fingerprints: A list of fingerprints to retrieve.
+ :returns: A list of keys that were found.
+ """
+
def registerWriteHook(hook_callable):
"""Register a write hook.
=== modified file 'lib/lp/services/gpg/tests/test_gpghandler.py'
--- lib/lp/services/gpg/tests/test_gpghandler.py 2016-03-14 22:23:19 +0000
+++ lib/lp/services/gpg/tests/test_gpghandler.py 2016-03-14 22:23:19 +0000
@@ -16,6 +16,7 @@
from zope.security.proxy import removeSecurityProxy
from lp.registry.interfaces.gpg import IGPGKeySet
+from lp.registry.interfaces.person import IPersonSet
from lp.services.config.fixture import (
ConfigFixture,
ConfigUseFixture,
@@ -254,7 +255,9 @@
def test_get_key_for_user_with_sampledata(self):
client = getUtility(IGPGClient)
- data = client.getKeysForOwner('name16_oid')
+ person = getUtility(IPersonSet).getByName('name16')
+ openid_id = getUtility(IGPGKeySet).getOwnerIdForPerson(person)
+ data = client.getKeysForOwner(openid_id)
self.assertThat(data, ContainsDict({'keys': HasLength(1)}))
def test_get_key_for_unknown_user(self):
=== modified file 'lib/lp/soyuz/tests/test_archive_subscriptions.py'
--- lib/lp/soyuz/tests/test_archive_subscriptions.py 2015-09-24 11:30:24 +0000
+++ lib/lp/soyuz/tests/test_archive_subscriptions.py 2016-03-14 22:23:19 +0000
@@ -21,7 +21,7 @@
StormStatementRecorder,
TestCaseWithFactory,
)
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import DatabaseFunctionalLayer, LaunchpadFunctionalLayer
from lp.testing.mail_helpers import pop_notifications
from lp.testing.matchers import HasQueryCount
from lp.testing.pages import (
@@ -129,7 +129,7 @@
class PrivateArtifactsViewTestCase(BrowserTestCase):
""" Tests that private team archives can be viewed."""
- layer = DatabaseFunctionalLayer
+ layer = LaunchpadFunctionalLayer
def setUp(self):
"""Create a test archive."""
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2016-03-14 22:23:19 +0000
+++ lib/lp/testing/factory.py 2016-03-14 22:23:19 +0000
@@ -235,6 +235,7 @@
from lp.services.gpg.interfaces import (
GPG_WRITE_TO_GPGSERVICE_FEATURE_FLAG,
GPGKeyAlgorithm,
+ IGPGClient,
IGPGHandler,
)
from lp.services.identity.interfaces.account import (
=== modified file 'lib/lp/testing/gpgservice/_fixture.py'
--- lib/lp/testing/gpgservice/_fixture.py 2016-02-16 05:36:37 +0000
+++ lib/lp/testing/gpgservice/_fixture.py 2016-03-14 22:23:19 +0000
@@ -99,7 +99,7 @@
test_data = {
'keys': [
{
- 'owner': 'name16_oid',
+ 'owner': config.launchpad.openid_provider_root + '+id/name16_oid',
'id': '12345678',
'fingerprint': 'ABCDEF0123456789ABCDDCBA0000111112345678',
'size': 1024,
=== modified file 'lib/lp/testing/gpgservice/tests/test_fixture.py'
--- lib/lp/testing/gpgservice/tests/test_fixture.py 2016-03-14 22:23:19 +0000
+++ lib/lp/testing/gpgservice/tests/test_fixture.py 2016-03-14 22:23:19 +0000
@@ -47,7 +47,8 @@
def test_fixture_can_create_test_data(self):
fixture = self.useFixture(GPGKeyServiceFixture())
conn = httplib.HTTPConnection(fixture.bind_address)
- user = base64.b64encode('name16_oid', altchars='-_')
+ user = base64.b64encode(
+ config.launchpad.openid_provider_root + '+id/name16_oid', altchars='-_')
conn.request('GET', '/users/%s/keys' % user)
resp = conn.getresponse()
self.assertEqual(200, resp.status)
Follow ups
