Merge into db-devel : expose-sshkeys-bug-357235 : Code : Launchpad itself

Status:	Merged
Approved by:	Michael Hudson-Doyle on 2010-03-24
Approved revision:	no longer in the source branch.
Merged at revision:	not available
Proposed branch:	lp:~didrocks/launchpad/expose-sshkeys-bug-357235
Merge into:	lp:launchpad/db-devel
Diff against target:	200 lines (+98/-9) 5 files modified lib/lp/registry/browser/configure.zcml (+6/-0) lib/lp/registry/browser/tests/test_sshkey.py (+31/-0) lib/lp/registry/interfaces/person.py (+8/-3) lib/lp/registry/interfaces/ssh.py (+10/-5) lib/lp/registry/stories/webservice/xx-person.txt (+43/-1)
To merge this branch:	bzr merge lp:~didrocks/launchpad/expose-sshkeys-bug-357235
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Francis J. Lacoste (community)		2010-03-24	Approve on 2010-03-24
Graham Binns	code	2010-03-24	Pending
Review via email: mp+22067@code.launchpad.net

Revision history for this message

Graham Binns (gmb) wrote on 2010-03-09: Posted in a previous version of this proposal

#

Hi Didier,

I'm not comfortable reviewing this branch. There's no description of the change in the merge proposal description and I don't know whether you've had a pre-implementation discussion about the branch.

You need to include the following items when you submit a Launchpad branch for review:

* A description of the change, including a brief list of changes by file
* The output of `make lint`, run in the root of your branch.
* Details of the person with whom you had a pre implementation discussion, including (if necessary) details of why you chose the solution you did.

I'm going to reject this branch; please resubmit it with the above items included. If you want to have a pre-implementation discussion this afternoon I'll be happy to make myself available.

review: Needs Resubmitting

Revision history for this message

Didier Roche-Tolomelli (didrocks) wrote on 2010-03-09: Posted in a previous version of this proposal

#

Download full text (6.3 KiB)

The branch has been made in a pair programming session with jml, that's why I didn't added any more detail about it.
You can see inspiration and pair session programming on jml's gpg branch: http://bazaar.launchpad.net/~jml/launchpad/expose-gpgkeys-bug-389872/

The change is for exposing ssh key to the API needed for Quickly. We don't allow uploading or setting it directly yet (this will be an UDS discussion).

https://bugs.edge.launchpad.net/launchpad-registry/+bug/357235 is for the corresponding bug report. Again, we don't upload the ssh key/setting is right now.

output of make lint, I added also the testsuite call too:
$ make lint
utilities/shhh.py PYTHONPATH= python2.5 bootstrap.py\
--ez_setup-source=ez_setup.py \
--download-base=download-cache/dist --eggs=eggs
Enter passphrase for key '/home/ubuntu/.ssh/id_rsa':
= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/tests/test_sshkey.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/interfaces/ssh.py
  lib/lp/registry/stories/webservice/xx-person.txt

== Pyflakes notices ==

lib/lp/registry/interfaces/ssh.py
    19: 'export_read_operation' imported but unused
    19: 'export_as_webservice_collection' imported but unused
    19: 'operation_parameters' imported but unused
    19: 'collection_default_content' imported but unused
    19: 'operation_returns_collection_of' imported but unused

== Pylint notices ==

