Add CORS Middleware

[rcmurphy]

Description of the Change

Adds automatic CORS header generating middleware. As discussed in #287 and #300.

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added
• documentation updated
• CHANGELOG.md updated (only for user relevant changes)
• author name in AUTHORS

[n2ygk]

@rcmurphy Thanks for this. Please take a look at the failed tests. I recommend running tox locally before pushing up to github. Details here: https://django-oauth-toolkit.readthedocs.io/en/latest/contributing.html

[rcmurphy]

@n2ygk Yep! I am. I thought I was getting false negatives from not having every version of python installed; once I realized there were significant changes that need to be made I tried to convert it to a draft PR until I had it sorted, but was unable to do so. The recent couple force pushes were trying to figure out why pre-commit is still saying there need to be changes even though it passes locally. Edit: turns out some of my changes weren't getting pushed properly, causing the failures.

[n2ygk]

@rcmurphy I've converted it to draft. Let me know when ready for review. I'll try to make time this weekend.

[codecov]

Codecov Report

Merging #1150 (509df8c) into master (155bef3) will increase coverage by 0.00%.
The diff coverage is 97.36%.

@@           Coverage Diff           @@
##           master    #1150   +/-   ##
=======================================
  Coverage   96.90%   96.91%           
=======================================
  Files          31       31           
  Lines        1812     1850   +38     
=======================================
+ Hits         1756     1793   +37     
- Misses         56       57    +1     

Impacted Files	Coverage Δ
oauth2_provider/models.py	98.55% <88.88%> (-0.26%)	⬇️
oauth2_provider/middleware.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 155bef3...509df8c. Read the comment docs.

[rcmurphy]

@n2ygk I am passing tests locally, so I expect this to be ready for review. I added f51a741 while self-reviewing the change waiting for CI to pass, as I realized we could remove a try/except from control flow.

[n2ygk]

@rcmurphy I'm going to need a little extra time to try and understand this code by running it through a few test cases...

[rcmurphy]

@rcmurphy I'm going to need a little extra time to try and understand this code by running it through a few test cases...

@n2ygk - Not a problem; would love to encode them in unit tests if that would be pertinent.

[jaap3]

My issue with this middleware approach (and I have had a similar issue with django-cors-headers) is that it applies to all urls that are handled by Django. I.e. with this middleware installed client apps are also allowed to access Django's admin urls.

I've worked around this in projects in several ways. One is a rather complicated cors_request_allowed signal handler (for django-cors-headers). Another one was wrapping django-cors-headers in Django's decorator_from_middleware and applying it only to the views I want to be more liberal in regards to cross origin requests.

I'm not sure what the best approach is to be honest. The middleware is convenient if all views in the Django application should be accesible to clients, but it's hard to tell if that's the majority of use cases.

[n2ygk]

My issue with this middleware approach (and I have had a similar issue with django-cors-headers) is that it applies to all urls that are handled by Django. I.e. with this middleware installed client apps are also allowed to access Django's admin urls.

I've worked around this in projects in several ways. One is a rather complicated cors_request_allowed signal handler (for django-cors-headers). Another one was wrapping django-cors-headers in Django's decorator_from_middleware and applying it only to the views I want to be more liberal in regards to cross origin requests.

I'm not sure what the best approach is to be honest. The middleware is convenient if all views in the Django application should be accesible to clients, but it's hard to tell if that's the majority of use cases.

@jaap3 Would you agree that @rcmurphy's approach is an improvement, even if not the perfect fix? Incremental progress beyond CORS_ORIGIN_ALLOW_ALL = True feels like a pretty good step.

I'm just trying to understand the implications and still need to set up my test to see how this PR changes things. Sorry @rcmurphy but "next week" has become "next month"!

[jaap3]

Well CORS (cross-origin resource sharing) is about removing security barriers that are in place by default, so I'm hesitant in saying that this is an improvement. IMHO an overly permissive CORS policy is worse than not providing CORS headers at all, as it can have unforeseen consequences. From a library perspective DOT should really only apply CORS headers to resources it provides and can be reasonably expected to be requested by cross origin scripts.

Going through the views that are in DOT I'd say that the following views should probably be served with CORS headers, as they are likely used by public clients:

• base.TokenView
• base.RevokeTokenView
• introspect.IntrospectTokenView
• oidc.ConnectDiscoveryInfoView
• oidc.JwksInfoView
• oidc.UserInfoView

I'd say that having CORS headers for e.g. the AuthorizationView might even pose a security risk. Is there any OAuth2/OIDC flow that requires a public client to directly access the authorization endpoint (e.g. not through a redirect)?

If instead of a middleware a mixin approach were used I'd be much more comfortable.

I.e. (untested example code):

class CrossOriginSharedResource:
    def dispatch(self, request, *args, **kwargs):
        # If this is a preflight-request, we must always return 200
        if request.method == "OPTIONS" and "HTTP_ACCESS_CONTROL_REQUEST_METHOD" in request.META:
            response = http.HttpResponse()
        else:
            response = super().dispatch(request, *args, **kwargs)
        try:
            cors_allow_origin = _get_cors_allow_origin_header(request)
        except AbstractApplication.NoSuitableOriginFoundError:
            pass
        else:
            response["Vary"] = "Origin"
            response["Access-Control-Allow-Origin"] = cors_allow_origin
            response["Access-Control-Allow-Credentials"] = "true"
            if request.method == "OPTIONS":
                response["Access-Control-Allow-Headers"] = ", ".join(HEADERS)
                response["Access-Control-Allow-Methods"] = ", ".join(METHODS)
        return response

