launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Good start, thanks, but some bits to fix up.

Diff comments:

> === modified file 'lib/lp/soyuz/interfaces/archive.py'
> --- lib/lp/soyuz/interfaces/archive.py	2016-01-26 15:47:37 +0000
> +++ lib/lp/soyuz/interfaces/archive.py	2016-07-07 14:30:49 +0000
> @@ -151,6 +151,12 @@
>      """Raised when creating an archive subscription for a public archive."""
>  
>  
> +@error_status(httplib.FORBIDDEN)
> +class ArchiveNotOwner(Exception):
> +    """Raised when the user is not the owner or a member of the owner team
> +    of an archive."""

Not used, and shouldn't be (the security adapter handles this instead), so delete.

> +
> +
>  @error_status(httplib.BAD_REQUEST)
>  class NoTokensForTeams(Exception):
>      """Raised when creating a token for a team, rather than a person."""
> @@ -312,6 +318,12 @@
>              self._fmt % {'processor': processor.name})
>  
>  
> +@error_status(httplib.BAD_REQUEST)

httplib.CONFLICT would be more accurate.

> +class DuplicateTokenName(Exception):
> +    """Raised when creating a named token and an active token for this archive
> +     with this name already exists."""
> +
> +
>  class IArchivePublic(IPrivacy, IHasOwner):
>      """An Archive interface for publicly available operations."""
>      # Most of this stuff should really be on View, but it's needed for
> @@ -2074,6 +2087,57 @@
>          :param dependency: is an `IArchive` object.
>          """
>  
> +    @operation_parameters(
> +        name=TextLine(title=_("Authorization token name"), required=True))

token needs to be declared here as well (with required=False), or else it won't be possible to use it over the webservice.

> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def newNamedAuthToken(name, token=None, date_created=None):
> +        """Create a new named authorization token.
> +
> +        :param name: An identifier string for this token.
> +        :param token: Optional unicode text to use as the token. One will be
> +            generated if not given.
> +        :param date_created: Optional, defaults to now.
> +
> +        :return: A dictionary where the value of `token` is the secret and
> +            the value of `archive_url` is the externally-usable archive URL
> +            including basic auth.
> +        """
> +
> +    @operation_parameters(
> +        name=TextLine(title=_("Authorization token name"), required=True))
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getNamedAuthToken(name):
> +        """Return a named authorization token for a given archive and name.

Perhaps "... for the given name in this archive".

> +
> +        :param name: The identifier string for a token.
> +
> +        :return: A dictionary where the value of `token` is the secret and
> +            the value of `archive_url` is the externally-usable archive URL
> +            including basic auth.
> +        """
> +
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getNamedAuthTokens():
> +        """Return a list of named authorization tokens for a given archive.

Perhaps "... for this archive".