lib/lp/registry/interfaces/person.py
    520: [C0301] Line too long (80/78)
    53: [F0401] Unable to import 'lazr.enum' (No module named enum)
    54: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    55: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    56: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    63: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    410: [E1002, PersonNameField._validate] Use super on an old style class
    1404: [C0322, IPersonEditRestricted.addMember] Operator not preceded by a space
    status=copy_field(ITeamMembership['status']),
    ^
    comment=Text(required=False))
    @export_write_operation()
    def addMember(person, reviewer, status=TeamMembershipStatus.APPROVED,
    comment=None, force_team_add=False,
    may_subscribe_to_list=True):
    1445: [C0322, IPersonEditRestricted.acceptInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def acceptInvitationToBeMemberOf(team, comment):
    1457: [C0322, IPersonEditRestricted.declineInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def declineInvitationToBeMemberOf(team, comment):
    1755: [C0322, IPersonSet.newTeam] Operator not preceded by a space
    defaultmembershipperiod='default_membership_period',
    ^
    defaultrenewalperiod='default_renewal_period')
    @operation_parameters(
    subscriptionpolicy=Choice(
    title=_('Subs...

The branch has been made in a pair programming session with jml, that's why I didn't added any more detail about it.
You can see inspiration and pair session programming on jml's gpg branch: http://bazaar.launchpad.net/~jml/launchpad/expose-gpgkeys-bug-389872/

The change is for exposing ssh key to the API needed for Quickly. We don't allow uploading or setting it directly yet (this will be an UDS discussion).

https://bugs.edge.launchpad.net/launchpad-registry/+bug/357235 is for the corresponding bug report. Again, we don't upload the ssh key/setting is right now.

output of make lint, I added also the testsuite call too:
$ make lint
utilities/shhh.py PYTHONPATH= python2.5 bootstrap.py\
                --ez_setup-source=ez_setup.py \
		--download-base=download-cache/dist --eggs=eggs
Enter passphrase for key '/home/ubuntu/.ssh/id_rsa': 
= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/tests/test_sshkey.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/interfaces/ssh.py
  lib/lp/registry/stories/webservice/xx-person.txt

== Pyflakes notices ==

lib/lp/registry/interfaces/ssh.py
    19: 'export_read_operation' imported but unused
    19: 'export_as_webservice_collection' imported but unused
    19: 'operation_parameters' imported but unused
    19: 'collection_default_content' imported but unused
    19: 'operation_returns_collection_of' imported but unused

== Pylint notices ==

lib/lp/registry/interfaces/person.py
    520: [C0301] Line too long (80/78)
    53: [F0401] Unable to import 'lazr.enum' (No module named enum)
    54: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    55: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    56: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    63: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    410: [E1002, PersonNameField._validate] Use super on an old style class
    1404: [C0322, IPersonEditRestricted.addMember] Operator not preceded by a space
    status=copy_field(ITeamMembership['status']),
    ^
    comment=Text(required=False))
    @export_write_operation()
    def addMember(person, reviewer, status=TeamMembershipStatus.APPROVED,
    comment=None, force_team_add=False,
    may_subscribe_to_list=True):
    1445: [C0322, IPersonEditRestricted.acceptInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def acceptInvitationToBeMemberOf(team, comment):
    1457: [C0322, IPersonEditRestricted.declineInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def declineInvitationToBeMemberOf(team, comment):
    1755: [C0322, IPersonSet.newTeam] Operator not preceded by a space
    defaultmembershipperiod='default_membership_period',
    ^
    defaultrenewalperiod='default_renewal_period')
    @operation_parameters(
    subscriptionpolicy=Choice(
    title=_('Subscription policy'), vocabulary=TeamSubscriptionPolicy,
    required=False, default=TeamSubscriptionPolicy.MODERATED))
    @export_factory_operation(
    ITeam, ['name', 'displayname', 'teamdescription',
    'defaultmembershipperiod', 'defaultrenewalperiod'])
    def newTeam(teamowner, name, displayname, teamdescription=None,
    subscriptionpolicy=TeamSubscriptionPolicy.MODERATED,
    defaultmembershipperiod=None, defaultrenewalperiod=None):
    1824: [C0322, IPersonSet.findPerson] Operator not preceded by a space
    created_after=Datetime(
    ^
    title=_("Created after"), required=False),
    created_before=Datetime(
    title=_("Created before"), required=False),
    )
    @operation_returns_collection_of(IPerson)
    @export_read_operation()
    def findPerson(text="", exclude_inactive_accounts=True,
    must_have_email=False,
    created_after=None, created_before=None):

lib/lp/registry/interfaces/ssh.py
    18: [F0401] Unable to import 'lazr.enum' (No module named enum)
    19: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    19: [W0611] Unused import export_read_operation
    19: [W0611] Unused import operation_returns_collection_of
    19: [W0611] Unused import collection_default_content
    19: [W0611] Unused import operation_parameters
    19: [W0611] Unused import export_as_webservice_collection
ubuntu@ubuntu:~/launchpad/lp-branches/devel$ make^C
ubuntu@ubuntu:~/launchpad/lp-branches/devel$ ./bin/test -1cvvt webservice/xx-person.txt
Running tests at level 1
Running canonical.testing.layers.PageTestLayer tests:
  Set up canonical.testing.layers.BaseLayer in 0.048 seconds.
  Set up canonical.testing.layers.DatabaseLayer in 9.500 seconds.
  Set up canonical.testing.layers.LibrarianLayer in 31.812 seconds.
  Set up canonical.testing.layers.MemcachedLayer in 0.262 seconds.
  Set up canonical.testing.layers.LaunchpadLayer in 0.000 seconds.
  Set up canonical.testing.layers.FunctionalLayer in 8.642 seconds.
  Set up canonical.testing.layers.LaunchpadFunctionalLayer in 0.000 seconds.
  Set up canonical.testing.layers.GoogleServiceLayer in 5.243 seconds.
  Set up canonical.testing.layers.PageTestLayer in 0.007 seconds.
  Running:
 lib/lp/registry/tests/../stories/webservice/xx-person.txt
  Ran 45 tests with 0 failures and 0 errors in 16.329 seconds.
Tearing down left over layers:
  Tear down canonical.testing.layers.PageTestLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LaunchpadFunctionalLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LaunchpadLayer in 0.000 seconds.
  Tear down canonical.testing.layers.LibrarianLayer in 0.849 seconds.
  Tear down canonical.testing.layers.MemcachedLayer in 5.023 seconds.
  Tear down canonical.testing.layers.GoogleServiceLayer in 0.126 seconds.
  Tear down canonical.testing.layers.FunctionalLayer ... not supported
  Tear down canonical.testing.layers.DatabaseLayer in 0.078 seconds.
  Tear down canonical.testing.layers.BaseLayer in 0.000 seconds.

There are some import that can be cleaned, but it was the same on jml's branch and other interface. Not sure if you want to get that remove for futur extension easyness.

So, I'm resubmit the branch now :)

Revision history for this message

Graham Binns (gmb) wrote on 2010-03-09: Posted in a previous version of this proposal

#

Hi Didier, thanks for adding the details.

== Pyflakes notices ==

> lib/lp/registry/interfaces/ssh.py
> 19: 'export_read_operation' imported but unused
> 19: 'export_as_webservice_collection' imported but unused
> 19: 'operation_parameters' imported but unused
> 19: 'collection_default_content' imported but unused
> 19: 'operation_returns_collection_of' imported but unused
>
> == Pylint notices ==
>
> lib/lp/registry/interfaces/person.py
> 520: [C0301] Line too long (80/78)

You can fix these easily enough. You can pretty much ignore the rest;
Pylint produces a lot of noise and should be taken out and shot.

Other than that I'm happy with this branch. Let me know if you want me
to land it for you (I don't know whether it needs to be landed with
jml's work or not).

review: Approve (code)

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2010-03-09: Posted in a previous version of this proposal

#

On March 9, 2010, Didier Roche wrote:
> class ISSHKey(Interface):
> """SSH public key"""
> - id = Int(title=_("Database ID"), required=True, readonly=True)
> +
> + export_as_webservice_entry('ssh_key')
> +
> + id = exported(Int(title=_("Database ID"), required=True,
> readonly=True)) person = Int(title=_("Owner"), required=True,
> readonly=True)
> personID = Int(title=_('Owner ID'), required=True, readonly=True)
> - keytype = Choice(title=_("Key type"), required=True,
> - vocabulary=SSHKeyType)
> - keytext = TextLine(title=_("Key text"), required=True)
> - comment = TextLine(title=_("Comment describing this key"),
> - required=True)
> + keytype = exported(Choice(title=_("Key type"), required=True,
> + vocabulary=SSHKeyType))
> + keytext = exported(TextLine(title=_("Key text"), required=True))
> + comment = exported(TextLine(title=_("Comment describing this key"),
> + required=True))

These fields should all be exported as readonly=True.

--
Francis J. Lacoste
<email address hidden>

Revision history for this message

Francis J. Lacoste (flacoste) on 2010-03-09: Posted in a previous version of this proposal

#

review: Needs Fixing

Revision history for this message

Didier Roche-Tolomelli (didrocks) wrote on 2010-03-09: Posted in a previous version of this proposal

#

Download full text (3.5 KiB)

ok, those are fixed now. (readonly, uneeded import and too long lines). testsuite is still ok and here is the new make lint output:

$ make lint
utilities/shhh.py PYTHONPATH= python2.5 bootstrap.py\
--ez_setup-source=ez_setup.py \
--download-base=download-cache/dist --eggs=eggs
Enter passphrase for key '/home/ubuntu/.ssh/id_rsa':
Enter passphrase for key '/home/ubuntu/.ssh/id_rsa':
= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/tests/test_sshkey.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/interfaces/ssh.py
  lib/lp/registry/stories/webservice/xx-person.txt

== Pylint notices ==

lib/lp/registry/interfaces/person.py
    53: [F0401] Unable to import 'lazr.enum' (No module named enum)
    54: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    55: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    56: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    63: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    410: [E1002, PersonNameField._validate] Use super on an old style class
    1404: [C0322, IPersonEditRestricted.addMember] Operator not preceded by a space
    status=copy_field(ITeamMembership['status']),
    ^
    comment=Text(required=False))
    @export_write_operation()
    def addMember(person, reviewer, status=TeamMembershipStatus.APPROVED,
    comment=None, force_team_add=False,
    may_subscribe_to_list=True):
    1445: [C0322, IPersonEditRestricted.acceptInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def acceptInvitationToBeMemberOf(team, comment):
    1457: [C0322, IPersonEditRestricted.declineInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def declineInvitationToBeMemberOf(team, comment):
    1755: [C0322, IPersonSet.newTeam] Operator not preceded by a space
    defaultmembershipperiod='default_membership_period',
    ^
    defaultrenewalperiod='default_renewal_period')
    @operation_parameters(
    subscriptionpolicy=Choice(
    title=_('Subscription policy'), vocabulary=TeamSubscriptionPolicy,
    required=False, default=TeamSubscriptionPolicy.MODERATED))
    @export_factory_operation(
    ITeam, ['name', 'displayname', 'teamdescription',
    'defaultmembershipperiod', 'defaultrenewalperiod'])
    def newTeam(teamowner, name, displayname, teamdescription=None,
    subscriptionpolicy=TeamSubscriptionPolicy.MODERATED,
    defaultmembershipperiod=None, defaultrenewalperiod=None):
    1824: [C0322, IPersonSet.findPerson] Operator not preceded by a space
    created_after=Datetime(
    ^
    title=_("Created after"), required=False),
    created_before=Datetime(
    title=_("Created before"), required=False),
    )
    @operation_returns_collection_of(IPerson)
    @export_read_operation()
    def findPerson(text="", exclude_inactive_accounts=True,
    must...

ok, those are fixed now. (readonly, uneeded import and too long lines). testsuite is still ok and here is the new make lint output:

$  make lint
utilities/shhh.py PYTHONPATH= python2.5 bootstrap.py\
                --ez_setup-source=ez_setup.py \
		--download-base=download-cache/dist --eggs=eggs
Enter passphrase for key '/home/ubuntu/.ssh/id_rsa': 
Enter passphrase for key '/home/ubuntu/.ssh/id_rsa': 
= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/tests/test_sshkey.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/interfaces/ssh.py
  lib/lp/registry/stories/webservice/xx-person.txt

== Pylint notices ==

lib/lp/registry/interfaces/person.py
    53: [F0401] Unable to import 'lazr.enum' (No module named enum)
    54: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    55: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    56: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    63: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    410: [E1002, PersonNameField._validate] Use super on an old style class
    1404: [C0322, IPersonEditRestricted.addMember] Operator not preceded by a space
    status=copy_field(ITeamMembership['status']),
    ^
    comment=Text(required=False))
    @export_write_operation()
    def addMember(person, reviewer, status=TeamMembershipStatus.APPROVED,
    comment=None, force_team_add=False,
    may_subscribe_to_list=True):
    1445: [C0322, IPersonEditRestricted.acceptInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def acceptInvitationToBeMemberOf(team, comment):
    1457: [C0322, IPersonEditRestricted.declineInvitationToBeMemberOf] Operator not preceded by a space
    comment=Text())
    ^
    @export_write_operation()
    def declineInvitationToBeMemberOf(team, comment):
    1755: [C0322, IPersonSet.newTeam] Operator not preceded by a space
    defaultmembershipperiod='default_membership_period',
    ^
    defaultrenewalperiod='default_renewal_period')
    @operation_parameters(
    subscriptionpolicy=Choice(
    title=_('Subscription policy'), vocabulary=TeamSubscriptionPolicy,
    required=False, default=TeamSubscriptionPolicy.MODERATED))
    @export_factory_operation(
    ITeam, ['name', 'displayname', 'teamdescription',
    'defaultmembershipperiod', 'defaultrenewalperiod'])
    def newTeam(teamowner, name, displayname, teamdescription=None,
    subscriptionpolicy=TeamSubscriptionPolicy.MODERATED,
    defaultmembershipperiod=None, defaultrenewalperiod=None):
    1824: [C0322, IPersonSet.findPerson] Operator not preceded by a space
    created_after=Datetime(
    ^
    title=_("Created after"), required=False),
    created_before=Datetime(
    title=_("Created before"), required=False),
    )
    @operation_returns_collection_of(IPerson)
    @export_read_operation()
    def findPerson(text="", exclude_inactive_accounts=True,
    must_have_email=False,
    created_after=None, created_before=None):

lib/lp/registry/interfaces/ssh.py
    18: [F0401] Unable to import 'lazr.enum' (No module named enum)
    19: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)

