Merge ~ines-almeida/launchpad:project-tokens/refactor-access-token-tests into launchpad:master

[Ines Almeida]

Refactor auth model and webapp auth tests

Preparatory refactoring to make it cleaner to add tests for new access token targets

c9bebdc... by Ines Almeida on 2023-09-15

Minor refactoring of xmlrpc.tests.test_git

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 0467947..15e4f09 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -1161,6 +1161,15 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
+    def _makeGitRepositoryWithRefs(self, **kwargs):
+        """Helper method to create a git repository with a default branch"""
+        repository = self.factory.makeGitRepository(**kwargs)
+        self.factory.makeGitRefs(
+            repository=repository, paths=["refs/heads/main"]
+        )
+        removeSecurityProxy(repository).default_branch = "refs/heads/main"
+        return repository
+
     def test_confirm_git_repository_creation(self):
         owner = self.factory.makePerson()
         repo = removeSecurityProxy(self.factory.makeGitRepository(owner=owner))
@@ -2821,11 +2830,7 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         # pushed by a user that has their ordinary privileges on the
         # corresponding repository.
         requester_owner = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester_owner)
-        self.factory.makeGitRefs(
-            repository=repository, paths=["refs/heads/master"]
-        )
-        removeSecurityProxy(repository).default_branch = "refs/heads/master"
+        repository = self._makeGitRepositoryWithRefs(owner=requester_owner)
         pushed_branch = "branch1"
         self.assertHasMergeProposalURL(
             repository, pushed_branch, {"uid": requester_owner.id}
@@ -2857,12 +2862,8 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
 
         self.pushConfig("codehosting", git_macaroon_secret_key="some-secret")
         requester = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester)
+        repository = self._makeGitRepositoryWithRefs(owner=requester)
         issuer = getUtility(IMacaroonIssuer, "git-repository")
-        self.factory.makeGitRefs(
-            repository=repository, paths=["refs/heads/master"]
-        )
-        removeSecurityProxy(repository).default_branch = "refs/heads/master"
 
         pushed_branch = "branch1"
         with person_logged_in(requester):
@@ -2890,11 +2891,7 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         )
         requesters = [self.factory.makePerson() for _ in range(2)]
         owner = self.factory.makeTeam(members=requesters)
-        repository = self.factory.makeGitRepository(owner=owner)
-        self.factory.makeGitRefs(
-            repository=repository, paths=["refs/heads/master"]
-        )
-        removeSecurityProxy(repository).default_branch = "refs/heads/master"
+        repository = self._makeGitRepositoryWithRefs(owner=owner)
         pushed_branch = "branch1"
         macaroon = issuer.issueMacaroon(repository)
 
@@ -2935,11 +2932,7 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         # pushed by a user with a suitable access token that has their
         # ordinary privileges on the corresponding repository.
         requester = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester)
-        self.factory.makeGitRefs(
-            repository=repository, paths=["refs/heads/main"]
-        )
-        removeSecurityProxy(repository).default_branch = "refs/heads/main"
+        repository = self._makeGitRepositoryWithRefs(owner=requester)
         _, token = self.factory.makeAccessToken(
             owner=requester,
             target=repository,
@@ -2954,11 +2947,7 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         # getMergeProposalURL refuses access tokens for a different
         # repository.
         requester = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester)
-        self.factory.makeGitRefs(
-            repository=repository, paths=["refs/heads/main"]
-        )
-        removeSecurityProxy(repository).default_branch = "refs/heads/main"
+        repository = self._makeGitRepositoryWithRefs(owner=requester)
         _, token = self.factory.makeAccessToken(
             owner=requester, scopes=[AccessTokenScope.REPOSITORY_PUSH]
         )
diff --git a/lib/lp/services/auth/tests/test_model.py b/lib/lp/services/auth/tests/test_model.py
index b33ccf7..ca33eec 100644
--- a/lib/lp/services/auth/tests/test_model.py
+++ b/lib/lp/services/auth/tests/test_model.py
@@ -46,31 +46,36 @@ from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import webservice_for_person
 
 
-class TestAccessToken(TestCaseWithFactory):
+class TestAccessTokenBase:
     layer = DatabaseFunctionalLayer
 
     def test_owner_can_edit(self):
         owner = self.factory.makePerson()
-        _, token = self.factory.makeAccessToken(owner=owner)
+        _, token = self.factory.makeAccessToken(
+            owner=owner, target=self.makeTarget()
+        )
         login_person(owner)
         self.assertTrue(check_permission("launchpad.Edit", token))
 
     def test_target_owner_can_edit(self):
         target_owner = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=target_owner)
-        _, token = self.factory.makeAccessToken(target=repository)
+        _, token = self.factory.makeAccessToken(
+            target=self.makeTarget(target_owner)
+        )
         login_person(target_owner)
         self.assertTrue(check_permission("launchpad.Edit", token))
 
     def test_other_user_cannot_edit(self):
-        _, token = self.factory.makeAccessToken()
+        _, token = self.factory.makeAccessToken(target=self.makeTarget())
         login_person(self.factory.makePerson())
         self.assertFalse(check_permission("launchpad.Edit", token))
 
     def test_updateLastUsed_never_used(self):
         # If the token has never been used, we update its last-used date.
         owner = self.factory.makePerson()
-        _, token = self.factory.makeAccessToken(owner=owner)
+        _, token = self.factory.makeAccessToken(
+            owner=owner, target=self.makeTarget()
+        )
         login_person(owner)
         self.assertIsNone(token.date_last_used)
         transaction.commit()
@@ -82,7 +87,9 @@ class TestAccessToken(TestCaseWithFactory):
         # If the token's last-used date was updated recently, we leave it
         # alone.
         owner = self.factory.makePerson()
-        _, token = self.factory.makeAccessToken(owner=owner)
+        _, token = self.factory.makeAccessToken(
+            owner=owner, target=self.makeTarget()
+        )
         login_person(owner)
         recent = datetime.now(timezone.utc) - timedelta(minutes=1)
         removeSecurityProxy(token).date_last_used = recent
@@ -94,7 +101,9 @@ class TestAccessToken(TestCaseWithFactory):
         # If the token's last-used date is outside our update resolution, we
         # update it.
         owner = self.factory.makePerson()
-        _, token = self.factory.makeAccessToken(owner=owner)
+        _, token = self.factory.makeAccessToken(
+            owner=owner, target=self.makeTarget()
+        )
         login_person(owner)
         recent = datetime.now(timezone.utc) - timedelta(hours=1)
         removeSecurityProxy(token).date_last_used = recent
@@ -107,7 +116,9 @@ class TestAccessToken(TestCaseWithFactory):
         # If the token is locked by another transaction, we leave it alone.
         owner = self.factory.makePerson()
         owner_email = removeSecurityProxy(owner.preferredemail).email
-        secret, token = self.factory.makeAccessToken(owner=owner)
+        secret, token = self.factory.makeAccessToken(
+            owner=owner, target=self.makeTarget()
+        )
         login_person(owner)
         self.assertIsNone(token.date_last_used)
         transaction.commit()
@@ -150,7 +161,9 @@ class TestAccessToken(TestCaseWithFactory):
     def test_is_expired(self):
         owner = self.factory.makePerson()
         login_person(owner)
-        _, current_token = self.factory.makeAccessToken(owner=owner)
+        _, current_token = self.factory.makeAccessToken(
+            owner=owner, target=self.makeTarget()
+        )
         _, expired_token = self.factory.makeAccessToken(
             owner=owner,
             date_expires=datetime.now(timezone.utc) - timedelta(minutes=1),
@@ -161,7 +174,9 @@ class TestAccessToken(TestCaseWithFactory):
     def test_revoke(self):
         owner = self.factory.makePerson()
         _, token = self.factory.makeAccessToken(
-            owner=owner, scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]
+            owner=owner,
+            scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS],
+            target=self.makeTarget(),
         )
         login_person(owner)
         self.assertThat(
@@ -177,7 +192,12 @@ class TestAccessToken(TestCaseWithFactory):
         )
 
 
-class TestAccessTokenSet(TestCaseWithFactory):
+class TestAccessTokenGitRepository(TestAccessTokenBase, TestCaseWithFactory):
+    def makeTarget(self, owner=None):
+        return self.factory.makeGitRepository(owner=owner)
+
+
+class TestAccessTokenSetBase:
     layer = DatabaseFunctionalLayer
 
     def test_new(self):
@@ -185,7 +205,7 @@ class TestAccessTokenSet(TestCaseWithFactory):
         self.assertEqual(64, len(secret))
         owner = self.factory.makePerson()
         description = "Test token"
