launchpad-reviewers team mailing list archive

[Colin Watson <mp+451901@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:person-id-permissions into launchpad:master.

Commit message:
Allow commercial admins to read Person.id on the webservice

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/451901

This will help with https://portal.admin.canonical.com/C158967 ("Migrate away from email and SSH key cron jobs on loganberry").
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:person-id-permissions into launchpad:master.

diff --git a/lib/lp/registry/browser/tests/test_person_webservice.py b/lib/lp/registry/browser/tests/test_person_webservice.py
index e0dfcb9..0081dc8 100644
--- a/lib/lp/registry/browser/tests/test_person_webservice.py
+++ b/lib/lp/registry/browser/tests/test_person_webservice.py
@@ -179,6 +179,22 @@ class TestPersonExportedID(TestCaseWithFactory):
         )
         self.assertEqual(person_id, body["id"])
 
+    def test_commercial_admin_can_see_id(self):
+        # A member of ~commercial-admins can read the `id` field.
+        person = self.factory.makePerson()
+        person_id = person.id
+        person_url = api_url(person)
+
+        body = (
+            webservice_for_person(
+                self.factory.makeCommercialAdmin(),
+                permission=OAuthPermission.WRITE_PRIVATE,
+            )
+            .get(person_url, api_version="devel")
+            .jsonBody()
+        )
+        self.assertEqual(person_id, body["id"])
+
 
 class TestPersonRepresentation(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
diff --git a/lib/lp/registry/security.py b/lib/lp/registry/security.py
index 03692fe..259455c 100644
--- a/lib/lp/registry/security.py
+++ b/lib/lp/registry/security.py
@@ -126,10 +126,28 @@ class ModerateProjectGroupSet(ModerateByRegistryExpertsOrAdmins):
     usedfor = IProjectGroupSet
 
 
-class ModeratePerson(ModerateByRegistryExpertsOrAdmins):
+class ModeratePerson(AuthorizationBase):
     permission = "launchpad.Moderate"
     usedfor = IPerson
 
+    def checkAuthenticated(self, user):
+        """Allow admins, commercial admins, and registry experts.
+
+        Allowing commercial admins here is a bit of a cheat, but it allows
+        IS automation to see Person.id
+        (https://portal.admin.canonical.com/C158967) without needing to use
+        an account that's a fully-fledged member of ~admins.  The only extra
+        exposure here is that commercial admins gain the ability to set the
+        status of other people's accounts, which isn't completely ideal, but
+        in practice people in the commercial admins team are always
+        highly-privileged anyway.
+        """
+        return (
+            user.in_admin
+            or user.in_commercial_admin
+            or user.in_registry_experts
+        )
+
 
 class ViewPillar(AuthorizationBase):
     usedfor = IPillar