The changes are independent from jml's change. So if you can merge it, it would rocks! Thanks a lot!

Revision history for this message

Karl Fogel (kfogel) wrote on 2010-03-24: Posted in a previous version of this proposal

#

I'm not a reviewer, but this looks pretty easy to review (and at least on the surface the change seems correct to me). The formatting fix at @@ -519,8 +520,8 @@ is unrelated -- personally, I find those distracting in a branch containing functional changes, but YMMV.

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2010-03-24: Posted in a previous version of this proposal

#

Download full text (5.9 KiB)

Your test is not working yet, but we are getting there!

> === modified file 'lib/lp/registry/browser/configure.zcml'
> --- lib/lp/registry/browser/configure.zcml 2010-03-08 01:51:58 +0000
> +++ lib/lp/registry/browser/configure.zcml 2010-03-09 19:30:49 +0000
> @@ -2172,4 +2172,9 @@
> classes="
> PersonProductFacets"
> module="lp.registry.browser.personproduct"/>
> + <browser:url
> + for="lp.registry.interfaces.ssh.ISSHKey"
> + path_expression="string:+ssh-keys/${id}"
> + rootsite="api"
> + attribute_to_parent="person" />
> </configure>

We usually refrain from exposing DB id publically. In URLs, especially. We
have a couple of exception and since there is no alternative here, I think
it's fine.

