jazzband / django-oauth-toolkit

OAuth2 goodies for the Djangonauts!
https://django-oauth-toolkit.readthedocs.io
Other
3.12k stars 792 forks source link

Client Credentials app throws assertion on auth code flow #958

Open Amertz08 opened 3 years ago

Amertz08 commented 3 years ago

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

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

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,

rtpg commented 3 years ago

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

panosangelopoulos commented 3 years ago

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 commented 3 years ago

A new PR created