launchpad-reviewers team mailing list archive

[Colin Watson <mp+412524@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:access-token-target-container into launchpad:master.

Commit message:
Allow access token targets to be containers

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

A test failure in https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/410373 revealed that it's cumbersome to require access token targets to match the context object exactly: that requires all the interesting methods to be implemented directly on the access token target, and in the case of (for example) updating a revision status report on a commit in a Git repository, that's quite cumbersome.

A better approach would be to work the same way as OAuth token scopes: in those cases, an exact match is fine, but so is a match for the nearest `LaunchpadContainer` in its URL chain, or one of its parent containers.  This allows access tokens for Git repositories to be valid for other objects that are subordinate to those repositories.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:access-token-target-container into launchpad:master.

diff --git a/lib/lp/services/webapp/tests/test_servers.py b/lib/lp/services/webapp/tests/test_servers.py
index a5ffe0a..7b288fe 100644
--- a/lib/lp/services/webapp/tests/test_servers.py
+++ b/lib/lp/services/webapp/tests/test_servers.py
@@ -897,6 +897,14 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
             repository,
             ["repository:build_status", "repository:another_scope"])
 
+    def test_checkRequest_contains_context(self):
+        [ref] = self.factory.makeGitRefs()
+        self._makeAccessTokenVerifiedRequest(
+            target=ref.repository,
+            scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS])
+        getUtility(IWebServiceConfiguration).checkRequest(
+            ref, ["repository:build_status", "repository:another_scope"])
+
     def test_checkRequest_bad_context(self):
         repository = self.factory.makeGitRepository()
         self._makeAccessTokenVerifiedRequest(
diff --git a/lib/lp/services/webservice/configuration.py b/lib/lp/services/webservice/configuration.py
index 8b56842..665c64e 100644
--- a/lib/lp/services/webservice/configuration.py
+++ b/lib/lp/services/webservice/configuration.py
@@ -15,8 +15,13 @@ from zope.security.interfaces import Unauthorized
 from lp.app import versioninfo
 from lp.services.config import config
 from lp.services.database.sqlbase import block_implicit_flushes
+from lp.services.webapp.canonicalurl import nearest_adapter
 from lp.services.webapp.interaction import get_interaction_extras
-from lp.services.webapp.interfaces import ILaunchBag
+from lp.services.webapp.interfaces import (
+    ILaunchBag,
+    ILaunchpadContainer,
+    )
+from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.servers import (
     WebServiceClientRequest,
     WebServicePublication,
@@ -102,9 +107,19 @@ class LaunchpadWebServiceConfiguration(BaseWebServiceConfiguration):
         access_token = get_interaction_extras().access_token
         if access_token is None:
             return
-        if access_token.target != context:
-            raise Unauthorized(
-                "Current authentication does not allow access to this object.")
+
+        # The access token must be for a target that either exactly matches
+        # or contains the context object.
+        if access_token.target == context:
+            pass
+        else:
+            container = nearest_adapter(context, ILaunchpadContainer)
+            if not container.isWithin(
+                    canonical_url(access_token.target, force_local_path=True)):
+                raise Unauthorized(
+                    "Current authentication does not allow access to this "
+                    "object.")
+
         if not required_scopes:
             raise Unauthorized(
                 "Current authentication only allows calling scoped methods.")