> === modified file 'lib/lp/registry/interfaces/person.py'
> --- lib/lp/registry/interfaces/person.py 2010-03-05 14:50:47 +0000

>
> oauth_request_tokens = Attribute(_("Non-expired request tokens"))
>
> - sshkeys = Attribute(_('List of SSH keys'))
> + sshkeys = exported(
> + CollectionField(
> + title= _('List of SSH keys'),
> + readonly=False, required=False,
> + value_type=Reference(schema=ISSHKey)))

Can we rename that to ssh_keys? Unfortunately, you can't use exported_as
because of bug 546324. I think there isn't that many call sites, but feel free
to push back if this is too daunting a task.

> === modified file 'lib/lp/registry/interfaces/ssh.py'
> --- lib/lp/registry/interfaces/ssh.py 2009-06-25 04:06:00 +0000
> +++ lib/lp/registry/interfaces/ssh.py 2010-03-09 19:30:49 +0000
> @@ -16,6 +16,7 @@
> from zope.schema import Choice, Int, TextLine
> from zope.interface import Interface
> from lazr.enum import DBEnumeratedType, DBItem
> +from lazr.restful.declarations import (export_as_webservice_entry, exported)
>
> from canonical.launchpad import _
>
> @@ -42,14 +43,18 @@
>
> class ISSHKey(Interface):
> """SSH public key"""
> - id = Int(title=_("Database ID"), required=True, readonly=True)
> +
> + export_as_webservice_entry('ssh_key')
> +
> + id = exported(Int(title=_("Database ID"), required=True, readonly=True))
> person = Int(title=_("Owner"), required=True, readonly=True)

