Merge lp:~al-maisan/launchpad/bedit-347768 into lp:launchpad

[Muharem Hrnjadovic]

[Muharem Hrnjadovic (al-maisan)]

Hello there!

The branch at hand extends the 'launchpad.Edit' authorisation check for
branches targeting source packages as follows: if a user is allowed to
upload the linked source package he is also authorised to edit the
branch in question.

Pre-implementation call (belated) with jml.

Tests to run:

    bin/test -t test_branch.TestWriteToBranch

No lint errors or warning.

[Jonathan Lange (jml)]

Hi Muharem,

Thanks for looking into this and following up on our implementation call. This branch is good, but it's missing a crucial behaviour: we only want this extended permission checking for officially linked source package branches. That is, for branches like lp:ubuntu/karmic/ssss rather than lp:~jml/ubuntu/karmic/ssss/thingamajig.

This linking is also tied into how we determine the pocket for a branch. I've made some comments on this below.

Let me know if you have any questions.

jml

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2009-10-16 15:00:55 +0000
> +++ lib/canonical/launchpad/security.py 2009-10-19 10:45:43 +0000
> @@ -7,9 +7,10 @@
> __all__ = ['AuthorizationBase']
>
> from zope.interface import implements, Interface
> -from zope.component import getAdapter, getUtility
> +from zope.component import getUtility
>
> from canonical.launchpad.interfaces.account import IAccount
> +from lp.archiveuploader.permission import verify_upload
> from lp.registry.interfaces.announcement import IAnnouncement
> from lp.soyuz.interfaces.archive import IArchive
> from lp.soyuz.interfaces.archivepermission import (
> @@ -1591,7 +1592,57 @@
>
> def checkAuthenticated(self, user):
> return (user.inTeam(self.obj.owner) or
> - user_has_special_branch_access(user))
> + user_has_special_branch_access(user) or
> + can_upload_linked_package(user, self.obj))
> +
> +
> +def can_upload_linked_package(person, branch):
> + """True if person may upload the package linked to `branch`."""

Blank line here please.

> + def get_current_release():
> + """Get current release for the source package linked to branch."""
> + ds = branch.distroseries
> + package = branch.sourcepackage
> + releases = ds.getCurrentSourceReleases([package.sourcepackagename])
> + return releases.get(package, None)
> +
> + def get_property_value(obj, property_name):
> + """Get value of requested property or None."""
> + value = None
> + try:
> + value = getattr(obj, property_name)
> + except AttributeError:
> + return None
> + return value
> +

I don't know what the purpose of this method is. 'getattr(obj, name, None)'
will do exactly the same thing.

> + # Check whether we're dealing with a source package branch and
> + # whether person is authorised to upload the respective source
> + # package.
> + if branch.sourcepackage is None:
> + # No package .. hmm .. this can't be a source package branch
> + # then. Abort.
> + return False
> +
> + if branch.distroseries is None:
> + # No distro series? Abort.
> + return False
> +

There's a database constraint to ensure that you cannot have sourcepackage
without distroseries. I don't have a problem with doing an extra check here,
but you can also feel free to remove it.

> + current_release = get_current_release()
> +
> + # Do we have a pocket and, if yes, can we upload to it?
> + pocket = get_property_value(current_release, 'pocket')
> + if not (pocket is None ...

review: Needs Fixing

[Muharem Hrnjadovic (al-maisan)]

Jonathan Lange wrote:
> Review: Needs Fixing
> Hi Muharem,
>
> Thanks for looking into this and following up on our implementation
> call. This branch is good, but it's missing a crucial behaviour: we
> only want this extended permission checking for officially linked
> source package branches. That is, for branches like
> lp:ubuntu/karmic/ssss rather than
> lp:~jml/ubuntu/karmic/ssss/thingamajig.

Hello Jonathan,

thanks for looking :) What is the recommended way to test for an
officially linked branch? Test the URL?
Check for the presence of a SeriesSourcePackageBranch instance?
Use SeriesSourcePackageBranchSet.findForBranch() and take it from there?

> This linking is also tied into how we determine the pocket for a
> branch. I've made some comments on this below.
>
> Let me know if you have any questions.
>
> jml
>
>> === modified file 'lib/canonical/launchpad/security.py'
>> --- lib/canonical/launchpad/security.py 2009-10-16 15:00:55 +0000
>> +++ lib/canonical/launchpad/security.py 2009-10-19 10:45:43 +0000
>> @@ -7,9 +7,10 @@
>> __all__ = ['AuthorizationBase']
>>
>> from zope.interface import implements, Interface
>> -from zope.component import getAdapter, getUtility
>> +from zope.component import getUtility
>>
>> from canonical.launchpad.interfaces.account import IAccount
>> +from lp.archiveuploader.permission import verify_upload
>> from lp.registry.interfaces.announcement import IAnnouncement
>> from lp.soyuz.interfaces.archive import IArchive
>> from lp.soyuz.interfaces.archivepermission import (
>> @@ -1591,7 +1592,57 @@
>>
>> def checkAuthenticated(self, user):
>> return (user.inTeam(self.obj.owner) or
>> - user_has_special_branch_access(user))
>> + user_has_special_branch_access(user) or
>> + can_upload_linked_package(user, self.obj))
>> +
>> +
>> +def can_upload_linked_package(person, branch):
>> + """True if person may upload the package linked to `branch`."""
>
> Blank line here please.

