Attempt a redirect (once) if there's a state mismatch

This commit is contained in:
Rachid Mrad 2024-03-15 15:34:53 -04:00
parent 63c0aec4b8
commit ea43a34dee
No known key found for this signature in database
4 changed files with 34 additions and 20 deletions

View file

@ -33,8 +33,8 @@ class AuthenticationFailed(OIDCException):
friendly_message = "This login attempt didn't work." friendly_message = "This login attempt didn't work."
class NoStateDefined(OIDCException): class StateMismatch(OIDCException):
friendly_message = "The session state is None." friendly_message = "State mismatch. This login attempt didn't work."
class InternalError(OIDCException): class InternalError(OIDCException):

View file

@ -182,14 +182,20 @@ class Client(oic.Client):
if authn_response["state"] != session.get("state", None): if authn_response["state"] != session.get("state", None):
# this most likely means the user's Django session vanished # this most likely means the user's Django session vanished
logger.error(
f"Received state not the same as expected for {state}"
f"authn_response['state'] = {authn_response['state']}"
f"session.get('state', None) = {session.get('state', None)}"
)
if session.get("state", None) is None: if session.get("state", None) is None:
raise o_e.NoStateDefined() logger.error(
raise o_e.AuthenticationFailed(locator=state) f"The OP state {state} does not match the session state."
f"The session state is None."
f"authn_response['state'] = {authn_response['state']}"
f"session.get('state', None) = {session.get('state', None)}"
)
else:
logger.error(
f"The OP state {state} does not match the session state."
f"authn_response['state'] = {authn_response['state']}"
f"session.get('state', None) = {session.get('state', None)}"
)
raise o_e.StateMismatch()
if self.behaviour.get("response_type") == "code": if self.behaviour.get("response_type") == "code":
# need an access token to get user info (and to log the user out later) # need an access token to get user info (and to log the user out later)

View file

@ -4,7 +4,7 @@ from django.http import HttpResponse
from django.test import Client, TestCase, RequestFactory from django.test import Client, TestCase, RequestFactory
from django.urls import reverse from django.urls import reverse
from djangooidc.exceptions import NoStateDefined, InternalError from djangooidc.exceptions import StateMismatch, InternalError
from ..views import login_callback from ..views import login_callback
from .common import less_console_noise from .common import less_console_noise
@ -129,14 +129,14 @@ class ViewsTest(TestCase):
self.assertContains(response, "Hi") self.assertContains(response, "Hi")
def test_login_callback_with_no_session_state(self, mock_client): def test_login_callback_with_no_session_state(self, mock_client):
"""If the local session is None (ie the server restarted while user was logged out), """If the local session does not match the OP session,
we do not throw an exception. Rather, we attempt to login again.""" we do not throw an exception. Rather, we attempt to login again."""
with less_console_noise(): with less_console_noise():
# MOCK # MOCK
# mock the acr_value to some string # mock the acr_value to some string
# mock the callback function to raise the NoStateDefined Exception # mock the callback function to raise the StateMismatch Exception
mock_client.get_default_acr_value.side_effect = self.create_acr mock_client.get_default_acr_value.side_effect = self.create_acr
mock_client.callback.side_effect = NoStateDefined() mock_client.callback.side_effect = StateMismatch()
# TEST # TEST
# test the login callback # test the login callback
response = self.client.get(reverse("openid_login_callback")) response = self.client.get(reverse("openid_login_callback"))

View file

@ -108,16 +108,24 @@ def login_callback(request):
if user: if user:
login(request, user) login(request, user)
logger.info("Successfully logged in user %s" % user) logger.info("Successfully logged in user %s" % user)
# Double login bug (1507)? # Clear the flag if the exception is not caught
request.session.pop("redirect_attempted", None)
return redirect(request.session.get("next", "/")) return redirect(request.session.get("next", "/"))
else: else:
raise o_e.BannedUser() raise o_e.BannedUser()
except o_e.NoStateDefined as nsd_err: except o_e.StateMismatch as nsd_err:
# In the event that a user is in the middle of a login when the app is restarted, # Check if the redirect has already been attempted
# their session state will no longer be available, so redirect the user to the if not request.session.get("redirect_attempted", False):
# beginning of login process without raising an error to the user. # Set the flag to indicate that the redirect has been attempted
logger.warning(f"No State Defined: {nsd_err}") request.session["redirect_attempted"] = True
return redirect(request.session.get("next", "/"))
# In the event of a sstate mismatch between OP and session, redirect the user to the
# beginning of login process without raising an error to the user. Attempt once.
logger.warning(f"No State Defined: {nsd_err}")
return redirect(request.session.get("next", "/"))
else:
# Clear the redirect_attempted flag?
return error_page(request, nsd_err)
except Exception as err: except Exception as err:
return error_page(request, err) return error_page(request, err)