launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08534
[Merge] lp:~stevenk/launchpad/branch-subscribe-aag into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/branch-subscribe-aag into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #933768 in Launchpad itself: "Update branch to use information_visibility_policy"
https://bugs.launchpad.net/launchpad/+bug/933768
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/branch-subscribe-aag/+merge/108881
IBranch.subscribe() now forbids open and delegated teams from being subscribed to private branches. This necessitated moving the exception into lp.app from bugs. I have also carried over the test for the webservice, and have written a few tests for the branch case.
I have also added the ability to IBranch.subscribe() to add a AccessArtifactGrant if required when a person is subscribed to a branch and added a test for that behaviour too.
--
https://code.launchpad.net/~stevenk/launchpad/branch-subscribe-aag/+merge/108881
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/branch-subscribe-aag into lp:launchpad.
=== modified file 'lib/lp/app/errors.py'
--- lib/lp/app/errors.py 2011-06-16 20:12:00 +0000
+++ lib/lp/app/errors.py 2012-06-06 06:11:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Cross application type errors for launchpad."""
@@ -9,6 +9,7 @@
'NameLookupFailed',
'NotFoundError',
'POSTToNonCanonicalURL',
+ 'SubscriptionPrivacyViolation',
'TranslationUnavailable',
'UnexpectedFormData',
'UserCannotUnsubscribePerson',
@@ -75,5 +76,10 @@
"""User does not have permission to unsubscribe person or team."""
+@error_status(httplib.BAD_REQUEST)
+class SubscriptionPrivacyViolation(Exception):
+ """The subscription would violate privacy policies."""
+
+
# Slam a 401 response code onto all ForbiddenAttribute errors.
error_status(httplib.UNAUTHORIZED)(ForbiddenAttribute)
=== added file 'lib/lp/app/tests/test_errors.py'
--- lib/lp/app/tests/test_errors.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/tests/test_errors.py 2012-06-06 06:11:04 +0000
@@ -0,0 +1,33 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from httplib import (
+ BAD_REQUEST,
+ UNAUTHORIZED,
+ )
+
+from lp.app.errors import (
+ SubscriptionPrivacyViolation,
+ UserCannotUnsubscribePerson,
+ )
+from lp.testing import TestCase
+from lp.testing.layers import FunctionalLayer
+from lp.testing.views import create_webservice_error_view
+
+
+class TestWebServiceErrors(TestCase):
+ """ Test that errors are correctly mapped to HTTP status codes."""
+
+ layer = FunctionalLayer
+
+ def test_UserCannotUnsubscribePerson_unauthorized(self):
+ error_view = create_webservice_error_view(
+ UserCannotUnsubscribePerson())
+ self.assertEqual(UNAUTHORIZED, error_view.status)
+
+ def test_SubscriptionPrivacyViolation_bad_request(self):
+ error_view = create_webservice_error_view(
+ SubscriptionPrivacyViolation())
+ self.assertEqual(BAD_REQUEST, error_view.status)
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2012-03-13 00:45:33 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2012-06-06 06:11:04 +0000
@@ -37,11 +37,11 @@
LaunchpadFormView,
ReturnToReferrerMixin,
)
+from lp.app.errors import SubscriptionPrivacyViolation
from lp.bugs.browser.structuralsubscription import (
expose_structural_subscription_data_to_js,
)
from lp.bugs.enums import BugNotificationLevel
-from lp.bugs.errors import SubscriptionPrivacyViolation
from lp.bugs.interfaces.bug import IBug
from lp.bugs.interfaces.bugsubscription import IBugSubscription
from lp.bugs.model.personsubscriptioninfo import PersonSubscriptions
=== modified file 'lib/lp/bugs/errors.py'
--- lib/lp/bugs/errors.py 2011-11-25 04:25:00 +0000
+++ lib/lp/bugs/errors.py 2012-06-06 06:11:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Errors used in the lp/bugs modules."""
@@ -8,7 +8,6 @@
'BugCannotBePrivate',
'InvalidBugTargetType',
'InvalidDuplicateValue',
- 'SubscriptionPrivacyViolation',
]
import httplib
@@ -29,10 +28,5 @@
@error_status(httplib.BAD_REQUEST)
-class SubscriptionPrivacyViolation(Exception):
- """The subscription would violate privacy policies."""
-
-
-@error_status(httplib.BAD_REQUEST)
class BugCannotBePrivate(Exception):
"""The bug is not allowed to be private."""
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2012-06-05 02:03:44 +0000
+++ lib/lp/bugs/model/bug.py 2012-06-06 06:11:04 +0000
@@ -85,6 +85,7 @@
from lp.app.enums import ServiceUsage
from lp.app.errors import (
NotFoundError,
+ SubscriptionPrivacyViolation,
UserCannotUnsubscribePerson,
)
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
@@ -105,7 +106,6 @@
from lp.bugs.errors import (
BugCannotBePrivate,
InvalidDuplicateValue,
- SubscriptionPrivacyViolation,
)
from lp.bugs.interfaces.bug import (
IBug,
@@ -177,8 +177,8 @@
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.productseries import IProductSeries
from lp.registry.interfaces.role import IPersonRoles
+from lp.registry.interfaces.series import SeriesStatus
from lp.registry.interfaces.sharingjob import IRemoveBugSubscriptionsJobSource
-from lp.registry.interfaces.series import SeriesStatus
from lp.registry.interfaces.sourcepackage import ISourcePackage
from lp.registry.model.accesspolicy import reconcile_access_for_artifact
from lp.registry.model.person import (
=== modified file 'lib/lp/bugs/tests/test_errors.py'
--- lib/lp/bugs/tests/test_errors.py 2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/tests/test_errors.py 2012-06-06 06:11:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd. This software is licensed under the
+# Copyright 2011-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for bugs errors."""
@@ -15,7 +15,6 @@
from lp.bugs.errors import (
InvalidBugTargetType,
InvalidDuplicateValue,
- SubscriptionPrivacyViolation,
)
from lp.testing import TestCase
from lp.testing.layers import FunctionalLayer
@@ -35,8 +34,3 @@
error_view = create_webservice_error_view(
InvalidDuplicateValue("Dup"))
self.assertEqual(EXPECTATION_FAILED, error_view.status)
-
- def test_SubscriptionPrivacyViolation_bad_request(self):
- error_view = create_webservice_error_view(
- SubscriptionPrivacyViolation())
- self.assertEqual(BAD_REQUEST, error_view.status)
=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/branchsubscription.py 2012-06-06 06:11:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -20,6 +20,7 @@
LaunchpadEditFormView,
LaunchpadFormView,
)
+from lp.app.errors import SubscriptionPrivacyViolation
from lp.code.enums import BranchSubscriptionNotificationLevel
from lp.code.interfaces.branchsubscription import IBranchSubscription
from lp.services.webapp import (
@@ -226,14 +227,17 @@
person = data['person']
subscription = self.context.getSubscription(person)
if subscription is None:
- self.context.subscribe(
- person, notification_level, max_diff_lines, review_level,
- self.user)
-
- self.add_notification_message(
- '%s has been subscribed to this branch with: '
- % person.displayname, notification_level, max_diff_lines,
- review_level)
+ try:
+ self.context.subscribe(
+ person, notification_level, max_diff_lines, review_level,
+ self.user)
+ except SubscriptionPrivacyViolation as error:
+ self.setFieldError('person', unicode(error))
+ else:
+ self.add_notification_message(
+ '%s has been subscribed to this branch with: '
+ % person.displayname, notification_level, max_diff_lines,
+ review_level)
else:
self.add_notification_message(
'%s was already subscribed to this branch with: '
=== modified file 'lib/lp/code/browser/tests/test_branchsubscription.py'
--- lib/lp/code/browser/tests/test_branchsubscription.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/tests/test_branchsubscription.py 2012-06-06 06:11:04 +0000
@@ -1,13 +1,18 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Unit tests for BranchSubscriptions."""
__metaclass__ = type
+from lp.registry.enums import InformationType
from lp.services.webapp.interfaces import IPrimaryContext
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_initialized_view
class TestBranchSubscriptionPrimaryContext(TestCaseWithFactory):
@@ -22,3 +27,26 @@
self.assertEqual(
IPrimaryContext(subscription).context,
IPrimaryContext(subscription.branch).context)
+
+
+class TestBranchSubscriptionAddOtherView(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_cannot_subscribe_open_team_to_private_branch(self):
+ owner = self.factory.makePerson()
+ branch = self.factory.makeBranch(
+ information_type=InformationType.USERDATA, owner=owner)
+ team = self.factory.makeTeam()
+ form = {
+ 'field.person': team.name,
+ 'field.notification_level': 'NOEMAIL',
+ 'field.max_diff_lines': 'NODIFF',
+ 'field.review_level': 'NOEMAIL',
+ 'field.actions.subscribe_action': 'Subscribe'}
+ with person_logged_in(owner):
+ view = create_initialized_view(
+ branch, '+addsubscriber', pricipal=owner, form=form)
+ self.assertContentEqual(
+ ['Open and delegated teams cannot be subscribed to private '
+ 'branches.'], view.errors)
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-06-05 02:03:44 +0000
+++ lib/lp/code/model/branch.py 2012-06-06 06:11:04 +0000
@@ -2,8 +2,6 @@
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0611,W0212,W0141,F0401
-from lp.bugs.interfaces.bugtarget import IBugTarget
-from lp.registry.interfaces.accesspolicy import IAccessPolicyArtifactSource, IAccessPolicySource, IAccessArtifactSource
__metaclass__ = type
__all__ = [
@@ -52,11 +50,15 @@
)
from lp import _
-from lp.app.errors import UserCannotUnsubscribePerson
+from lp.app.errors import (
+ SubscriptionPrivacyViolation,
+ UserCannotUnsubscribePerson,
+ )
from lp.app.interfaces.launchpad import (
ILaunchpadCelebrities,
IPrivacy,
)
+from lp.app.interfaces.services import IService
from lp.bugs.interfaces.bugtask import (
BugTaskSearchParams,
IBugTaskSet,
@@ -155,6 +157,7 @@
SQLBase,
sqlvalues,
)
+from lp.services.features import getFeatureFlag
from lp.services.helpers import shortlist
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
@@ -817,6 +820,11 @@
def subscribe(self, person, notification_level, max_diff_lines,
code_review_level, subscribed_by):
"""See `IBranch`."""
+ if (person.is_team and self.information_type in
+ PRIVATE_INFORMATION_TYPES and person.anyone_can_join()):
+ raise SubscriptionPrivacyViolation(
+ "Open and delegated teams cannot be subscribed to private "
+ "branches.")
# If the person is already subscribed, update the subscription with
# the specified notification details.
subscription = self.getSubscription(person)
@@ -831,6 +839,17 @@
subscription.notification_level = notification_level
subscription.max_diff_lines = max_diff_lines
subscription.review_level = code_review_level
+ # Grant the subscriber access if they can't see the branch (if the
+ # database triggers aren't going to do it for us).
+ trigger_flag = 'disclosure.access_mirror_triggers.removed'
+ if bool(getFeatureFlag(trigger_flag)):
+ service = getUtility(IService, 'sharing')
+ branches, ignored = service.getVisibleArtifacts(
+ person, branches=[self])
+ if not branches:
+ service.ensureAccessGrants(
+ subscribed_by, person, branches=[self],
+ ignore_permissions=True)
return subscription
def getSubscription(self, person):
@@ -858,13 +877,12 @@
"""See `IBranch`."""
return self.getSubscription(person) is not None
- def unsubscribe(self, person, unsubscribed_by, **kwargs):
+ def unsubscribe(self, person, unsubscribed_by, ignore_permissions=False):
"""See `IBranch`."""
subscription = self.getSubscription(person)
if subscription is None:
# Silent success seems order of the day (like bugs).
return
- ignore_permissions = kwargs.get('ignore_permissions', False)
if (not ignore_permissions
and not subscription.canBeUnsubscribedByUser(unsubscribed_by)):
raise UserCannotUnsubscribePerson(
=== modified file 'lib/lp/code/model/tests/test_branchsubscription.py'
--- lib/lp/code/model/tests/test_branchsubscription.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_branchsubscription.py 2012-06-06 06:11:04 +0000
@@ -1,15 +1,19 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for the BranchSubscrptions model object.."""
__metaclass__ = type
-from lp.app.errors import UserCannotUnsubscribePerson
+from lp.app.errors import (
+ SubscriptionPrivacyViolation,
+ UserCannotUnsubscribePerson,
+ )
from lp.code.enums import (
BranchSubscriptionNotificationLevel,
CodeReviewNotificationLevel,
)
+from lp.registry.enums import InformationType
from lp.testing import (
person_logged_in,
TestCaseWithFactory,
@@ -67,6 +71,30 @@
subscription.person,
self.factory.makePerson())
+ def test_cannot_subscribe_open_team_to_private_branch(self):
+ """It is forbidden to subscribe a open team to a private branch."""
+ owner = self.factory.makePerson()
+ branch = self.factory.makeBranch(
+ information_type=InformationType.USERDATA, owner=owner)
+ team = self.factory.makeTeam()
+ with person_logged_in(owner):
+ self.assertRaises(
+ SubscriptionPrivacyViolation, branch.subscribe, team, None,
+ None, None, owner)
+
+ def test_subscribe_gives_access(self):
+ """Subscribing a user to a branch gives them access."""
+ owner = self.factory.makePerson()
+ branch = self.factory.makeBranch(
+ information_type=InformationType.USERDATA, owner=owner)
+ subscribee = self.factory.makePerson()
+ with person_logged_in(owner):
+ self.assertFalse(branch.visibleByUser(subscribee))
+ branch.subscribe(
+ subscribee, BranchSubscriptionNotificationLevel.NOEMAIL,
+ None, CodeReviewNotificationLevel.NOEMAIL, owner)
+ self.assertTrue(branch.visibleByUser(subscribee))
+
class TestBranchSubscriptionCanBeUnsubscribedbyUser(TestCaseWithFactory):
"""Tests for BranchSubscription.canBeUnsubscribedByUser."""
Follow ups
