launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

There are probably some security.cfg changes that can be dropped, although it may not be worth it since you'd have to go through a full test run to make sure it was OK.

The gpgservice section should eventually be removed from lib/lp/services/config/schema-lazr.conf, but you'll need to get it removed from the production configs first as otherwise the application will fail to start up.  It may also be worth removing the gpgservice reference from the comment on launchpad.openid_canonical_root.

Diff comments:

> 
> === modified file 'lib/lp/registry/browser/tests/test_gpgkey.py'
> --- lib/lp/registry/browser/tests/test_gpgkey.py	2016-07-25 00:07:01 +0000
> +++ lib/lp/registry/browser/tests/test_gpgkey.py	2016-11-03 15:22:08 +0000

I suspect that TestCanonicalUrl and TestPersonGPGView in this file can go back to DatabaseFunctionalLayer now (see r17956).

> @@ -5,17 +5,6 @@
>  
>  __metaclass__ = type
>  
> -from testtools.matchers import (
> -    Not,
> -    Raises,
> -    raises,
> -    )
> -
> -from lp.services.features.testing import FeatureFixture
> -from lp.services.gpg.interfaces import (
> -    GPG_DATABASE_READONLY_FEATURE_FLAG,
> -    GPGReadOnly,
> -    )
>  from lp.services.webapp import canonical_url
>  from lp.testing import (
>      login_person,
> @@ -53,25 +42,3 @@
>          expected_url = (
>              '%s/+editpgpkeys/+login?reauth=1' % canonical_url(person))
>          self.assertEqual(expected_url, response.getHeader('location'))
> -
> -    def test_gpgkeys_POST_readonly_with_feature_flag_set(self):
> -        self.useFixture(FeatureFixture({
> -            GPG_DATABASE_READONLY_FEATURE_FLAG: True,
> -        }))
> -        person = self.factory.makePerson()
> -        login_person(person)
> -        view = create_initialized_view(
> -            person, "+editpgpkeys", principal=person, method='POST',
> -            have_fresh_login=True)
> -        self.assertThat(view.render, raises(GPGReadOnly))
> -
> -    def test_gpgkeys_GET_readonly_with_feature_flag_set(self):
> -        self.useFixture(FeatureFixture({
> -            GPG_DATABASE_READONLY_FEATURE_FLAG: True,
> -        }))
> -        person = self.factory.makePerson()
> -        login_person(person)
> -        view = create_initialized_view(
> -            person, "+editpgpkeys", principal=person, method='GET',
> -            have_fresh_login=True)
> -        self.assertThat(view.render, Not(Raises()))
> 
> === modified file 'lib/lp/registry/model/gpgkey.py'
> --- lib/lp/registry/model/gpgkey.py	2016-07-22 03:07:45 +0000
> +++ lib/lp/registry/model/gpgkey.py	2016-11-03 15:22:08 +0000

This can probably have some more things removed.  GPGServiceKey should no longer be needed, and GPGKeySet.activate can perhaps go back to using getByFingerprint.

> @@ -25,17 +25,12 @@
>      SQLBase,
>      sqlvalues,
>      )
> -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,
>      )
>  from lp.services.openid.interfaces.openid import IOpenIDPersistentIdentity
>  from lp.services.openid.model.openididentifier import OpenIdIdentifier
> -from lp.services.verification.interfaces.logintoken import ILoginTokenSet
>  
>  
>  @implementer(IGPGKey)
> @@ -154,27 +149,6 @@
>              lp_key = self.new(
>                  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)
> -            owner_id = self.getOwnerIdForPerson(requester)
> -            # 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 - this happens even if the
> -            # read FF is not set:
> -            key_data = client.getKeyByFingerprint(fingerprint)
> -            if key_data:
> -                owner_id = key_data['owner']
> -            allowed_owner_ids = self.getAllOwnerIdsForPerson(requester)
> -            assert owner_id in allowed_owner_ids
> -            gpgservice_key = GPGServiceKey(
> -                client.addKeyForOwner(owner_id, key.fingerprint))
> -            if getFeatureFlag(GPG_READ_FROM_GPGSERVICE_FEATURE_FLAG):
> -                is_new = key_data is None
> -                lp_key = gpgservice_key
>          return lp_key, is_new
>  
>      def deactivate(self, key):
> @@ -182,84 +156,35 @@
>          # active attribute. Retrieve it by fingerprint:
>          lp_key = GPGKey.selectOneBy(fingerprint=key.fingerprint)
>          lp_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)
> -            key_data = client.getKeyByFingerprint(key.fingerprint)
> -            if not key_data:
> -                # We get here if we're asked to deactivate a key that was never
> -                # activated. This should probably never happen.
> -                return
> -            openid_identifier = key_data['owner']
> -            client.disableKeyForOwner(openid_identifier, key.fingerprint)
>  
>      def getByFingerprint(self, fingerprint, default=None):
>          """See `IGPGKeySet`"""
> -        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
> +        result = GPGKey.selectOneBy(fingerprint=fingerprint)
> +        if result is None:
> +            return default
> +        return result
>  
>      def getByFingerprints(self, fingerprints):
>          """See `IGPGKeySet`"""
>          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)))
> +        return list(IStore(GPGKey).find(
> +            GPGKey, GPGKey.fingerprint.is_in(fingerprints)))
>  
>      def getGPGKeysForPerson(self, owner, active=True):
> -        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_list = client.getKeysForOwner(owner_id)['keys']
> -                gpg_keys.extend([
> -                    GPGServiceKey(d) for d in key_data_list
> -                    if d['enabled'] == active])
> -            if active is False:
> -                login_tokens = getUtility(ILoginTokenSet).getPendingGPGKeys(
> -                    owner.id)
> -                token_fingerprints = [t.fingerprint for t in login_tokens]
> -                return [
> -                    k for k in gpg_keys
> -                    if k.fingerprint not in token_fingerprints]
> -            return gpg_keys
> +        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:
> -            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_canonical_url
> -        assert url is not None
> -        return url
> +            query = 'active=true'
> +        query += ' AND owner=%s' % sqlvalues(owner.id)
> +        return list(GPGKey.select(query, orderBy='id'))
>  
>      def getAllOwnerIdsForPerson(self, owner):