> +
> +        :return: A list of dictionaries where the value of `token` is the
> +            secret and the value of `archive_url` is the externally-usable
> +            archive URL including basic auth.
> +        """
> +
> +    @operation_parameters(
> +        name=TextLine(title=_("Authorization token name"), required=True))
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def revokeNamedAuthToken(name):
> +        """Deactivates a named authorization token.
> +
> +        :param name: The identifier string for a token.
> +        """
> +
>  
>  class IArchiveAdmin(Interface):
>      """Archive interface for operations restricted by commercial."""
> 
> === modified file 'lib/lp/soyuz/interfaces/archiveauthtoken.py'
> --- lib/lp/soyuz/interfaces/archiveauthtoken.py	2013-01-07 02:40:55 +0000
> +++ lib/lp/soyuz/interfaces/archiveauthtoken.py	2016-07-07 14:30:49 +0000
> @@ -56,9 +56,19 @@
>          description=_(
>              "External archive URL including basic auth for this person"))
>  
> -    def deactivate(self):
> +    name = TextLine(
> +        title=_("Name"), required=False, readonly=True,
> +        description=_("The name for this named authorization token."))

I'd say "The name in the case of a named authorization token, or None."

> +
> +    def deactivate():
>          """Deactivate the token by setting date_deactivated to UTC_NOW."""
>  
> +    def as_dict():

LP naming style would be "asDict"; there are a few other instances of "asDict" in the codebase, but none of "as_dict".

> +        """Returns a dictionary where the value of `token` is the secret and
> +        the value of `archive_url` is the externally-usable archive URL
> +        including basic auth.
> +        """
> +
>  
>  class IArchiveAuthTokenEdit(Interface):
>      """Interface for Archive Auth Tokens requiring launchpad.Edit."""
> @@ -99,3 +109,18 @@
>          :param person: The person to which the token corresponds.
>          :return An object conforming to IArchiveAuthToken or None.
>          """
> +
> +    def getActiveNamedTokenForArchive(archive, name):
> +        """Retrieve an active named token for the given archive and name.
> +
> +        :param archive: The archive to which the token corresponds.
> +        :param name: The name of a named authorization token.
> +        :return An object conforming to IArchiveAuthToken or None.

Missing colon after "return", and I'd probably just say "An `IArchiveAuthToken` or None" rather than the "conforming" stuff.

> +        """
> +
> +    def getActiveNamedTokensForArchive(archive):
> +        """Retrieve all active named tokens for the given archive.
> +
> +        :param archive: The archive to which the tokens correspond.
> +        :return: A result set containing `IArchiveAuthToken`s.
> +        """
> 
> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py	2016-03-14 23:42:45 +0000
> +++ lib/lp/soyuz/model/archive.py	2016-07-07 14:30:49 +0000
> @@ -1948,6 +1950,53 @@
>          IStore(ArchiveAuthToken).add(archive_auth_token)
>          return archive_auth_token
>  
> +    def newNamedAuthToken(self, name, token=None, date_created=None):

This method needs to be either temporarily not webservice-exported or (preferably) hidden behind a feature flag right from the start, in order that we can deploy this change without confusing people by letting them create tokens that we might not write out anywhere yet.

> +        """See `IArchive`."""
> +
> +        # Bail if the archive isn't private
> +        if not self.private:
> +            raise ArchiveNotPrivate("Archive must be private.")
> +
> +        if self.getNamedAuthToken(name) is not None:
> +            raise DuplicateTokenName(
> +                "An active token with name %s for archive %s alread exists." %

"already"

> +                (name, self.displayname))
> +
> +        # Now onto the actual token creation:
> +        if token is None:
> +            token = create_token(20)
> +        archive_auth_token = ArchiveAuthToken()
> +        archive_auth_token.archive = self
> +        archive_auth_token.name = name
> +        archive_auth_token.token = token
> +        if date_created is not None:
> +            archive_auth_token.date_created = date_created
> +        IStore(ArchiveAuthToken).add(archive_auth_token)
> +        return archive_auth_token.as_dict()
> +
> +    def getNamedAuthToken(self, name):
> +        """See `IArchive`."""
> +        token_set = getUtility(IArchiveAuthTokenSet)
> +        archive_auth_token = token_set.getActiveNamedTokenForArchive(self, name)
> +        if archive_auth_token is not None:
> +            return archive_auth_token.as_dict()

Shouldn't this raise NotFoundError if archive_auth_token is None?  (I know getAuthToken returns None instead, but it isn't exported on the webservice.)  If so, remember to document that in the interface too.

