CORS Implementation

[dopry]

CORS support is important for security. We should figure out our CORS specification to help guide the implementation. There are currently 2 CORS related PRs, #1150 , #1229.

in #1150 the implementation uses a middleware, while the discussion has guided it toward a decorator on the views that need CORS support.

in #1229, the approach enables upstream handling of CORS using the OAuthLib implementation which defers the is_origin_allowed check to the OAuth2Validator. This approach with work for the OAuthLib provided views, but not the views that are specific to DjangoOauthToolkit like OIDC RP initiated logout.

1. I'd like to get to a consensus on the best approach, whether to lean into using the OAuth2Validator and ensure we're utilize it consistently in the DOT views, or whether to move forward with a decorator or middleware approach with our views...

2. I'd like to get to a consensus on how to make a good out of the box UX and DX that provides consistent CORS support either by implementing a base validator or decorators that are aware of the DOT model and settings.

[tonial]

Hi @dopry

I agree with your arguments against a global middleware : we only need to handle CORS on specific views.

Each application should have specific CORS (none, the redirect_uris, or anything else). This means the piece of code handling the CORS header should be aware of the application.

I went digging in OAuthLib a bit, and I saw that there a method on the validator to do that : oauthlib.oauth2.RequestValidator.is_origin_allowed() (see here).

I think we should override it in Oauth2Validator and compare the origin with the application's redirect_uris, or a new field (Application.allowed_origins ?).

Since the RP logout view already relies on OAuth2Validator (through oauth2_provider.views.oidc.validate_logout_request that calls oauth2_provider.views.oidc._validate_claims), we could also use it to add the cors headers (as is OAuthLib : https://github.com/oauthlib/oauthlib/blob/master/oauthlib/oauth2/rfc6749/grant_types/base.py#L253-L268)

I'm just not sure if we need CORS headers on the logout view : it's not an API that can be called by some javascript, so I don't see when we would need it.

EDIT : after some testing, this will probably not be enough. I have a RP that requires CORS headers on the .well-known/openid-configuration endpoint, which isn't using OAuth2Validator.
Maybe the mixin approach is better, that way we can add it to every oidc view.

[dopry]

@tonial Maybe a both approach would work here... We use the OAuth2Validator where it is applicable and we supplement it with a mixin or decorator on views like .well-known/* where the header support isn't coming from upstream. I think it would be important to document that with something like we prefer upstream CORS header support, but this endpoint doesn't have that so we're using this decorator/mixin instead or Upstream endpoints provide CORS support through OAuth2Validator.is_origin_allowed, this is a local endpoint specific to django-oauth-toolkit. for these cases we use this decorator/mixin. If this endpoint ever ends up stream we should use the upstream CORS mechanism instead. depending on the scenario. thoughts?

[rcoup]

We've been using #1229: it works well, can be customised/limited per-client/etc.

For example, we only enable CORS on the /token/ endpoint, for certain clients, and restricted to a set of known origin domains built from the redirect URI list.

Feels like that'd be very difficult with a middleware approach.

[tonial]

@dopry I think it's a great idea.

[dopry]

@rcoup since you've been working with the implementation for multiple clients, do you have any thoughts on good default behavior? We've been discussing just adding an allowed origins list to the application or inferring them from redirect_uris... do you think that would work as a good default for the implementations you've done? Would you have a preference for explicit allowed origins vs inferred? Are there any other controls you might want over default behavior?

[rcoup]

@dopry I inferred it from redirect_uris, and I also limited it to public clients. I could imagine cases where someone would want explicit origins, but feels like redirect_uris will be the same in most setups.

Vaguely something like this might be useful as a default? And it's straightforward to override with any other logic.

class MyClient(AbstractApplication):
    ...
    def is_origin_allowed(self, origin):
        """ Detemine whether or not to enable CORS """
        if self.client_type == self.CLIENT_PUBLIC and self.authorization_grant_type == self.GRANT_AUTHORIZATION_CODE:
            # re-use loopback handling logic in redirect_to_uri_allowed, but strip the path/querystring components
            redirect_uri_origins = []
            for redirect_uri in self.redirect_uris.split():
                parsed_redirect_uri = urlparse(redirect_uri)
                redirect_uri_origins.append(f"{parsed_redirect_uri.scheme}://{parsed_redirect_uri.netloc}")

            if redirect_to_uri_allowed(origin, redirect_uri_origins):
                return True

        return False

redirect_to_uri_allowed() could probably be refactored a little to support this without the above hackery. It did 90% of what I needed :-)

[dopry]

yeah. I agree and I believe redirect_uris will be the allowed_origins in most cases... but as a maintainer of a shared library, I'm a little worried about making the assumption for people.

Let's take a poorly advised scenario for an internationalized site as strawman for discussion. Imaging example.com has a redirect_uri of localizeme.example.com/oauth/callback which redirects users to the locale specific domain ejemplo.com. Let's assume for whatever reason the redirector (localizaeme.example.com) isn't trusted enough to actually request tokens, but is trusted enough to pass along an authorization code. In this case the we wouldn't want localizeme.example.com to be an allowed origin, we would only want ejemplo.com to be an allowed origin.

I know the scenario is far fetched, and would be the last thing I would ever do... but I've been in many an enterprise environment where I was directed to do many a thing I strongly advised against. I'm not sure offhand if this is something the spec's have explicit direction around.

I feel like keeping the allowed origin explicit would enable this scenario.. but it comes at a small cost of convenience and some PEBKAC copy paste mistakes. @rcoup @n2ygk thoughts?

I believe the implementation by @akanstantsinau in #1229 is explicit... I don't want to ask them to do additional work to get this issue closed either since we're down to what looks like some minor doc changes and naming issues.

[rcoup]

I guess with empty allowed-origins: CORS doesn't work by default, and it's obvious what's going on.
Downsides: More configuration. OAuth is complex enough without people trying to understand Origins vs Redirect URIs.

I agree and I believe redirect_uris will be the allowed_origins in most cases...

FWIW Microsoft & OneLogin don't have Origin settings for OAuth (AFAICT), they just use the redirect URIs. Okta has explicit origins.

but as a maintainer of a shared library, I'm a little worried about making the assumption for people.

If you have a method you can delegate to/override (ie: via a validator or Client.is_allowed_origin()) then it's always possible to do trickier things?

Anyway, I don't feel strongly about it... I'm happy that support is landing 😄

[dopry]

CORS on /token is fixed by #1229... That leaves /userinfo ....