Get rid of IdentityBasedPolicy base class

Factor out the `IdentityBasedPolicy` base class, favouring helper
methods that're used by composition instead.

This base class is making future refactorings (see
#8860 and
#8861) more difficult to reason
about by coupling together different security policy classes that I want
to evolve in separate directions.

The base class is also responsible for some disturbing unintended
behaviours. For example, on `main`:

* `IdentityBasedPolicy.authenticated_userid()` calls `identity()` (which
  must be implemented by `IdentityBasedPolicy` sub classes).

* `CookiePolicy` has an `identity()` method and inherits from
  `IdentityBasedPolicy`, so `CookiePolicy` inherits
  `IdentityBasedPolicy`'s `authenticated_userid()` method that calls
  `CookiePolicy.identity()`

* As with all h's security policies `CookiePolicy` is never used
  directly as a security policy, rather it's always delegated to by
  `TopLevelPolicy`

* `TopLevelPolicy` has an `identity()` method that delegates to
  `CookiePolicy.identity()` for HTML pages, and `TopLevelPolicy` also
  inherits from `IdentityBasedPolicy`, so `TopLevelPolicy` inherits an
  `authenticated_userid()` method that calls `TopLevelPolicy.identity()`.

* So the end result is that `CookiePolicy.authenticated_userid()` (which
  is inherited from `IdentityBasedPolicy`) is never actually used!
  Instead `TopLevelPolicy`'s copy of `authenticated_userid()` (also
  inherited from `TopLevelPolicy`) is used and calls
  `TopLevelPolicy.identity()` which delegates to
  `CookiePolicy.identity()`.

The same thing happens when `APIPolicy` (which also inherits from
`IdentityBasedPolicy`) delegates to API subpolicies.

This is true for the `authenticated_userid()` and `permits()` methods of
all h's security policies other than `TopLevelPolicy`: none of them are
ever called.

This hasn't resulted in any bugs because all the sub-policies and
`TopLevelPolicy` inherit same `authenticated_userid()` and `permits()`
methods from `IdentityBasedPolicy`, so `TopLevelPolicy`'s by-passing the
`authenticated_userid()` and `permits()` methods of sub-policies makes
no difference. In fact you could remove the `IdentityBasedPolicy` base
class from all classes except `TopLevelPolicy` and it would make no
difference, the methods that all those classes inherit from
`IdentityBasedPolicy` are never called.

But clearly this is very confusing and unintended, and would lead to
bugs if there's ever a sub-policy in future that has a different
`authenticated_userid()` or `permits()` method.

h/security/identity.py

@@ -1,7 +1,7 @@
 """Data classes used to represent authenticated users."""
 
 from dataclasses import dataclass
-from typing import List, Optional
+from typing import List, Optional, Self
 
 from h.models import AuthClient, Group, User
 
@@ -94,3 +94,11 @@ def from_models(cls, user: User = None, auth_client: AuthClient = None):
                 LongLivedAuthClient.from_model(auth_client) if auth_client else None
             ),
         )
+
+    @staticmethod
+    def authenticated_userid(identity: Self | None) -> str | None:
+        """Return the authenticated_userid from the given identity."""
+        if identity and identity.user:
+            return identity.user.userid
+
+        return None

h/security/policy/_api.py

@@ -1,10 +1,11 @@
 from pyramid.request import Request, RequestLocalCache
+from pyramid.security import Allowed, Denied
 
 from h.security.identity import Identity
-from h.security.policy._identity_base import IdentityBasedPolicy
+from h.security.permits import identity_permits
 
 
-class APIPolicy(IdentityBasedPolicy):
+class APIPolicy:
     """The security policy for API requests. Delegates to subpolicies."""
 
     def __init__(self, sub_policies):
@@ -18,10 +19,16 @@ def forget(self, *_args, **_kwargs):
     def identity(self, request) -> Identity | None:
         return self._identity_cache.get_or_create(request)
 
+    def authenticated_userid(self, request):
+        return Identity.authenticated_userid(self.identity(request))
+
     def remember(self, *_args, **_kwargs):
         # remember() isn't supported for stateless API requests.
         return []
 
