launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Needs Information code

Hi Ian.

I life this branch, but I think the rule that permits me to subscribe an unshared user to branch that I cannot edit contradicts your own work to to reconcile APGs ans AAGs with subscriptions. See my questions below.

> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py	2012-07-14 07:50:13 +0000
> +++ lib/lp/code/model/branch.py	2012-07-16 11:16:27 +0000
> @@ -878,6 +896,23 @@
>              service.ensureAccessGrants(
>                  [person], subscribed_by, branches=[self],
>                  ignore_permissions=True)
> +
> +        if not check_stacked_visibility:
> +            return subscription
> 
> +        # We now grant access to any stacked on branches which are not
> +        # currently accessible to the person but which the subscribed_by user
> +        # has edit permissions for.
> +        service = getUtility(IService, 'sharing')
> +        ignored, invisible_stacked_branches = service.getInvisibleArtifacts(
> +            person, branches=self.getStackedOnBranches())
> +        editable_stacked_on_branches = [
> +            branch for branch in invisible_stacked_branches
> +            if check_permission('launchpad.Edit', branch)]
> +        for invisible_branch in editable_stacked_on_branches:
> +            invisible_branch.subscribe(
> +                person, notification_level, max_diff_lines, code_review_level,
> +                subscribed_by, check_stacked_visibility=False)

What prevents me a member of ~ canonical from trying to share a branch
with a contractor? ~canonical does not own any code. The person I
subscribe will not have access, yet I think I did share the branch via a
subscription. Only the project maintainers and drivers can subscribe
unshared users to branches. See my comments in the test for this.

> === modified file 'lib/lp/code/model/tests/test_branchcollection.py'
> --- lib/lp/code/model/tests/test_branchcollection.py	2012-07-14 07:50:13 +0000
> +++ lib/lp/code/model/tests/test_branchcollection.py	2012-07-16 11:16:27 +0000
> @@ -650,12 +658,14 @@
>          self.assertEqual([self.public_branch], list(branches.getBranches()))
>  
>      def test_owner_sees_own_branches(self):
> -        # Users can always see the branches that they own, as well as public
> -        # branches.
> +        # Users can always see the branches that they own, those their own
> +        # branches are stacked on, as well as public branches.
>          owner = removeSecurityProxy(self.private_branch1).owner
>          branches = self.all_branches.visibleByUser(owner)
>          self.assertEqual(
> -            sorted([self.public_branch, self.private_branch1]),
> +            sorted([self.public_branch, self.private_branch1,
> +                    self.public_stacked_on_branch,
> +                    self.private_stacked_on_branch]),
>              sorted(branches.getBranches()))

The comment is not true. Project maintainers can remove APG and AAG
access to a branch from an owner. Private teams cannot access their
personal branches at this very moment because there is no project to
create the APG or AAG. We need to fix this case.

Is this test wrong, or is this just old code that will be superseded?

> === modified file 'lib/lp/code/model/tests/test_branchsubscription.py'
> --- lib/lp/code/model/tests/test_branchsubscription.py	2012-07-04 10:36:37 +0000
> +++ lib/lp/code/model/tests/test_branchsubscription.py	2012-07-16 11:16:27 +0000
> +    def test_subscribe_with_non_editable_stacked_branch(self):
> +        # Subscribing to a branch ignores any stacked on private branches
> +        # which the subscribed_by person can not edit.
> +        owner = self.factory.makePerson()
> +        product = self.factory.makeProduct(owner=owner)
> +        private_stacked_on_branch = self.factory.makeBranch(
> +            product=product,
> +            information_type=InformationType.EMBARGOEDSECURITY)
> +        branch = self.factory.makeBranch(
> +            product=product, owner=owner,
> +            stacked_on=private_stacked_on_branch,
> +            information_type=InformationType.USERDATA)
> +
> +        service = getUtility(IService, 'sharing')

> +        with person_logged_in(owner):
> +            # Subscribe to the top level branch.
> +            grantee = self.factory.makePerson()
> +            branch.subscribe(
> +                grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
> +                None, CodeReviewNotificationLevel.NOEMAIL, owner)
> +            # The stacked on branch should not be visible.
> +            ignored, visible_branches = service.getVisibleArtifacts(
> +                grantee, branches=[private_stacked_on_branch])
> +            self.assertContentEqual([], visible_branches)
> +            self.assertNotIn(
> +                grantee, private_stacked_on_branch.subscribers)

This case is somewhat vague, and I think the expected result is wrong. I
think users would understand this example if we said only branch owners
can subscribe users to private branches. For example. a member of
~canonical, I cannot subscribe someone to a branch PES project branch.
The project drivers who do own the branch can subscribe anyone.

I think this test should show there is no subscription to any branches in
this case. recall that subscriptions imply access. We decided to remove
subscriptions when access is removed because we want people to trust
Launchpad -- there cannot be a subscription because the artefact is not
shared. If this test really happened in the UI, the project would want
to audit the user to find out what he has access too, but he is not
listed on +sharing, leading to more distrust -- how can this be? How
does the project remove the user? I think the model and UI should show
an error or return a message to indicate that *no* subscriptions were
added.

> === modified file 'lib/lp/registry/interfaces/sharingservice.py'
> --- lib/lp/registry/interfaces/sharingservice.py	2012-07-16 10:25:45 +0000
> +++ lib/lp/registry/interfaces/sharingservice.py	2012-07-16 11:16:27 +0000
> @@ -77,6 +77,18 @@
>          :return: a collection of artifacts the person can see.
>          """
>  
> +    def getInvisibleArtifacts(person, branches=None, bugs=None):
> +        """Return the artifacts which are not shared with person.
> +
> +        Given lists of artifacts, return those a person does not have access to
> +        either via a policy grant or artifact grant.
> +
> +        :param person: the person whose access is being checked.
> +        :param branches: the branches to check for which a person has access.
> +        :param bugs: the bugs to check for which a person has access.
> +        :return: a collection of artifacts the person can not see.
> +        """

This is dangerous like inTeam(). I think we want a comment in this to remind
everyone that this method cannot be exported. It is for internal use only.
-- 
https://code.launchpad.net/~wallyworld/launchpad/view-private-stacked-branch2-393566/+merge/115112
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.