Use a separate auth cookie for API requests (fixed version)

Fixes #8914.

This reverts commit 87b8133 and adds a
change that should avoid ever getting into a situation where a browser
has an HTML auth cookie with a valid auth ticket in the DB but has an
API auth cookie whose auth ticket has expired:

Each API auth cookie and its paired HTML auth cookie now share the same
auth ticket in the DB.

This is a second attempt at getting h to use two separate auth cookies:
one for HTML page requests and another for JSON API requests. This
enables the API auth cookie to be more secure: it can use
`SameSite=Strict` rather than `Lax`, which gives better protection
against CSRF attacks but would not be desirable for the HTML auth cookie
(because whenever a user followed a link from another site to Hypothesis
they would find themselves not authenticated).

History:

* An earlier attempt to introduce separate auth cookies was made in:
  #8861

* There was an issue that the API auth cookie and it's paired HTML auth
  cookie each had their own corresponding auth _ticket_ in the DB. The
  HTML auth cookie's ticket's expiry time would be reset each time an
  HTML page request was made, but the API auth cookie's ticket's expiry
  time would only be reset when a JSON API request was made (which is much
  rarer). As a result the API auth cookies would end up unusable because
  their auth tickets had expired, whilst the HTML auth cookies were still
  usable. This created a situation where the browser had a usable HTML
  auth cookie so it could load HTML pages and the user would appear to be
  logged in, but the browser did not have a usable API auth cookie so any
  API requests made by those HTML pages would fail and the user would see
  errors. See: #8914

* As a result of this issue the API auth cookie was "reverted" in
  #8913 (note: not an actual revert,
  just the minimal change necessary to make h once again use a single
  cookie for both HTML and API requests).

* This commit now reintroduces the reverted change, plus a fix for the
  issue.

h/security/policy/_api_cookie.py

@@ -29,7 +29,7 @@ def handles(request: Request) -> bool:
         ) in COOKIE_AUTHENTICATABLE_API_REQUESTS
 
     def identity(self, request: Request) -> Identity | None:
-        identity = self.helper.identity(self.cookie, request)
+        identity, _ = self.helper.identity(self.cookie, request)
 
         if identity is None:
             return identity

h/security/policy/_cookie.py