We don't want to export the DB id. It's not useful at all. I know it's leaked
into the URL, but that's an artefact.

> personID = Int(title=_('Owner ID'), required=True, readonly=True)
> - keytype = Choice(title=_("Key type"), required=True,
> - vocabulary=SSHKeyType)
> - keytext = TextLine(title=_("Key text"), required=True)
> - comment = TextLine(title=_("Comment describing this key"),
> - required=True)
> + keytype = exported(Choice(title=_("Key type"), required=True,
> + vocabulary=SSHKeyType, readonly=True))
> + keytext = exported(TextLine(title=_("Key text"), required=True,
> + readonly=True))
> + comment = exported(TextLine(title=_("Comment describing this key"),
> + required=True, readonly=True))

> === modified file 'lib/lp/registry/stories/webservice/xx-person.txt'

> +== SSH keys ===
> +
> +People have SSH keys which we can manipulate over the API.
> +
> +The sample person "name12" doesn't have any keys to begin with:
> +
> + ...

Your test is not working yet, but we are getting there!

> === modified file 'lib/lp/registry/browser/configure.zcml'
> --- lib/lp/registry/browser/configure.zcml	2010-03-08 01:51:58 +0000
> +++ lib/lp/registry/browser/configure.zcml	2010-03-09 19:30:49 +0000
> @@ -2172,4 +2172,9 @@
>          classes="
>              PersonProductFacets"
>          module="lp.registry.browser.personproduct"/>
> +    <browser:url
> +        for="lp.registry.interfaces.ssh.ISSHKey"
> +        path_expression="string:+ssh-keys/${id}"
> +        rootsite="api"
> +        attribute_to_parent="person" />
>  </configure>

We usually refrain from exposing DB id publically. In URLs, especially. We
have a couple of exception and since there is no alternative here, I think
it's fine.

> === modified file 'lib/lp/registry/interfaces/person.py'
> --- lib/lp/registry/interfaces/person.py	2010-03-05 14:50:47 +0000

>  
>      oauth_request_tokens = Attribute(_("Non-expired request tokens"))
>  
> -    sshkeys = Attribute(_('List of SSH keys'))
> +    sshkeys = exported(
> +             CollectionField(
> +                title= _('List of SSH keys'),
> +                readonly=False, required=False,
> +                value_type=Reference(schema=ISSHKey)))

Can we rename that to ssh_keys? Unfortunately, you can't use exported_as
because of bug 546324. I think there isn't that many call sites, but feel free
to push back if this is too daunting a task.

> === modified file 'lib/lp/registry/interfaces/ssh.py'
> --- lib/lp/registry/interfaces/ssh.py 2009-06-25 04:06:00 +0000
> +++ lib/lp/registry/interfaces/ssh.py 2010-03-09 19:30:49 +0000
> @@ -16,6 +16,7 @@
> from zope.schema import Choice, Int, TextLine
> from zope.interface import Interface
> from lazr.enum import DBEnumeratedType, DBItem
> +from lazr.restful.declarations import (export_as_webservice_entry, exported)
> 
> from canonical.launchpad import _
> 
> @@ -42,14 +43,18 @@
> 
> class ISSHKey(Interface):
> """SSH public key"""
> - id = Int(title=_("Database ID"), required=True, readonly=True)
> +
> + export_as_webservice_entry('ssh_key')
> +
> + id = exported(Int(title=_("Database ID"), required=True, readonly=True))
> person = Int(title=_("Owner"), required=True, readonly=True)

We don't want to export the DB id. It's not useful at all. I know it's leaked
into the URL, but that's an artefact.

