diff --git a/h/security/policy/_api_cookie.py b/h/security/policy/_api_cookie.py index f7ef733916a..9b84625fc6c 100644 --- a/h/security/policy/_api_cookie.py +++ b/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 diff --git a/h/security/policy/_cookie.py b/h/security/policy/_cookie.py index c01f3204288..fbde0f3e65a 100644 --- a/h/security/policy/_cookie.py +++ b/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( diff --git a/h/security/policy/helpers.py b/h/security/policy/helpers.py index 2fcd5749bed..e7139698c79 100644 --- a/h/security/policy/helpers.py +++ b/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): diff --git a/h/security/policy/top_level.py b/h/security/policy/top_level.py index d7db295c6de..e6b63de858a 100644 --- a/h/security/policy/top_level.py +++ b/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()) diff --git a/h/services/auth_ticket.py b/h/services/auth_ticket.py index 67d46d05a8b..308fb9b18d4 100644 --- a/h/services/auth_ticket.py +++ b/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): diff --git a/tests/unit/h/security/policy/_api_cookie_test.py b/tests/unit/h/security/policy/_api_cookie_test.py index 42cc73dc647..b670b1d7859 100644 --- a/tests/unit/h/security/policy/_api_cookie_test.py +++ b/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): diff --git a/tests/unit/h/security/policy/_cookie_test.py b/tests/unit/h/security/policy/_cookie_test.py index 51a372f48c6..59c30727545 100644 --- a/tests/unit/h/security/policy/_cookie_test.py +++ b/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,7 +31,9 @@ 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 @@ -39,8 +41,8 @@ def test_identity_issues_api_authcookie( @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): diff --git a/tests/unit/h/security/policy/helpers_test.py b/tests/unit/h/security/policy/helpers_test.py index 6b19845d7f7..a0587c3ec73 100644 --- a/tests/unit/h/security/policy/helpers_test.py +++ b/tests/unit/h/security/policy/helpers_test.py @@ -29,39 +29,47 @@ class TestAuthTicketCookieHelper: def test_identity( self, auth_ticket_service, cookie, helper, pyramid_request, Identity ): - identity = helper.identity(cookie, pyramid_request) + ticket = auth_ticket_service.verify_ticket.return_value + user = ticket.user + user.deleted = False + + result = helper.identity(cookie, pyramid_request) auth_ticket_service.verify_ticket.assert_called_once_with( sentinel.userid, sentinel.ticket_id ) - Identity.from_models.assert_called_once_with( - user=auth_ticket_service.verify_ticket.return_value - ) - assert identity == Identity.from_models.return_value + Identity.from_models.assert_called_once_with(user=user) + assert result == (Identity.from_models.return_value, ticket) def test_identity_when_no_user( self, auth_ticket_service, cookie, helper, pyramid_request ): auth_ticket_service.verify_ticket.return_value = None - assert helper.identity(cookie, pyramid_request) is None + assert helper.identity(cookie, pyramid_request) == (None, None) def test_identity_when_user_deleted( self, auth_ticket_service, cookie, helper, pyramid_request ): - auth_ticket_service.verify_ticket.return_value.deleted = True + auth_ticket_service.verify_ticket.return_value.user.deleted = True - assert helper.identity(cookie, pyramid_request) is None + assert helper.identity(cookie, pyramid_request) == (None, None) - def test_remember( - self, auth_ticket_service, cookie, helper, pyramid_request, AuthTicket - ): - headers = helper.remember(cookie, pyramid_request, "test_userid") + def test_add_ticket(self, helper, auth_ticket_service, pyramid_request, AuthTicket): + ticket_id = helper.add_ticket(pyramid_request, sentinel.userid) AuthTicket.generate_ticket_id.assert_called_once_with() - ticket_id = AuthTicket.generate_ticket_id.return_value - auth_ticket_service.add_ticket.assert_called_once_with("test_userid", ticket_id) - cookie.get_headers.assert_called_once_with(["test_userid", ticket_id]) + 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 + + def test_remember(self, cookie, helper): + headers = helper.remember(cookie, sentinel.userid, sentinel.ticket_id) + + cookie.get_headers.assert_called_once_with( + [sentinel.userid, sentinel.ticket_id] + ) assert headers == cookie.get_headers.return_value def test_forget(self, auth_ticket_service, cookie, helper, pyramid_request): diff --git a/tests/unit/h/security/policy/top_level_test.py b/tests/unit/h/security/policy/top_level_test.py index f4b7a4f7abf..5bd7fa6d48c 100644 --- a/tests/unit/h/security/policy/top_level_test.py +++ b/tests/unit/h/security/policy/top_level_test.py @@ -70,16 +70,10 @@ def test_api_request( AuthTicketCookieHelper, ): is_api_request.return_value = True - html_authcookie = create_autospec( - SignedCookieProfile, instance=True, spec_set=True - ) api_authcookie = create_autospec( SignedCookieProfile, instance=True, spec_set=True ) - webob.cookies.SignedCookieProfile.side_effect = [ - api_authcookie, - html_authcookie, - ] + webob.cookies.SignedCookieProfile.return_value = api_authcookie policy = get_subpolicy(pyramid_request) @@ -89,26 +83,17 @@ def test_api_request( call( secret="test_h_api_auth_cookie_secret", salt="test_h_api_auth_cookie_salt", - cookie_name="h_api_authcookie", + cookie_name="h_api_authcookie.v2", max_age=31539600, httponly=True, secure=True, samesite="strict", - ), - call( - secret="test_h_auth_cookie_secret", - salt="authsanity", - cookie_name="auth", - max_age=31536000, - httponly=True, - secure=True, - ), + ) ] api_authcookie.bind.assert_called_once_with(pyramid_request) - html_authcookie.bind.assert_called_once_with(pyramid_request) AuthTicketCookieHelper.assert_called_once_with() APICookiePolicy.assert_called_once_with( - html_authcookie.bind.return_value, + api_authcookie.bind.return_value, AuthTicketCookieHelper.return_value, ) APIPolicy.assert_called_once_with( @@ -146,7 +131,7 @@ def test_non_api_request( call( secret="test_h_api_auth_cookie_secret", salt="test_h_api_auth_cookie_salt", - cookie_name="h_api_authcookie", + cookie_name="h_api_authcookie.v2", max_age=31539600, httponly=True, secure=True, diff --git a/tests/unit/h/services/auth_ticket_test.py b/tests/unit/h/services/auth_ticket_test.py index 5674d849106..f1871fe76ec 100644 --- a/tests/unit/h/services/auth_ticket_test.py +++ b/tests/unit/h/services/auth_ticket_test.py @@ -2,6 +2,7 @@ from unittest.mock import sentinel import pytest +from sqlalchemy import inspect from h.models import AuthTicket from h.services.auth_ticket import AuthTicketService, factory @@ -17,18 +18,18 @@ class TestAuthTicketService: def test_verify_ticket(self, service, auth_ticket): assert ( service.verify_ticket(auth_ticket.user.userid, auth_ticket.id) - == auth_ticket.user + == auth_ticket ) - # We also set the cache as a side effect + # We also set the cache as a side effect. + assert service._ticket == auth_ticket # pylint:disable=protected-access - assert service._user == auth_ticket.user # pylint: disable=protected-access - - def test_verify_ticket_short_circuits_if_user_cache_is_set(self, service): + def test_verify_ticket_short_circuits_if_ticket_cache_is_set(self, service): # pylint: disable=protected-access - service._user = sentinel.user + service._ticket = sentinel.ticket assert ( - service.verify_ticket(sentinel.userid, sentinel.ticket_id) == service._user + service.verify_ticket(sentinel.userid, sentinel.ticket_id) + == sentinel.ticket ) @pytest.mark.usefixtures("auth_ticket") @@ -75,20 +76,20 @@ def test_verify_ticket_updates_the_expiry_time( else: assert auth_ticket.expires == expires - def test_add_ticket(self, service, user, user_service, db_session): + def test_add_ticket(self, service, user, user_service): user_service.fetch.return_value = user - service.add_ticket(sentinel.userid, "test_ticket_id") + auth_ticket = service.add_ticket(sentinel.userid, "test_ticket_id") user_service.fetch.assert_called_once_with(sentinel.userid) - auth_ticket = db_session.query(AuthTicket).one() assert auth_ticket.user == user assert auth_ticket.user_userid == user.userid assert auth_ticket.id == "test_ticket_id" assert_nearly_equal( auth_ticket.expires, datetime.utcnow() + AuthTicketService.TICKET_TTL ) - assert service._user == user # pylint: disable=protected-access + assert service._ticket == auth_ticket # pylint: disable=protected-access + assert inspect(auth_ticket).pending is True def test_add_ticket_raises_if_user_is_missing(self, service, user_service): user_service.fetch.return_value = None @@ -101,7 +102,7 @@ def test_add_ticket_raises_if_user_is_missing(self, service, user_service): def test_remove_ticket(self, auth_ticket, service, db_session): service.remove_ticket(auth_ticket.id) - assert service._user is None # pylint: disable=protected-access + assert service._ticket is None # pylint: disable=protected-access assert db_session.query(AuthTicket).first() is None @pytest.fixture