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

add OAS securitySchemes and security objects #7516

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

n2ygk
Copy link
Contributor

@n2ygk n2ygk commented Sep 4, 2020

Description

adds openapi securitySchemes component and security objects both root level and to operations if needed.

For example:

components:
  ...
  securitySchemes:
    basicAuth:
      type: http
      scheme: basic
      description: Basic Authentication
    sessionAuth:
      type: apiKey
      in: cookie
      name: JSESSIONID
      description: Session authentication

...
paths:
  /v1/courses/:
    get:
      operationId: List/v1/courses/
      description: A course of instruction. e.g. COMSW1002 Computing in Context
      security:
      - basicAuth: []
      - sessionAuth: []

Adds two new classmethods to authentication:

  • openapi_security_scheme: returns an OAS securityScheme object
  • openapi_security_requirement: returns an OAS security requirements object

The logic behind doing it this way is that anyone that extends BaseAuthentication will be able to add their own OAS securitySchemes and security objects, for example, for OAuth 2.0 in django-oauth-toolkit's TokenMatchesOASRequirements()...

Tests and documentation still need updating but I wanted to get feedback on the general approach before proceeding.

@n2ygk n2ygk marked this pull request as draft September 4, 2020 12:51
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this. It seems to me that the vast majority of projects set auth once at the project level and don’t modify it per-view, but here we’re adding the extra details with every operation. It seems like overkill.

Ideally we’d add at the schema level and then allow overrides, by using an AutoSchema subclass on the few views with the different authentication classes. In that case helpers to map to the OAS objects would be handy. Maybe we could provide that in a mixin... 🤔

This would need docs and tests.

@n2ygk
Copy link
Contributor Author

n2ygk commented Sep 4, 2020

Authentication is top-level in the components.securitySchemes which itemizes which alternative schemes are available (e.g. basic auth, session auth, token auth, etc).

What you describe is putting the authorization security requirement object list at the root level which then applies to the entire schema, unless overridden at the operation level.

If authorizations are in fact the same for all operations, one would need to somehow deduce that for each view iterated over in get_schema() they share a common set of authentication and permission classes.... That would certainly be a good optimization to handle the common project-level case and would enable reducing some duplicated operation-level security objects.

But, in the case where they do differ, by putting the security requirement objects at the operation level, they are consistent with the way CBV's are declared.

Once one gets to permission_classes, especially with OAuth 2.0 scopes (not yet implemented in this PR, but I've done it before and plan on adding that to this PR next -- unless this gets shot down;-), I hope you will see the value of documenting the requirements at the operation level. See the example at https://django-oauth-toolkit.readthedocs.io/en/latest/rest-framework/permissions.html#tokenmatchesoasrequirements reproduced here:

openapi: "3.0.0"
info:
  title: songs
  version: v1
components:
  securitySchemes:
    song_auth:
      type: oauth2
      flows:
        implicit:
          authorizationUrl: http://localhost:8000/o/authorize
          scopes:
            read: read about a song
            create: create a new song
            update: update an existing song
            delete: delete a song
            post: create a new song
            widget: widget scope
            scope2: scope too
            scope3: another scope
paths:
  /songs:
    get:
      security:
        - song_auth: [read]
      responses:
        '200':
          description: A list of songs.
    post:
      security:
        - song_auth: [create]
        - song_auth: [post, widget]
      responses:
        '201':
          description: new song added
    put:
      security:
        - song_auth: [update]
        - song_auth: [put, widget]
      responses:
        '204':
          description: song updated
    delete:
      security:
        - song_auth: [delete]
        - song_auth: [scope2, scope3]
      responses:
        '200':
          description: song deleted

This documents for the client app developer which (alternative sets of) scopes are required by the server for each given path and operation.

Basically for the security object, the name is the type of authentication and the list is the authorizations. When authorization is done strictly inside the app, the list is empty, but some level of high-level authorization can be indicated in the OAS schema as well.

Here's a snippet example for declaring the OAuth2 securityScheme component. I've used this stuff with Swagger-UI "Try It" and it actually works;-)

