launchpad-reviewers team mailing list archive

[Colin Watson <mp+436489@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:git-delete-expired-access-tokens into launchpad:master.

Commit message:
Delete expired access tokens when deleting a Git repository

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Attempting to delete a repository with an expired access token failed with:

  update or delete on table "gitrepository" violates foreign key constraint "accesstoken_git_repository_fkey" on table "accesstoken"

Delete expired access tokens as well as valid ones.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-delete-expired-access-tokens into launchpad:master.

diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 902f87c..f78c20c 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -2121,7 +2121,9 @@ class GitRepository(
         # activity logs for removed repositories anyway.
         self.grants.remove()
         self.rules.remove()
-        removeSecurityProxy(self.getAccessTokens()).remove()
+        removeSecurityProxy(
+            self.getAccessTokens(include_expired=True)
+        ).remove()
         getUtility(IRevisionStatusReportSet).deleteForRepository(self)
         getUtility(ICIBuildSet).deleteByGitRepository(self)
 
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 63a9a09..b21e131 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -1400,6 +1400,10 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
 
     def test_related_access_tokens_deleted(self):
         _, token = self.factory.makeAccessToken(target=self.repository)
+        _, expired_token = self.factory.makeAccessToken(
+            target=self.repository,
+            date_expires=datetime.now(pytz.UTC) - timedelta(minutes=1),
+        )
         other_repository = self.factory.makeGitRepository()
         _, other_token = self.factory.makeAccessToken(target=other_repository)
         self.repository.destroySelf()
@@ -1408,6 +1412,12 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
         self.assertRaises(
             LostObjectError, getattr, removeSecurityProxy(token), "target"
         )
+        self.assertRaises(
+            LostObjectError,
+            getattr,
+            removeSecurityProxy(expired_token),
+            "target",
+        )
         # An unrelated repository's access tokens are still present.
         self.assertEqual(
             other_repository, removeSecurityProxy(other_token).target
diff --git a/lib/lp/services/auth/interfaces.py b/lib/lp/services/auth/interfaces.py
index dd3315c..be90837 100644
--- a/lib/lp/services/auth/interfaces.py
+++ b/lib/lp/services/auth/interfaces.py
@@ -175,12 +175,15 @@ class IAccessTokenSet(Interface):
         :param owner: An `IPerson`.
         """
 
-    def findByTarget(target, visible_by_user=None):
+    def findByTarget(target, visible_by_user=None, include_expired=False):
         """Return all access tokens for this target.
 
         :param target: An `IAccessTokenTarget`.
         :param visible_by_user: If given, return only access tokens visible
             by this user.
+        :param include_expired: If True, include expired access tokens.
+            This must only be used for non-authentication purposes, such as
+            deleting database rows.
         """
 
     def getByTargetAndID(target, token_id, visible_by_user=None):
@@ -208,8 +211,15 @@ class IAccessTokenTarget(Interface):
     @operation_returns_collection_of(IAccessToken)
     @export_read_operation()
     @operation_for_version("devel")
-    def getAccessTokens(visible_by_user=None):
-        """Return personal access tokens for this target."""
+    def getAccessTokens(visible_by_user=None, include_expired=False):
+        """Return personal access tokens for this target.
+
+        :param visible_by_user: If given, return only access tokens visible
+            by this user.
+        :param include_expired: If True, include expired access tokens.
+            This must only be used for non-authentication purposes, such as
+            deleting database rows.
+        """
 
 
 patch_reference_property(IAccessToken, "target", IAccessTokenTarget)
diff --git a/lib/lp/services/auth/model.py b/lib/lp/services/auth/model.py
index c436650..f2e1d3a 100644
--- a/lib/lp/services/auth/model.py
+++ b/lib/lp/services/auth/model.py
@@ -175,7 +175,9 @@ class AccessTokenSet:
         """See `IAccessTokenSet`."""
         return IStore(AccessToken).find(AccessToken, owner=owner)
 
-    def findByTarget(self, target, visible_by_user=None):
+    def findByTarget(
+        self, target, visible_by_user=None, include_expired=False
+    ):
         """See `IAccessTokenSet`."""
         clauses = []
         if IGitRepository.providedBy(target):
@@ -203,12 +205,13 @@ class AccessTokenSet:
                 )
         else:
             raise TypeError("Unsupported target: {!r}".format(target))
-        clauses.append(
-            Or(
-                AccessToken.date_expires == None,
-                AccessToken.date_expires > UTC_NOW,
+        if not include_expired:
+            clauses.append(
+                Or(
+                    AccessToken.date_expires == None,
+                    AccessToken.date_expires > UTC_NOW,
+                )
             )
-        )
         return (
             IStore(AccessToken)
             .find(AccessToken, *clauses)
@@ -227,8 +230,10 @@ class AccessTokenSet:
 class AccessTokenTargetMixin:
     """Mix this into classes that implement `IAccessTokenTarget`."""
 
-    def getAccessTokens(self, visible_by_user=None):
+    def getAccessTokens(self, visible_by_user=None, include_expired=False):
         """See `IAccessTokenTarget`."""
         return getUtility(IAccessTokenSet).findByTarget(
-            self, visible_by_user=visible_by_user
+            self,
+            visible_by_user=visible_by_user,
+            include_expired=include_expired,
         )
diff --git a/lib/lp/services/auth/tests/test_model.py b/lib/lp/services/auth/tests/test_model.py
index 0823994..81f3023 100644
--- a/lib/lp/services/auth/tests/test_model.py
+++ b/lib/lp/services/auth/tests/test_model.py
@@ -297,6 +297,12 @@ class TestAccessTokenSet(TestCaseWithFactory):
             [current_token, expires_soon_token],
             getUtility(IAccessTokenSet).findByTarget(target),
         )
+        self.assertContentEqual(
+            [current_token, expires_soon_token, expired_token],
+            getUtility(IAccessTokenSet).findByTarget(
+                target, include_expired=True
+            ),
+        )
 
     def test_getByTargetAndID(self):
         targets = [self.factory.makeGitRepository() for _ in range(3)]
@@ -420,6 +426,26 @@ class TestAccessTokenTargetBase:
             [entry["description"] for entry in response.jsonBody()["entries"]],
         )
 
+    def test_getAccessTokens_excludes_expired(self):
+        with person_logged_in(self.owner):
+            self.factory.makeAccessToken(
+                owner=self.owner, description="Current", target=self.target
+            )
+            self.factory.makeAccessToken(
+                owner=self.owner,
+                description="Expired",
+                target=self.target,
+                date_expires=datetime.now(pytz.UTC) - timedelta(minutes=1),
+            )
+        response = self.webservice.named_get(
+            self.target_url, "getAccessTokens", api_version="devel"
+        )
+        self.assertEqual(200, response.status)
+        self.assertContentEqual(
+            ["Current"],
+            [entry["description"] for entry in response.jsonBody()["entries"]],
+        )
+
     def test_getAccessTokens_permissions(self):
         webservice = webservice_for_person(None)
         response = webservice.named_get(