Skip to content

Commit

Permalink
Refactor: IdG sign out to use client_id (#2700)
Browse files Browse the repository at this point in the history
  • Loading branch information
lalver1 authored Mar 11, 2025
2 parents 7bc9b91 + 9f05252 commit c955ac9
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 46 deletions.
22 changes: 11 additions & 11 deletions benefits/core/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
_FLOW = "flow"
_LANG = "lang"
_OAUTH_CLAIMS = "oauth_claims"
_OAUTH_TOKEN = "oauth_token"
_OAUTH_AUTHORIZED = "oauth_authorized"
_ORIGIN = "origin"
_START = "start"
_UID = "uid"
Expand Down Expand Up @@ -59,7 +59,7 @@ def context_dict(request):
_ENROLLMENT_TOKEN: enrollment_token(request),
_ENROLLMENT_TOKEN_EXP: enrollment_token_expiry(request),
_LANG: language(request),
_OAUTH_TOKEN: oauth_token(request),
_OAUTH_AUTHORIZED: oauth_authorized(request),
_OAUTH_CLAIMS: oauth_claims(request),
_ORIGIN: origin(request),
_START: start(request),
Expand Down Expand Up @@ -143,17 +143,17 @@ def language(request):

def logged_in(request):
"""Check if the current session has an OAuth token."""
return bool(oauth_token(request))
return bool(oauth_authorized(request))


def logout(request):
"""Reset the session claims and tokens."""
update(request, oauth_claims=[], oauth_token=False, enrollment_token=False)
update(request, oauth_claims=[], oauth_authorized=False, enrollment_token=False)


def oauth_token(request):
"""Get the oauth token from the request's session, or None"""
return request.session.get(_OAUTH_TOKEN)
def oauth_authorized(request):
"""Get the oauth authorization status from the request's session, or None"""
return request.session.get(_OAUTH_AUTHORIZED)


def oauth_claims(request):
Expand Down Expand Up @@ -189,7 +189,7 @@ def reset(request):
request.session[_ENROLLMENT_EXP] = None
request.session[_ENROLLMENT_TOKEN] = None
request.session[_ENROLLMENT_TOKEN_EXP] = None
request.session[_OAUTH_TOKEN] = None
request.session[_OAUTH_AUTHORIZED] = False
request.session[_OAUTH_CLAIMS] = None

if _UID not in request.session or not request.session[_UID]:
Expand Down Expand Up @@ -248,7 +248,7 @@ def update(
enrollment_expiry=None,
enrollment_token=None,
enrollment_token_exp=None,
oauth_token=None,
oauth_authorized=None,
oauth_claims=None,
origin=None,
):
Expand All @@ -271,8 +271,8 @@ def update(
if enrollment_token is not None:
request.session[_ENROLLMENT_TOKEN] = enrollment_token
request.session[_ENROLLMENT_TOKEN_EXP] = enrollment_token_exp
if oauth_token is not None:
request.session[_OAUTH_TOKEN] = oauth_token
if oauth_authorized is not None:
request.session[_OAUTH_AUTHORIZED] = oauth_authorized
if oauth_claims is not None:
request.session[_OAUTH_CLAIMS] = oauth_claims
if origin is not None:
Expand Down
4 changes: 2 additions & 2 deletions benefits/oauth/redirects.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from . import analytics


def deauthorize_redirect(request, oauth_client, token, redirect_uri):
def deauthorize_redirect(request, oauth_client, redirect_uri):
"""Helper implements OIDC signout via the `end_session_endpoint`."""

# Authlib has not yet implemented `end_session_endpoint` as the OIDC Session Management 1.0 spec is still in draft
Expand All @@ -23,7 +23,7 @@ def deauthorize_redirect(request, oauth_client, token, redirect_uri):

end_session_endpoint = metadata.get("end_session_endpoint")

params = dict(id_token_hint=token, post_logout_redirect_uri=redirect_uri)
params = dict(client_id=oauth_client.client_id, post_logout_redirect_uri=redirect_uri)
encoded_params = urlencode(params)
end_session_url = f"{end_session_endpoint}?{encoded_params}"

Expand Down
11 changes: 5 additions & 6 deletions benefits/oauth/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ def authorize(request):

logger.debug("OAuth access token authorized")

# We store the id_token in the user's session. This is the minimal amount of information needed later to log the user out.
id_token = token["id_token"]
# oauth_token_authorized will be stored in the user's session to mark the user as authorized
oauth_token_authorized = True

# We store the returned claim in case it can be used later in eligibility verification.
flow_claims = flow.claims_all_claims
Expand All @@ -144,7 +144,7 @@ def authorize(request):
elif claim_value >= 10:
error_claim[claim] = claim_value

session.update(request, oauth_token=id_token, oauth_claims=stored_claims)
session.update(request, oauth_authorized=oauth_token_authorized, oauth_claims=stored_claims)
analytics.finished_sign_in(request, error=error_claim)

return redirect(routes.ELIGIBILITY_CONFIRM)
Expand Down Expand Up @@ -176,8 +176,7 @@ def logout(request):

analytics.started_sign_out(request)

# overwrite the oauth session token, the user is signed out of the app
token = session.oauth_token(request)
# the user is signed out of the app
session.logout(request)

route = reverse(routes.OAUTH_POST_LOGOUT)
Expand All @@ -187,7 +186,7 @@ def logout(request):

# send the user through the end_session_endpoint, redirecting back to
# the post_logout route
return redirects.deauthorize_redirect(request, oauth_client, token, redirect_uri)
return redirects.deauthorize_redirect(request, oauth_client, redirect_uri)


@decorator_from_middleware(FlowUsesClaimsVerificationSessionRequired)
Expand Down
4 changes: 2 additions & 2 deletions tests/pytest/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,8 @@ def mocked_session_eligible(mocker):


@pytest.fixture
def mocked_session_oauth_token(mocker):
return mocker.patch("benefits.core.session.oauth_token", autospec=True, return_value="token")
def mocked_session_oauth_authorized(mocker):
return mocker.patch("benefits.core.session.oauth_authorized", autospec=True, return_value=True)


@pytest.fixture
Expand Down
2 changes: 1 addition & 1 deletion tests/pytest/core/test_middleware_login_required.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def test_login_flow_does_not_use_claims_verification(app_request, model_Enrollme
@pytest.mark.usefixtures("mocked_session_flow_uses_claims_verification")
def test_logged_in(app_request, mocked_view, decorated_view):
# log in
session.update(app_request, oauth_token="something")
session.update(app_request, oauth_authorized=True)

decorated_view(app_request)
mocked_view.assert_called_once()
22 changes: 11 additions & 11 deletions tests/pytest/core/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,35 +185,35 @@ def test_logged_in_default(app_request):

@pytest.mark.django_db
def test_logged_in_False(app_request):
session.update(app_request, oauth_token=False)
session.update(app_request, oauth_authorized=False)

assert not session.logged_in(app_request)


@pytest.mark.django_db
def test_logged_in_True(app_request):
session.update(app_request, oauth_token=True)
session.update(app_request, oauth_authorized=True)

assert session.logged_in(app_request)


@pytest.mark.django_db
def test_logout(app_request):
session.update(app_request, oauth_claims=["oauth_claim"], oauth_token="oauth_token", enrollment_token="enrollment_token")
session.update(app_request, oauth_claims=["oauth_claim"], oauth_authorized=True, enrollment_token="enrollment_token")
assert session.logged_in(app_request)
assert session.oauth_claims(app_request)

session.logout(app_request)

assert not session.logged_in(app_request)
assert not session.enrollment_token(app_request)
assert not session.oauth_token(app_request)
assert not session.oauth_authorized(app_request)
assert not session.oauth_claims(app_request)


@pytest.mark.django_db
def test_oauth_token_default(app_request):
assert not session.oauth_token(app_request)
def test_oauth_authorized_default(app_request):
assert not session.oauth_authorized(app_request)


@pytest.mark.django_db
Expand Down Expand Up @@ -268,12 +268,12 @@ def test_reset_enrollment(app_request):

@pytest.mark.django_db
def test_reset_oauth(app_request):
app_request.session[session._OAUTH_TOKEN] = "oauthtoken456"
app_request.session[session._OAUTH_AUTHORIZED] = True
app_request.session[session._OAUTH_CLAIMS] = ["claim"]

session.reset(app_request)

assert session.oauth_token(app_request) is None
assert session.oauth_authorized(app_request) is False
assert session.oauth_claims(app_request) is None


Expand Down Expand Up @@ -421,10 +421,10 @@ def test_update_enrollment_token(app_request):


@pytest.mark.django_db
def test_update_oauth_token(app_request):
session.update(app_request, oauth_token="token")
def test_update_oauth_authorized(app_request):
session.update(app_request, oauth_authorized=True)

assert session.oauth_token(app_request) == "token"
assert session.oauth_authorized(app_request) is True


@pytest.mark.django_db
Expand Down
10 changes: 6 additions & 4 deletions tests/pytest/eligibility/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ def mocked_analytics_module(mocked_analytics_module):


@pytest.fixture
def mocked_eligibility_auth_request(mocked_eligibility_request_session, mocked_session_oauth_token):
def mocked_eligibility_auth_request(mocked_eligibility_request_session, mocked_session_oauth_authorized):
"""
Stub fixture combines mocked_eligibility_request_session and mocked_session_oauth_token
Stub fixture combines mocked_eligibility_request_session and mocked_session_oauth_authorized
so that session behaves like in an authenticated request to the eligibility app
"""
pass
Expand Down Expand Up @@ -260,7 +260,9 @@ def test_confirm_get_verified(client, mocked_session_update):


@pytest.mark.django_db
@pytest.mark.usefixtures("mocked_session_agency", "mocked_session_flow_uses_claims_verification", "mocked_session_oauth_token")
@pytest.mark.usefixtures(
"mocked_session_agency", "mocked_session_flow_uses_claims_verification", "mocked_session_oauth_authorized"
)
def test_confirm_get_oauth_verified(mocker, client, mocked_session_update, mocked_analytics_module):
mocker.patch("benefits.eligibility.verify.eligibility_from_oauth", return_value=True)

Expand All @@ -277,7 +279,7 @@ def test_confirm_get_oauth_verified(mocker, client, mocked_session_update, mocke
@pytest.mark.usefixtures(
"mocked_session_agency",
"mocked_session_flow_uses_claims_verification",
"mocked_session_oauth_token",
"mocked_session_oauth_authorized",
"mocked_session_update",
)
def test_confirm_get_oauth_unverified(mocker, client):
Expand Down
7 changes: 4 additions & 3 deletions tests/pytest/oauth/test_redirects.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@ def mocked_sentry_sdk_module(mocker):


def test_deauthorize_redirect(app_request, mocked_oauth_client):
mocked_oauth_client.client_id = "test-client-id"
mocked_oauth_client.load_server_metadata.return_value = {"end_session_endpoint": "https://server/endsession"}

result = deauthorize_redirect(app_request, mocked_oauth_client, "token", "https://localhost/redirect_uri")
result = deauthorize_redirect(app_request, mocked_oauth_client, "https://localhost/redirect_uri")

mocked_oauth_client.load_server_metadata.assert_called()
assert result.status_code == 302
assert (
result.url
== "https://server/endsession?id_token_hint=token&post_logout_redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri"
== "https://server/endsession?client_id=test-client-id&post_logout_redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri"
)


Expand All @@ -36,7 +37,7 @@ def test_deauthorize_redirect_load_server_metadata_error(
):
mocked_oauth_client.load_server_metadata.side_effect = Exception("Side effect")

result = deauthorize_redirect(app_request, mocked_oauth_client, "token", "https://localhost/redirect_uri")
result = deauthorize_redirect(app_request, mocked_oauth_client, "https://localhost/redirect_uri")

assert result.status_code == 302
assert result.url == reverse(routes.OAUTH_SYSTEM_ERROR)
Expand Down
12 changes: 6 additions & 6 deletions tests/pytest/oauth/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ def test_authorize_success(
mocked_oauth_client.authorize_access_token.assert_called_with(app_request)
mocked_analytics_module.finished_sign_in.assert_called_once()
assert session.logged_in(app_request)
assert session.oauth_token(app_request) == "token"
assert session.oauth_authorized(app_request) is True
assert result.status_code == 302
assert result.url == reverse(routes.ELIGIBILITY_CONFIRM)

Expand Down Expand Up @@ -410,20 +410,20 @@ def test_logout(app_request, mocker, mocked_oauth_client_or_error_redirect__clie
mocked_oauth_client = mocked_oauth_client_or_error_redirect__client.return_value
mocked_redirect = mocker.patch("benefits.oauth.views.redirects.deauthorize_redirect", return_value=HttpResponse(message))

token = "token"
session.update(app_request, oauth_token=token)
assert session.oauth_token(app_request) == token
oauth_token_authorized = True
session.update(app_request, oauth_authorized=oauth_token_authorized)
assert session.oauth_authorized(app_request) == oauth_token_authorized

result = logout(app_request)

mocked_redirect.assert_called_with(app_request, mocked_oauth_client, token, "https://testserver/oauth/post_logout")
mocked_redirect.assert_called_with(app_request, mocked_oauth_client, "https://testserver/oauth/post_logout")
mocked_analytics_module.started_sign_out.assert_called_once()
assert result.status_code == 200
assert message in str(result.content)

assert not session.logged_in(app_request)
assert session.enrollment_token(app_request) is False
assert session.oauth_token(app_request) is False
assert session.oauth_authorized(app_request) is False
assert session.oauth_claims(app_request) == []


Expand Down

0 comments on commit c955ac9

Please sign in to comment.