Skip to content

Commit

Permalink
Make HTML and API auth cookies share the same auth ticket
Browse files Browse the repository at this point in the history
Make each HTML and API auth cookie pair share a single auth ticket so
that it's not possible to get into a situation where the HTML auth
cookie's ticket is still valid but the API auth cookie's ticket has
expired.

Fixes #8914.

This commit also renames the API auth cookie so that existing API auth
cookies in the wild, which have their own auth tickets that're separate
from the auth tickets of the HTML auth cookies and that will have
expired, will no longer be used and new API auth cookies with the new
cookie name will be issued to these browsers.
  • Loading branch information
seanh committed Sep 11, 2024
1 parent a2f6e52 commit 844aa75
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 21 deletions.
14 changes: 8 additions & 6 deletions h/security/policy/_cookie.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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

Expand Down Expand Up @@ -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):
Expand All @@ -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

Expand All @@ -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(
Expand Down
10 changes: 9 additions & 1 deletion h/security/policy/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,17 @@ def identity(

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):
Expand Down
2 changes: 1 addition & 1 deletion h/security/policy/top_level.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
11 changes: 7 additions & 4 deletions tests/unit/h/security/policy/_cookie_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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[0].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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
20 changes: 13 additions & 7 deletions tests/unit/h/security/policy/helpers_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,21 @@ def test_identity_when_user_deleted(

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):
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/h/security/policy/top_level_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def test_api_request(
webob.cookies.SignedCookieProfile.assert_called_once_with(
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=5184000,
httponly=True,
secure=True,
Expand Down Expand Up @@ -129,7 +129,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=5184000,
httponly=True,
secure=True,
Expand Down

0 comments on commit 844aa75

Please sign in to comment.