+    def permits(self, request, context, permission) -> Allowed | Denied:
+        return identity_permits(self.identity(request), context, permission)
+
     def _load_identity(self, request: Request) -> Identity | None:
         for policy in applicable_policies(request, self.sub_policies):
             identity = policy.identity(request)

h/security/policy/_auth_client.py

@@ -1,16 +1,17 @@
 import hmac
 
 from pyramid.authentication import extract_http_basic_credentials
+from pyramid.security import Allowed, Denied
 from sqlalchemy.exc import StatementError
 
 from h.exceptions import InvalidUserId
 from h.models import AuthClient
 from h.models.auth_client import GrantType
 from h.security.identity import Identity
-from h.security.policy._identity_base import IdentityBasedPolicy
+from h.security.permits import identity_permits
 
 
-class AuthClientPolicy(IdentityBasedPolicy):
+class AuthClientPolicy:
     """
     An authentication policy for registered AuthClients.
 
@@ -80,6 +81,12 @@ def identity(self, request):
 
         return Identity.from_models(auth_client=auth_client, user=user)
 
+    def authenticated_userid(self, request):
+        return Identity.authenticated_userid(self.identity(request))
+
+    def permits(self, request, context, permission) -> Allowed | Denied:
+        return identity_permits(self.identity(request), context, permission)
+
     @classmethod
     def _get_auth_client(cls, request):
         """Get a matching auth client if the credentials are valid."""

h/security/policy/_cookie.py

@@ -3,13 +3,14 @@
 from os import urandom
 
 import webob
+from pyramid.security import Allowed, Denied
 
 from h.security.identity import Identity
-from h.security.policy._identity_base import IdentityBasedPolicy
+from h.security.permits import identity_permits
 from h.services.auth_ticket import AuthTicketService
 
 
-class CookiePolicy(IdentityBasedPolicy):
+class CookiePolicy:
     """
     An authentication policy based on cookies.
 
@@ -32,6 +33,9 @@ def identity(self, request):
 
         return Identity.from_models(user=user)
 
+    def authenticated_userid(self, request):
+        return Identity.authenticated_userid(self.identity(request))
+
     def remember(self, request, userid, **kw):  # pylint:disable=unused-argument
         """Get a list of headers which will remember the user in a cookie."""
 
@@ -70,6 +74,9 @@ def forget(self, request):
 
         return self.cookie.get_headers(None, max_age=0)
 
+    def permits(self, request, context, permission) -> Allowed | Denied:
+        return identity_permits(self.identity(request), context, permission)
+
     @staticmethod
     @lru_cache  # Ensure we only add this once per request
     def _add_vary_by_cookie(request):

h/security/policy/bearer_token.py

@@ -1,11 +1,12 @@
 from pyramid.request import RequestLocalCache
+from pyramid.security import Allowed, Denied
 
 from h.security.identity import Identity
-from h.security.policy._identity_base import IdentityBasedPolicy
+from h.security.permits import identity_permits
 from h.security.policy.helpers import is_api_request
 
 
