launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py	2016-07-14 00:25:14 +0000
> +++ lib/lp/soyuz/model/archive.py	2016-07-14 00:25:14 +0000
> @@ -1985,20 +1986,57 @@
>          else:
>              return archive_auth_token
>  
> -    def getNamedAuthToken(self, name):
> +    def newNamedAuthTokens(self, names, as_dict=True):

Same comments about as_dict here as in https://code.launchpad.net/~maxiberta/launchpad/named-auth-tokens-htaccess/+merge/300002, and likewise in the other methods taking this argument.

> +        """See `IArchive`."""
> +
> +        if not getFeatureFlag(NAMED_AUTH_TOKEN_FEATURE_FLAG):
> +            raise NamedAuthTokenFeatureDisabled()
> +
> +        # Bail if the archive isn't private
> +        if not self.private:
> +            raise ArchiveNotPrivate("Archive must be private.")
> +
> +        # Check for duplicate names.
> +        token_set = getUtility(IArchiveAuthTokenSet)
> +        active_tokens = token_set.getActiveNamedTokensForArchive(self)
> +        active_tokens = removeSecurityProxy(active_tokens)

It should be OK here, but removing the proxy is best avoided where possible anyway since doing so makes it more effort to reason about correctness.  The security proxy is applied by the proxy on methods on the object you get back from getUtility() (due to the use of <securedutility/> in ZCML registration), so the proper fix for this is to push the names restriction into a method on ArchiveAuthTokenSet.  I'd add a names=None keyword argument to ArchiveAuthTokenSet.getActiveNamedTokensForArchive, and if it's non-None then use ArchiveAuthToken.name.is_in(names) as the extra find clause instead of ArchiveAuthToken.name != None.

> +        dup_tokens = active_tokens.find(ArchiveAuthToken.name.is_in(names))
> +        dup_names = [token.name for token in dup_tokens]

Should be set(token.name for ...) rather than a list for faster membership tests, since you don't care about the order.

> +
> +        values = [
> +            (name, create_token(20), self) for name in names
> +            if name not in dup_names]

Maybe just "for name in set(names) - dup_names" since again order is unimportant, though it probably doesn't make much difference.

> +        tokens = create(
> +            (ArchiveAuthToken.name, ArchiveAuthToken.token,
> +            ArchiveAuthToken.archive), values, get_objects=True)
> +
> +        # Return all requested tokens, including duplicates.
> +        tokens.extend(dup_tokens)
> +        if as_dict:
> +            return {token.name: token.asDict() for token in tokens}
> +        else:
> +            return tokens
> +
> +    def getNamedAuthToken(self, name, as_dict=True):
>          """See `IArchive`."""
>          token_set = getUtility(IArchiveAuthTokenSet)
>          auth_token = token_set.getActiveNamedTokenForArchive(self, name)
>          if auth_token is not None:
> -            return auth_token.asDict()
> +            if as_dict:
> +                return auth_token.asDict()
> +            else:
> +                return auth_token
>          else:
>              raise NotFoundError(name)
>  
> -    def getNamedAuthTokens(self):
> +    def getNamedAuthTokens(self, as_dict=True):
>          """See `IArchive`."""
>          token_set = getUtility(IArchiveAuthTokenSet)
>          auth_tokens = token_set.getActiveNamedTokensForArchive(self)
> -        return [auth_token.asDict() for auth_token in auth_tokens]
> +        if as_dict:
> +            return [auth_token.asDict() for auth_token in auth_tokens]
> +        else:
> +            return auth_tokens
>  
>      def revokeNamedAuthToken(self, name):
>          """See `IArchive`."""
> @@ -2009,6 +2047,14 @@
>          else:
>              raise NotFoundError(name)
>  
> +    def revokeNamedAuthTokens(self, names):
> +        """See `IArchive`."""
> +        token_set = getUtility(IArchiveAuthTokenSet)
> +        active_tokens = token_set.getActiveNamedTokensForArchive(self)
> +        active_tokens = removeSecurityProxy(active_tokens)

Same as above, except that since you're using set() you'll probably need to push the whole of this method down into ArchiveAuthTokenSet.deactivateNamedTokensForArchive or similar.  That seems like a good thing anyway since it brings the tokens.set(date_deactivated=UTC_NOW) code closer to the implementation of ArchiveAuthToken.deactivate which it sort-of-duplicates.

> +        tokens = active_tokens.find(ArchiveAuthToken.name.is_in(names))
> +        tokens.set(date_deactivated=UTC_NOW)
> +
>      def newSubscription(self, subscriber, registrant, date_expires=None,
>                          description=None):
>          """See `IArchive`."""


-- 
https://code.launchpad.net/~maxiberta/launchpad/named-auth-tokens-bulk-api/+merge/300016
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.