@@ -30,7 +30,7 @@ def __init__(
 
     def identity(self, request):
         self.helper.add_vary_by_cookie(request)
-        identity = self.helper.identity(self.html_authcookie, request)
+        identity, ticket_id = self.helper.identity(self.html_authcookie, request)
 
         # If a request was successfully authenticated using the HTML auth
         # cookie and that request did *not* also contain the API auth cookie,
@@ -47,7 +47,7 @@ def identity(self, request):
         # this won't happen but it could happen if the API auth cookie (but not
         # the HTML one) was deleted somehow, for example by the user manually
         # deleting the cookie in the browser's developer tools, or another way.
-        self._issue_api_authcookie(identity, request)
+        self._issue_api_authcookie(identity, request, ticket_id)
 
         return identity
 
@@ -76,9 +76,11 @@ def remember(self, request, userid, **kw):  # pylint:disable=unused-argument
         # would set the same cookie twice.
         request.h_api_authcookie_headers_added = True
 
+        ticket_id = self.helper.add_ticket(request, userid)
+
         return [
-            *self.helper.remember(self.html_authcookie, request, userid),
-            *self.helper.remember(self.api_authcookie, request, userid),
+            *self.helper.remember(self.html_authcookie, userid, ticket_id),
+            *self.helper.remember(self.api_authcookie, userid, ticket_id),
         ]
 
     def forget(self, request):
@@ -91,7 +93,7 @@ def forget(self, request):
     def permits(self, request, context, permission) -> Allowed | Denied:
         return identity_permits(self.identity(request), context, permission)
 
-    def _issue_api_authcookie(self, identity, request):
+    def _issue_api_authcookie(self, identity, request, ticket_id):
         if not identity:
             return
 
@@ -105,7 +107,7 @@ def _issue_api_authcookie(self, identity, request):
             return
 
         headers = self.helper.remember(
-            self.api_authcookie, request, identity.user.userid
+            self.api_authcookie, identity.user.userid, ticket_id
         )
 
         def add_api_authcookie_headers(

h/security/policy/helpers.py

@@ -16,19 +16,29 @@ def is_api_request(request) -> bool:
 class AuthTicketCookieHelper:
     def identity(
         self, cookie: SignedCookieProfile, request: Request
-    ) -> Identity | None:
+    ) -> tuple[Identity, AuthTicket] | tuple[None, None]:
         userid, ticket_id = self.get_cookie_value(cookie)
 
-        user = request.find_service(AuthTicketService).verify_ticket(userid, ticket_id)
+        ticket = request.find_service(AuthTicketService).verify_ticket(
+            userid, ticket_id
+        )
 
-        if (not user) or user.deleted:
-            return None
+        if (not ticket) or ticket.user.deleted:
+            return (None, None)
 
-        return Identity.from_models(user=user)
+        return (Identity.from_models(user=ticket.user), ticket)
 
-    def remember(self, cookie: SignedCookieProfile, request: Request, userid: str):
+    def add_ticket(self, request: Request, userid):
+        """
+        Add a new auth ticket for the given user to the DB.
+
+        Returns the ID of the newly-created auth ticket.
+        """
         ticket_id = AuthTicket.generate_ticket_id()
         request.find_service(AuthTicketService).add_ticket(userid, ticket_id)
+        return ticket_id
+
+    def remember(self, cookie: SignedCookieProfile, userid: str, ticket_id):
         return cookie.get_headers([userid, ticket_id])
 
     def forget(self, cookie: SignedCookieProfile, request: Request):

h/security/policy/top_level.py

@@ -48,7 +48,7 @@ def get_subpolicy(request):
     api_authcookie = webob.cookies.SignedCookieProfile(
         secret=request.registry.settings["h_api_auth_cookie_secret"],
         salt=request.registry.settings["h_api_auth_cookie_salt"],
-        cookie_name="h_api_authcookie",
+        cookie_name="h_api_authcookie.v2",
         httponly=True,
         secure=request.scheme == "https",
         samesite="strict",
@@ -59,6 +59,15 @@ def get_subpolicy(request):
     )
     api_authcookie = api_authcookie.bind(request)
 
+    if is_api_request(request):
+        return APIPolicy(
+            [
+                BearerTokenPolicy(),
+                AuthClientPolicy(),
+                APICookiePolicy(api_authcookie, AuthTicketCookieHelper()),
+            ]
+        )
+
     # The cookie that's used to authenticate HTML page requests.
     html_authcookie = webob.cookies.SignedCookieProfile(
         secret=request.registry.settings["h_auth_cookie_secret"],
@@ -69,22 +78,4 @@ def get_subpolicy(request):
         secure=request.scheme == "https",
     )
     html_authcookie = html_authcookie.bind(request)
-
-    if is_api_request(request):
-        return APIPolicy(
-            [
-                BearerTokenPolicy(),
-                AuthClientPolicy(),
-                APICookiePolicy(
-                    # Use html_authcookie rather than api_authcookie to
-                    # authenticate API requests, at least for now. This is a
-                    # hopefully temporary workaround for an undiagnosed issue
-                    # we've been seeing in production, see:
-                    # https://hypothes-is.slack.com/archives/C2BLQDKHA/p1725041696040459
-                    html_authcookie,
-                    AuthTicketCookieHelper(),
-                ),
-            ]
-        )
-
     return CookiePolicy(html_authcookie, api_authcookie, AuthTicketCookieHelper())

h/services/auth_ticket.py

@@ -2,7 +2,7 @@
 
 import sqlalchemy as sa
 
-from h.models import AuthTicket, User
+from h.models import AuthTicket
 
 
 class AuthTicketService:
@@ -16,19 +16,21 @@ class AuthTicketService:
     def __init__(self, session, user_service):
         self._session = session
         self._user_service = user_service
-        self._user = None
+        self._ticket = None
 
-    def verify_ticket(self, userid: str | None, ticket_id: str | None) -> User | None:
+    def verify_ticket(
+        self, userid: str | None, ticket_id: str | None
+    ) -> AuthTicket | None:
         """
-        Return the User object matching the given userid and ticket_id, or None.
+        Return the AuthTicket matching the given userid and ticket_id, or None.
 
         Verify that there is an unexpired AuthTicket in the DB matching the
-        given `userid` and `ticket_id` and if so return the corresponding User.
+        given `userid` and `ticket_id` and if so return the AuthTicket.
         """
 
-        if self._user:
-            # We've already vetted the user!
-            return self._user
+        if self._ticket:
+            # We've already verified this request's ticket.
+            return self._ticket
 
         if not userid or not ticket_id:
             return None
@@ -53,34 +55,40 @@ def verify_ticket(self, userid: str | None, ticket_id: str | None) -> User | Non
         if (datetime.utcnow() - ticket.updated) > self.TICKET_REFRESH_INTERVAL:
             ticket.expires = datetime.utcnow() + self.TICKET_TTL
 
-        # Update the user cache to allow quick checking if we are called again
-        self._user = ticket.user
+        # Update the cache to allow quick checking if we are called again
+        self._ticket = ticket
 
-        return self._user
+        return self._ticket
 
     def add_ticket(self, userid: str, ticket_id: str) -> None:
         """Add a new auth ticket for the given userid and token_id to the DB."""
 
-        # Update the user cache to allow quick checking if we are called again
-        self._user = self._user_service.fetch(userid)
-        if self._user is None:
+        user = self._user_service.fetch(userid)
+
+        if user is None:
             raise ValueError(f"Cannot find user with userid {userid}")
 
         ticket = AuthTicket(
             id=ticket_id,
-            user=self._user,
-            user_userid=self._user.userid,
+            user=user,
+            user_userid=user.userid,
             expires=datetime.utcnow() + self.TICKET_TTL,
         )
+
         self._session.add(ticket)
 
+        # Update the cache to allow quick checking if we are called again.
+        self._ticket = ticket
+
+        return ticket
+
     def remove_ticket(self, ticket_id: str) -> None:
         """Remove any ticket with the given ID from the DB."""
 
         self._session.query(AuthTicket).filter_by(id=ticket_id).delete()
 
-        # Empty the cached user to force revalidation.
-        self._user = None
+        # Empty the cache to force revalidation.
+        self._ticket = None
 
 
 def factory(_context, request):

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

@@ -4,6 +4,7 @@
 from pyramid.csrf import SessionCSRFStoragePolicy
 from pyramid.exceptions import BadCSRFOrigin, BadCSRFToken
 
+from h.security.identity import Identity as Identity_
 from h.security.policy._api_cookie import APICookiePolicy
 from h.security.policy.helpers import AuthTicketCookieHelper
 
@@ -31,12 +32,12 @@ def test_identity(self, api_cookie_policy, helper, pyramid_csrf_request):
 
         helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request)
         helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request)