class MyOAuth2Auth(OAuth2Authentication):
    openapi_security_scheme_name = 'oauth2ForYou'

    @classmethod
    def openapi_security_scheme(cls):
        scheme = {
            cls.openapi_security_scheme_name: {
                'type': 'oauth2',
                'description': 'OAuth 2.0 authentication'
            }
        }
        flows = {}
        if 'authorization_code' in settings.OAUTH2_CONFIG['grant_types_supported']:
            flows['authorizationCode'] = {
                'authorizationUrl': settings.OAUTH2_CONFIG['authorization_endpoint'],
                'tokenUrl': settings.OAUTH2_CONFIG['token_endpoint'],
                'refreshUrl': settings.OAUTH2_CONFIG['token_endpoint'],
                'scopes': {s: s for s in settings.OAUTH2_CONFIG['scopes_supported']}}
        if 'implicit' in settings.OAUTH2_CONFIG['grant_types_supported']:
            flows['implicit'] = {
                'authorizationUrl': settings.OAUTH2_CONFIG['authorization_endpoint'],
                'scopes': {s: s for s in settings.OAUTH2_CONFIG['scopes_supported']}}
        if 'client_credentials' in settings.OAUTH2_CONFIG['grant_types_supported']:
            flows['clientCredentials'] = {'tokenUrl': settings.OAUTH2_CONFIG['token_endpoint'],
                'refreshUrl': settings.OAUTH2_CONFIG['token_endpoint'],
                'scopes': {s: s for s in settings.OAUTH2_CONFIG['scopes_supported']}}
        if 'password' in settings.OAUTH2_CONFIG['grant_types_supported']:
            flows['password'] = {'tokenUrl': settings.OAUTH2_CONFIG['token_endpoint'],
                'refreshUrl': settings.OAUTH2_CONFIG['token_endpoint'],
                'scopes': {s: s for s in settings.OAUTH2_CONFIG['scopes_supported']}}
        scheme[cls.openapi_security_scheme_name]['flows'] = flows

This generates this OAS schema fragment:

  securitySchemes:
    oauth2ForYou:
      type: oauth2
      description: OAuth 2.0 authentication
      flows:
        authorizationCode:
          authorizationUrl: https://oauth-test.cc.columbia.edu/as/authorization.oauth2
          tokenUrl: https://oauth-test.cc.columbia.edu/as/token.oauth2
          refreshUrl: https://oauth-test.cc.columbia.edu/as/token.oauth2
          scopes:
            address: address
            read: read
            openid: openid
            profile: profile
            update: update
            auth-columbia: auth-columbia
            delete: delete
            auth-none: auth-none
            offline_access: offline_access
            https://api.columbia.edu/scope/group: https://api.columbia.edu/scope/group
            create: create
            demo-djt-sla-bronze: demo-djt-sla-bronze
            email: email
            cas-tsc-sla-gold: cas-tsc-sla-gold
        implicit:
          authorizationUrl: https://oauth-test.cc.columbia.edu/as/authorization.oauth2
          scopes:
            address: address
            read: read
            openid: openid
       ...

I still need to get to the openapi_security_requirement() definition which needs to inspect the CBV's permission_classes. I have code that does it, but first wanted to get feedback on whether you think this is a good approach to automating making a fully functional OAS document that Swagger UI can use.

@carltongibson
Copy link
Collaborator

Okay, good.

So I guess here's my question, does it make sense to skip adding the security details when authentication_classes == api_settings.DEFAULT_AUTHENTICATION_CLASSES? (I.e. they've not been overridden?)

@carltongibson
Copy link
Collaborator

@n2ygk Just to clarify: I think this is a good addition. Please do continue. 🙂

@n2ygk
Copy link
Contributor Author

n2ygk commented Sep 5, 2020

Okay, good.

So I guess here's my question, does it make sense to skip adding the security details when authentication_classes == api_settings.DEFAULT_AUTHENTICATION_CLASSES? (I.e. they've not been overridden?)

That's a great idea. I'll also use the defaults, if set, to create a root-level security requirements object.

@n2ygk n2ygk changed the title WIP: add OAS securitySchemes and security objects add OAS securitySchemes and security objects Sep 5, 2020
@n2ygk n2ygk marked this pull request as ready for review September 5, 2020 20:42
@n2ygk
Copy link
Contributor Author

n2ygk commented Sep 5, 2020

@carltongibson I've incorporated your suggestions, cleaned up the tests and added new ones as well as documentation, and I've rebased. I look forward to your feedback. Cheers!

@n2ygk
Copy link
Contributor Author

n2ygk commented Sep 8, 2020

Oops this last rebase broke due to url vs. path. I should have run tox locally first. Fixing it now.