> personID = Int(title=_('Owner ID'), required=True, readonly=True)
> - keytype = Choice(title=_("Key type"), required=True,
> - vocabulary=SSHKeyType)
> - keytext = TextLine(title=_("Key text"), required=True)
> - comment = TextLine(title=_("Comment describing this key"),
> - required=True)
> + keytype = exported(Choice(title=_("Key type"), required=True,
> + vocabulary=SSHKeyType, readonly=True))
> + keytext = exported(TextLine(title=_("Key text"), required=True,
> + readonly=True))
> + comment = exported(TextLine(title=_("Comment describing this key"),
> + required=True, readonly=True))

> === modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
 
> +== SSH keys ===
> +
> +People have SSH keys which we can manipulate over the API.
> +
> +The sample person "name12" doesn't have any keys to begin with:
> +
> +    >>> sample_person = webservice.get("/~name12").jsonBody()
> +    >>> sshkeys = sample_person['sshkeys_collection_link']
> +    >>> print sshkeys
> +    http://.../~name12/sshkeys
> +    >>> print_self_link_of_entries(webservice.get(sshkeys).jsonBody())
> +    http://.../~name12/+ssh-keys/1

It seems it has a key! (You say it doesn't) You should also ellide the DB id
in the URL.

You probably want to use no-priv here who shouldn't have any key to begin
with.

> +
> +Let's give "name12" a key via the back door of our internal Python APIs:
> +
> +    >>> from zope.component import getUtility
> +    >>> from lp.registry.interfaces.person import IPersonSet
> +    >>> login(ANONYMOUS)
> +    >>> ssh_user = getUtility(IPersonSet).getByName('name12')
> +    >>> ssh_key = factory.makeGPGKey(ssh_user)

You are creating a GPG key here, not a SSH key. (makeSSHKey is what you want
to use)

> +    >>> logout()
> +
> +Now when we get the sshkey collection for 'name12' again, the key should show
> +up:
> +
> +    >>> keys = webservice.get(sshkeys).jsonBody()
> +    >>> print_self_link_of_entries(keys)
> +    http://.../~name12/+ssh-keys/1
> +

I would expect a new key here, not the one you showed previously.

> +
> +And then we can actually retrieve the key:
> +
> +    >>> pprint_entry(keys['entries'][0])
> +    comment: u'andrew@trogdor'
> +    id: 1
> +    keytext: u'AAAAB3NzaC1kc3MAAAEBAPfhCA15ZaT08brwVXwpJjcZT6QFIipzF1sGy57HY7QPi/W+uljr1VcCHzWdlSmda7YpTCTx0NFYYQIccQRGX6zYL8v1w9FSRCAnxxUJmqEhsUDFYFdVTa9uLCrs3MSbmh7wwFPdRrGrO6X5x7T4dMZQwykSZrOVdpLcCHRgrMZslLomIAjERn6OAQNiGFz7B2tEi/3Soqd52bGJwOtGymRiAXkPSLbH7KfzSCe34ytdh6BD+4SrgSoa+TL3VDV70QAdlOFXD42ZHl3Sc0Tde4LbZeYq2Uf84DOATLZBbOYpRSqTLkM9XngpnvCRVb6dxEQfgODDw783tEuPpySLj2EAAAAVANpUVgivDjt9gFibN/AXfYy1meeBAAABAB6FtnMywmWZg2lr2I3nDfE5U5QbGUQB/ZEP98ZkSkhOcF29VlnGOxyb2/VZbVTLa/btlPF82L4An/c8VKtKZnel7LnAlMoArdgzQNXGVQQVtnaWwM26ydgDzkSSIes3elNZgsfnPRBvaF0ol9Tqju0rNGKjnr3ZOX/NX+42bxpjRnxYj1h56yP2jKKeGfjorI6JK1YfqBAiTxzaDMzSpknnrbztaKJoh7IFqMMOp9ANSFh7H106pEaCv3ebCTJZprtWqNKjb2zum7OQPRz3upA0qx22ocTokjv4itXJ6yj/BvGu9qdOIQFXuB2rsFtLZtS8ATueOly0GzyeiZBx/AEAAAEBAO8jRYjL7tAYnVlO1p6UzPOicAuGCFWfNbBEDRAXoSgLNdj451jStw+eUc9ZVz7tG/XRVZsiavtFHb2cbrcfX1YOd69xi0m+IY6mo3yKt3irQRokDtt376sHoUdHgj2ozySZJgG8IJndtoS+VQQy6NdClA3fNFb96bF865eNaRYoHJO9ZI84lkWQL++MLzIuyFfCs1hSlapyyuHC8kFmF7AQdrVZvbohSbnWs+w53nIW8nAA7z21wAukvE1Pl6AQyG0e7U1sYS8Pc8dtmzJvdtVZWBl02/gqQJ7f06mFvnsN45rR1Uyxnrwl6rbFwqabZDlyD5Ac6Icbvz9SG1gBOiI='
> +    keytype: u'DSA'
> +    resource_type_link: u'http://.../#ssh_key'
> +    self_link: u'http://.../~name12/+ssh-keys/1'

You'll want to ellide the DB id in the resulting URL. Probably also the
keytext which should be random. You probably want to modify the makeSSH
factory method to take a fixed comment field. That way you could show it in
the output.

review: Needs Fixing

Revision history for this message

Didier Roche-Tolomelli (didrocks) wrote on 2010-03-24:

#

Thanks for your review!

Sorry for the test errors. Let's blame being tired about creating a GPG key and listing the existing ssh one :)
That's fixed in creating a new user, as "no-priv" had also an ssh key :)

In fact, keytext and keycomment aren't random (there is just a number appended at the end of generic-stringX. I ellided the number and so, don't thing we need to add a specific comment in makeSSHKey
I removed as well the exposal of id in ssh.py.

"Can we rename that to ssh_keys? Unfortunately, you can't use exported_as
because of bug 546324. I think there isn't that many call sites, but feel free
to push back if this is too daunting a task."
I'm more concerned about this one to be out of my poor knowledge of launchpad. I knew exported_as but as it seems to be broken with CollectionField… To be honest I'm a little bit scared on breaking everything in renaming it as Launchpad is so big :)