Yup.

>
>> + def get_current_release():
>> + """Get current release for the source package linked to branch."""
>> + ds = branch.distroseries
>> + package = branch.sourcepackage
>> + releases = ds.getCurrentSourceReleases([package.sourcepackagename])
>> + return releases.get(package, None)
>> +
>> + def get_property_value(obj, property_name):
>> + """Get value of requested property or None."""
>> + value = None
>> + try:
>> + value = getattr(obj, property_name)
>> + except AttributeError:
>> + return None
>> + return value
>> +
>
> I don't know what the purpose of this method is. 'getattr(obj, name, None)'
> will do exactly the same thing.

Whoops .. sorry .. EHACKINGLATE :P

>> + # Check whether we're dealing with a source package branch and
>> + # whether person is authorised to upload the respective source
>> + # package.
>> + if branch.sourcepackage is None:
>> + # No package .. hmm .. this can't be a source package branch
>> + # then. Abort.
>> + return False
>> +
>> + if branch.distroseries is None:
>> + # No distro ...

[Jonathan Lange (jml)]

On Mon, Oct 19, 2009 at 12:51 PM, Muharem Hrnjadovic
<email address hidden> wrote:
> Jonathan Lange wrote:
>> Review: Needs Fixing
>> Hi Muharem,
>>
>> Thanks for looking into this and following up on our implementation
>> call.  This branch is good, but it's missing a crucial behaviour: we
>> only want this extended permission checking for officially linked
>> source package branches.  That is, for branches like
>> lp:ubuntu/karmic/ssss rather than
>> lp:~jml/ubuntu/karmic/ssss/thingamajig.
>
> Hello Jonathan,
>
> thanks for looking :) What is the recommended way to test for an
> officially linked branch? Test the URL?
> Check for the presence of a SeriesSourcePackageBranch instance?
> Use SeriesSourcePackageBranchSet.findForBranch() and take it from there?
>

I would use getUtility(IFindOfficialBranchLinks).findForBranch(). I
would do this check instead of checking that the branch is a
sourcepackage branch, and I would check for upload rights to any of
the packages that are officially linked.

...
>>
>>> +    def get_current_release():
>>> +        """Get current release for the source package linked to branch."""
>>> +        ds = branch.distroseries
>>> +        package = branch.sourcepackage
>>> +        releases = ds.getCurrentSourceReleases([package.sourcepackagename])
>>> +        return releases.get(package, None)
>>> +
>>> +    def get_property_value(obj, property_name):
>>> +        """Get value of requested property or None."""
>>> +        value = None
>>> +        try:
>>> +            value = getattr(obj, property_name)
>>> +        except AttributeError:
>>> +            return None
>>> +        return value
>>> +
>>
>> I don't know what the purpose of this method is. 'getattr(obj, name, None)'
>> will do exactly the same thing.
>
> Whoops .. sorry .. EHACKINGLATE :P
>

No worries :)

>>> +    # Check whether we're dealing with a source package branch and
>>> +    # whether person is authorised to upload the respective source
>>> +    # package.
>>> +    if branch.sourcepackage is None:
>>> +        # No package .. hmm .. this can't be a source package branch
>>> +        # then. Abort.
>>> +        return False
>>> +
>>> +    if branch.distroseries is None:
>>> +        # No distro series? Abort.
>>> +        return False
>>> +
>>
>> There's a database constraint to ensure that you cannot have
>> sourcepackage without distroseries. I don't have a problem with doing
>> an extra check here, but you can also feel free to remove it.
>
> OK. Will do.
>

As I mention above, I think that if you use findForBranch, you won't
need these checks at all.

>>> +    current_release = get_current_release()
>>> +
>>> +    # Do we have a pocket and, if yes, can we upload to it?
>>> +    pocket = get_property_value(current_release, 'pocket')
>>> +    if not (pocket is None or branch.distroseries.canUploadToPocket(pocket)):
>>> +        return False
>>> +
>>
>> People have different preferences here, but I find this way of
>> phrasing the 'if' statement to be a little confusing. I mentally have
>> to apply DeMorgan's Law.
>>
>> if pocket is not None and not branch.distroseries.canUploadToPocket(pocket):
>>     return False
>
> Fair enough.
>

