launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/registry/interfaces/person.py'
> --- lib/lp/registry/interfaces/person.py	2016-05-25 05:33:39 +0000
> +++ lib/lp/registry/interfaces/person.py	2016-05-25 05:33:39 +0000
> @@ -2316,6 +2316,33 @@
>              compromised key.
>          """
>  
> +    @call_with(user=REQUEST_USER)
> +    @operation_parameters(
> +        openid_identifier=TextLine(
> +            title=_("OpenID identifier suffix"), required=True),
> +        key_text=TextLine(
> +            title=_("SSH key text"), required=True),
> +        dry_run=Bool(_("Don't save changes")))
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def deleteSSHKeyForPersonFromSSO(user, openid_identifier, key_text,
> +                                     dry_run=False):

This is on PersonSet, and the other methods don't say "ForPerson", so I'd drop the "ForPerson" here.

> +        """Restricted SSH key deletion API for SSO.
> +
> +        This method can only be called by the Ubuntu SSO service. It deletes an
> +        SSH key from the account identified by 'openid_identifier' based on the
> +        'key_text' parameter.
> +
> +        :param user: the `IPerson` performing the operation. Only the
> +            ubuntu-sso celebrity is allowed.
> +        :param openid_identifier: OpenID identifier suffix for the user.
> +            This is *not* the full URL, just the unique suffix portion.
> +        :param key_text: The full text of the SSH Key.
> +        :raises NoSuchAccount: If the openid_identifier specified does not
> +            match any account.
> +        :raises KeyAdditionError: If the key text is invalid.
> +        """
> +
>      @call_with(teamowner=REQUEST_USER)
>      @rename_parameters_as(
>          teamdescription='team_description',
> 
> === modified file 'lib/lp/registry/interfaces/ssh.py'
> --- lib/lp/registry/interfaces/ssh.py	2016-05-25 05:33:39 +0000
> +++ lib/lp/registry/interfaces/ssh.py	2016-05-25 05:33:39 +0000
> @@ -106,6 +110,14 @@
>      def getByPeople(people):
>          """Return SSHKey object associated to the people provided."""
>  
> +    def getByPersonAndKeyText(person, sshkey):
> +        """Get an SSH key for a person with a specific key text.
> +
> +        :param person: The person who owns the key.
> +        :param sshkey: The full ssh key text.

This is called "key_text" elsewhere, to contrast it with an actual SSHKey object. Might be worth being consistent.

> +        :raises SSHKeyAdditionError: If 'sshkey' is invalid.
> +        """
> +
>  
>  @error_status(httplib.BAD_REQUEST)
>  class SSHKeyAdditionError(Exception):
> 
> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py	2016-05-25 05:33:39 +0000
> +++ lib/lp/registry/model/person.py	2016-05-25 05:33:39 +0000
> @@ -3484,6 +3485,26 @@
>          getUtility(ISSHKeySet).new(
>              IPerson(account), key_text, False, dry_run=dry_run)
>  
> +    def deleteSSHKeyForPersonFromSSO(self, user, openid_identifier, key_text,
> +                                     dry_run=False):
> +        """See `IPersonSet`."""
> +        if user != getUtility(ILaunchpadCelebrities).ubuntu_sso:
> +            raise Unauthorized()
> +        try:
> +            account = getUtility(IAccountSet).getByOpenIDIdentifier(
> +                openid_identifier)
> +        except LookupError:
> +            raise NoSuchAccount("No account found for openid identifier '%s'"
> +                                % openid_identifier)
> +        keys = getUtility(ISSHKeySet).getByPersonAndKeyText(
> +            IPerson(account),
> +            key_text)
> +        if not dry_run:
> +            # ISSHKeySet does not restrict the same SSH key being added
> +            # multiple times, so make sure we delte them all:

delete

> +            for key in keys:
> +                key.destroySelf()
> +
>      def newTeam(self, teamowner, name, display_name, teamdescription=None,
>                  membership_policy=TeamMembershipPolicy.MODERATED,
>                  defaultmembershipperiod=None, defaultrenewalperiod=None,
> @@ -4118,6 +4126,27 @@
>              SSHKey.person IN %s
>              """ % sqlvalues([person.id for person in people]))
>  
> +    def getByPersonAndKeyText(self, person, sshkey):
> +        keytype, keytext, comment = self._extract_ssh_key_components(sshkey)
> +        return IStore(SSHKey).find(
> +            SSHKey,
> +            person=person, keytype=keytype, keytext=keytext, comment=comment)
> +
> +    def _extract_ssh_key_components(self, sshkey):

_parse_ssh_key_text?

> +        try:
> +            kind, keytext, comment = sshkey.split(' ', 2)
> +        except (ValueError, AttributeError):
> +            raise SSHKeyAdditionError("Invalid SSH key data: '%s'" % sshkey)

sed is easy, so I'd probably rename the exception as you suggest.

> +
> +        if not (kind and keytext and comment):
> +            raise SSHKeyAdditionError("Invalid SSH key data: '%s'" % sshkey)
> +
> +        keytype = SSH_TEXT_TO_KEY_TYPE.get(kind)
> +        if keytype is None:
> +            raise SSHKeyAdditionError(
> +                "Invalid SSH key type: '%s'" % kind)
> +        return keytype, keytext, comment
> +
>  
>  @implementer(IWikiName)
>  class WikiName(SQLBase, HasOwnerMixin):


-- 
https://code.launchpad.net/~thomir/launchpad/devel-add-ssh-delete-key-api/+merge/295665
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.