I think all the non-test callers of this method have been removed.

>          identifiers = IStore(OpenIdIdentifier).find(
> 
> === modified file 'lib/lp/services/verification/browser/tests/test_logintoken.py'
> --- lib/lp/services/verification/browser/tests/test_logintoken.py	2016-03-21 05:37:40 +0000
> +++ lib/lp/services/verification/browser/tests/test_logintoken.py	2016-11-03 15:22:08 +0000

Can TestCancelActionOnLoginTokenViews and LoginTokenReadOnlyTests go back to DatabaseFunctionalLayer?  Same for lp.soyuz.tests.test_archive_subscriptions.PrivateArtifactsViewTestCase.

> @@ -1,16 +1,9 @@
>  # Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
> -from testtools.matchers import raises
> -
>  from zope.component import getUtility
>  from zope.security.proxy import removeSecurityProxy
>  
> -from lp.services.features.testing import FeatureFixture
> -from lp.services.gpg.interfaces import (
> -    GPG_DATABASE_READONLY_FEATURE_FLAG,
> -    GPGReadOnly,
> -    )
>  from lp.services.identity.interfaces.account import AccountStatus
>  from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
>  from lp.services.verification.browser.logintoken import (
> @@ -82,28 +75,6 @@
>          self.assertEquals(harness.view.next_url, self.expected_next_url)
>  
>  
> -class LoginTokenReadOnlyTests(TestCaseWithFactory):
> -
> -    layer = LaunchpadFunctionalLayer
> -
> -    def test_continue_action_failed_with_gpg_database_in_ro_mode(self):
> -        self.useFixture(FeatureFixture({
> -            GPG_DATABASE_READONLY_FEATURE_FLAG: True,
> -        }))
> -        person = self.factory.makePerson(name='test-user')
> -        email = removeSecurityProxy(person).preferredemail.email
> -        gpg_key = self.factory.makeGPGKey(person)
> -        token = getUtility(ILoginTokenSet).new(
> -            person, email, email, LoginTokenType.VALIDATEGPG,
> -            fingerprint=gpg_key.fingerprint)
> -
> -        harness = LaunchpadFormHarness(token, ValidateGPGKeyView)
> -        self.assertThat(
> -            lambda: harness.submit('continue', {}),
> -            raises(GPGReadOnly)
> -        )
> -
> -
>  class TestClaimTeamView(TestCaseWithFactory):
>      """Test the claiming of a team via login token."""
>  
> 
> === modified file 'versions.cfg'
> --- versions.cfg	2016-09-21 02:51:38 +0000
> +++ versions.cfg	2016-11-03 15:22:08 +0000
> @@ -38,8 +38,6 @@
>  flask = 0.10.1
>  FormEncode = 1.2.4
>  funkload = 1.16.1
> -gpgservice = 0.1.2
> -gpgservice-client = 0.0.10
>  grokcore.component = 1.6
>  gunicorn = 19.4.5
>  html5browser = 0.0.9

Based on r17926 I suspect you can probably remove a few more things from here.



-- 
https://code.launchpad.net/~thomir/launchpad/kill-gpgservice-with-fire-and-stakes/+merge/309968
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.