Skip to content

Commit

Permalink
Fixed #4 -- Introduced a USE_STATE setting
Browse files Browse the repository at this point in the history
  • Loading branch information
ellmetha committed Oct 26, 2018
1 parent 613b425 commit b8bc701
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 10 deletions.
2 changes: 1 addition & 1 deletion oidc_rp/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def authenticate(self, nonce, request):

# Don't go further if the state value or the authorization code is not present in the GET
# parameters because we won't be able to get a valid token for the user in that case.
if state is None or code is None:
if (state is None and oidc_rp_settings.USE_STATE) or code is None:
raise SuspiciousOperation('Authorization code or state value is missing')

# Prepares the token payload that will be used to request an authentication token to the
Expand Down
5 changes: 5 additions & 0 deletions oidc_rp/conf/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@
CLIENT_ID = getattr(settings, 'OIDC_RP_CLIENT_ID', None)
CLIENT_SECRET = getattr(settings, 'OIDC_RP_CLIENT_SECRET', None)

# The 'USE_STATE' settings defines whether or not states should be used in the authentication flow.
# The state value is a recommended one (it is not required by the OpenID Connect specification). It
# is used to maintain state between the authentication request and the callback.
USE_STATE = getattr(settings, 'OIDC_USE_STATE', True)

# The 'STATE_LENGTH' setting defines the length of the opaque value used to maintain state between
# the authentication request and the callback. It is notably usefull to mitigate Cross-Site Request
# Forgery (CSRF, XSRF) by cryptographically binding the value with a cookie / a session key.
Expand Down
23 changes: 14 additions & 9 deletions oidc_rp/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,26 +37,28 @@ class OIDCAuthRequestView(View):
def get(self, request):
""" Processes GET requests. """
# Defines common parameters used to bootstrap the authentication request.
state = get_random_string(oidc_rp_settings.STATE_LENGTH)
authentication_request_params = request.GET.dict()
authentication_request_params.update({
'scope': oidc_rp_settings.SCOPES,
'response_type': 'code',
'client_id': oidc_rp_settings.CLIENT_ID,
'redirect_uri': request.build_absolute_uri(reverse('oidc_auth_callback')),
'state': state,
})

# Nonces should be used! In that case the generated nonce is stored both in the
# States should be used! They are recommended in order to maintain state between the
# authentication request and the callback.
if oidc_rp_settings.USE_STATE:
state = get_random_string(oidc_rp_settings.STATE_LENGTH)
authentication_request_params.update({'state': state})
request.session['oidc_auth_state'] = state

# Nonces should be used too! In that case the generated nonce is stored both in the
# authentication request parameters and in the user's session.
if oidc_rp_settings.USE_NONCE:
nonce = get_random_string(oidc_rp_settings.NONCE_LENGTH)
authentication_request_params.update({'nonce': nonce, })
request.session['oidc_auth_nonce'] = nonce

# The generated state value must be stored in the user's session for further use.
request.session['oidc_auth_state'] = state

# Stores the "next" URL in the session if applicable.
next_url = request.GET.get('next')
request.session['oidc_auth_next_url'] = next_url \
Expand Down Expand Up @@ -97,12 +99,15 @@ def get(self, request):
# NOTE: a redirect to the failure page should be return if some required GET parameters are
# missing or if no state can be retrieved from the current session.

if ((nonce and oidc_rp_settings.USE_NONCE) or not oidc_rp_settings.USE_NONCE) and \
('code' in callback_params and 'state' in callback_params) and state:
if (
((nonce and oidc_rp_settings.USE_NONCE) or not oidc_rp_settings.USE_NONCE) and
((state and oidc_rp_settings.USE_STATE) or not oidc_rp_settings.USE_STATE) and
('code' in callback_params and 'state' in callback_params)
):
# Ensures that the passed state values is the same as the one that was previously
# generated when forging the authorization request. This is necessary to mitigate
# Cross-Site Request Forgery (CSRF, XSRF).
if callback_params['state'] != state:
if oidc_rp_settings.USE_STATE and callback_params['state'] != state:
raise SuspiciousOperation('Invalid OpenID Connect callback state value')

# Authenticates the end-user.
Expand Down
13 changes: 13 additions & 0 deletions tests/integration/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,19 @@ def test_do_not_embed_a_nonce_in_the_request_parameters_if_the_related_setting_i
assert parsed_parameters['state']
assert 'nonce' not in parsed_parameters

@unittest.mock.patch('oidc_rp.conf.settings.USE_STATE', False)
def test_do_not_embed_a_state_in_the_request_parameters_if_the_related_setting_is_disabled(
self, client):
url = reverse('oidc_auth_request')
response = client.get(url, follow=False)
assert response.status_code == 302
parsed_parameters = parse_qs(urlparse(response.url).query)
assert parsed_parameters['response_type'] == ['code', ]
assert parsed_parameters['scope'] == ['openid email', ]
assert parsed_parameters['client_id'] == ['DUMMY_CLIENT_ID', ]
assert parsed_parameters['redirect_uri'] == ['http://testserver/oidc/auth/cb/', ]
assert 'state' not in parsed_parameters

def test_saves_the_authorization_state_value_in_the_user_session(self, client):
url = reverse('oidc_auth_request')
response = client.get(url, follow=False)
Expand Down

0 comments on commit b8bc701

Please sign in to comment.