Merge lp:~jpds/launchpad/fix_517020 into lp:launchpad

[Jonathan Davies]

Various fixes to exported() values in the newly exposed distribution_mirror's API.

[Jonathan Davies (jpds)]

= Summary =

This branch fixes various problems with the newly exposed distribution_mirror's as described in bug #517020.

It also exposes various other things whose permissions to access are restricted by Zope's configuration.

[Michael Nelson (michael.nelson)]

Hi Jonathan,

It's great that you're so quick to get in there and fix his. Other than some formatting issues, the only point that I think needs addressing here is testing the writable of the fields you're changing/exporting.

> === modified file 'lib/lp/registry/interfaces/distributionmirror.py'
> --- a/lib/lp/registry/interfaces/distributionmirror.py 2010-02-01 23:17:27 +0000
> +++ b/lib/lp/registry/interfaces/distributionmirror.py 2010-02-04 13:07:13 +0000
> @@ -287,17 +287,19 @@
>
> id = Int(title=_('The unique id'), required=True, readonly=True)
> owner = exported(PublicPersonChoice(
> - title=_('Owner'), required=False, readonly=True,
> - vocabulary='ValidOwner'))
> - reviewer = PublicPersonChoice(
> - title=_('Reviewer'), required=False, readonly=False,
> - vocabulary='ValidPersonOrTeam')
> + title=_('Owner'), description=_("The person who is set as the "
> + "current administrator of this mirror."), required=True,
> + readonly=False, vocabulary='ValidOwner'))

So the owner attribute is no-longer readonly? If that's intentional, we really
should have that tested/documented (ie. who can/can't write to it).

Also, just a few small indentation things looking at
the style guide. It might be easier to format this like:

    title=_('Owner'), description=_(
 "The person who is set as the current administrator of this mirror."),
    required=True, readonly=False, vocabulary='ValidOwner')

or

    title=_('Owner'), required=True, readonly=False, vocabulary='ValidOwner'
    description=_(
 "The person who is set as the current administrator of this mirror."))

> + reviewer = exported(PublicPersonChoice(
> + title=_('Reviewer'), description=_("The person who last reviewed this "
> + "mirror."), required=False, readonly=True,
> + vocabulary='ValidPersonOrTeam'))

So the main change is that readonly is now true. Please ensure this is tested
also.

Similar formatting issues here. Although, I can't see anything in the
style-guide, but I thought that concatenated strings needed to be aligned (if
you have to break them).

> distribution = exported(
> Reference(
> Interface,
> # Really IDistribution, circular import fixed in
> # _schema_circular_imports.
> - title=_("Distribution"), required=True,
> + title=_("Distribution"), required=True, readonly=True,

Again, this needs to be tested (so it doesn't happen again).

> description=_("The distribution that is mirrored")))
> name = exported(DistributionMirrorNameField(
> title=_('Name'), required=True, readonly=False,
> @@ -325,7 +327,7 @@
> description=_('e.g.: rsync://archive.ubuntu.com/ubuntu/')))
> enabled = exported(Bool(
> title=_('This mirror was probed successfully.'),
> - required=False, readonly=False, default=False))
> + required=False, readonly=True, default=False))

And here too.