M...

[Muharem Hrnjadovic (al-maisan)]

Jonathan Lange wrote:
> On Mon, Oct 19, 2009 at 12:51 PM, Muharem Hrnjadovic
> <email address hidden> wrote:
>> Jonathan Lange wrote:
>>> Review: Needs Fixing
>>> Hi Muharem,
>>>
>>> Thanks for looking into this and following up on our implementation
>>> call. This branch is good, but it's missing a crucial behaviour: we
>>> only want this extended permission checking for officially linked
>>> source package branches. That is, for branches like
>>> lp:ubuntu/karmic/ssss rather than
>>> lp:~jml/ubuntu/karmic/ssss/thingamajig.
>> Hello Jonathan,
>>
>> thanks for looking :) What is the recommended way to test for an
>> officially linked branch? Test the URL?
>> Check for the presence of a SeriesSourcePackageBranch instance?
>> Use SeriesSourcePackageBranchSet.findForBranch() and take it from there?
>>
>
> I would use getUtility(IFindOfficialBranchLinks).findForBranch(). I
> would do this check instead of checking that the branch is a
> sourcepackage branch, and I would check for upload rights to any of
> the packages that are officially linked.

Packages? Is this a one-branch-to-many-packages relationship? From
looking at the code it looked like a 1-to-1 relationship to me ..

Anyway, after revising the code as suggested I get security exceptions
(please see http://pastebin.ubuntu.com/296865/).
I guess the ISeriesSourcePackageBranch interface needs to be split into
a read-only and an edit portion respectively. As it stands all access
to it requires the "launchpad.Edit" permissions.

  <class

class="lp.code.model.seriessourcepackagebranch.SeriesSourcePackageBranch">
    <allow
interface="lp.code.interfaces.seriessourcepackagebranch.ISeriesSourcePackageBranch"/>
    <require
        permission="launchpad.Edit"

set_schema="lp.code.interfaces.seriessourcepackagebranch.ISeriesSourcePackageBranch"/>
  </class>

>
> ...
>>>> + def get_current_release():
>>>> + """Get current release for the source package linked to branch."""
>>>> + ds = branch.distroseries
>>>> + package = branch.sourcepackage
>>>> + releases = ds.getCurrentSourceReleases([package.sourcepackagename])
>>>> + return releases.get(package, None)
>>>> +
>>>> + def get_property_value(obj, property_name):
>>>> + """Get value of requested property or None."""
>>>> + value = None
>>>> + try:
>>>> + value = getattr(obj, property_name)
>>>> + except AttributeError:
>>>> + return None
>>>> + return value
>>>> +
>>> I don't know what the purpose of this method is. 'getattr(obj, name, None)'
>>> will do exactly the same thing.
>> Whoops .. sorry .. EHACKINGLATE :P
>>
>
> No worries :)
>
>>>> + # Check whether we're dealing with a source package branch and
>>>> + # whether person is authorised to upload the respective source
>>>> + # package.
>>>> + if branch.sourcepackage is None:
>>>> + # No package .. hmm .. this can't be a source package branch
>>>> + # then. Abort.
>>>> + return False
>>>> +
>>>> + if branch.distroseries is None:
>>>> + # No distro series? Abort.
>>>> + return False
>>>> +
>>> There's a database constraint to ensure that y...

[Jonathan Lange (jml)]

On Mon, Oct 19, 2009 at 3:15 PM, Muharem Hrnjadovic
<email address hidden> wrote:
> Jonathan Lange wrote:
>> On Mon, Oct 19, 2009 at 12:51 PM, Muharem Hrnjadovic
>> <email address hidden> wrote:
>>> Jonathan Lange wrote:
>>>> Review: Needs Fixing
>>>> Hi Muharem,
>>>>
>>>> Thanks for looking into this and following up on our implementation
>>>> call.  This branch is good, but it's missing a crucial behaviour: we
>>>> only want this extended permission checking for officially linked
>>>> source package branches.  That is, for branches like
>>>> lp:ubuntu/karmic/ssss rather than
>>>> lp:~jml/ubuntu/karmic/ssss/thingamajig.
>>> Hello Jonathan,
>>>
>>> thanks for looking :) What is the recommended way to test for an
>>> officially linked branch? Test the URL?
>>> Check for the presence of a SeriesSourcePackageBranch instance?
>>> Use SeriesSourcePackageBranchSet.findForBranch() and take it from there?
>>>
>>
>> I would use getUtility(IFindOfficialBranchLinks).findForBranch(). I
>> would do this check instead of checking that the branch is a
>> sourcepackage branch, and I would check for upload rights to any of
>> the packages that are officially linked.
>
> Packages? Is this a one-branch-to-many-packages relationship? From
> looking at the code it looked like a 1-to-1 relationship to me ..
>

One branch can belong to at most one package.

