Launchpad itself

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

  1. expose-sshkeys-bug-357235
  2. Merge into db-devel
Proposed by Didier Roche-Tolomelli
Status: Superseded
Proposed branch: lp:~didrocks/launchpad/expose-sshkeys-bug-357235
Merge into: lp:launchpad/db-devel
Diff against target: 187 lines (+96/-9)
5 files modified
lib/lp/registry/browser/configure.zcml (+5/-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 (+11/-6)
lib/lp/registry/stories/webservice/xx-person.txt (+41/-0)
To merge this branch: bzr merge lp:~didrocks/launchpad/expose-sshkeys-bug-357235
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) Needs Fixing
Graham Binns (community) code Approve
Review via email: mp+20975@code.launchpad.net

This proposal supersedes a proposal from 2010-03-05.

This proposal has been superseded by a proposal from 2010-03-09.

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) wrote : 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 : 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...

Read more...

Revision history for this message
Graham Binns (gmb) wrote :

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 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) :
review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/configure.zcml'
2--- lib/lp/registry/browser/configure.zcml 2010-03-08 01:51:58 +0000
3+++ lib/lp/registry/browser/configure.zcml 2010-03-09 19:29:35 +0000
4@@ -2172,4 +2172,9 @@
5 classes="
6 PersonProductFacets"
7 module="lp.registry.browser.personproduct"/>
8+ <browser:url
9+ for="lp.registry.interfaces.ssh.ISSHKey"
10+ path_expression="string:+ssh-keys/${id}"
11+ rootsite="api"
12+ attribute_to_parent="person" />
13 </configure>
14
15=== added file 'lib/lp/registry/browser/tests/test_sshkey.py'
16--- lib/lp/registry/browser/tests/test_sshkey.py 1970-01-01 00:00:00 +0000
17+++ lib/lp/registry/browser/tests/test_sshkey.py 2010-03-09 19:29:35 +0000
18@@ -0,0 +1,31 @@
19+# Copyright 2010 Canonical Ltd. This software is licensed under the
20+# GNU Affero General Public License version 3 (see the file LICENSE).
21+
22+"""Tests for GPG key on the web."""
23+
24+__metaclass__ = type
25+
26+import unittest
27+
28+from canonical.launchpad.webapp import canonical_url
29+from canonical.testing.layers import DatabaseFunctionalLayer
30+from lp.testing import TestCaseWithFactory
31+
32+
33+class TestCanonicalUrl(TestCaseWithFactory):
34+
35+ layer = DatabaseFunctionalLayer
36+
37+ def test_canonical_url(self):
38+ # The canonical URL of a GPG key is ssh-keys
39+ person = self.factory.makePerson()
40+ sshkey = self.factory.makeSSHKey(person)
41+ self.assertEqual(
42+ '%s/+ssh-keys/%s' % (
43+ canonical_url(person, rootsite='api'), sshkey.id),
44+ canonical_url(sshkey))
45+
46+
47+def test_suite():
48+ return unittest.TestLoader().loadTestsFromName(__name__)
49+
50
51=== modified file 'lib/lp/registry/interfaces/person.py'
52--- lib/lp/registry/interfaces/person.py 2010-03-05 14:50:47 +0000
53+++ lib/lp/registry/interfaces/person.py 2010-03-09 19:29:35 +0000
54@@ -93,6 +93,7 @@
55 from lp.registry.interfaces.mailinglistsubscription import (
56 MailingListAutoSubscribePolicy)
57 from lp.registry.interfaces.mentoringoffer import IHasMentoringOffers
58+from lp.registry.interfaces.ssh import ISSHKey
59 from lp.registry.interfaces.teammembership import (
60 ITeamMembership, ITeamParticipation, TeamMembershipStatus)
61 from lp.registry.interfaces.wikiname import IWikiName
62@@ -519,8 +520,8 @@
63 description=_(
64 "An image of exactly 64x64 pixels that will be displayed in "
65 "the heading of all pages related to you. Traditionally this "
66- "is a logo, a small picture or a personal mascot. It should be "
67- "no bigger than 50kb in size.")))
68+ "is a logo, a small picture or a personal mascot. It should "
69+ "be no bigger than 50kb in size.")))
70 logoID = Int(title=_('Logo ID'), required=True, readonly=True)
71
72 mugshot = exported(MugshotImageUpload(
73@@ -605,7 +606,11 @@
74
75 oauth_request_tokens = Attribute(_("Non-expired request tokens"))
76
77- sshkeys = Attribute(_('List of SSH keys'))
78+ sshkeys = exported(
79+ CollectionField(
80+ title= _('List of SSH keys'),
81+ readonly=False, required=False,
82+ value_type=Reference(schema=ISSHKey)))
83
84 account_status = Choice(
85 title=_("The status of this person's account"), required=False,
86
87=== modified file 'lib/lp/registry/interfaces/ssh.py'
88--- lib/lp/registry/interfaces/ssh.py 2009-06-25 04:06:00 +0000
89+++ lib/lp/registry/interfaces/ssh.py 2010-03-09 19:29:35 +0000
90@@ -16,6 +16,7 @@
91 from zope.schema import Choice, Int, TextLine
92 from zope.interface import Interface
93 from lazr.enum import DBEnumeratedType, DBItem
94+from lazr.restful.declarations import (export_as_webservice_entry, exported)
95
96 from canonical.launchpad import _
97
98@@ -42,14 +43,18 @@
99
100 class ISSHKey(Interface):
101 """SSH public key"""
102- id = Int(title=_("Database ID"), required=True, readonly=True)
103+
104+ export_as_webservice_entry('ssh_key')
105+
106+ id = exported(Int(title=_("Database ID"), required=True, readonly=True))
107 person = Int(title=_("Owner"), required=True, readonly=True)
108 personID = Int(title=_('Owner ID'), required=True, readonly=True)
109- keytype = Choice(title=_("Key type"), required=True,
110- vocabulary=SSHKeyType)
111- keytext = TextLine(title=_("Key text"), required=True)
112- comment = TextLine(title=_("Comment describing this key"),
113- required=True)
114+ keytype = exported(Choice(title=_("Key type"), required=True,
115+ vocabulary=SSHKeyType, readonly=True))
116+ keytext = exported(TextLine(title=_("Key text"), required=True,
117+ readonly=True))
118+ comment = exported(TextLine(title=_("Comment describing this key"),
119+ required=True, readonly=True))
120
121 def destroySelf():
122 """Remove this SSHKey from the database."""
123
124=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
125--- lib/lp/registry/stories/webservice/xx-person.txt 2010-02-24 16:53:30 +0000
126+++ lib/lp/registry/stories/webservice/xx-person.txt 2010-03-09 19:29:35 +0000
127@@ -44,6 +44,7 @@
128 proposed_members_collection_link: u'http://.../~salgado/proposed_members'
129 resource_type_link: u'http://.../#person'
130 self_link: u'http://.../~salgado'
131+ sshkeys_collection_link: u'http://.../~salgado/sshkeys'
132 sub_teams_collection_link: u'http://.../~salgado/sub_teams'
133 super_teams_collection_link: u'http://.../~salgado/super_teams'
134 team_owner_link: None
135@@ -92,6 +93,7 @@
136 renewal_policy: u'invite them to apply for renewal'
137 resource_type_link: u'http://.../#team'
138 self_link: u'http://.../~ubuntu-team'
139+ sshkeys_collection_link: u'http://.../~ubuntu-team/sshkeys'
140 sub_teams_collection_link: u'http://.../~ubuntu-team/sub_teams'
141 subscription_policy: u'Moderated Team'
142 super_teams_collection_link: u'http://.../~ubuntu-team/super_teams'
143@@ -147,6 +149,45 @@
144 HTTP/1.1 404 Not Found
145 ...
146
147+== SSH keys ===
148+
149+People have SSH keys which we can manipulate over the API.
150+
151+The sample person "name12" doesn't have any keys to begin with:
152+
153+ >>> sample_person = webservice.get("/~name12").jsonBody()
154+ >>> sshkeys = sample_person['sshkeys_collection_link']
155+ >>> print sshkeys
156+ http://.../~name12/sshkeys
157+ >>> print_self_link_of_entries(webservice.get(sshkeys).jsonBody())
158+ http://.../~name12/+ssh-keys/1
159+
160+Let's give "name12" a key via the back door of our internal Python APIs:
161+
162+ >>> from zope.component import getUtility
163+ >>> from lp.registry.interfaces.person import IPersonSet
164+ >>> login(ANONYMOUS)
165+ >>> ssh_user = getUtility(IPersonSet).getByName('name12')
166+ >>> ssh_key = factory.makeGPGKey(ssh_user)
167+ >>> logout()
168+
169+Now when we get the sshkey collection for 'name12' again, the key should show
170+up:
171+
172+ >>> keys = webservice.get(sshkeys).jsonBody()
173+ >>> print_self_link_of_entries(keys)
174+ http://.../~name12/+ssh-keys/1
175+
176+
177+And then we can actually retrieve the key:
178+
179+ >>> pprint_entry(keys['entries'][0])
180+ comment: u'andrew@trogdor'
181+ id: 1
182+ keytext: u'AAAAB3NzaC1kc3MAAAEBAPfhCA15ZaT08brwVXwpJjcZT6QFIipzF1sGy57HY7QPi/W+uljr1VcCHzWdlSmda7YpTCTx0NFYYQIccQRGX6zYL8v1w9FSRCAnxxUJmqEhsUDFYFdVTa9uLCrs3MSbmh7wwFPdRrGrO6X5x7T4dMZQwykSZrOVdpLcCHRgrMZslLomIAjERn6OAQNiGFz7B2tEi/3Soqd52bGJwOtGymRiAXkPSLbH7KfzSCe34ytdh6BD+4SrgSoa+TL3VDV70QAdlOFXD42ZHl3Sc0Tde4LbZeYq2Uf84DOATLZBbOYpRSqTLkM9XngpnvCRVb6dxEQfgODDw783tEuPpySLj2EAAAAVANpUVgivDjt9gFibN/AXfYy1meeBAAABAB6FtnMywmWZg2lr2I3nDfE5U5QbGUQB/ZEP98ZkSkhOcF29VlnGOxyb2/VZbVTLa/btlPF82L4An/c8VKtKZnel7LnAlMoArdgzQNXGVQQVtnaWwM26ydgDzkSSIes3elNZgsfnPRBvaF0ol9Tqju0rNGKjnr3ZOX/NX+42bxpjRnxYj1h56yP2jKKeGfjorI6JK1YfqBAiTxzaDMzSpknnrbztaKJoh7IFqMMOp9ANSFh7H106pEaCv3ebCTJZprtWqNKjb2zum7OQPRz3upA0qx22ocTokjv4itXJ6yj/BvGu9qdOIQFXuB2rsFtLZtS8ATueOly0GzyeiZBx/AEAAAEBAO8jRYjL7tAYnVlO1p6UzPOicAuGCFWfNbBEDRAXoSgLNdj451jStw+eUc9ZVz7tG/XRVZsiavtFHb2cbrcfX1YOd69xi0m+IY6mo3yKt3irQRokDtt376sHoUdHgj2ozySZJgG8IJndtoS+VQQy6NdClA3fNFb96bF865eNaRYoHJO9ZI84lkWQL++MLzIuyFfCs1hSlapyyuHC8kFmF7AQdrVZvbohSbnWs+w53nIW8nAA7z21wAukvE1Pl6AQyG0e7U1sYS8Pc8dtmzJvdtVZWBl02/gqQJ7f06mFvnsN45rR1Uyxnrwl6rbFwqabZDlyD5Ac6Icbvz9SG1gBOiI='
183+ keytype: u'DSA'
184+ resource_type_link: u'http://.../#ssh_key'
185+ self_link: u'http://.../~name12/+ssh-keys/1'
186
187 === Team memberships ===
188

Subscribers

People subscribed via source and target branches

to status/vote changes: