Revert "Revert "Use a separate auth cookie for API requests (fixed ve…

…rsion)""

This reverts commit 5820e93.

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

@@ -50,7 +50,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",
@@ -61,6 +61,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"],
@@ -71,22 +80,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):