-        target = self.factory.makeGitRepository()
+        target = self.makeTarget()
         scopes = [AccessTokenScope.REPOSITORY_BUILD_STATUS]
         _, token = self.factory.makeAccessToken(
             secret=secret,
@@ -206,13 +226,13 @@ class TestAccessTokenSet(TestCaseWithFactory):
         )
 
     def test_getByID(self):
-        secret, token = self.factory.makeAccessToken()
+        secret, token = self.factory.makeAccessToken(target=self.makeTarget())
         token_id = removeSecurityProxy(token).id
         self.assertEqual(token, getUtility(IAccessTokenSet).getByID(token_id))
         self.assertIsNone(getUtility(IAccessTokenSet).getByID(token_id + 1))
 
     def test_getBySecret(self):
-        secret, token = self.factory.makeAccessToken()
+        secret, token = self.factory.makeAccessToken(target=self.makeTarget())
         self.assertEqual(
             token, getUtility(IAccessTokenSet).getBySecret(secret)
         )
@@ -225,9 +245,15 @@ class TestAccessTokenSet(TestCaseWithFactory):
     def test_findByOwner(self):
         owners = [self.factory.makePerson() for _ in range(3)]
         tokens = [
-            self.factory.makeAccessToken(owner=owners[0])[1],
-            self.factory.makeAccessToken(owner=owners[0])[1],
-            self.factory.makeAccessToken(owner=owners[1])[1],
+            self.factory.makeAccessToken(
+                owner=owners[0], target=self.makeTarget()
+            )[1],
+            self.factory.makeAccessToken(
+                owner=owners[0], target=self.makeTarget()
+            )[1],
+            self.factory.makeAccessToken(
+                owner=owners[1], target=self.makeTarget()
+            )[1],
         ]
         self.assertContentEqual(
             tokens[:2], getUtility(IAccessTokenSet).findByOwner(owners[0])
@@ -240,7 +266,7 @@ class TestAccessTokenSet(TestCaseWithFactory):
         )
 
     def test_findByTarget(self):
-        targets = [self.factory.makeGitRepository() for _ in range(3)]
+        targets = [self.makeTarget() for _ in range(3)]
         tokens = [
             self.factory.makeAccessToken(target=targets[0])[1],
             self.factory.makeAccessToken(target=targets[0])[1],
@@ -257,7 +283,7 @@ class TestAccessTokenSet(TestCaseWithFactory):
         )
 
     def test_findByTarget_visible_by_user(self):
