launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/services/oauth/model.py'
> --- lib/lp/services/oauth/model.py	2015-07-08 16:05:11 +0000
> +++ lib/lp/services/oauth/model.py	2016-01-26 15:52:17 +0000
> @@ -62,32 +62,34 @@
>  class OAuthBase:
>      """Base class for all OAuth database classes."""
>  
> -    @staticmethod
> -    def _get_store():
> -        """See `SQLBase`.
> +    @classmethod
> +    def _getStore(cls):
> +        """Return the correct store for this class.
>  
>          We want all OAuth classes to be retrieved from the master flavour.  If
>          they are retrieved from the slave, there will be problems in the
>          authorization exchange, since it will be done across applications that
>          won't share the session cookies.
>          """
> -        return IMasterStore(LibraryFileAlias)
> -
> -    getStore = _get_store
> +        return IMasterStore(cls)

I'm not convinced that this method is pulling its weight any more.

>  
>  
>  @implementer(IOAuthConsumer)
> -class OAuthConsumer(OAuthBase, SQLBase):
> +class OAuthConsumer(OAuthBase, StormBase):
>      """See `IOAuthConsumer`."""
>  
> -    date_created = UtcDateTimeCol(default=UTC_NOW, notNull=True)
> -    disabled = BoolCol(notNull=True, default=False)
> -    key = StringCol(notNull=True)
> -    _secret = StringCol(dbName="secret", notNull=False, default='')
> +    __storm_table__ = 'OAuthConsumer'
> +
> +    id = Int(primary=True)
> +    date_created = DateTime(tzinfo=pytz.UTC, allow_none=False, default=UTC_NOW)
> +    disabled = Bool(allow_none=False, default=False)
> +    key = Unicode(allow_none=False)
> +    _secret = Unicode(name='secret', allow_none=True, default=u'')
>  
>      def __init__(self, key, secret):
> -        secret = hashlib.sha256(secret).hexdigest()
> -        super(OAuthConsumer, self).__init__(key=key, _secret=secret)
> +        super(OAuthConsumer, self).__init__()
> +        self.key = key
> +        self._secret = unicode(hashlib.sha256(secret).hexdigest())

I'd prefer to explicitly decode as ASCII. unicode() does that implicitly, and it's fine here, but unicode() is usually a smell that indicates someone has just used a big and wrong hammer.

>  
>      # This regular expression singles out a consumer key that
>      # represents any and all apps running on a specific computer. The
> 
> === modified file 'lib/lp/services/webapp/authentication.py'
> --- lib/lp/services/webapp/authentication.py	2015-07-08 16:05:11 +0000
> +++ lib/lp/services/webapp/authentication.py	2016-01-26 15:52:17 +0000
> @@ -288,7 +295,11 @@
>      If the signature is incorrect or its method is not supported, set the
>      appropriate status in the request's response and return False.
>      """
> -    authorization = get_oauth_authorization(request)
> +    try:
> +        authorization = get_oauth_authorization(request)
> +    except UnicodeDecodeError:
> +        request.response.setStatus(400)

It might be more polite to include an explanation, but I see the non-PLAINTEXT case already doesn't.

> +        return False
>  
>      if authorization.get('oauth_signature_method') != 'PLAINTEXT':
>          # XXX: 2008-03-04, salgado: Only the PLAINTEXT method is supported


-- 
https://code.launchpad.net/~cjwatson/launchpad/stormify-oauth/+merge/283985
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.