> speed = exported(Choice(
> title=_('Link Speed'), required=True, readonly=False,
> vocabulary=MirrorSpeed))
> @@ -343,7 +345,8 @@
> title=_('Apply to...

review: Needs Fixing (code)

[Jonathan Davies (jpds)]

> === modified file 'lib/lp/registry/interfaces/distributionmirror.py'
> > --- a/lib/lp/registry/interfaces/distributionmirror.py 2010-02-01 23:17:27 +0000
> > +++ b/lib/lp/registry/interfaces/distributionmirror.py 2010-02-04 13:07:13 +0000
> > @@ -287,17 +287,19 @@
> >
> > id = Int(title=_('The unique id'), required=True, readonly=True)
> > owner = exported(PublicPersonChoice(
> > - title=_('Owner'), required=False, readonly=True,
> > - vocabulary='ValidOwner'))
> > - reviewer = PublicPersonChoice(
> > - title=_('Reviewer'), required=False, readonly=False,
> > - vocabulary='ValidPersonOrTeam')
> > + title=_('Owner'), description=_("The person who is set as the "
> > + "current administrator of this mirror."), required=True,
> > + readonly=False, vocabulary='ValidOwner'))
>
> So the owner attribute is no-longer readonly? If that's intentional, we really
> should have that tested/documented (ie. who can/can't write to it).

This is now tested in registry/stories/webservice/xx-distribution-mirror.txt.

> Also, just a few small indentation things looking at
> the style guide. It might be easier to format this like:
>
> title=_('Owner'), description=_(
> "The person who is set as the current administrator of this mirror."),
> required=True, readonly=False, vocabulary='ValidOwner')
>
> or
>
> title=_('Owner'), required=True, readonly=False, vocabulary='ValidOwner'
> description=_(
> "The person who is set as the current administrator of this mirror."))

I've changed all the fields to the latter.

> > + reviewer = exported(PublicPersonChoice(
> > + title=_('Reviewer'), description=_("The person who last reviewed this "
> > + "mirror."), required=False, readonly=True,
> > + vocabulary='ValidPersonOrTeam'))
>
> So the main change is that readonly is now true. Please ensure this is tested
> also.

Also done.

> Similar formatting issues here. Although, I can't see anything in the
> style-guide, but I thought that concatenated strings needed to be aligned (if
> you have to break them).
>
> > distribution = exported(
> > Reference(
> > Interface,
> > # Really IDistribution, circular import fixed in
> > # _schema_circular_imports.
> > - title=_("Distribution"), required=True,
> > + title=_("Distribution"), required=True, readonly=True,
>
> Again, this needs to be tested (so it doesn't happen again).

This is now tested.

> > description=_("The distribution that is mirrored")))
> > name = exported(DistributionMirrorNameField(
> > title=_('Name'), required=True, readonly=False,
> > @@ -325,7 +327,7 @@
> > description=_('e.g.: rsync://archive.ubuntu.com/ubuntu/')))
> > enabled = exported(Bool(
> > title=_('This mirror was probed successfully.'),
> > - required=False, readonly=False, default=False))
> > + required=False, readonly=True, default=False))
>
> And here too.

Likewise.

