Launchpad itself

Merge lp:~julian-edwards/launchpad/p3a-sub-private-teams-bug-597783 into lp:launchpad

  1. p3a-sub-private-teams-bug-597783
  2. Merge into devel
Proposed by Julian Edwards
Status: Merged
Merged at revision: 11067
Proposed branch: lp:~julian-edwards/launchpad/p3a-sub-private-teams-bug-597783
Merge into: lp:launchpad
Diff against target: 93 lines (+70/-1)
2 files modified
lib/canonical/launchpad/security.py (+13/-1)
lib/lp/soyuz/tests/test_archive_subscriptions.py (+57/-0)
To merge this branch: bzr merge lp:~julian-edwards/launchpad/p3a-sub-private-teams-bug-597783
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Māris Fogels (community) Approve
Curtis Hovey (community) code Approve
Review via email: mp+28621@code.launchpad.net

Description of the change

= Summary =
Subscribers of P3As owned by private teams should be able to see the team

== Proposed fix ==
https://bugs.edge.launchpad.net/soyuz/+bug/597783

Currently a subscriber to a private PPA owned by a private team will get a 403
when they visit their +archivesubscriptions page because the security adapter
still prevents access from seeing that team, but which is required to build a
URL to the repo location in the subscription token.

This branch changes the security adapter to take the subscriptions a user
might have, and allows launchpad.View to the team if it has a PPA and the user
has a subscription.

== Pre-implementation notes ==
Talked to Curtis, we both agreed there's not much else we can do here.

== Implementation details ==
As above.

== Tests ==
bin/test -cvv test_archive_subscriptions

== Demo and Q/A ==
QA Plan:

 * Create a private team
 * Give the team a private PPA
 * Confirm that Joe User can't see the team, but can see his
+archivesubscriptions
 * Subscribe Joe User to the PPA
 * Confirm that Joe User can see the subscription on his +archivesubscriptions
page

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/tests/test_archive_subscriptions.py
  lib/canonical/launchpad/security.py

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

/me grumbles at the stupid person picker

Maris, that's an r-c stamp I need from you please, you already gave me one on Friday for this but I'd like it if you can make it official.

Cheers.

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Julian. Thanks for working on this. I have one trivial remark: There should be two blank lines before `def test_suite():`. I think we should consider this for an RC.

review: Approve (code)
Revision history for this message
Māris Fogels (mars) wrote :

Looks good to me, release-critical=mars

review: Approve
Revision history for this message
Henning Eggers (henninge) wrote :

Thanks for the fix and the improvement. Now just remove the unused import for IArchiveSet. ;)

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/security.py'
2--- lib/canonical/launchpad/security.py 2010-06-24 18:52:56 +0000
3+++ lib/canonical/launchpad/security.py 2010-06-28 14:56:38 +0000
4@@ -14,7 +14,6 @@
5 from canonical.launchpad.interfaces.account import IAccount
6 from canonical.launchpad.interfaces.emailaddress import IEmailAddress
7 from lp.registry.interfaces.announcement import IAnnouncement
8-from lp.soyuz.interfaces.archive import IArchive
9 from lp.soyuz.interfaces.archivepermission import (
10 IArchivePermissionSet)
11 from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthToken
12@@ -735,6 +734,19 @@
13 if (invitee.is_team and
14 invitee in user.person.getAdministratedTeams()):
15 return True
16+
17+ if (self.obj.is_team
18+ and self.obj.visibility == PersonVisibility.PRIVATE):
19+ # Grant visibility to people with subscriptions on a private
20+ # team's private PPA.
21+ subscriptions = getUtility(
22+ IArchiveSubscriberSet).getBySubscriber(user.person)
23+ subscriber_archive_ids = set(
24+ sub.archive.id for sub in subscriptions)
25+ team_ppa_ids = set(
26+ ppa.id for ppa in self.obj.ppas if ppa.private)
27+ if len(subscriber_archive_ids.intersection(team_ppa_ids)) > 0:
28+ return True
29 return False
30
31
32
33=== added file 'lib/lp/soyuz/tests/test_archive_subscriptions.py'
34--- lib/lp/soyuz/tests/test_archive_subscriptions.py 1970-01-01 00:00:00 +0000
35+++ lib/lp/soyuz/tests/test_archive_subscriptions.py 2010-06-28 14:56:38 +0000
36@@ -0,0 +1,57 @@
37+# Copyright 2009 Canonical Ltd. This software is licensed under the
38+# GNU Affero General Public License version 3 (see the file LICENSE).
39+
40+"""Test Archive features."""
41+
42+import unittest
43+
44+from zope.security.interfaces import Unauthorized
45+
46+from canonical.testing import DatabaseFunctionalLayer
47+
48+from lp.registry.interfaces.person import PersonVisibility
49+from lp.testing import login_person, TestCaseWithFactory
50+
51+
52+class TestArchiveSubscriptions(TestCaseWithFactory):
53+ """Edge-case tests for private PPA subscribers.
54+
55+ See also lib/lp/soyuz/stories/ppa/xx-private-ppa-subscription-stories.txt
56+ """
57+
58+ layer = DatabaseFunctionalLayer
59+
60+ def setUp(self):
61+ """Create a test archive."""
62+ super(TestArchiveSubscriptions, self).setUp()
63+ self.private_team = self.factory.makeTeam(
64+ visibility=PersonVisibility.PRIVATE, name="subscribertest")
65+ login_person(self.private_team.teamowner)
66+ self.archive = self.factory.makeArchive(
67+ private=True, owner=self.private_team)
68+ self.subscriber = self.factory.makePerson()
69+
70+ def test_subscriber_can_access_private_team_ppa(self):
71+ # As per bug 597783, we need to make sure a subscriber can see
72+ # a private team's PPA after they have been given a subscription.
73+ # This is essentially allowing access for the subscriber to see
74+ # the private team.
75+
76+ def get_name():
77+ return self.archive.owner.name
78+
79+ # Before a subscription, accessing the team name will raise.
80+ login_person(self.subscriber)
81+ self.assertRaises(Unauthorized, get_name)
82+
83+ login_person(self.private_team.teamowner)
84+ self.archive.newSubscription(
85+ self.subscriber, registrant=self.archive.owner)
86+
87+ # When a subscription exists, it's fine.
88+ login_person(self.subscriber)
89+ self.assertEqual(self.archive.owner.name, "subscribertest")
90+
91+
92+def test_suite():
93+ return unittest.TestLoader().loadTestsFromName(__name__)