-        assert identity == helper.identity.return_value
+        assert identity == helper.identity.return_value[0]
 
     def test_identity_with_no_auth_cookie(
         self, api_cookie_policy, helper, pyramid_request
     ):
-        helper.identity.return_value = None
+        helper.identity.return_value = (None, None)
 
         assert api_cookie_policy.identity(pyramid_request) is None
 
@@ -64,7 +65,7 @@ def test_authenticated_userid(
         helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request)
         helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request)
         Identity.authenticated_userid.assert_called_once_with(
-            helper.identity.return_value
+            helper.identity.return_value[0]
         )
         assert authenticated_userid == Identity.authenticated_userid.return_value
 
@@ -78,13 +79,18 @@ def test_permits(
         helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request)
         helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request)
         identity_permits.assert_called_once_with(
-            helper.identity.return_value, sentinel.context, sentinel.permission
+            helper.identity.return_value[0], sentinel.context, sentinel.permission
         )
         assert permits == identity_permits.return_value
 
     @pytest.fixture
-    def helper(self):
-        return create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
+    def helper(self, factories):
+        helper = create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
+        helper.identity.return_value = (
+            create_autospec(Identity_, instance=True, spec_set=True),
+            factories.AuthTicket(),
+        )
+        return helper
 
     @pytest.fixture
     def api_cookie_policy(self, helper):

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

