Code review comment for lp:~jpds/launchpad/fix_517020

Thanks for the changes Jonathan. I've approved the review, but do have a couple of things below that I'm interested to hear back on (they could be just oversights on my part).

> Done and full diff since r10278:
> === modified file 'lib/lp/registry/stories/webservice/xx-distribution-
> mirror.txt'
> --- lib/lp/registry/stories/webservice/xx-distribution-mirror.txt
> 2010-02-04 12:22:19 +0000
> +++ lib/lp/registry/stories/webservice/xx-distribution-mirror.txt
> 2010-02-05 11:32:41 +0000
> @@ -57,6 +57,88 @@
> status: u'Official'
> whiteboard: None
>
> += Security checks =
> +
> +People who are not mirror listing admins or the mirrors registrar may not
> +change the owner's of mirrors:
> +
> + >>> from canonical.launchpad.testing.pages import webservice_for_person
> + >>> from canonical.launchpad.webapp.interfaces import OAuthPermission
> + >>> from zope.component import getUtility
> + >>> from lp.registry.interfaces.person import IPersonSet
> + >>> from simplejson import dumps
> + >>> login(ANONYMOUS)
> + >>> karl_db = getUtility(IPersonSet).getByName('karl')
> + >>> test_db = getUtility(IPersonSet).getByName('name12')
> + >>> karl_webservice = webservice_for_person(karl_db,
> + ... permission=OAuthPermission.WRITE_PUBLIC)
> + >>> test_webservice = webservice_for_person(test_db,
> + ... permission=OAuthPermission.WRITE_PUBLIC)
> + >>> logout()
> + >>> karl = webservice.get("/~karl").jsonBody()
> + >>> patch = {
> + ... u'owner_link': karl['self_link']
> + ... }
> +
> +Now trying to set the owner using Sample Person's webservice is not
> authorized.
> +
> + >>> response = test_webservice.patch(
> + ... canonical_archive['self_link'], 'application/json', dumps(patch))
> + >>> print response.getheader('status')
> + 401 Unauthorized
> +
> +But if we use Karl, the mirror listing admin's, webservice, we can update the

s/admin's,/admin's

> owner.
> +
> + >>> response = karl_webservice.patch(
> + ... canonical_archive['self_link'], 'application/json', dumps(patch))
> + >>> print response.getheader('status')
> + 209 Content Returned
> +
> + >>> patched_canonical_archive = response.jsonBody()
> + >>> print patched_canonical_archive['owner_link']
> + http://.../~karl
> +
> +Some attributes are read-only via the API:
> +
> + >>> distros = webservice.get("/distros").jsonBody()
> + >>> distro = distros['entries'][3]
> + >>> debian = webservice.get(distro['self_link']).jsonBody()

I haven't checked, but is the above line necessary? you're getting distro['self_link'] which should be equivalent to distro? (ie. distro == debian?). Actually, you only seem to use debian['self_link'] below anyway (which should be equivalent to distro['self_link'] unless I'm missing something).

> + >>> patch = {
> + ... u'date_reviewed' : u'2010-02-04T17:19:16.424198+00:00',
> + ... u'distribution_link' : debian['self_link'],
> + ... u'enabled' : False,
> + ... u'reviewer_link' : karl['self_link']
> + ... }
> + >>> response = karl_webservice.patch(
> + ... canonical_releases['self_link'], 'application/json',
> dumps(patch))
> + >>> print response
> + HTTP/1.1 400 Bad Request
> + Status: 400 Bad Request
> + ...
> + enabled: You tried to modify a read-only attribute.
> + date_reviewed: You tried to modify a read-only attribute.
> + reviewer_link: You tried to modify a read-only attribute.
> + distribution_link: You tried to modify a read-only attribute.
> +

Nice test!

> +While others can be set with the appropriate authorization:
> +
> + >>> patch = {
> + ... u'status' : 'Unofficial',
> + ... u'whiteboard' : u'This mirror is too shiny to be true'
> + ... }
> + >>> response = test_webservice.patch(
> + ... canonical_releases['self_link'], 'application/json',
> dumps(patch))
> + >>> print response.getheader('status')
> + 401 Unauthorized
> +
> + >>> response = karl_webservice.patch(
> + ... canonical_releases['self_link'], 'application/json',
> dumps(patch)).jsonBody()
> + >>> pprint_entry(response)
> + content: u'CD Image'
> + ...
> + status: u'Unofficial'
> + whiteboard: u'This mirror is too shiny to be true'
> +
> = Distribution Mirror Custom Operations =
>
> DistributionMirror has some custom operations.
> @@ -67,7 +149,7 @@
> >>> is_official_mirror =
> webservice.named_get(canonical_releases['self_link'],
> ... 'isOfficial').jsonBody()
> >>> print is_official_mirror
> - True
> + False

I can't see anything that should have changed the expected value of this test? Why did it change here?