Skip to content

/ oauthlib

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

confirm_redirect_uri after validate_code feels wrong #688

Open
bertjwregeer opened this issue Jul 18, 2019 · 1 comment
Open

confirm_redirect_uri after validate_code feels wrong #688

bertjwregeer opened this issue Jul 18, 2019 · 1 comment
Labels
Discussion OAuth2-Provider

Comments

@bertjwregeer
Copy link

@bertjwregeer bertjwregeer commented Jul 18, 2019 •
edited

I'm implementing a validator, and I am finding the behaviour of validate_token_request in

oauthlib/oauthlib/oauth2/rfc6749/grant_types/authorization_code.py

Lines 435 to 543 in d7b90fc

def validate_token_request(self, request):
"""
:param request: OAuthlib request.
:type request: oauthlib.common.Request
"""
# REQUIRED. Value MUST be set to "authorization_code".
if request.grant_type not in ('authorization_code', 'openid'):
raise errors.UnsupportedGrantTypeError(request=request)
for validator in self.custom_validators.pre_token:
validator(request)
if request.code is None:
raise errors.InvalidRequestError(
description='Missing code parameter.', request=request)
for param in ('client_id', 'grant_type', 'redirect_uri'):
if param in request.duplicate_params:
raise errors.InvalidRequestError(description='Duplicate %s parameter.' % param,
request=request)
if self.request_validator.client_authentication_required(request):
# If the client type is confidential or the client was issued client
# credentials (or assigned other authentication requirements), the
# client MUST authenticate with the authorization server as described
# in Section 3.2.1.
# https://tools.ietf.org/html/rfc6749#section-3.2.1
if not self.request_validator.authenticate_client(request):
log.debug('Client authentication failed, %r.', request)
raise errors.InvalidClientError(request=request)
elif not self.request_validator.authenticate_client_id(request.client_id, request):
# REQUIRED, if the client is not authenticating with the
# authorization server as described in Section 3.2.1.
# https://tools.ietf.org/html/rfc6749#section-3.2.1
log.debug('Client authentication failed, %r.', request)
raise errors.InvalidClientError(request=request)
if not hasattr(request.client, 'client_id'):
raise NotImplementedError('Authenticate client must set the '
'request.client.client_id attribute '
'in authenticate_client.')
request.client_id = request.client_id or request.client.client_id
# Ensure client is authorized use of this grant type
self.validate_grant_type(request)
# REQUIRED. The authorization code received from the
# authorization server.
if not self.request_validator.validate_code(request.client_id,
request.code, request.client, request):
log.debug('Client, %r (%r), is not allowed access to scopes %r.',
request.client_id, request.client, request.scopes)
raise errors.InvalidGrantError(request=request)
# OPTIONAL. Validate PKCE code_verifier
challenge = self.request_validator.get_code_challenge(request.code, request)
if challenge is not None:
if request.code_verifier is None:
raise errors.MissingCodeVerifierError(request=request)
challenge_method = self.request_validator.get_code_challenge_method(request.code, request)
if challenge_method is None:
raise errors.InvalidGrantError(request=request, description="Challenge method not found")
if challenge_method not in self._code_challenge_methods:
raise errors.ServerError(
description="code_challenge_method {} is not supported.".format(challenge_method),
request=request
)
if not self.validate_code_challenge(challenge,
challenge_method,
request.code_verifier):
log.debug('request provided a invalid code_verifier.')
raise errors.InvalidGrantError(request=request)
elif self.request_validator.is_pkce_required(request.client_id, request) is True:
if request.code_verifier is None:
raise errors.MissingCodeVerifierError(request=request)
raise errors.InvalidGrantError(request=request, description="Challenge not found")
for attr in ('user', 'scopes'):
if getattr(request, attr, None) is None:
log.debug('request.%s was not set on code validation.', attr)
# REQUIRED, if the "redirect_uri" parameter was included in the
# authorization request as described in Section 4.1.1, and their
# values MUST be identical.
if request.redirect_uri is None:
request.using_default_redirect_uri = True
request.redirect_uri = self.request_validator.get_default_redirect_uri(
request.client_id, request)
log.debug('Using default redirect_uri %s.', request.redirect_uri)
if not request.redirect_uri:
raise errors.MissingRedirectURIError(request=request)
else:
request.using_default_redirect_uri = False
log.debug('Using provided redirect_uri %s', request.redirect_uri)
if not self.request_validator.confirm_redirect_uri(request.client_id, request.code,
request.redirect_uri, request.client,
request):
log.debug('Redirect_uri (%r) invalid for client %r (%r).',
request.redirect_uri, request.client_id, request.client)
raise errors.MismatchingRedirectURIError(request=request)
for validator in self.custom_validators.post_token:
validator(request)
a little bit frustrating.

Specifically the fact that validate_code is not required to validate the redirect_uri, and instead we basically get two calls that are fairly similar:

  • validate_code
    • client_id
    • code
    • client
    • request

Presumable in validate_code we do a bunch of work:

  1. Look up the code, validate that it matches the client_id, validate it hasn't expired (since these are supposed to be short-lived)
  2. We return .user, .state (if set), .scopes, .claims and the PKCE challenge stuff on request

Then a couple lines later we are asked to do:

  • confirm_redirect_uri
    • client_id
    • code
    • redirect_uri
    • client
    • request

Since we didn't return the original database object from the original lookup, to return True/False to this check, we basically have to look up the code again, especially since there is no requirement to set the redirect_uri we stored in the database on the request object.

Is there an expectation that we stash the database object somewhere ourselves?

Why not pass the redirect_uri to the validate_code function instead (or pull it from request.redirect_uri. This way I can even save time of doing validation in Python, instead I can just tell SQLAlchemy what I am looking for, and the database can do the sensible thing and not return a row if nothing matches (and thus I can return False).

If I were to write a patch that would move that functionality up, and update the docstrings to tell users of the library to properly validate the redirect_uri in validate_code would that get accepted by the maintainers?
@bertjwregeer bertjwregeer changed the title confirm_redirect_uri after valid_code feels wrong confirm_redirect_uri after validate_code feels wrong Jul 18, 2019
@JonathanHuot
Copy link
Member

@JonathanHuot JonathanHuot commented Jul 22, 2019

Hi,

Thanks for this detailed report. The overall idea of oauthlib is to provide a framework for OAuth2.0 and to put all the "RFC code" into the framework, and all the "implementation code" into your project.

If we tell users to validate themselves the redirect_uri in validate_code, it will break this fundamental rule.
Also, the different flows require different logic, see the diagram below:

image

BTW, in order to avoid the extra database lookup, I'd suggest to store the necessary thing (i.e. the database record) in the request object, and use it from the confirm_redirect_uri, without doing any extra database call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Discussion OAuth2-Provider
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@bertjwregeer @JonathanHuot
You can’t perform that action at this time.