Code review comment for lp:~didrocks/launchpad/expose-sshkeys-bug-357235

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.