However, a branch can be an official package branch for many
(sourcepackage, pocket) combinations, even if the branch itself does
not belong to a source package. This is deliberate.

> Anyway, after revising the code as suggested I get security exceptions
> (please see http://pastebin.ubuntu.com/296865/).
> I guess the ISeriesSourcePackageBranch interface needs to be split into
> a read-only and an edit portion respectively. As it stands all access
> to it requires the "launchpad.Edit" permissions.
>
>  <class
>
> class="lp.code.model.seriessourcepackagebranch.SeriesSourcePackageBranch">
>    <allow
> interface="lp.code.interfaces.seriessourcepackagebranch.ISeriesSourcePackageBranch"/>
>    <require
>        permission="launchpad.Edit"
>
> set_schema="lp.code.interfaces.seriessourcepackagebranch.ISeriesSourcePackageBranch"/>
>  </class>
>

Something's wrong here.

ISeriesSourcePackageBranch is used to determine the short names of
package branches (e.g. lp:ubuntu/emacs-snapshot). This works
anonymously. (bazaar_identity calls
branch.associatedSuiteSourcePackages() which gets
IFindOfficialBranchLinks).

From the pastebin,
  ForbiddenAttribute: ('pocket', <storm.store.ResultSet object at 0xb359a90>)

This means that you cannot get pocket from a ResultSet. This makes
sense, since result sets are storm objects and don't have pockets.

  File "/home/mhr/canonical/lp-branches/bedit-347768/lib/canonical/launchpad/security.py",
line 1616, in can_upload_linked_package
    if not branch.distroseries.canUploadToPocket(sspb.pocket):
AttributeError: 'NoneType' object has no attribute 'canUploadToPocket'

This error is correct, since the condition is checking the wrong
thing. The question is not, "can we upload to this pocket in the
distroseries that the branch belongs to" but rather "can we upload to
this pocket in the dist...

[Muharem Hrnjadovic (al-maisan)]

Jonathan Lange wrote:
> On Mon, Oct 19, 2009 at 3:15 PM, Muharem Hrnjadovic
> <email address hidden> wrote:
>> Jonathan Lange wrote:
>>> On Mon, Oct 19, 2009 at 12:51 PM, Muharem Hrnjadovic
>>> <email address hidden> wrote:
>>>> Jonathan Lange wrote:
>>>>> Review: Needs Fixing

[..]

>>>> thanks for looking :) What is the recommended way to test for an
>>>> officially linked branch? Test the URL?
>>>> Check for the presence of a SeriesSourcePackageBranch instance?
>>>> Use SeriesSourcePackageBranchSet.findForBranch() and take it from there?
>>>>
>>> I would use getUtility(IFindOfficialBranchLinks).findForBranch(). I
>>> would do this check instead of checking that the branch is a
>>> sourcepackage branch, and I would check for upload rights to any of
>>> the packages that are officially linked.

Right. This is what the revised can_upload_linked_package() logic now
does.

Please see the enclosed incremental diff for details.