Don't hesitate to report any further issue. Thanks again for reviewing this.

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2010-03-24:

#

OK, that's fine.

review: Approve

 === modified file 'lib/lp/registry/browser/configure.zcml'
 --- lib/lp/registry/browser/configure.zcml	2010-03-11 16:48:37 +0000
 +++ lib/lp/registry/browser/configure.zcml	2010-03-25 08:36:44 +0000
@@ -2174,6 +2174,12 @@
          module="lp.registry.browser.personproduct"/>
      <browser:url
++        for="lp.registry.interfaces.ssh.ISSHKey"
++        path_expression="string:+ssh-keys/${id}"
++        rootsite="api"
++        attribute_to_parent="person" />
++
++    <browser:url
          for="lp.registry.interfaces.gpg.IGPGKey"
          path_expression="string:+gpg-keys/${keyid}"
          rootsite="api"
 === added file 'lib/lp/registry/browser/tests/test_sshkey.py'
 --- lib/lp/registry/browser/tests/test_sshkey.py	1970-01-01 00:00:00 +0000
 +++ lib/lp/registry/browser/tests/test_sshkey.py	2010-03-25 08:36:44 +0000
@@ -0,0 +1,31 @@
++# Copyright 2010 Canonical Ltd.  This software is licensed under the
++# GNU Affero General Public License version 3 (see the file LICENSE).
++
++"""Tests for GPG key on the web."""
++
++__metaclass__ = type
++
++import unittest
++
++from canonical.launchpad.webapp import canonical_url
++from canonical.testing.layers import DatabaseFunctionalLayer
++from lp.testing import TestCaseWithFactory
++
++
++class TestCanonicalUrl(TestCaseWithFactory):
++
++    layer = DatabaseFunctionalLayer
++
++    def test_canonical_url(self):
++        # The canonical URL of a GPG key is ssh-keys
++        person = self.factory.makePerson()
++        sshkey = self.factory.makeSSHKey(person)
++        self.assertEqual(
++            '%s/+ssh-keys/%s' % (
++                canonical_url(person, rootsite='api'), sshkey.id),
++            canonical_url(sshkey))
++
++
++def test_suite():
++    return unittest.TestLoader().loadTestsFromName(__name__)
++
 === modified file 'lib/lp/registry/interfaces/person.py'
 --- lib/lp/registry/interfaces/person.py	2010-03-11 20:59:17 +0000
 +++ lib/lp/registry/interfaces/person.py	2010-03-25 08:36:44 +0000
@@ -94,6 +94,7 @@
  from lp.registry.interfaces.mailinglistsubscription import (
      MailingListAutoSubscribePolicy)
  from lp.registry.interfaces.mentoringoffer import IHasMentoringOffers
++from lp.registry.interfaces.ssh import ISSHKey
  from lp.registry.interfaces.teammembership import (
      ITeamMembership, ITeamParticipation, TeamMembershipStatus)
  from lp.registry.interfaces.wikiname import IWikiName
