django-oauth-toolkit icon indicating copy to clipboard operation
django-oauth-toolkit copied to clipboard

Client Credentials app throws assertion on auth code flow

Open Amertz08 opened this issue 3 years ago • 3 comments

Describe the bug

If you try and use a client credentials app as an authorization code app flow it will cause an assertion error to be thrown.

To Reproduce

  • Create a client_id as a client_credentials app w/o a redirect_uri
  • Attempt Authorization code flow using client_id
  • observe 500 and AssertionError: If you are using implicit, authorization_codeor all-in-one grant_type, you must define redirect_uris field in your Application model

Expected behavior

I would expect a 400 back to the client letting them know they are using the wrong client type or the 500 be handled.

Version

django-oauth-toolkit==1.4.1

  • [ ] I have tested with the latest published release and it's still a problem.
  • [ ] I have tested with the master branch and it's still a problem.

Additional context

App creation form does not do any validation on redirect_uris for a client_credentials app. So it appears an end user could create a bad app and the server not really tell them what is happening. It is up to the Oauth server owner to check the logs for the exception then communicate that to the app owner. Also the error does not appear handled as it does not go though the exception block here,

Amertz08 avatar Apr 06 '21 17:04 Amertz08

There's two ways of tackling this.

One is to add a model-level validation for a non-empty redirect_uris. I think that would not work well for a couple reasons. Firstly, there's a "data migration" issue that we would be forcing upon users on upgrades. Secondly, I think that "create an app, fill in the redirect_uris later" is a pretty normal workflow for this model! For example a user requests the app, the admins set it up, but the users are allowed to edit the redirect_uris.

Another way here would be to add validation before use of the field. I think that this makes a good amount of sense. If we set stuff up to raise a specific kind of error (either an OAuthToolkitError or a ValidationError even?) then we could easily generate 4xx-style responses when the values are not set up correctly.

#954 is a similar "need to handle validation errors cleanly in the view stack" problem

rtpg avatar Apr 13 '21 00:04 rtpg

I totally agree with the first part of your answer @rtpg, IMO adding a validator for a non-empty field doesn't make any sense.

Regarding the second part, there is already a validator (RedirectURIValidator) in case the user adds a value to the Redirect uris field.

Instead of this approach what do you think of something like

    def get_default_redirect_uri(self, client_id, request, *args, **kwargs):
        try:
            return request.client.default_redirect_uri
        except Exception as e:
            raise Http404(e)

https://github.com/jazzband/django-oauth-toolkit/blob/master/oauth2_provider/oauth2_validators.py#L290

panosangelopoulos avatar Apr 20 '21 15:04 panosangelopoulos

A new PR created

panosangelopoulos avatar May 04 '21 16:05 panosangelopoulos