> +
> +    def getNamedAuthTokens(self):
> +        """See `IArchive`."""
> +        token_set = getUtility(IArchiveAuthTokenSet)
> +        archive_auth_tokens = token_set.getActiveNamedTokensForArchive(self)
> +        return [archive_auth_token.as_dict()
> +            for archive_auth_token in archive_auth_tokens]
> +
> +    def revokeNamedAuthToken(self, name):
> +        """See `IArchive`."""
> +        token_set = getUtility(IArchiveAuthTokenSet)
> +        archive_auth_token = token_set.getActiveNamedTokenForArchive(self, name)
> +        if archive_auth_token is not None:
> +            archive_auth_token.deactivate()
> +        else:
> +            raise NotFoundError(name)
> +
>      def newSubscription(self, subscriber, registrant, date_expires=None,
>                          description=None):
>          """See `IArchive`."""
> 
> === modified file 'lib/lp/soyuz/model/archiveauthtoken.py'
> --- lib/lp/soyuz/model/archiveauthtoken.py	2015-10-21 09:37:08 +0000
> +++ lib/lp/soyuz/model/archiveauthtoken.py	2016-07-07 14:30:49 +0000
> @@ -59,9 +61,12 @@
>          """Return a custom archive url for basic authentication."""
>          normal_url = URI(self.archive.archive_url)
>          auth_url = normal_url.replace(
> -            userinfo="%s:%s" % (self.person.name, self.token))
> +            userinfo="%s:%s" % (self.name or self.person.name, self.token))

We can't allow named tokens to clash with tokens that have a person.  If self.name is non-None then it will need to be mangled somehow in userinfo to avoid clashing; in the design doc I suggested prefixing "+".

>          return str(auth_url)
>  
> +    def as_dict(self):
> +        return {"token": self.token, "archive_url": self.archive_url}
> +
>  
>  @implementer(IArchiveAuthTokenSet)
>  class ArchiveAuthTokenSet:
> @@ -88,8 +93,18 @@
>      def getActiveTokenForArchiveAndPerson(self, archive, person):
>          """See `IArchiveAuthTokenSet`."""
>          store = Store.of(archive)

The "store" local variable is no longer used in either this or the two following methods.

> -        return store.find(
> -            ArchiveAuthToken,
> -            ArchiveAuthToken.archive == archive,
> -            ArchiveAuthToken.person == person,
> -            ArchiveAuthToken.date_deactivated == None).one()
> +        return self.getByArchive(archive).find(
> +            ArchiveAuthToken.person == person).one()
> +
> +    def getActiveNamedTokenForArchive(self, archive, name):
> +        """See `IArchiveAuthTokenSet`."""
> +        store = Store.of(archive)
> +        return self.getByArchive(archive).find(
> +            ArchiveAuthToken.name == name).one()
> +
> +    def getActiveNamedTokensForArchive(self, archive):
> +        """See `IArchiveAuthTokenSet`."""
> +        store = Store.of(archive)
> +        return self.getByArchive(archive).find(
> +            ArchiveAuthToken.name != None)
> +
> 
> === modified file 'lib/lp/soyuz/tests/test_archive.py'
> --- lib/lp/soyuz/tests/test_archive.py	2016-04-07 00:04:42 +0000
> +++ lib/lp/soyuz/tests/test_archive.py	2016-07-07 14:30:49 +0000
> @@ -80,6 +82,8 @@
>      RedirectedPocket,
>      VersionRequiresName,
>      )
> +

Superfluous newline.

> +from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
>  from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
>  from lp.soyuz.interfaces.binarypackagebuild import BuildSetStatus
>  from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
> 
> === modified file 'lib/lp/testing/__init__.py'
> --- lib/lp/testing/__init__.py	2015-11-08 01:05:24 +0000
> +++ lib/lp/testing/__init__.py	2016-07-07 14:30:49 +0000
> @@ -619,7 +619,7 @@
>          self.assertIsNot(
>              None, pattern.search(normalise_whitespace(text)), text)
>  
> -    def assertIsInstance(self, instance, assert_class):
> +    def assertIsInstance(self, instance, assert_class, msg=None):

This seems incomplete (you haven't made anything use msg) and nothing else in this MP uses this extra argument.

>          """Assert that an instance is an instance of assert_class.
>  
>          instance and assert_class have the same semantics as the parameters


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