-        targets = [self.factory.makeGitRepository() for _ in range(3)]
+        targets = [self.makeTarget() for _ in range(3)]
         owners = [self.factory.makePerson() for _ in range(3)]
         tokens = [
             self.factory.makeAccessToken(
@@ -290,7 +316,7 @@ class TestAccessTokenSet(TestCaseWithFactory):
             )
 
     def test_findByTarget_excludes_expired(self):
-        target = self.factory.makeGitRepository()
+        target = self.makeTarget()
         _, current_token = self.factory.makeAccessToken(target=target)
         _, expires_soon_token = self.factory.makeAccessToken(
             target=target,
@@ -312,7 +338,7 @@ class TestAccessTokenSet(TestCaseWithFactory):
         )
 
     def test_getByTargetAndID(self):
-        targets = [self.factory.makeGitRepository() for _ in range(3)]
+        targets = [self.makeTarget() for _ in range(3)]
         tokens = [
             self.factory.makeAccessToken(target=targets[0])[1],
             self.factory.makeAccessToken(target=targets[0])[1],
@@ -337,7 +363,7 @@ class TestAccessTokenSet(TestCaseWithFactory):
         )
 
     def test_getByTargetAndID_visible_by_user(self):
-        targets = [self.factory.makeGitRepository() for _ in range(3)]
+        targets = [self.makeTarget() for _ in range(3)]
         owners = [self.factory.makePerson() for _ in range(3)]
         tokens = [
             self.factory.makeAccessToken(
@@ -374,7 +400,7 @@ class TestAccessTokenSet(TestCaseWithFactory):
                     self.assertIsNone(fetched_token)
 
     def test_getByTargetAndID_excludes_expired(self):
-        target = self.factory.makeGitRepository()
+        target = self.makeTarget()
         _, current_token = self.factory.makeAccessToken(target=target)
         _, expires_soon_token = self.factory.makeAccessToken(
             target=target,
@@ -403,6 +429,13 @@ class TestAccessTokenSet(TestCaseWithFactory):
         )
 
 
+class TestGitRepositoryAccessTokenSet(
+    TestAccessTokenSetBase, TestCaseWithFactory
+):
+    def makeTarget(self):
+        return self.factory.makeGitRepository()
+
+
 class TestAccessTokenTargetBase:
     layer = DatabaseFunctionalLayer
 
diff --git a/lib/lp/services/webapp/tests/test_servers.py b/lib/lp/services/webapp/tests/test_servers.py
index f865502..e777ac2 100644
--- a/lib/lp/services/webapp/tests/test_servers.py
+++ b/lib/lp/services/webapp/tests/test_servers.py
@@ -778,7 +778,7 @@ class LoggingTransaction:
         self.log.append("ABORT")
 
 
-class TestWebServiceAccessTokens(TestCaseWithFactory):
+class TestWebServiceAccessTokensBase:
     """Test personal access tokens for the webservice.
 
     These are bearer tokens with an owner, a context, and some scopes.  We
@@ -791,7 +791,9 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
     def test_valid(self):
         owner = self.factory.makePerson()
         secret, token = self.factory.makeAccessToken(
-            owner=owner, scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]
+            owner=owner,
+            target=self.makeTarget(owner=owner),
+            scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS],
         )
         self.assertIsNone(removeSecurityProxy(token).date_last_used)
         transaction.commit()
@@ -828,6 +830,7 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
         owner = self.factory.makePerson()
         secret, token = self.factory.makeAccessToken(
             owner=owner,
+            target=self.makeTarget(owner=owner),
             date_expires=datetime.now(timezone.utc) - timedelta(days=1),
         )
         transaction.commit()
@@ -859,7 +862,9 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
 
     def test_inactive_account(self):
         owner = self.factory.makePerson(account_status=AccountStatus.SUSPENDED)
-        secret, token = self.factory.makeAccessToken(owner=owner)
+        secret, token = self.factory.makeAccessToken(
+            owner=owner, target=self.makeTarget(owner=owner)
+        )
         transaction.commit()
 
         request, publication = get_request_and_publication(
@@ -889,13 +894,13 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
         request.setPrincipal(principal)
 
     def test_checkRequest_valid(self):
-        repository = self.factory.makeGitRepository()
+        target = self.makeTarget()
         self._makeAccessTokenVerifiedRequest(
-            target=repository,
+            target=target,
             scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS],
         )
         getUtility(IWebServiceConfiguration).checkRequest(
-            repository, ["repository:build_status", "repository:another_scope"]
+            target, ["repository:build_status", "repository:another_scope"]
         )
 
     def test_checkRequest_contains_context(self):
@@ -909,9 +914,9 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
         )
 
     def test_checkRequest_bad_context(self):
-        repository = self.factory.makeGitRepository()
+        target = self.makeTarget()
         self._makeAccessTokenVerifiedRequest(
-            target=repository,
+            target=target,
             scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS],
         )
         self.assertRaisesWithContent(
@@ -923,23 +928,23 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
         )
 
     def test_checkRequest_unscoped_method(self):
-        repository = self.factory.makeGitRepository()
+        target = self.makeTarget()
         self._makeAccessTokenVerifiedRequest(
-            target=repository,
+            target=target,
             scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS],
         )
         self.assertRaisesWithContent(
             Unauthorized,
             "Current authentication only allows calling scoped methods.",
             getUtility(IWebServiceConfiguration).checkRequest,
-            repository,
+            target,
             None,
         )
 
     def test_checkRequest_wrong_scope(self):
-        repository = self.factory.makeGitRepository()
+        target = self.makeTarget()
         self._makeAccessTokenVerifiedRequest(
-            target=repository,
+            target=target,
             scopes=[
                 AccessTokenScope.REPOSITORY_BUILD_STATUS,
                 AccessTokenScope.REPOSITORY_PUSH,
@@ -951,11 +956,18 @@ class TestWebServiceAccessTokens(TestCaseWithFactory):
             "(one of these scopes is required: "
             "'repository:scope_1', 'repository:scope_2').",
             getUtility(IWebServiceConfiguration).checkRequest,
-            repository,
+            target,
             ["repository:scope_1", "repository:scope_2"],
         )
 
 
+class TestWebServiceAccessTokensGitRepository(
+    TestWebServiceAccessTokensBase, TestCaseWithFactory
+):
+    def makeTarget(self, owner=None):
+        return self.factory.makeGitRepository(owner=owner)
+
+
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(