> === 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, > > vocabulary=MirrorSpeed)) > > @@ -343,7 +345,8 @@ > > title=_('Apply to be an official mirror of this distribution'), > > required=False, readonly=False, default=True)) > > status = exported(Choice( > > - title=_('Status'), required=True, readonly=False, > > + title=_('Status'), description=_("The current status of a mirror's " > > + "registration."), required=True, readonly=False, > > If it's not already, this should be tested too (ie. who is allowed to write > to this field. > > And same formatting issues as before. Tested and fixed. > > vocabulary=MirrorStatus)) > > > > title = Attribute('The title of this mirror') > > @@ -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=_('Date Created'), description=_("The date on which this mirror " > > + "was registered"), required=True, readonly=True)) > > Same formatting issues. Done. > > + date_reviewed = exported(Datetime( > > + title=_('Date reviewed'), description=_("The date on which this " > > + "mirror was last reviewed by a mirror admin."), required=False, > > + readonly=True)) > > Same formatting issues. Done. > > + whiteboard = exported(Whiteboard( > > title=_('Whiteboard'), required=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)."))) > > It would be good to explicitely define whether htis is readonly. Done and full diff since r10278: === modified file 'lib/lp/registry/interfaces/distributionmirror.py' --- lib/lp/registry/interfaces/distributionmirror.py 2010-02-04 12:22:59 +0000 +++ lib/lp/registry/interfaces/distributionmirror.py 2010-02-05 11:15:20 +0000 @@ -287,13 +287,13 @@ id = Int(title=_('The unique id'), required=True, readonly=True) owner = exported(PublicPersonChoice( - title=_('Owner'), description=_("The person who is set as the " - "current administrator of this mirror."), required=True, - readonly=False, vocabulary='ValidOwner')) + 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'), description=_("The person who last reviewed this " - "mirror."), required=False, readonly=True, - vocabulary='ValidPersonOrTeam')) + title=_('Reviewer'), required=False, readonly=True, + vocabulary='ValidPersonOrTeam', description=_( + "The person who last reviewed this mirror."))) distribution = exported( Reference( Interface, @@ -345,9 +345,9 @@ title=_('Apply to be an official mirror of this distribution'), required=False, readonly=False, default=True)) status = exported(Choice( - title=_('Status'), description=_("The current status of a mirror's " - "registration."), required=True, readonly=False, - vocabulary=MirrorStatus)) + title=_('Status'), required=True, readonly=False, + vocabulary=MirrorStatus, + description=_("The current status of a mirror's registration."))) title = Attribute('The title of this mirror') cdimage_series = Attribute( @@ -362,14 +362,14 @@ 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'), description=_("The date on which this mirror " - "was registered"), required=True, readonly=True)) + title=_('Date Created'), required=True, readonly=True, + description=_("The date on which this mirror was registered."))) date_reviewed = exported(Datetime( - title=_('Date reviewed'), description=_("The date on which this " - "mirror was last reviewed by a mirror admin."), required=False, - readonly=True)) + 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, + title=_('Whiteboard'), required=False, readonly=False, description=_("Notes on the current status of the mirror (only " "visible to admins and the mirror's registrant)."))) === 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 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() + >>> 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. + +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 "getOverallFreshness" returns the freshness of the mirror determined by the mirror prober from the mirror's last probe. === modified file 'lib/lp/registry/interfaces/distributionmirror.py' --- lib/lp/registry/interfaces/distributionmirror.py 2010-02-04 12:22:59 +0000 +++ lib/lp/registry/interfaces/distributionmirror.py 2010-02-05 11:15:20 +0000 @@ -287,13 +287,13 @@ id = Int(title=_('The unique id'), required=True, readonly=True) owner = exported(PublicPersonChoice( - title=_('Owner'), description=_("The person who is set as the " - "current administrator of this mirror."), required=True, - readonly=False, vocabulary='ValidOwner')) + 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'), description=_("The person who last reviewed this " - "mirror."), required=False, readonly=True, - vocabulary='ValidPersonOrTeam')) + title=_('Reviewer'), required=False, readonly=True, + vocabulary='ValidPersonOrTeam', description=_( + "The person who last reviewed this mirror."))) distribution = exported( Reference( Interface, @@ -345,9 +345,9 @@ title=_('Apply to be an official mirror of this distribution'), required=False, readonly=False, default=True)) status = exported(Choice( - title=_('Status'), description=_("The current status of a mirror's " - "registration."), required=True, readonly=False, - vocabulary=MirrorStatus)) + title=_('Status'), required=True, readonly=False, + vocabulary=MirrorStatus, + description=_("The current status of a mirror's registration."))) title = Attribute('The title of this mirror') cdimage_series = Attribute( @@ -362,14 +362,14 @@ 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'), description=_("The date on which this mirror " - "was registered"), required=True, readonly=True)) + title=_('Date Created'), required=True, readonly=True, + description=_("The date on which this mirror was registered."))) date_reviewed = exported(Datetime( - title=_('Date reviewed'), description=_("The date on which this " - "mirror was last reviewed by a mirror admin."), required=False, - readonly=True)) + 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, + title=_('Whiteboard'), required=False, readonly=False, description=_("Notes on the current status of the mirror (only " "visible to admins and the mirror's registrant)."))) === 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 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() + >>> 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. + +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 "getOverallFreshness" returns the freshness of the mirror determined by the mirror prober from the mirror's last probe.