-class BearerTokenPolicy(IdentityBasedPolicy):
+class BearerTokenPolicy:
     """
     A Bearer token authentication policy.
 
@@ -37,6 +38,12 @@ def identity(self, request):
 
         return self._identity_cache.get_or_create(request)
 
+    def authenticated_userid(self, request):
+        return Identity.authenticated_userid(self.identity(request))
+
+    def permits(self, request, context, permission) -> Allowed | Denied:
+        return identity_permits(self.identity(request), context, permission)
+
     def _load_identity(self, request):
         token_svc = request.find_service(name="auth_token")
         token_str = None

h/security/policy/top_level.py

@@ -1,17 +1,17 @@
 import webob
 from pyramid.request import RequestLocalCache
+from pyramid.security import Allowed, Denied
 
 from h.security.identity import Identity
 from h.security.policy._api import APIPolicy
 from h.security.policy._api_cookie import APICookiePolicy
 from h.security.policy._auth_client import AuthClientPolicy
 from h.security.policy._cookie import CookiePolicy
-from h.security.policy._identity_base import IdentityBasedPolicy
 from h.security.policy.bearer_token import BearerTokenPolicy
 from h.security.policy.helpers import is_api_request
 
 
-class TopLevelPolicy(IdentityBasedPolicy):
+class TopLevelPolicy:
     """The top-level security policy. Delegates to subpolicies."""
 
     def __init__(self):
@@ -24,10 +24,16 @@ def forget(self, request, **kw):
     def identity(self, request) -> Identity | None:
         return self._identity_cache.get_or_create(request)
 
+    def authenticated_userid(self, request):
+        return get_subpolicy(request).authenticated_userid(request)
+
     def remember(self, request, userid, **kw):
         self._identity_cache.clear(request)
         return get_subpolicy(request).remember(request, userid, **kw)
 
+    def permits(self, request, context, permission) -> Allowed | Denied:
+        return get_subpolicy(request).permits(request, context, permission)
+
     def _load_identity(self, request):
         return get_subpolicy(request).identity(request)
 

tests/unit/h/security/identity_test.py

@@ -82,6 +82,29 @@ def test_from_models_with_None(self, LongLivedUser, LongLivedAuthClient):
             {"user": None, "auth_client": None}
         )
 
+    @pytest.mark.parametrize(
+        "identity,authenticated_userid",
+        [
+            (None, None),
+            (Identity(user=None), None),
+            (
+                Identity(
+                    user=LongLivedUser(
+                        id=sentinel.id,
+                        userid=sentinel.userid,
+                        authority=sentinel.authority,
+                        groups=[],
+                        staff=False,
+                        admin=False,
+                    )
+                ),
+                sentinel.userid,
+            ),
+        ],
+    )
+    def test_authenticated_userid(self, identity, authenticated_userid):
+        assert Identity.authenticated_userid(identity) == authenticated_userid
+
     @pytest.fixture(autouse=True)
     def LongLivedUser(self, patch):
         return patch("h.security.identity.LongLivedUser")

tests/unit/h/security/policy/_api_test.py

@@ -3,7 +3,6 @@
 import pytest
 from pyramid.request import Request
 
-from h.security.identity import Identity
 from h.security.policy._api import APIPolicy, applicable_policies
 
 
@@ -60,13 +59,37 @@ def test_identity_when_there_are_no_applicable_policies(
 
         assert identity is None
 
+    def test_authenticated_userid(self, api_policy, pyramid_request, Identity, mocker):
+        mocker.spy(api_policy, "identity")
+
+        authenticated_userid = api_policy.authenticated_userid(pyramid_request)
+
+        api_policy.identity.assert_called_once_with(pyramid_request)
+        Identity.authenticated_userid.assert_called_once_with(
+            api_policy.identity.spy_return
+        )
+        assert authenticated_userid == Identity.authenticated_userid.return_value
+
     def test_remember(self, api_policy, pyramid_request):
         assert api_policy.remember(pyramid_request, sentinel.userid, foo="bar") == []
 
+    def test_permits(self, api_policy, pyramid_request, identity_permits, mocker):
+        mocker.spy(api_policy, "identity")
+
+        result = api_policy.permits(
+            pyramid_request, sentinel.context, sentinel.permission
+        )
+
+        api_policy.identity.assert_called_once_with(pyramid_request)
+        identity_permits.assert_called_once_with(
+            api_policy.identity.spy_return, sentinel.context, sentinel.permission
+        )
+        assert result == identity_permits.return_value
+
     @pytest.fixture(autouse=True)
     def applicable_policies(self, mocker):
         class SubpolicySpec:
-            def identity(self, request: Request) -> Identity | None:
+            def identity(self, request: Request):
                 """Return the identity of the current user."""
 
         return mocker.patch(
@@ -112,3 +135,15 @@ def handles(request) -> bool:
         for policy in policies:
             policy.handles.assert_called_once_with(pyramid_request)
         assert returned_policies == [policies[index] for index in expected_policies]
+
+
+@pytest.fixture(autouse=True)
+def Identity(mocker):
+    return mocker.patch("h.security.policy._api.Identity", autospec=True, spec_set=True)
+
+
+@pytest.fixture(autouse=True)
+def identity_permits(mocker):
+    return mocker.patch(
+        "h.security.policy._api.identity_permits", autospec=True, spec_set=True
+    )