Fix crash when issuing missing API cookie

Fix a crash when issuing an API auth cookie to a browser that has an
HTML auth cookie but no API one: https://hypothesis.sentry.io/issues/5906066249/

`AuthTicketCookieHelper.identity()` returns the entire
`AuthTicket`, not just the `AuthTicket.id` string as
`CookiePolicy.identity()` thought it did. Fix some variable names in
`CooCookiePolicy` to reflect this. There's no actual functional change
to `CookiePolicy`, just some names are changed.

Also change `AuthTicketCookieHelper.remember()` to accept the entire
`AuthTicket` as an argument not just the `AuthTicket.id` string since
the entire `AuthTicket` is what `CookiePolicy` actually passes to it.
This type/argument confusion was the cause of the Sentry issue linked
above.

Change `AuthTicketCookieHelper.add_ticket()` to return the entire
`AuthTicket` not just the `AuthTicket.id` string. `CookiePolicy` calls
`AuthTicketCookieHelper.add_ticket()` and then passes the return value
to `AuthTicketCookieHelper.remember()` which now expects an entire
`AuthTicket` not just an `AuthTicket.id` string.

This makes things nice and symmetrical: `AuthTicketCookieHelper` always
returns entire `AuthTicket`'s and accepts entire `AuthTicket`'s as
arguments, it never deals in just `AuthTicket.id` strings.

h/security/policy/_cookie.py

@@ -30,7 +30,7 @@ def __init__(
 
     def identity(self, request):
         self.helper.add_vary_by_cookie(request)
-        identity, ticket_id = self.helper.identity(self.html_authcookie, request)
+        identity, auth_ticket = 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, ticket_id)
+        self._issue_api_authcookie(identity, request, auth_ticket)
 
         return identity
 
@@ -76,11 +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)
+        auth_ticket = self.helper.add_ticket(request, userid)
 
         return [
-            *self.helper.remember(self.html_authcookie, userid, ticket_id),
-            *self.helper.remember(self.api_authcookie, userid, ticket_id),
+            *self.helper.remember(self.html_authcookie, userid, auth_ticket),
+            *self.helper.remember(self.api_authcookie, userid, auth_ticket),
         ]
 
     def forget(self, request):
@@ -93,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, ticket_id):
+    def _issue_api_authcookie(self, identity, request, auth_ticket):
         if not identity:
             return
 
@@ -107,7 +107,7 @@ def _issue_api_authcookie(self, identity, request, ticket_id):
             return
 
         headers = self.helper.remember(
-            self.api_authcookie, identity.user.userid, ticket_id
+            self.api_authcookie, identity.user.userid, auth_ticket
         )
 
         def add_api_authcookie_headers(

h/security/policy/helpers.py

@@ -28,18 +28,20 @@ def identity(
 
         return (Identity.from_models(user=ticket.user), ticket)
 
-    def add_ticket(self, request: Request, userid):
+    def add_ticket(self, request: Request, userid) -> AuthTicket:
         """
         Add a new auth ticket for the given user to the DB.
 
-        Returns the ID of the newly-created auth ticket.
+        Returns the the newly-created auth ticket.
         """
-        ticket_id = AuthTicket.generate_ticket_id()
-        request.find_service(AuthTicketService).add_ticket(userid, ticket_id)
-        return ticket_id
+        return request.find_service(AuthTicketService).add_ticket(
+            userid, AuthTicket.generate_ticket_id()
+        )
 
-    def remember(self, cookie: SignedCookieProfile, userid: str, ticket_id):
-        return cookie.get_headers([userid, ticket_id])
+    def remember(
+        self, cookie: SignedCookieProfile, userid: str, auth_ticket: AuthTicket
+    ):
+        return cookie.get_headers([userid, auth_ticket.id])
 
     def forget(self, cookie: SignedCookieProfile, request: Request):
         request.session.invalidate()

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

@@ -56,20 +56,20 @@ def test_identity_when_user_deleted(
         assert helper.identity(cookie, pyramid_request) == (None, None)
 
     def test_add_ticket(self, helper, auth_ticket_service, pyramid_request, AuthTicket):
-        ticket_id = helper.add_ticket(pyramid_request, sentinel.userid)
+        auth_ticket = helper.add_ticket(pyramid_request, sentinel.userid)
 
         AuthTicket.generate_ticket_id.assert_called_once_with()
         auth_ticket_service.add_ticket.assert_called_once_with(
             sentinel.userid, AuthTicket.generate_ticket_id.return_value
         )
-        assert ticket_id == AuthTicket.generate_ticket_id.return_value
+        assert auth_ticket == auth_ticket_service.add_ticket.return_value
 
-    def test_remember(self, cookie, helper):
-        headers = helper.remember(cookie, sentinel.userid, sentinel.ticket_id)
+    def test_remember(self, cookie, helper, factories):
+        auth_ticket = factories.AuthTicket()
 
-        cookie.get_headers.assert_called_once_with(
-            [sentinel.userid, sentinel.ticket_id]
-        )
+        headers = helper.remember(cookie, sentinel.userid, auth_ticket)
+
+        cookie.get_headers.assert_called_once_with([sentinel.userid, auth_ticket.id])
         assert headers == cookie.get_headers.return_value
 
     def test_forget(self, auth_ticket_service, cookie, helper, pyramid_request):