@@ -14,7 +14,7 @@ def test_identity(self, cookie_policy, helper, pyramid_request, html_authcookie)
 
         helper.add_vary_by_cookie.assert_called_once_with(pyramid_request)
         helper.identity.assert_called_once_with(html_authcookie, pyramid_request)
-        assert identity == helper.identity.return_value
+        assert identity == helper.identity.return_value[0]
 
     def test_identity_issues_api_authcookie(
         self, cookie_policy, helper, pyramid_request, api_authcookie
@@ -31,16 +31,18 @@ def test_identity_issues_api_authcookie(
             pyramid_request.response
         )
         helper.remember.assert_called_once_with(
-            api_authcookie, pyramid_request, helper.identity.return_value.user.userid
+            api_authcookie,
+            helper.identity.return_value[0].user.userid,
+            helper.identity.return_value[1],
         )
         for header in helper.remember.return_value:
             assert header in pyramid_request.response.headerlist
 
     @pytest.mark.parametrize(
         "identity",
         [
-            None,
-            Identity_(user=None, auth_client=None),
+            (None, None),
+            (Identity_(user=None, auth_client=None), sentinel.auth_ticket),
         ],
     )
     def test_identity_doesnt_issue_api_authcookie_if_user_not_authenticated(
@@ -94,7 +96,7 @@ def test_authenticated_userid(
         helper.add_vary_by_cookie.assert_called_once_with(pyramid_request)
         helper.identity.assert_called_once_with(html_authcookie, pyramid_request)
         Identity.authenticated_userid.assert_called_once_with(
-            helper.identity.return_value
+            helper.identity.return_value[0]
         )
         assert authenticated_userid == Identity.authenticated_userid.return_value
 
@@ -122,9 +124,10 @@ def remember(cookie, *_args, **_kwargs):
         headers = cookie_policy.remember(pyramid_request, sentinel.userid, foo="bar")
 
         assert not pyramid_request.session
+        helper.add_ticket.assert_called_once_with(pyramid_request, sentinel.userid)
         assert helper.remember.call_args_list == [
-            call(html_authcookie, pyramid_request, sentinel.userid),
-            call(api_authcookie, pyramid_request, sentinel.userid),
+            call(html_authcookie, sentinel.userid, helper.add_ticket.return_value),
+            call(api_authcookie, sentinel.userid, helper.add_ticket.return_value),
         ]
         assert headers == [
             sentinel.html_authcookie_header_1,
@@ -190,7 +193,7 @@ def test_permits(
         helper.add_vary_by_cookie.assert_called_once_with(pyramid_request)
         helper.identity.assert_called_once_with(html_authcookie, pyramid_request)
         identity_permits.assert_called_once_with(
-            helper.identity.return_value, sentinel.context, sentinel.permission
+            helper.identity.return_value[0], sentinel.context, sentinel.permission
         )
         assert permits == identity_permits.return_value
 
@@ -203,7 +206,7 @@ def html_authcookie(self, pyramid_request):
     @pytest.fixture
     def api_authcookie(self, pyramid_request):
         api_authcookie = SignedCookieProfile(
-            "secret", "salt", cookie_name="h_api_authcookie"
+            "secret", "salt", cookie_name="h_api_authcookie.v2"
         )
 
         # Add the API auth cookie to the request.
@@ -215,8 +218,13 @@ def api_authcookie(self, pyramid_request):
         return api_authcookie.bind(pyramid_request)
 
     @pytest.fixture
-    def helper(self):
-        return create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
+    def helper(self, factories):
+        helper = create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
+        helper.identity.return_value = (
+            create_autospec(Identity_, instance=True, spec_set=True),
+            factories.AuthTicket(),
+        )
+        return helper
 
     @pytest.fixture
     def cookie_policy(self, html_authcookie, api_authcookie, helper):