>> Packages? Is this a one-branch-to-many-packages relationship? From
>> looking at the code it looked like a 1-to-1 relationship to me ..
>>
>
> One branch can belong to at most one package.
>
> However, a branch can be an official package branch for many
> (sourcepackage, pocket) combinations, even if the branch itself does
> not belong to a source package. This is deliberate.
>
>> Anyway, after revising the code as suggested I get security exceptions
>> (please see http://pastebin.ubuntu.com/296865/).
>> I guess the ISeriesSourcePackageBranch interface needs to be split into
>> a read-only and an edit portion respectively. As it stands all access
>> to it requires the "launchpad.Edit" permissions.
>>
>> <class
>>
>> class="lp.code.model.seriessourcepackagebranch.SeriesSourcePackageBranch">
>> <allow
>> interface="lp.code.interfaces.seriessourcepackagebranch.ISeriesSourcePackageBranch"/>
>> <require
>> permission="launchpad.Edit"
>>
>> set_schema="lp.code.interfaces.seriessourcepackagebranch.ISeriesSourcePackageBranch"/>
>> </class>
>>
>
> Something's wrong here.
>
> ISeriesSourcePackageBranch is used to determine the short names of
> package branches (e.g. lp:ubuntu/emacs-snapshot). This works
> anonymously. (bazaar_identity calls
> branch.associatedSuiteSourcePackages() which gets
> IFindOfficialBranchLinks).
>
>>From the pastebin,
> ForbiddenAttribute: ('pocket', <storm.store.ResultSet object at 0xb359a90>)
>
> This means that you cannot get pocket from a ResultSet. This makes
> sense, since result sets are storm objects and don't have pockets.
>
> File "/home/mhr/canonical/lp-branches/bedit-347768/lib/canonical/launchpad/security.py",
> line 1616, in can_upload_linked_package
> if not branch.distroseries.canUploadToPocket(sspb.pocket):
> AttributeError: 'NoneType' object has no attribute 'canUploadToPocket'
>
> This error is correct, since the condition is checking the wrong
> thing. The question is not, "can we upload to this pocket in the
> distroseries that the branch belongs to" but rather "can we upload to
> this pocket in the distroseries that the branch is official for".
>
> i.e. sspb.distroseries.canUploadToPocket(sspb.pocket)

I am both relieved (that the error is so easil...

[Jonathan Lange (jml)]

Looks good, thanks.

You might want to try:
  sspbs = branch.associatedSuiteSourcePackages()

rather than:
  result_set = getUtility(IFindOfficialBranchLinks).findForBranch(branch)

(I only remembered the first one after I mentioned the second).

But no big deal.

Thanks again,
jml

review: Approve

[Muharem Hrnjadovic (al-maisan)]

Jonathan Lange wrote:
> Review: Approve
> Looks good, thanks.
>
> You might want to try:
> sspbs = branch.associatedSuiteSourcePackages()
>
> rather than:
> result_set = getUtility(IFindOfficialBranchLinks).findForBranch(branch)
>
> (I only remembered the first one after I mentioned the second).

Done. Please see attached incremental diff.

Thanks for suggesting this!

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

[Jonathan Lange (jml)]

On Tue, Oct 20, 2009 at 10:54 AM, Muharem Hrnjadovic
<email address hidden> wrote:
> Jonathan Lange wrote:
>> Review: Approve
>> Looks good, thanks.
>>
>> You might want to try:
>>   sspbs = branch.associatedSuiteSourcePackages()
>>
>> rather than:
>>   result_set = getUtility(IFindOfficialBranchLinks).findForBranch(branch)
>>
>> (I only remembered the first one after I mentioned the second).
>
> Done. Please see attached incremental diff.
>
> Thanks for suggesting this!
>

My pleasure.

Please go ahead and merge the branch.

jml

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2009-10-16 15:00:55 +0000
+++ lib/canonical/launchpad/security.py	2009-10-20 09:49:18 +0000
@@ -7,9 +7,10 @@
 __all__ = ['AuthorizationBase']
 
 from zope.interface import implements, Interface
-from zope.component import getAdapter, getUtility
+from zope.component import getUtility
 
 from canonical.launchpad.interfaces.account import IAccount
+from lp.archiveuploader.permission import verify_upload
 from lp.registry.interfaces.announcement import IAnnouncement
 from lp.soyuz.interfaces.archive import IArchive
 from lp.soyuz.interfaces.archivepermission import (
@@ -83,7 +84,7 @@
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.project import IProject, IProjectSet
 from lp.code.interfaces.seriessourcepackagebranch import (
-    ISeriesSourcePackageBranch, IMakeOfficialBranchLinks)
+    IMakeOfficialBranchLinks, ISeriesSourcePackageBranch)
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.soyuz.interfaces.sourcepackagerelease import (
     ISourcePackageRelease)
@@ -1591,7 +1592,59 @@
 
     def checkAuthenticated(self, user):
         return (user.inTeam(self.obj.owner) or
-                user_has_special_branch_access(user))
+                user_has_special_branch_access(user) or
+                can_upload_linked_package(user, self.obj))
+
+
+def can_upload_linked_package(person, branch):
+    """True if person may upload the package linked to `branch`."""
+
+    def get_current_release(ssp):
+        """Get current release for the source package linked to branch.
+
+        This function uses the `ISuiteSourcePackage` instance supplied.
+        """
+        package = ssp.sourcepackage
+        releases = ssp.distroseries.getCurrentSourceReleases(
+            [package.sourcepackagename])
+        return releases.get(package, None)
+
+    # No associated `ISuiteSourcePackage` data -> not an official branch.
+    # Abort.
+    ssp_list = branch.associatedSuiteSourcePackages()
+    if len(ssp_list) < 1:
+        return False
+
+    # XXX al-maisan, 2009-10-20: a branch may currently be associated with a
+    # number of (distroseries, sourcepackagename, pocket) combinations.
+    # This does not seem right. But until the database model is fixed we work
+    # around this by assuming that things are fine as long as we find at least
+    # one combination that allows us to upload the corresponding source
+    # package.
+
+    # Go through the associated `ISuiteSourcePackage` instances and see
+    # whether we can upload to any of the distro series/pocket combinations.
+    ssp = None
+    for ssp in ssp_list:
+        # Can we upload to the respective pocket?
+        if ssp.distroseries.canUploadToPocket(ssp.pocket):
+            break
+    else:
+        # Loop terminated normally i.e. we could not upload to any of the
+        # (distroseries, pocket) combinations found.
+        return False
+
+    archive = ssp.distroseries.distribution.main_archive
+    # Find the component the linked source package was published in.
+    current_release = get_current_release(ssp)
+    component = getattr(current_release, 'component', None)
+
+    # Is person authorised to upload the source package this branch
+    # is targeting?
+    result = verify_upload(person, ssp.sourcepackagename, archive, component)
+    # verify_upload() indicates that person *is* allowed to upload by
+    # returning None.
+    return result is None
 
 
 class AdminBranch(AuthorizationBase):

=== modified file 'lib/lp/code/tests/test_branch.py'
--- lib/lp/code/tests/test_branch.py	2009-06-25 04:06:00 +0000
+++ lib/lp/code/tests/test_branch.py	2009-10-20 09:49:18 +0000
@@ -8,50 +8,104 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.webapp.authorization import check_permission
+from canonical.testing import DatabaseFunctionalLayer
+
+from lp.archiveuploader.permission import verify_upload
 from lp.code.enums import (
     BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel)
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
-from canonical.launchpad.security import AccessBranch
-from lp.testing import TestCaseWithFactory
-from canonical.testing import DatabaseFunctionalLayer
-
-
-class TestAccessBranch(TestCaseWithFactory):
+from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
+from lp.registry.interfaces.distroseries import DistroSeriesStatus
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
+from lp.testing import run_with_login, TestCaseWithFactory
+
+
+class PermissionTest(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
-    def assertAuthenticatedAccess(self, branch, person, can_access):
-        branch = removeSecurityProxy(branch)
-        self.assertEqual(
-            can_access, AccessBranch(branch).checkAuthenticated(person))
-
-    def assertUnauthenticatedAccess(self, branch, can_access):
-        branch = removeSecurityProxy(branch)
-        self.assertEqual(
-            can_access, AccessBranch(branch).checkUnauthenticated())
+    def assertPermission(self, can_access, person, secure_object, permission):
+        """Assert that 'person' can or cannot access 'secure_object'.
+
+        :param can_access: Whether or not the person can access the object.
+        :param person: The `IPerson` who is trying to access the object.
+        :param secure_object: The secured object.
+        :param permission: The Launchpad permission that 'person' is trying to
+            access 'secure_object' with.
+        """
+        self.assertEqual(
+            can_access,
+            run_with_login(
+                person, check_permission, permission, secure_object))
+
+    def assertAuthenticatedView(self, branch, person, can_access):
+        """Can 'branch' be accessed by 'person'?
+
+        :param branch: The `IBranch` we're curious about.
+        :param person: The `IPerson` trying to access it.
+        :param can_access: Whether we expect 'person' be able to access it.
+        """
+        self.assertPermission(can_access, person, branch, 'launchpad.View')
+
+    def assertUnauthenticatedView(self, branch, can_access):
+        """Can 'branch' be accessed anonymously?
+
+        :param branch: The `IBranch` we're curious about.
+        :param can_access: Whether we expect to access it anonymously.
+        """
+        self.assertAuthenticatedView(branch, None, can_access)
+
+    def assertCanEdit(self, person, secured_object):
+        """Assert 'person' can edit 'secured_object'.
+
+        That is, assert 'person' has 'launchpad.Edit' permissions on
+        'secured_object'.
+
+        :param person: An `IPerson`. None means anonymous.
+        :param secured_object: An object, secured through the Zope security
+            layer.
+        """
+        self.assertPermission(True, person, secured_object, 'launchpad.Edit')
+
+    def assertCannotEdit(self, person, secured_object):
+        """Assert 'person' cannot edit 'secured_object'.
+
+        That is, assert 'person' does not have 'launchpad.Edit' permissions on
+        'secured_object'.
+
+        :param person: An `IPerson`. None means anonymous.
+        :param secured_object: An object, secured through the Zope security
+            layer.
+        """
+        self.assertPermission(False, person, secured_object, 'launchpad.Edit')
+
+
+class TestAccessBranch(PermissionTest):
 
     def test_publicBranchUnauthenticated(self):
         # Public branches can be accessed without authentication.
         branch = self.factory.makeAnyBranch()
-        self.assertUnauthenticatedAccess(branch, True)
+        self.assertUnauthenticatedView(branch, True)
 
     def test_publicBranchArbitraryUser(self):
         # Public branches can be accessed by anyone.
         branch = self.factory.makeAnyBranch()
         person = self.factory.makePerson()
-        self.assertAuthenticatedAccess(branch, person, True)
+        self.assertAuthenticatedView(branch, person, True)
 
     def test_privateBranchUnauthenticated(self):
         # Private branches cannot be accessed without authentication.
         branch = self.factory.makeAnyBranch(private=True)
-        self.assertUnauthenticatedAccess(branch, False)
+        self.assertUnauthenticatedView(branch, False)
 
     def test_privateBranchOwner(self):
         # The owner of a branch can always access it.
         owner = self.factory.makePerson()
         branch = self.factory.makeAnyBranch(private=True, owner=owner)
-        self.assertAuthenticatedAccess(branch, owner, True)
+        self.assertAuthenticatedView(branch, owner, True)
 
     def test_privateBranchOwnerMember(self):
         # Any member of the team that owns the branch can access it.
@@ -60,19 +114,20 @@
         person = self.factory.makePerson()
         removeSecurityProxy(team).addMember(person, team_owner)
         branch = self.factory.makeAnyBranch(private=True, owner=team)
-        self.assertAuthenticatedAccess(branch, person, True)
+        self.assertAuthenticatedView(branch, person, True)
 
     def test_privateBranchBazaarExperts(self):
         # The Bazaar experts can access any branch.
         celebs = getUtility(ILaunchpadCelebrities)
         branch = self.factory.makeAnyBranch(private=True)
-        self.assertAuthenticatedAccess(branch, celebs.bazaar_experts, True)
+        self.assertAuthenticatedView(
+            branch, celebs.bazaar_experts.teamowner, True)
 
     def test_privateBranchAdmins(self):
         # Launchpad admins can access any branch.
         celebs = getUtility(ILaunchpadCelebrities)
         branch = self.factory.makeAnyBranch(private=True)
-        self.assertAuthenticatedAccess(branch, celebs.admin, True)
+        self.assertAuthenticatedView(branch, celebs.admin.teamowner, True)
 
     def test_privateBranchSubscriber(self):
         # If you are subscribed to a branch, you can access it.
@@ -82,13 +137,13 @@
             person, BranchSubscriptionNotificationLevel.NOEMAIL,
             BranchSubscriptionDiffSize.NODIFF,
             CodeReviewNotificationLevel.NOEMAIL)
-        self.assertAuthenticatedAccess(branch, person, True)
+        self.assertAuthenticatedView(branch, person, True)
 
     def test_privateBranchAnyoneElse(self):
         # In general, you can't access a private branch.
         branch = self.factory.makeAnyBranch(private=True)
         person = self.factory.makePerson()
-        self.assertAuthenticatedAccess(branch, person, False)
+        self.assertAuthenticatedView(branch, person, False)
 
     def test_stackedOnPrivateBranchUnauthenticated(self):
         # If a branch is stacked on a private branch, then you cannot access
@@ -96,7 +151,7 @@
         stacked_on_branch = self.factory.makeAnyBranch(private=True)
         stacked_branch = self.factory.makeAnyBranch(
             stacked_on=stacked_on_branch)
-        self.assertUnauthenticatedAccess(stacked_branch, False)
+        self.assertUnauthenticatedView(stacked_branch, False)
 
     def test_stackedOnPrivateBranchAuthenticated(self):
         # If a branch is stacked on a private branch, you can only access it
@@ -105,7 +160,7 @@
         stacked_branch = self.factory.makeAnyBranch(
             stacked_on=stacked_on_branch)
         person = self.factory.makePerson()
-        self.assertAuthenticatedAccess(stacked_branch, person, False)
+        self.assertAuthenticatedView(stacked_branch, person, False)
 
     def test_manyLevelsOfStackingUnauthenticated(self):
         # If a branch is stacked on a branch stacked on a private branch, you
@@ -113,7 +168,7 @@
         stacked_on_branch = self.factory.makeAnyBranch(private=True)
         branch_a = self.factory.makeAnyBranch(stacked_on=stacked_on_branch)
         branch_b = self.factory.makeAnyBranch(stacked_on=branch_a)
-        self.assertUnauthenticatedAccess(branch_b, False)
+        self.assertUnauthenticatedView(branch_b, False)
 
     def test_manyLevelsOfStackingAuthenticated(self):
         # If a branch is stacked on a branch stacked on a private branch, you
@@ -122,7 +177,7 @@
         branch_a = self.factory.makeAnyBranch(stacked_on=stacked_on_branch)
         branch_b = self.factory.makeAnyBranch(stacked_on=branch_a)
         person = self.factory.makePerson()
-        self.assertAuthenticatedAccess(branch_b, person, False)
+        self.assertAuthenticatedView(branch_b, person, False)
 
     def test_loopedPublicStackedOn(self):
         # It's possible, although nonsensical, for branch stackings to form a
@@ -132,7 +187,7 @@
         stacked_branch = self.factory.makeAnyBranch()
         removeSecurityProxy(stacked_branch).stacked_on = stacked_branch
         person = self.factory.makePerson()
-        self.assertAuthenticatedAccess(stacked_branch, person, True)
+        self.assertAuthenticatedView(stacked_branch, person, True)
 
     def test_loopedPrivateStackedOn(self):
         # It's possible, although nonsensical, for branch stackings to form a
@@ -142,7 +197,7 @@
         stacked_branch = self.factory.makeAnyBranch(private=True)
         removeSecurityProxy(stacked_branch).stacked_on = stacked_branch
         person = self.factory.makePerson()
-        self.assertAuthenticatedAccess(stacked_branch, person, False)
+        self.assertAuthenticatedView(stacked_branch, person, False)
 
     def test_loopedPublicStackedOnUnauthenticated(self):
         # It's possible, although nonsensical, for branch stackings to form a
@@ -151,8 +206,106 @@
         # being logged in.
         stacked_branch = self.factory.makeAnyBranch()
         removeSecurityProxy(stacked_branch).stacked_on = stacked_branch
-        self.assertUnauthenticatedAccess(stacked_branch, True)
-
+        self.assertUnauthenticatedView(stacked_branch, True)
+
+
+class TestWriteToBranch(PermissionTest):
+    """Test who can write to branches."""
+
+    def test_owner_can_write(self):
+        # The owner of a branch can write to the branch.
+        branch = self.factory.makeAnyBranch()
+        self.assertCanEdit(branch.owner, branch)
+
+    def test_random_person_cannot_write(self):
+        # Arbitrary logged in people cannot write to branches.
+        branch = self.factory.makeAnyBranch()
+        person = self.factory.makePerson()
+        self.assertCannotEdit(person, branch)
+
+    def test_member_of_owning_team_can_write(self):
+        # Members of the team that owns a branch can write to the branch.
+        team = self.factory.makeTeam()
+        person = self.factory.makePerson()
+        removeSecurityProxy(team).addMember(person, team.teamowner)
+        branch = self.factory.makeAnyBranch(owner=team)
+        self.assertCanEdit(person, branch)
+
+    def makeOfficialPackageBranch(self):
+        """Make a branch linked to the pocket of a source package."""
+        branch = self.factory.makePackageBranch()
+        # Make sure the (distroseries, pocket) combination used allows us to
+        # upload to it.
+        stable_states = (
+            DistroSeriesStatus.SUPPORTED, DistroSeriesStatus.CURRENT)
+        if branch.distroseries.status in stable_states:
+            pocket = PackagePublishingPocket.BACKPORTS
+        else:
+            pocket = PackagePublishingPocket.RELEASE
+        sourcepackage = branch.sourcepackage
+        suite_sourcepackage = sourcepackage.getSuiteSourcePackage(pocket)
+        registrant = self.factory.makePerson()
+        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
+        run_with_login(
+            ubuntu_branches.teamowner,
+            ICanHasLinkedBranch(suite_sourcepackage).setBranch,
+            branch, registrant)
+        return branch
+
+    def test_owner_can_write_to_official_package_branch(self):
+        # The owner of an official package branch can write to it, just like a
+        # regular person.
+        branch = self.makeOfficialPackageBranch()
+        self.assertCanEdit(branch.owner, branch)
+
+    def assertCanUpload(self, person, spn, archive, component,
+                        strict_component=True):
+        """Assert that 'person' can upload 'spn' to 'archive'."""
+        # For now, just check that doesn't raise an exception.
+        self.assertIs(
+            None,
+            verify_upload(person, spn, archive, component, strict_component))
+
+    def assertCannotUpload(self, reason, person, spn, archive, component):
+        """Assert that 'person' cannot upload to the archive.
+
+        :param reason: The expected reason for not being able to upload. A
+            string.
+        :param person: The person trying to upload.
+        :param spn: The `ISourcePackageName` being uploaded to. None if the
+            package does not yet exist.
+        :param archive: The `IArchive` being uploaded to.
+        :param component: The IComponent to which the package belongs.
+        """
+        exception = verify_upload(person, spn, archive, component)
+        self.assertEqual(reason, str(exception))
+
+    def test_package_upload_permissions_grant_branch_edit(self):
+        # If you can upload to the package, then you are also allowed to write
+        # to the branch.
+
+        permission_set = getUtility(IArchivePermissionSet)
+        # Only admins or techboard members can add permissions normally. That
+        # restriction isn't relevant to these tests.
+        self.permission_set = removeSecurityProxy(permission_set)
+        branch = self.makeOfficialPackageBranch()
+        package = branch.sourcepackage
+        person = self.factory.makePerson()
+
+        # Person is not allowed to edit the branch presently.
+        self.assertCannotEdit(person, branch)
+
+        # Now give 'person' permission to upload to 'package'.
+        archive = branch.distroseries.distribution.main_archive
+        spn = package.sourcepackagename
+        self.permission_set.newPackageUploader(archive, person, spn)
+        # Make sure person *is* authorised to upload the source package
+        # targeted by the branch at hand.
+        self.assertCanUpload(person, spn, archive, None)
+
+        # Now person can edit the branch on the basis of the upload
+        # permissions granted above.
+        self.assertCanEdit(person, branch)
 
 
 def test_suite():