> > speed = exported(Choice(
> > title=_('Link Speed'), required=True, readonly=False,
> > vocabul...

[Michael Nelson (michael.nelson)]

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',
> dum...

review: Approve (code)

[Jonathan Davies (jpds)]

The branch failed a test on distribution.txt, as I was just testing distribution-mirror.txt. The last commit fixes this and this is the diff:

=== modified file 'lib/lp/registry/stories/webservice/xx-distribution.txt'
--- lib/lp/registry/stories/webservice/xx-distribution.txt 2010-02-01 23:17:27 +0000
+++ lib/lp/registry/stories/webservice/xx-distribution.txt 2010-02-05 18:18:28 +0000
@@ -124,18 +124,20 @@
     >>> pprint_entry(canonical_releases)
     content: u'CD Image'
     date_created: u'2006-10-16T18:31:43.434567+00:00'
+ date_reviewed: None
     description: None
     displayname: None
     distribution_link: u'http://.../ubuntu'
     enabled: True
     ftp_base_url: None
- has_ftp_or_rsync_base_url: False
     http_base_url: u'http://releases.ubuntu.com/'
     name: u'canonical-releases'
     official_candidate: True
     owner_link: u'http://.../~mark'
+ reviewer_link: None
     resource_type_link: u'http://.../#distribution_mirror'
     rsync_base_url: None
     self_link: u'http://.../ubuntu/+mirror/canonical-releases'
     speed: u'100 Mbps'
     status: u'Official'
+ whiteboard: None

[Jonathan Davies (jpds)]

>> >>> 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?

For the record:

<noodles> Also, I can't see why the expected value of is_official_mirror changed at the end of your diff?
<jpds> 13:47:01 <jpds> is_o_m> IT changes because I patch it with: u'status' : 'Unofficial'

[Brad Crittenden (bac)]

Your additional test changes look good. Thanks.

review: Approve (code)

=== modified file 'lib/lp/registry/interfaces/distributionmirror.py'
--- lib/lp/registry/interfaces/distributionmirror.py	2010-02-01 23:17:27 +0000
+++ lib/lp/registry/interfaces/distributionmirror.py	2010-02-05 18:56:19 +0000
@@ -287,17 +287,19 @@
 
     id = Int(title=_('The unique id'), required=True, readonly=True)
     owner = exported(PublicPersonChoice(
-        title=_('Owner'), required=False, readonly=True,
-        vocabulary='ValidOwner'))
-    reviewer = PublicPersonChoice(
-        title=_('Reviewer'), required=False, readonly=False,
-        vocabulary='ValidPersonOrTeam')
+        title=_('Owner'), readonly=False, vocabulary='ValidOwner',
+        required=True, description=_(
+            "The person who is set as the current administrator of this mirror.")))
+    reviewer = exported(PublicPersonChoice(
+        title=_('Reviewer'), required=False, readonly=True,
+        vocabulary='ValidPersonOrTeam', description=_(
+            "The person who last reviewed this mirror.")))
     distribution = exported(
         Reference(
             Interface,
             # Really IDistribution, circular import fixed in
             # _schema_circular_imports.
-            title=_("Distribution"), required=True,
+            title=_("Distribution"), required=True, readonly=True,
             description=_("The distribution that is mirrored")))
     name = exported(DistributionMirrorNameField(
         title=_('Name'), required=True, readonly=False,
@@ -325,7 +327,7 @@
         description=_('e.g.: rsync://archive.ubuntu.com/ubuntu/')))
     enabled = exported(Bool(
         title=_('This mirror was probed successfully.'),
-        required=False, readonly=False, default=False))
+        required=False, readonly=True, default=False))
     speed = exported(Choice(
         title=_('Link Speed'), required=True, readonly=False,
         vocabulary=MirrorSpeed))
@@ -344,7 +346,8 @@
         required=False, readonly=False, default=True))
     status = exported(Choice(
         title=_('Status'), required=True, readonly=False,
-        vocabulary=MirrorStatus))
+        vocabulary=MirrorStatus,
+        description=_("The current status of a mirror's registration.")))
 
     title = Attribute('The title of this mirror')
     cdimage_series = Attribute(
@@ -355,17 +358,20 @@
     last_probe_record = Attribute(
         'The last MirrorProbeRecord for this mirror.')
     all_probe_records = Attribute('All MirrorProbeRecords for this mirror.')
-    has_ftp_or_rsync_base_url = exported(Bool(
-        title=_('Does this mirror have a ftp or rsync base URL?')))
+    has_ftp_or_rsync_base_url = Bool(
+        title=_('Does this mirror have a FTP or Rsync base URL?'))
     base_url = Attribute('The HTTP or FTP base URL of this mirror')
     date_created = exported(Datetime(
-        title=_('Date Created'), required=True, readonly=True))
-    date_reviewed = Datetime(
-        title=_('Date reviewed'), required=False, readonly=False)
-    whiteboard = Whiteboard(
-        title=_('Whiteboard'), required=False,
+        title=_('Date Created'), required=True, readonly=True,
+        description=_("The date on which this mirror was registered.")))
+    date_reviewed = exported(Datetime(
+        title=_('Date reviewed'), required=False, readonly=True,
+        description=_(
+            "The date on which this mirror was last reviewed by a mirror admin.")))
+    whiteboard = exported(Whiteboard(
+        title=_('Whiteboard'), required=False, readonly=False,
         description=_("Notes on the current status of the mirror (only "
-                      "visible to admins and the mirror's registrant)."))
+                      "visible to admins and the mirror's registrant).")))
 
     @invariant
     def mirrorMustHaveHTTPOrFTPURL(mirror):

=== modified file 'lib/lp/registry/stories/webservice/xx-distribution-mirror.txt'
--- lib/lp/registry/stories/webservice/xx-distribution-mirror.txt	2010-02-01 23:17:27 +0000
+++ lib/lp/registry/stories/webservice/xx-distribution-mirror.txt	2010-02-05 18:56:19 +0000
@@ -13,21 +13,23 @@
     >>> pprint_entry(canonical_archive_json)
     content: u'Archive'
     date_created: u'2006-10-16T18:31:43.434567+00:00'
+    date_reviewed: None
     description: None
     displayname: None
     distribution_link: u'http://.../ubuntu'
     enabled: True
     ftp_base_url: None
-    has_ftp_or_rsync_base_url: False
     http_base_url: u'http://archive.ubuntu.com/ubuntu/'
     name: u'canonical-archive'
     official_candidate: True
     owner_link: u'http://.../~mark'
     resource_type_link: u'http://.../#distribution_mirror'
+    reviewer_link: None
     rsync_base_url: None
     self_link: u'http://.../ubuntu/+mirror/canonical-archive'
     speed: u'100 Mbps'
     status: u'Official'
+    whiteboard: None
 
 And CD image mirrors:
 
@@ -37,21 +39,104 @@
     >>> pprint_entry(canonical_releases_json)
     content: u'CD Image'
     date_created: u'2006-10-16T18:31:43.434567+00:00'
+    date_reviewed: None
     description: None
     displayname: None
     distribution_link: u'http://.../ubuntu'
     enabled: True
     ftp_base_url: None
-    has_ftp_or_rsync_base_url: False
     http_base_url: u'http://releases.ubuntu.com/'
     name: u'canonical-releases'
     official_candidate: True
     owner_link: u'http://.../~mark'
     resource_type_link: u'http://.../#distribution_mirror'
+    reviewer_link: None
     rsync_base_url: None
     self_link: u'http://.../ubuntu/+mirror/canonical-releases'
     speed: u'100 Mbps'
     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 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()
+    >>> debian_distro = distros['entries'][3]
+    >>> patch = {
+    ...     u'date_reviewed' : u'2010-02-04T17:19:16.424198+00:00',
+    ...     u'distribution_link' : debian_distro['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.
+
+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 =
 
@@ -63,7 +148,7 @@
     >>> is_official_mirror = webservice.named_get(canonical_releases['self_link'],
     ...     'isOfficial').jsonBody()
     >>> print is_official_mirror
-    True
+    False
 
 "getOverallFreshness" returns the freshness of the mirror determined by the
 mirror prober from the mirror's last probe.

=== modified file 'lib/lp/registry/stories/webservice/xx-distribution.txt'
--- lib/lp/registry/stories/webservice/xx-distribution.txt	2010-02-01 23:17:27 +0000
+++ lib/lp/registry/stories/webservice/xx-distribution.txt	2010-02-05 18:56:19 +0000
@@ -124,18 +124,20 @@
     >>> pprint_entry(canonical_releases)
     content: u'CD Image'
     date_created: u'2006-10-16T18:31:43.434567+00:00'
+    date_reviewed: None
     description: None
     displayname: None
     distribution_link: u'http://.../ubuntu'
     enabled: True
     ftp_base_url: None
-    has_ftp_or_rsync_base_url: False
     http_base_url: u'http://releases.ubuntu.com/'
     name: u'canonical-releases'
     official_candidate: True
     owner_link: u'http://.../~mark'
     resource_type_link: u'http://.../#distribution_mirror'
+    reviewer_link: None
     rsync_base_url: None
     self_link: u'http://.../ubuntu/+mirror/canonical-releases'
     speed: u'100 Mbps'
     status: u'Official'
+    whiteboard: None