@@ -520,8 +521,8 @@
              description=_(
                  "An image of exactly 64x64 pixels that will be displayed in "
                  "the heading of all pages related to you. Traditionally this "
--                "is a logo, a small picture or a personal mascot. It should be "
--                "no bigger than 50kb in size.")))
++                "is a logo, a small picture or a personal mascot. It should "
++                "be no bigger than 50kb in size.")))
      logoID = Int(title=_('Logo ID'), required=True, readonly=True)
      mugshot = exported(MugshotImageUpload(
@@ -606,7 +607,11 @@
      oauth_request_tokens = Attribute(_("Non-expired request tokens"))
--    sshkeys = Attribute(_('List of SSH keys'))
++    sshkeys = exported(
++             CollectionField(
++                title= _('List of SSH keys'),
++                readonly=False, required=False,
++                value_type=Reference(schema=ISSHKey)))
      account_status = Choice(
          title=_("The status of this person's account"), required=False,
 === modified file 'lib/lp/registry/interfaces/ssh.py'
 --- lib/lp/registry/interfaces/ssh.py	2009-06-25 04:06:00 +0000
 +++ lib/lp/registry/interfaces/ssh.py	2010-03-25 08:36:44 +0000
@@ -16,6 +16,7 @@
  from zope.schema import Choice, Int, TextLine
  from zope.interface import Interface
  from lazr.enum import DBEnumeratedType, DBItem
++from lazr.restful.declarations import (export_as_webservice_entry, exported)
  from canonical.launchpad import _
@@ -42,14 +43,18 @@
  class ISSHKey(Interface):
      """SSH public key"""
++
++    export_as_webservice_entry('ssh_key')
++
      id = Int(title=_("Database ID"), required=True, readonly=True)
      person = Int(title=_("Owner"), required=True, readonly=True)
      personID = Int(title=_('Owner ID'), required=True, readonly=True)
--    keytype = Choice(title=_("Key type"), required=True,
--                     vocabulary=SSHKeyType)
--    keytext = TextLine(title=_("Key text"), required=True)
--    comment = TextLine(title=_("Comment describing this key"),
--                       required=True)
++    keytype = exported(Choice(title=_("Key type"), required=True,
++                     vocabulary=SSHKeyType, readonly=True))
++    keytext = exported(TextLine(title=_("Key text"), required=True,
++                       readonly=True))
++    comment = exported(TextLine(title=_("Comment describing this key"),
++                       required=True, readonly=True))
      def destroySelf():
          """Remove this SSHKey from the database."""
 === modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
 --- lib/lp/registry/stories/webservice/xx-person.txt	2010-03-13 00:32:40 +0000
 +++ lib/lp/registry/stories/webservice/xx-person.txt	2010-03-25 08:36:44 +0000
@@ -46,6 +46,7 @@
      proposed_members_collection_link: u'http://.../~salgado/proposed_members'
      resource_type_link: u'http://.../#person'
      self_link: u'http://.../~salgado'
++    sshkeys_collection_link: u'http://.../~salgado/sshkeys'
      sub_teams_collection_link: u'http://.../~salgado/sub_teams'
      super_teams_collection_link: u'http://.../~salgado/super_teams'
      team_owner_link: None
@@ -96,6 +97,7 @@
      renewal_policy: u'invite them to apply for renewal'
      resource_type_link: u'http://.../#team'
      self_link: u'http://.../~ubuntu-team'
++    sshkeys_collection_link: u'http://.../~ubuntu-team/sshkeys'
      sub_teams_collection_link: u'http://.../~ubuntu-team/sub_teams'
      subscription_policy: u'Moderated Team'
      super_teams_collection_link: u'http://.../~ubuntu-team/super_teams'
@@ -151,6 +153,46 @@
      HTTP/1.1 404 Not Found
      ...
++== SSH keys ===
++
++People have SSH keys which we can manipulate over the API.
++
++The sample person "ssh-user" doesn't have any keys to begin with:
++
++    >>> login('test@canonical.com')
++    >>> person = factory.makePerson(name="ssh-user")
++    >>> logout()
++    >>> sample_person = webservice.get("/~ssh-user").jsonBody()
++    >>> sshkeys = sample_person['sshkeys_collection_link']
++    >>> print sshkeys
++    http://.../~ssh-user/sshkeys
++    >>> print_self_link_of_entries(webservice.get(sshkeys).jsonBody())
++
++Let's give "ssh-user" a key via the back door of our internal Python APIs:
++
++    >>> from zope.component import getUtility
++    >>> from lp.registry.interfaces.person import IPersonSet
++    >>> login(ANONYMOUS)
++    >>> ssh_user = getUtility(IPersonSet).getByName('ssh-user')
++    >>> ssh_key = factory.makeSSHKey(ssh_user)
++    >>> logout()
++
++Now when we get the sshkey collection for 'sssh-user' again, the key should show
++up:
++
++    >>> keys = webservice.get(sshkeys).jsonBody()
++    >>> print_self_link_of_entries(keys)
++    http://.../~ssh-user/+ssh-keys/...
++
++
++And then we can actually retrieve the key:
++
++    >>> pprint_entry(keys['entries'][0])
++    comment: u'generic-string...'
++    keytext: u'generic-string...'
++    keytype: u'RSA'
++    resource_type_link: u'http://.../#ssh_key'
++    self_link: u'http://.../~ssh-user/+ssh-keys/...'
  === GPG keys ===
@@ -178,7 +220,7 @@
      >>> keys = webservice.get(gpgkeys).jsonBody()
      >>> print_self_link_of_entries(keys)
--    http://api.launchpad.dev/beta/~name12/+gpg-keys/00000001
++    http://.../~name12/+gpg-keys/...
  And then we can actually retrieve the key:

Launchpad itself

Merge lp:~didrocks/launchpad/expose-sshkeys-bug-357235 into lp:launchpad/db-devel

Commit message

Description of the change

Preview Diff

Subscribers