Skip to content
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

Oauth2 provider: TokenMatchesOASRequirements doesn't pass validation #469

Closed
tomasgarzon opened this issue Jul 27, 2021 · 8 comments
Closed
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@tomasgarzon
Copy link

Describe the bug
I have a ModelViewSet with permission_classes = [TokenMatchesOASRequirements], generating the schema with --validate

Using django-oauth-toolkit.

jsonschema.exceptions.ValidationError: {'GET': [['v2:integration:account:client:read']], 'POST': [], 'PUT': [['v2:integration:account:client:update']], 'PATCH': [['v2:integration:account:client:update']], 'DELETE': []} is not of type 'array'

Failed validating 'type' in schema['properties']['paths']['patternProperties']['^\\/']['patternProperties']['^(get|put|post|delete|options|head|patch|trace)$']['properties']['security']['items']['additionalProperties']:
   {'items': {'type': 'string'}, 'type': 'array'}

On instance['paths']['/api/v2/integration/account/clients/']['get']['security'][0]['oauth2']:
   {'DELETE': [],
    'GET': [['v2:integration:account:client:read']],
    'PATCH': [['v2:integration:account:client:update']],
    'POST': [],
    'PUT': [['v2:integration:account:client:update']]}

To Reproduce
Create a ModelViewset with permission_classes=[TokenMatchesOASRequirements], generate schema with --validate

Expected behavior
Schema file generated without validation errors.

@tfranzel tfranzel added bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending labels Jul 30, 2021
@tfranzel
Copy link
Owner

turns out this case was simply not handled correctly. implemented a flattened list for the scopes, which now do pass validation.

however, OpenAPI has not concept of alternate groups. thus i just tried to achieve the closest possible thing by flattening the list of scope groups.

@tfranzel
Copy link
Owner

closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.

@tomasgarzon
Copy link
Author

Why do you close this issue? I thought you will include this commit in your next version

@tfranzel
Copy link
Owner

0.18.0 was just released with this fix. is there anything missing?

@tomasgarzon
Copy link
Author

tomasgarzon commented Aug 25, 2021 via email

@pbav
Copy link

pbav commented Aug 25, 2022

@tfranzel

implemented a flattened list for the scopes

It looks like you can't simply flatten the list of scopes, it breaks the OR logic - see
https://django-oauth-toolkit.readthedocs.io/en/latest/rest-framework/permissions.html#tokenmatchesoasrequirements

An example from there:

required_alternate_scopes = {
        "GET": [["read"]],
        "POST": [["create"], ["post", "widget"]],
        "PUT":  [["update"], ["put", "widget"]],
        "DELETE": [["delete"], ["scope2", "scope3"]],
    }

E.g., for DELETE, it is supposed to accept either a scope delete or a combination of scope2 and scope3.
In the schema that would be:

delete:
      security:
        - song_auth: [delete]
        - song_auth: [scope2, scope3]

When you flatten the list, it becomes

delete:
      security:
        - song_auth: [delete, scope2, scope3]

I.e. all the scopes are required at the same time.

@tfranzel
Copy link
Owner

ah I see. Somehow I didn't realize we could do multiple entries, but it is a list after all.

I will have a look at it shortly.

tfranzel added a commit that referenced this issue Aug 25, 2022


 #469: removes list flattening for oauth-toolkit as it is incorrect while
 the correct solution is indeed possible with OpenAPI3.
 #626: change AND to OR for dj_rest_auth JWTCookieAuthentication
@tfranzel
Copy link
Owner

@pbav you were absolutely right. Somehow I overlooked the AND/OR distinction. Just needed to slightly extend the OpenApiAuthenticationExtension.get_security_requirement interface.

This should now do the right thing finally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

3 participants