Merge ~lgp171188/launchpad:allow-distribution-bug-supervisor-to-create-structural-subscriptions into launchpad:master

[Guruprasad]

Allow the distribution bug supervisor to add structural subscriptions

LP: #2037777

[William Grant (wgrant)]

The intent of that additional check is to prevent random people from accidentally subscribing themselves to the flood of mail that is Ubuntu's bugs. I don't think we want to change that, and I don't think we want to relax the rule that people can't subscribe other unrelated people or teams -- ~ubuntu-bugcontrol is a fairly broad set of people.

review: Needs Fixing (code)

[William Grant (wgrant)]

Though reading further I see that that check would still be implemented in userCanAlterSubscription. Never mind me.

review: Abstain

[Guruprasad (lgp171188)]

William,

> The intent of that additional check is to prevent random people from accidentally subscribing themselves to the flood of mail that is Ubuntu's bugs. I don't think we want to change that, and I don't think we want to relax the rule that people can't subscribe other unrelated people or teams -- ~ubuntu-bugcontrol is a fairly broad set of people.

In https://git.launchpad.net/launchpad/tree/lib/lp/bugs/model/structuralsubscription.py#n425, the comment explains exactly what I have implemented in this MP even though the current implementation doesn't match it - it checks the 'subscribed' person instead of the 'subscribed_by' person.

I implemented this change based on the feedback from Colin in comment #3 of https://answers.launchpad.net/launchpad/+question/707908.

Please let me know if I misunderstood something here.

[Colin Watson (cjwatson)]

Right, I think this is largely simple confusion between `subscriber` and `subscribed_by` - I don't feel that "only the bug supervisor or its members can be subscribed to the distribution's bugs" is a rule that's coherent with the rest of Launchpad, as opposed to "only the bug supervisor or its members can subscribe anyone to the distribution's bugs" which is what the comment says but not what the implementation does. Ironically, the person who asked the question that prompted this wrote this code to start with.

I don't like the location of the tests here, though. Looking through `tig blame` for the original code, I find that it came along with the addition of `lp.bugs.tests.test_structuralsubscriptiontarget.TestStructuralSubscriptionForDistro`, which still has several tests that overlap with the test cases you've added here (they just unfortunately didn't keep as clear a distinction between the subscriber and the person doing the subscribing as might have helped to avoid this problem). Could you move your tests there and rework them so that they use the same style as those existing tests?

review: Needs Fixing

[Guruprasad (lgp171188)]

Colin, while trying to port my existing tests to the `TestStructuralSubscriptionForDistro` test class, I found that fixing the checks in `userCanAlterBugSubscription()` like I have done is insufficient to allow the subscription mentioned in the linked question to happen. This is because, `addBugSubscription()` calls `userCanAlterBugSubscription()` and that returns `True` because of my changes in this MP. It then calls `addSubscription()` which, in turn, calls `userCanAlterSubscription()` and that doesn't allow this subscription to happen.

I am unsure of tweaking `userCanAlterSubscription()` since that _could_ violate the restriction that William mentioned in his first comment. So what can I do here?

[Ines Almeida (ines-almeida)]

Reading through the thread and the related question, I think the change makes sense and seems quite contained. Just a very minor comment on one of the tests

review: Approve

[Otto Co-Pilot (otto-copilot)]

Voting criteria not met
https://jenkins.ols.ps5.canonical.com/job/launchpad/3579/

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/bugs/model/structuralsubscription.py b/lib/lp/bugs/model/structuralsubscription.py
index 1694f09..37e8be2 100644
--- a/lib/lp/bugs/model/structuralsubscription.py
+++ b/lib/lp/bugs/model/structuralsubscription.py
@@ -430,8 +430,8 @@ class StructuralSubscriptionTargetMixin:
             if subscriber is None or subscribed_by is None:
                 return False
             elif (
-                subscriber != self.bug_supervisor
-                and not subscriber.inTeam(self.bug_supervisor)
+                subscribed_by != self.bug_supervisor
+                and not subscribed_by.inTeam(self.bug_supervisor)
                 and not subscribed_by.inTeam(admins)
             ):
                 return False
diff --git a/lib/lp/bugs/tests/test_structuralsubscriptiontarget.py b/lib/lp/bugs/tests/test_structuralsubscriptiontarget.py
index 9eb6933..985df25 100644
--- a/lib/lp/bugs/tests/test_structuralsubscriptiontarget.py
+++ b/lib/lp/bugs/tests/test_structuralsubscriptiontarget.py
@@ -12,6 +12,7 @@ from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import ProxyFactory, removeSecurityProxy
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.interfaces.bug import CreateBugParams
 from lp.bugs.interfaces.structuralsubscription import (
     IStructuralSubscriptionTarget,
@@ -228,14 +229,32 @@ class TestStructuralSubscriptionForDistro(
             StructuralSubscription,
         )
 
-    def test_distribution_subscription_by_bug_supervisor_team(self):
-        # team admins can subscribe team if team is bug supervisor
+    def test_distribution_bug_supervisor_admin_can_subscribe_owned_teams(self):
+        # bug supervisor team admin can subscribe any owned team including
+        # the bug supervisor team itself.
         removeSecurityProxy(self.target).bug_supervisor = self.team
+        another_team = self.factory.makeTeam(owner=self.team_owner)
         login_person(self.team_owner)
         self.assertIsInstance(
             self.target.addBugSubscription(self.team, self.team_owner),
             StructuralSubscription,
         )
+        self.assertIsInstance(
+            self.target.addBugSubscription(another_team, self.team_owner),
+            StructuralSubscription,
+        )
+
+    def test_admin_can_subscribe_anyone_to_the_distribution(self):
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
+        login_person(admin)
+        self.assertIsInstance(
+            self.target.addBugSubscription(self.team_owner, admin),
+            StructuralSubscription,
+        )
+        self.assertIsInstance(
+            self.target.addBugSubscription(self.team, admin),
+            StructuralSubscription,
+        )
 
     def test_distribution_unsubscription_by_bug_supervisor_team(self):
         # team admins can unsubscribe team if team is bug supervisor