launchpad-reviewers team mailing list archive

[Colin Watson <mp+452312@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Allow reading Person.id using read-only API tokens

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

See https://portal.admin.canonical.com/C158967 ("Migrate away from email and SSH key cron jobs on loganberry").  We have to do some manual permission checks to make this work properly.

This partially reverts commit 0dd8a14be7715daeb59c52a6ea1b0edc2a5d017f.

Quoting my most recent comment from the ticket:

```
I see the problem: this is due to using credentials that are set to only
allow reading private data, not changing private data.  To be clear,
this is completely reasonable - by principles of least privilege, we
don't want the bot to have edit access to anything.  However, I'd put
the exported "id" attribute under the launchpad.Moderate permission on
Person, and that doesn't work because launchpad.Moderate is defined like
this:

  <permission
    id="launchpad.Moderate" title="Moderate something" access_level="write" />

The access_level="write" bit means that only tokens with some kind of
write permission can do anything with attributes that require that
permission, so that won't work.
```
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:person-id-permissions-again 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 0081dc8..df60d7e 100644
--- a/lib/lp/registry/browser/tests/test_person_webservice.py
+++ b/lib/lp/registry/browser/tests/test_person_webservice.py
@@ -149,6 +149,20 @@ class TestPersonAccountStatus(TestCaseWithFactory):
 class TestPersonExportedID(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
 
+    def test_anonymous_user_cannot_see_id(self):
+        # An anonymous user cannot read the `id` field.
+        person = self.factory.makePerson()
+        person_url = api_url(person)
+
+        body = (
+            webservice_for_person(
+                None, permission=OAuthPermission.WRITE_PRIVATE
+            )
+            .get(person_url, api_version="devel")
+            .jsonBody()
+        )
+        self.assertEqual("tag:launchpad.net:2008:redacted", body["id"])
+
     def test_normal_user_cannot_see_id(self):
         # A normal user cannot read the `id` field, not even their own.
         person = self.factory.makePerson()
@@ -172,7 +186,7 @@ class TestPersonExportedID(TestCaseWithFactory):
         body = (
             webservice_for_person(
                 self.factory.makeRegistryExpert(),
-                permission=OAuthPermission.WRITE_PRIVATE,
+                permission=OAuthPermission.READ_PRIVATE,
             )
             .get(person_url, api_version="devel")
             .jsonBody()
@@ -188,7 +202,7 @@ class TestPersonExportedID(TestCaseWithFactory):
         body = (
             webservice_for_person(
                 self.factory.makeCommercialAdmin(),
-                permission=OAuthPermission.WRITE_PRIVATE,
+                permission=OAuthPermission.READ_PRIVATE,
             )
             .get(person_url, api_version="devel")
             .jsonBody()
diff --git a/lib/lp/registry/interfaces/person.py b/lib/lp/registry/interfaces/person.py
index 4e66cfd..3aeae4c 100644
--- a/lib/lp/registry/interfaces/person.py
+++ b/lib/lp/registry/interfaces/person.py
@@ -849,6 +849,26 @@ class IPersonViewRestricted(
 ):
     """IPerson attributes that require launchpad.View permission."""
 
+    # Most API clients have no need for the ID, but some systems need it as
+    # a stable identifier for users even across username changes (see
+    # https://portal.admin.canonical.com/C158967).  Access to this is
+    # restricted by checks in the property implementation to limit the scope
+    # of privacy issues.
+    exported_id = exported(
+        doNotSnapshot(
+            Int(
+                title=_("ID"),
+                description=_(
+                    "Internal immutable identifier for this person.  Only "
+                    "visible by privileged users."
+                ),
+                required=True,
+                readonly=True,
+            )
+        ),
+        exported_as="id",
+    )
+
     account = Object(schema=IAccount)
     account_id = Int(title=_("Account ID"), required=True, readonly=True)
     karma = exported(
@@ -2198,23 +2218,6 @@ class IPersonModerate(IPersonSettingsModerate):
 class IPersonModerateRestricted(Interface):
     """IPerson attributes that require launchpad.Moderate permission."""
 
-    # Most API clients have no need for the ID, but some systems need it as
-    # a stable identifier for users even across username changes (see
-    # https://portal.admin.canonical.com/C158967).  Access to this is
-    # restricted to limit the scope of privacy issues.
-    exported_id = exported(
-        Int(
-            title=_("ID"),
-            description=_(
-                "Internal immutable identifier for this person.  Only visible "
-                "by privileged users."
-            ),
-            required=True,
-            readonly=True,
-        ),
-        exported_as="id",
-    )
-
     @call_with(user=REQUEST_USER)
     @operation_parameters(
         status=copy_field(IAccount["status"]),
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 1bc9a03..33cd2b9 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -495,7 +495,28 @@ class Person(
     @property
     def exported_id(self):
         """See `IPerson`."""
-        return self.id
+        # Unfortunately, none of the standard permissions are suitable for
+        # what we need here: launchpad.Moderate comes closest, but it isn't
+        # quite right because that permission is defined with
+        # access_level="write", so it only works with OAuth tokens that have
+        # write permission.  Instead, we have to just use launchpad.View and
+        # do the security checks manually.
+        user = getUtility(ILaunchBag).user
+        if user is None:
+            raise Unauthorized
+        roles = IPersonRoles(user)
+        if (
+            roles.in_admin
+            # Allowing commercial admins 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.
+            or roles.in_commercial_admin
+            or roles.in_registry_experts
+        ):
+            return self.id
+        else:
+            raise Unauthorized
 
     sortingColumns = SQL("person_sort_key(Person.displayname, Person.name)")
     # If we're using SELECT DISTINCT, then we can't use sortingColumns
diff --git a/lib/lp/registry/security.py b/lib/lp/registry/security.py
index 259455c..03692fe 100644
--- a/lib/lp/registry/security.py
+++ b/lib/lp/registry/security.py
@@ -126,28 +126,10 @@ class ModerateProjectGroupSet(ModerateByRegistryExpertsOrAdmins):
     usedfor = IProjectGroupSet
 
 
-class ModeratePerson(AuthorizationBase):
+class ModeratePerson(ModerateByRegistryExpertsOrAdmins):
     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