launchpad-reviewers team mailing list archive

[Colin Watson <mp+433826@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-personal-access-token-git into launchpad:master.

Commit message:
Fix handling of the access-token git parameter

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

turnip converts authentication parameters it receives from `authenticateWithPassword` to strings, causing authorization to fail with errors like this:

  remote: Unexpected Zope exception: TypeError: Expected int, found <class 'str'>: '16'

Convert the parameter back to an integer.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-personal-access-token-git into launchpad:master.

diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index cbec524..9793646 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -179,6 +179,12 @@ class GitAPI(LaunchpadXMLRPCView):
             access_token = access_token_set.getBySecret(secret)
         else:
             assert token_id is not None
+            try:
+                # turnip sends authentication parameters as strings.
+                # Convert this back.
+                token_id = int(token_id)
+            except ValueError:
+                return None
             access_token = access_token_set.getByID(token_id)
         if access_token is None:
             return None
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index e7bfe19..6cd603c 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -96,7 +96,10 @@ def _make_auth_params(
     if macaroon_raw is not None:
         auth_params["macaroon"] = macaroon_raw
     if access_token_id is not None:
-        auth_params["access-token"] = access_token_id
+        # turnip marshals its authentication parameters as strings even if
+        # it received them from authenticateWithPassword as integers, so
+        # emulate it.
+        auth_params["access-token"] = str(access_token_id)
     return auth_params
 
 
@@ -2768,6 +2771,24 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
             access_token_id=0,
         )
 
+    def test_translatePath_user_access_token_non_integer(self):
+        # Attempting to pass a non-integer access token ID returns
+        # Unauthorized.
+        requester = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(owner=requester)
+        self.assertUnauthorized(
+            requester,
+            "/%s" % repository.unique_name,
+            permission="read",
+            access_token_id="string",
+        )
+        self.assertUnauthorized(
+            requester,
+            "/%s" % repository.unique_name,
+            permission="write",
+            access_token_id="string",
+        )
+
     def test_getMergeProposalURL_user(self):
         # A merge proposal URL is returned by LP for a non-default branch
         # pushed by a user that has their ordinary privileges on the