(I added a Vary header as that might help prevent confusing caching issues, i.e. when using a caching proxy).

This can be combined with mixins.ProtectedResourceMixin to have protected shared resources.

In addition, I'm not sure if redirect_url is always a good indicator of where other request will be coming from. I have had clients that use something like com.example://callback as the redirect url, but subsequent API request are coming from example.com. This is the case for Electron apps and some "native" iOS and Android apps that use a custom scheme to handle these callbacks (and other in-app deep links).

So maybe an additional "origins" field needs to be added to applications?

I hope this makes sense?

P.S. I also believe confidential clients shouldn't really be taken into account when looking for CORS origins, as they normally are only really used by server side software where CORS does not apply.

[n2ygk]

@rcmurphy @jaap3 Would it be possible for oauth2_provider.middleware.CorsMiddleware to be made selective and only apply the CORS headers for the relevant DOT views? This would make it simple to enable for the library user while not over-applying middleware to no-relevant views. I'm thinking about uses cases where DOT is included in the same project that includes Oauth2-protected resource server views or even totally unrelated views where we wouldn't want the DOT middleware to change things.

[rcmurphy]

@n2ygk @jaap3 sorry, I've been swamped with my day job. Two thoughts:

1. Absolutely. We shouldn't open everything up to CO requests; we could add an array of regexes to settings to whitelist endpoints
2. Maybe the more pragmatic way is to add a decorator? I need to think about this more. We'd probably need both, as I wouldn't want to break, for example, DRF or django-ninja, but it would also be difficult to properly decorate those.

[jaap3]

To me the suggested mixin approach (which could probably also be turned into a decorator) seems to be the most pragmatic. It allows DOT to selectivly apply CORS to the minimal set of views, and gives developers the option to use DOT to provide CORS headers to other views.

Here's an example decorator (untested):

def cross_origin_shared_resource(view):
    def cors_view(request, *args, **kwargs):
        # If this is a preflight-request, we must always return 200
        if request.method == "OPTIONS" and "HTTP_ACCESS_CONTROL_REQUEST_METHOD" in request.META:
            response = http.HttpResponse()
        else:
            response = view(request, *args, **kwargs)
        try:
            cors_allow_origin = _get_cors_allow_origin_header(request)
        except AbstractApplication.NoSuitableOriginFoundError:
            pass
        else:
            response["Vary"] = "Origin"
            response["Access-Control-Allow-Origin"] = cors_allow_origin
            response["Access-Control-Allow-Credentials"] = "true"
            if request.method == "OPTIONS":
                response["Access-Control-Allow-Headers"] = ", ".join(HEADERS)
                response["Access-Control-Allow-Methods"] = ", ".join(METHODS)
        return response
    functools.update_wrapper(cors_view, view)

It could then be apllied to class based views using Django's django.utils.decorators.method_decorator.

[dopry]

@rcmurphy any inclination to continue working on this PR or rebased it on master to help keep it current?

[islam-kamel]

@rcmurphy you can fix this conflicts ?

[islam-kamel]

@rcmurphy This Function get_cors_header in -> oauth2_provider/models.py Line 206 Not Covered by Tests

[dopry]

I would strongly support an approach that adds allowed origins to application and utilizes decorators on the built in views. I'm not in favor of implementing this as a middleware that runs on every request when it can be done at the view level on this specific requests.

[dopry]

@rcmurphy first, I really appreciate the time and energy you've put into this. I hope you don't find my position on this PR discouraging.

I don't think the middleware approach here is the best approach. At the core of my concern is performance and support. With the middleware approach, we are processing on every request, not just for the DOT views. This creates overhead for every site that consumes it on requests that are not our purview. It could cause issues with unrelated applications which would likely become support overhead for the DOT maintainers. It could also create support overhead dealing with settings issues like middleware and application ordering.

I would strongly support an approach that adds allowed origins to application and utilizes decorators on the built in views.

Would you be willing to pursue this alternate implementation path?

[dopry]

We merged additional CORS work in Fix CORS by passing 'Origin' header to OAuthLib, in #1229. There is an example at https://github.com/jazzband/django-oauth-toolkit/blob/master/tests/app/idp/idp/apps.py of using the coreheaders middleware to set explicit headers for a given view for people who need additional help with cors. I feel that satisfies the middleware use case presented here with an existing middleware. It would still be nice to have an internal DOT way to set the cors headers on the userinfo endpoint.

@rcmurphy has your need with regard to this PR been addressed in that work? Or is there still something that could improve you situation that needs doing?

[dopry]

I'm closing this as stale and possibly resolved. The corsheaders.Middleware.CorsMiddleware from the django-cors-headers package can be configured to apply to routes selectively using the corsheaders.signals.check_request_enabled signal. There is an example of configuring it in [tests/apps/idp](https://github.com/jazzband/django-oauth-toolkit/tree/master/tests/app/idp. Generally it only needs to be applied to the userinfo and openid-configuration endpoints. It would be nice to embed correct CORS behavior for those endpoints. PRs that improve those endpoints would be welcome.