@n2ygk
Copy link
Contributor Author

n2ygk commented Sep 8, 2020

hmm, @carltongibson your review comment seems to have disappeared. Maybe github is being eventually consistent?

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this looks good. With more time I'd like to go over and regroup some of the newer additions, but that's not a criticism of this.

This should be a nice addition. We can come back and tidy later. Thanks @n2ygk

@carltongibson carltongibson added this to the 3.12 Release milestone Sep 8, 2020
@carltongibson
Copy link
Collaborator

@n2ygk — No I deleted it, it wasn't right. The code there is getting a little procedural — that's OK, we can look at it over time — and I'd misread my place. First pass, i think this does the job and we should probably have it.

I asked Tom to review as I didn't have time to make smaller comments. Maybe he will, maybe he won't.

n2ygk added a commit to keyz182/django-rest-framework-json-api that referenced this pull request Oct 12, 2020
n2ygk added a commit to keyz182/django-rest-framework-json-api that referenced this pull request Oct 14, 2020
n2ygk added a commit to keyz182/django-rest-framework-json-api that referenced this pull request Oct 23, 2020
@n2ygk
Copy link
Contributor Author

n2ygk commented Mar 24, 2021

@tomchristie I don't mean to be annoying but am wondering what the likelihood is of this PR being accepted. I see this milestone is due in a few days.

@tomchristie
Copy link
Member

Okay, just got through to this one in my notifications. As it happens I've just rolled a point release. Because.
But I'm okay with working through the 3.13 milestoned issues likely with a view to a 3.13 release in about a months time.

@n2ygk
Copy link
Contributor Author

n2ygk commented May 17, 2021

Okay, just got through to this one in my notifications. As it happens I've just rolled a point release. Because.
But I'm okay with working through the 3.13 milestoned issues likely with a view to a 3.13 release in about a months time.

@tomchristie FYI - I've rebased in hopeful anticipation of your imminent review ;-)

@n2ygk
Copy link
Contributor Author

n2ygk commented Dec 13, 2021

@tomchristie @carltongibson I see you've merged a new 3.13 release. Is this PR still on the queue for eventual review? Thanks.

@stale
Copy link

stale bot commented Mar 27, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 27, 2022
@n2ygk
Copy link
Contributor Author

n2ygk commented Apr 9, 2022

Not stale! See #8453

@stale stale bot removed the stale label Apr 9, 2022
Comment on lines +55 to +61
@classmethod
def openapi_security_scheme(cls):
"""
Override this to return an Open API Specification `securityScheme object
<http://spec.openapis.org/oas/v3.0.3#security-scheme-object>`_
"""
return {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I would rather see this sort of hook added via a decorator (or such) rather than added in as part of the DRF API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carltongibson Could you provide an example of how that might work? I don't quite understand.

@stale
Copy link

stale bot commented Jun 11, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 11, 2022
@stale
Copy link

stale bot commented Apr 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 2, 2023
@auvipy auvipy removed this from the 3.15 milestone Apr 2, 2023
@stale stale bot removed stale labels Apr 2, 2023
@stale
Copy link

stale bot commented Jun 10, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 10, 2023
@auvipy
Copy link
Member

auvipy commented Jun 13, 2023

@tfranzel can we or do we need this feature ported to drf-spectacular?

@stale stale bot removed the stale label Jun 13, 2023
@tfranzel
Copy link
Member

@auvipy yes I think so. Since schema information on auth was completely missing in DRF (unlike for ex. pagination), we created extensions that do exactly what was added here.

https://github.com/tfranzel/drf-spectacular/blob/ceff2fc9c6deb509b300bbf82ee8d88d00cd8eb6/drf_spectacular/extensions.py#L13

Based on that, we implemented extensions for all build-in DRF auth methods, as well as the most popular 3rd party auth apps:

https://github.com/tfranzel/drf-spectacular/blob/master/drf_spectacular/authentication.py
https://github.com/tfranzel/drf-spectacular/tree/master/drf_spectacular/contrib

We can make use of this added information in case there is no extensions. We would need to keep the existing extensions due to compatibility issues with older DRF versions anyway. I currently don't really have an opinion on how exactly this information should be added.

So from my side there are no blockers, however this thing has gotchas. We ended up extending the interface twice to cover missed use-cases, e.g. multiple headers for one auth. Just thought this is worth mentioning.

@stale
Copy link

stale bot commented Aug 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants