launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad/+git/security:inactive-authserver into launchpad:master.

Commit message:
Refuse various authentication tokens from inactive accounts

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Neither SSH authentication nor git-repository macaroons should work.  (At present, Launchpad issues no other macaroons associated with a user; all the rest are free-floating.)
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad/+git/security:inactive-authserver into launchpad:master.

diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 589dcbf..21a6f82 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -196,7 +196,10 @@ from lp.services.database.stormexpr import (
     Values,
     )
 from lp.services.features import getFeatureFlag
-from lp.services.identity.interfaces.account import IAccountSet
+from lp.services.identity.interfaces.account import (
+    AccountStatus,
+    IAccountSet,
+    )
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.macaroons.interfaces import IMacaroonIssuer
@@ -2059,7 +2062,10 @@ class GitRepositoryMacaroonIssuer(MacaroonIssuerBase):
                 caveat_value)
         except LookupError:
             return False
-        ok = IPerson.providedBy(user) and user.account == account
+        ok = (
+            IPerson.providedBy(user) and
+            user.account_status == AccountStatus.ACTIVE and
+            user.account == account)
         if ok:
             verified.user = user
         return ok
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index ce3b938..bdf636b 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -152,6 +152,7 @@ from lp.services.database.constants import UTC_NOW
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import get_transaction_timestamp
 from lp.services.features.testing import FeatureFixture
+from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import JobRunner
@@ -4712,6 +4713,21 @@ class TestGitRepositoryMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
              identifier],
             issuer, macaroon, repository, user=self.factory.makePerson())
 
+    def test_verifyMacaroon_inactive_account(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        naked_account = removeSecurityProxy(repository.owner).account
+        identifier = naked_account.openid_identifiers.any().identifier
+        with admin_logged_in():
+            repository.owner.setAccountStatus(
+                AccountStatus.SUSPENDED, None, "Bye")
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.principal.openid-identifier %s' failed." %
+             identifier],
+            issuer, macaroon, repository, user=repository.owner)
+
     def test_verifyMacaroon_closed_account(self):
         # A closed account no longer has an OpenID identifier, so the
         # corresponding caveat doesn't match.
diff --git a/lib/lp/services/authserver/tests/test_authserver.py b/lib/lp/services/authserver/tests/test_authserver.py
index 5949a80..195be52 100644
--- a/lib/lp/services/authserver/tests/test_authserver.py
+++ b/lib/lp/services/authserver/tests/test_authserver.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the internal codehosting API."""
@@ -24,6 +24,7 @@ from lp.services.authserver.interfaces import (
     )
 from lp.services.authserver.xmlrpc import AuthServerAPIView
 from lp.services.config import config
+from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.librarian.interfaces import (
     ILibraryFileAlias,
     ILibraryFileAliasSet,
@@ -108,6 +109,17 @@ class GetUserAndSSHKeysTests(TestCaseWithFactory):
                      keys=[(key.keytype.title, key.keytext)]),
                 self.authserver.getUserAndSSHKeys(new_person.name))
 
+    def test_inactive_user_with_keys(self):
+        # getUserAndSSHKeys returns the InactiveAccount fault if the given
+        # name refers to an inactive account.
+        new_person = self.factory.makePerson(
+            account_status=AccountStatus.SUSPENDED)
+        with person_logged_in(new_person):
+            self.factory.makeSSHKey(person=new_person)
+            self.assertEqual(
+                faults.InactiveAccount(new_person.name),
+                self.authserver.getUserAndSSHKeys(new_person.name))
+
     def test_via_xmlrpc(self):
         new_person = self.factory.makePerson()
         with person_logged_in(new_person):
diff --git a/lib/lp/services/authserver/xmlrpc.py b/lib/lp/services/authserver/xmlrpc.py
index 98ebb0c..4154424 100644
--- a/lib/lp/services/authserver/xmlrpc.py
+++ b/lib/lp/services/authserver/xmlrpc.py
@@ -25,6 +25,7 @@ from lp.services.authserver.interfaces import (
     IAuthServer,
     IAuthServerApplication,
     )
+from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.macaroons.interfaces import (
     BadMacaroonContext,
@@ -45,6 +46,8 @@ class AuthServerAPIView(LaunchpadXMLRPCView):
         person = getUtility(IPersonSet).getByName(name)
         if person is None:
             return faults.NoSuchPersonWithName(name)
+        if person.account_status != AccountStatus.ACTIVE:
+            return faults.InactiveAccount(name)
         return {
             'id': person.id,
             'name': person.name,
diff --git a/lib/lp/xmlrpc/faults.py b/lib/lp/xmlrpc/faults.py
index 53491c3..5b7d95b 100644
--- a/lib/lp/xmlrpc/faults.py
+++ b/lib/lp/xmlrpc/faults.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Launchpad XMLRPC faults."""
@@ -18,6 +18,7 @@ __all__ = [
     'FileBugGotProductAndDistro',
     'FileBugMissingProductOrDistribution',
     'GitRepositoryNotFound',
+    'InactiveAccount',
     'InvalidBranchIdentifier',
     'InvalidBranchName',
     'InvalidBranchUniqueName',
@@ -508,3 +509,13 @@ class Unauthorized(LaunchpadFault):
 
     def __init__(self, message="Authorisation required."):
         LaunchpadFault.__init__(self, message=message)
+
+
+class InactiveAccount(LaunchpadFault):
+    """The requested Launchpad account is inactive."""
+
+    error_code = 420
+    msg_template = 'Inactive account: %(person_name)s'
+
+    def __init__(self, person_name):
+        LaunchpadFault.__init__(self, person_name=person_name)