launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py	2016-01-26 15:47:37 +0000
> +++ lib/lp/registry/browser/person.py	2016-02-09 22:57:34 +0000
> @@ -2531,14 +2534,16 @@
>              IGPGHandler).getURLForKeyInServer(self.fingerprint, public=True)
>  
>      def form_action(self):
> +        if self.request.method != "POST":
> +            return ''
> +        if getFeatureFlag(GPG_SERVICE_READONLY_FEATURE_FLAG):
> +            raise GPGReadOnly()

It's safe to guard only browser code in this case, since gpg keys aren't editable through the API. But there's also one other thing that needs blocking: the verification workflow is completed by a GPGKeySet.activate call in lp.services.verification.model.logintoken.

>          permitted_actions = [
>              'claim_gpg',
>              'deactivate_gpg',
>              'remove_gpgtoken',
>              'reactivate_gpg',
>              ]
> -        if self.request.method != "POST":
> -            return ''
>          action = self.request.form.get('action')
>          if action not in permitted_actions:
>              raise UnexpectedFormData("Action not permitted: %s" % action)
> 
> === modified file 'lib/lp/registry/browser/tests/test_gpgkey.py'
> --- lib/lp/registry/browser/tests/test_gpgkey.py	2012-08-20 14:33:30 +0000
> +++ lib/lp/registry/browser/tests/test_gpgkey.py	2016-02-09 22:57:34 +0000
> @@ -42,3 +53,23 @@
>          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_SERVICE_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_SERVICE_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()))

We'd normally just self.assertIn("some obvious piece of content", view.render()) to check that it vaguely works, but I suppose this works too.

> 
> === modified file 'lib/lp/services/gpg/interfaces.py'
> --- lib/lp/services/gpg/interfaces.py	2016-01-26 15:47:37 +0000
> +++ lib/lp/services/gpg/interfaces.py	2016-02-09 22:57:34 +0000
> @@ -2,35 +2,54 @@
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  __all__ = [
> +    'GPG_SERVICE_READONLY_FEATURE_FLAG',
>      'GPGKeyAlgorithm',
>      'GPGKeyDoesNotExistOnServer',
>      'GPGKeyExpired',
> +    'GPGKeyNotFoundError',
>      'GPGKeyRevoked',
> -    'GPGKeyNotFoundError',
>      'GPGKeyTemporarilyNotFoundError',
> +    'GPGReadOnly',
>      'GPGUploadFailure',
>      'GPGVerificationError',
>      'IGPGHandler',
> +    'IPymeKey',
>      'IPymeSignature',
> -    'IPymeKey',
>      'IPymeUserId',
>      'MoreThanOneGPGKeyFound',
>      'SecretGPGKeyImportDetected',
> +    'valid_fingerprint',
>      'valid_keyid',
> -    'valid_fingerprint',
>      ]
>  
> -
> +import httplib
>  import re
>  
>  from lazr.enum import (
>      DBEnumeratedType,
>      DBItem,
>      )
> +from lazr.restful.declarations import error_status
>  from zope.interface import (
>      Attribute,
>      Interface,
>      )
> +from zope.security.interfaces import (
> +    Forbidden,
> +    )
> +
> +
> +@error_status(httplib.FORBIDDEN)
> +class GPGReadOnly(Forbidden):
> +    """GPG Service is in read-only mode."""
> +
> +    def __init__(self):
> +        super(GPGReadOnly, self).__init__(
> +            "The GPG key storage facilities of Launchpad are currently "
> +            "read-only. Please try again later.")
> +
> +
> +GPG_SERVICE_READONLY_FEATURE_FLAG = u"gpgservice.read_only"

The flag really disables editing of the gpg key database. The gpgservice isn't integrated yet, so saying that it's read-only is a tad confusing.

>  
>  
>  def valid_fingerprint(fingerprint):
> 
> === modified file 'lib/lp/services/webapp/login.py'
> --- lib/lp/services/webapp/login.py	2014-01-14 06:29:21 +0000
> +++ lib/lp/services/webapp/login.py	2016-02-09 22:57:34 +0000
> @@ -465,6 +465,8 @@
>  
>  def isFreshLogin(request):
>      """Return True if the principal login happened in the last 120 seconds."""
> +    if getattr(request, 'have_fresh_login', False):

I'd call it "force_fresh_login_for_testing" or similar to make it obvious that it's a test convenience and not something for general use.

> +        return True
>      session = ISession(request)
>      authdata = session['launchpad.authenticateduser']
>      logintime = authdata.get('logintime', None)
> 
> === modified file 'lib/lp/services/webapp/servers.py'
> --- lib/lp/services/webapp/servers.py	2016-01-26 15:14:01 +0000
> +++ lib/lp/services/webapp/servers.py	2016-02-09 22:57:34 +0000
> @@ -904,6 +904,11 @@
>      >>> request.charsets = ['utf-8']
>      >>> request.query_string_params == {'a': ['1'], 'b': ['2'], 'c': ['3']}
>      True
> +
> +    If have_fresh_login is set to True, views such as PersonGPGView that insist
> +    on the user being recently logged in will render their contents rather than
> +    redirecting to the login page.

That's an implementation detail of the particular views that use isFreshLogin at the moment. What it really does it short-circuit isFreshLogin, and it's only for use in tests.

> +    
>      """
>  
>      # These two attributes satisfy IParticipation.


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