Fix CORS by passing 'Origin' header to OAuthLib #1229

akanstantsinau · 2022-12-11T10:19:38Z

Fixes #

Description of the Change

Pass DJango HTTP_ORIGIN header to underlying OAuthLib

It is possible to control CORS by overriding is_origin_allowed method of RequestValidator class.
OAuthLib allows origin if:

RequestValidator.is_origin_allowed returns True for particular request
Request connection is secure
Request has Origin header

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated. No documentation. This is the fix for the feature documented in OAuthLib
CHANGELOG.md updated (only for user relevant changes) - change is not user relevant
author name in AUTHORS

n2ygk

Can you explain the use case for this given apps using Django CORS settings have been using this library successfully without it?

akanstantsinau · 2022-12-16T12:13:53Z

Using this PR it is possible to enable CORS for OAuth /o/token endpoint only without requirement to setup/configure Django CORS middleware.

Django CORS settings is an alternative way.

Use Case 1:

Application does not use Django CORS module (as there are no any third-party web clients)
Application needs to setup OIDC provider to let users use another service.
Solution:
Create subclass of class OAuth2Validator
Override method is_origin_allowed, e.g.:

def is_origin_allowed(client_id, origin, request, *args, **kwargs):
    # May be do some verification of client_id, origin etc.
    # We may need to check here if client uses PKCE and allow CORS only if it does.
    return true

Specify newly created class in OAUTH2_PROVIDER.OAUTH2_VALIDATOR_CLASS setting.

codecov · 2023-02-16T21:20:21Z

Codecov Report

Merging #1229 (afdfa45) into master (efdf897) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1229      +/-   ##
==========================================
+ Coverage   97.46%   97.48%   +0.02%     
==========================================
  Files          32       32              
  Lines        2092     2111      +19     
==========================================
+ Hits         2039     2058      +19     
  Misses         53       53

Files	Coverage Δ
oauth2_provider/models.py	`98.64% <100.00%> (+<0.01%)`	⬆️
oauth2_provider/settings.py	`100.00% <ø> (ø)`
oauth2_provider/validators.py	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dopry · 2023-05-20T23:43:28Z

@akanstantsinau Are you unable to meet your CORS needs using the CORS middleware? The docs recommend using cors-headers for this purpose. I'm wary of adding CORS handling. The scope of oauth support is pretty broad and there is already a lot of surface area in oauth_toolkit. I wouldn't want to add things we didn't have to and that weren't explicitly defined in OAuth/OIDC specifications. I'm willing to hear more about your case for adding this, but convenience isn't going to be a convincing argument.

akanstantsinau · 2023-06-11T20:41:14Z

@dopry Thank your for looking into this PR
I can meet the same functionality using CORS middleware with a bit more coding.
The use case is pretty typical:

We have a Django website with some settings in CORS middleware
We want to share user details with another service via OIDC (lets refer hose third party service as https://example.com)
OIDC client on example.com is web-based, so it requires CORS headers to be set on our /o/token endpoint which is provided by Django OAuth Toolkit
We want to allow origin https://example.com only for url /o/token

The solution would be trivial if we do not have existing CORS middelware configs.

CORS_ALLOWED_ORIGINS=["https://example.com"]
CORS_URLS_REGEX = r"^/o/token$"

In our case we already have set CORS_ALLOWED_ORIGINS for some origins for all urls. So to allow https://example.com for /o/token we may implement own handler and attach it to corsheaders.signals.check_request_enabled Django signal.
No issues with it, if you recommend this approach.

The only motivation for this PR was to reuse already existing CORS feature in oauthlib project. It does exactly what we expect - set CORS headers only on /o/token url.
But I see you concerns regarding the scope of support.

dopry

I think this is a good start to improving our CORS support.

The default OauthLib request validator always returns false, https://github.com/oauthlib/oauthlib/blob/d4b6699f8ccb608152b764919e0bd3d38a7b171f/oauthlib/oauth2/rfc6749/request_validator.py#L656

If we're going to enable this upstream with OAuthLib we also need to

implement of is_origin_allowed in oauth2_provider.oauth2_validators.OAuth2Validator.
add a field for the allowed origins on the application model
ensure all views which should implement CORS in DOT are using the validator

oauth2_provider/oauth2_backends.py

dopry · 2023-09-26T19:30:55Z

@akanstantsinau do you have the bandwidth to continue working on this issue?

akanstantsinau · 2023-09-27T17:32:36Z

@dopry Apologize for long delay. Yes I do have bandwidth now to continue work on this.
Your changes requested make sense for me. Will come back soon with updates.

rcoup · 2023-09-27T20:57:09Z

FWIW, we implemeted this, and the approach is working well: It allows us to customise is_origin_allowed() to only be enabled for specific clients for specific origins.

akanstantsinau · 2023-10-01T07:21:38Z

@dopry

implemented is_origin_allowed in oauth2_provider.oauth2_validators.OAuth2Validator.
added allowed_origins field for the allowed origins on the application model
only token endpoint uses is_origin_allowed according to oauthlib implementation. Added tests for grant and refresh token
OIDC public endpoints (oidc-connect-discovery-info, jwks-info) use Access-Control-Allow-Origin:*
OIDC user-info ignores is_origin_allowed

dopry

Awesome progress. This is great work and we're super close. I have a few small asks. Mostly simple renaming and docs, otherwise, from a code review perspective, we're pretty much there. After we can get those changes in I'll setup a test env and put it through it's paces!

oauth2_provider/models.py

oauth2_provider/oauth2_validators.py

tests/test_cors.py

dopry · 2023-10-04T16:37:46Z

@akanstantsinau This looks great. Can you look at the coverage issue in Codecov? It looks like your tests should be covering those cases. Can you look into it and confirm those cases are covered? Also double check the doc changes to make sure there isn't a typo that is throwing sphinx for a loop. I'll try to do a local test setup tomorrow.

akanstantsinau · 2023-10-04T19:05:41Z

@dopry I believe sphinx issue is something not related with changes in this PR:

According to build log , dependency sphinx-rtd-theme is not installed
sphinx_rtd_theme extension is required in docs/conf.py
That causes build failure with No module named 'sphinx_rtd_theme' error

The fix might be adding sphinx-rtd-theme dependency to docs/requirements.txt
New versions of sphinx and readthedocs-sphinx-ext were released in September 2023, so it may make things changed.

I'd prefer not to fix it in this PR, because its a different issue. Also my findings can be not correct.

Working on coverage issue.

rcoup · 2023-10-04T20:17:50Z

ah, don't suppose django-oauth-toolkit is hit by the recent RTD changes? https://blog.readthedocs.com/defaulting-latest-build-tools/ or https://blog.readthedocs.com/newsletter-september-2023/

dopry · 2023-10-05T13:18:25Z

@rcoup looks like you're right. I'll try to get a PR up to fix that today so we can get CI working again.

dopry · 2023-10-05T13:50:05Z

@akanstantsinau I updated the branch with a rebase in the Github UI to see if the RTD fixes are good. Feel free to force push your local over it.

dopry · 2023-10-05T15:26:30Z

@akanstantsinau it looks like everything is passing. I'll do some manual testing later today and hopefully we'll get this in.

akanstantsinau · 2023-10-05T16:05:51Z

@dopry that are awesome news! Thank you!

oauth2_provider/models.py

dopry

I wasn't able to do my local testing due to validation issues.

oauth2_provider/models.py

akanstantsinau · 2023-10-19T14:22:52Z

@dopry Added oauth_settings.ALLOWED_SCHEMES with default set to ["https"] and updated validation for allowed_origins

It is possible to control CORS by overriding is_origin_allowed method of RequestValidator class. OAuthLib allows origin if: - is_origin_allowed returns True for particular request - Request connection is secure - Request has 'Origin' header

for more information, see https://pre-commit.ci

n2ygk reviewed Dec 15, 2022

View reviewed changes

dopry requested changes Jun 16, 2023

View reviewed changes

oauth2_provider/oauth2_backends.py Outdated Show resolved Hide resolved

dopry mentioned this pull request Jun 16, 2023

CORS Implementation #1290

Open

dopry force-pushed the cors-oauthlib branch from 2bd7dba to bdc26e7 Compare September 26, 2023 19:32

akanstantsinau force-pushed the cors-oauthlib branch from 7cc9ab6 to cc4e470 Compare September 29, 2023 19:21

dopry requested changes Oct 2, 2023

View reviewed changes

oauth2_provider/models.py Outdated Show resolved Hide resolved

oauth2_provider/oauth2_validators.py Outdated Show resolved Hide resolved

tests/test_cors.py Outdated Show resolved Hide resolved

tests/test_cors.py Outdated Show resolved Hide resolved

dopry force-pushed the cors-oauthlib branch from 573948c to e51de47 Compare October 5, 2023 13:48

tonial reviewed Oct 6, 2023

View reviewed changes

oauth2_provider/models.py Outdated Show resolved Hide resolved

dopry requested changes Oct 10, 2023

View reviewed changes

oauth2_provider/models.py Outdated Show resolved Hide resolved

akanstantsinau requested a review from dopry October 19, 2023 14:23

akanstantsinau and others added 4 commits October 19, 2023 14:40

Fixed tests for Access-Control-Allow-Origin header returned by oauthlib

6cd4185

Added Allowed Origins application setting

09588a9

[pre-commit.ci] auto fixes from pre-commit.com hooks

86ec8ff

for more information, see https://pre-commit.ci

akanstantsinau added 7 commits October 19, 2023 14:50

Code and docs cleanup

c04ca48

Code cleanup

3e72f8e

Code review: update docs and test names

7092863

Added ALLOWED_SCHEMES setting for Allowed Orgins validation

4015e4d

Code cleanup

2f8789d

Add more tests for origin validators

84c8c4c

fix coverage

f7ae512

dopry force-pushed the cors-oauthlib branch from ffedb86 to f7ae512 Compare October 19, 2023 18:52

[pre-commit.ci] auto fixes from pre-commit.com hooks

afdfa45

for more information, see https://pre-commit.ci

dopry approved these changes Oct 19, 2023

View reviewed changes

dopry merged commit 2c83e6c into jazzband:master Oct 19, 2023
26 checks passed

dopry mentioned this pull request Oct 20, 2023

feat: update test idp to use new cors #1346

Merged

3 tasks

dopry mentioned this pull request Nov 10, 2023

Add CORS Middleware #1150

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CORS by passing 'Origin' header to OAuthLib #1229

Fix CORS by passing 'Origin' header to OAuthLib #1229

akanstantsinau commented Dec 11, 2022 •

edited

Loading

n2ygk left a comment

akanstantsinau commented Dec 16, 2022

codecov bot commented Feb 16, 2023 •

edited

Loading

dopry commented May 20, 2023

akanstantsinau commented Jun 11, 2023

dopry left a comment •

edited

Loading

dopry commented Sep 26, 2023

akanstantsinau commented Sep 27, 2023

rcoup commented Sep 27, 2023

akanstantsinau commented Oct 1, 2023

dopry left a comment

dopry commented Oct 4, 2023

akanstantsinau commented Oct 4, 2023

rcoup commented Oct 4, 2023

dopry commented Oct 5, 2023

dopry commented Oct 5, 2023

dopry commented Oct 5, 2023

akanstantsinau commented Oct 5, 2023

dopry left a comment

akanstantsinau commented Oct 19, 2023

Fix CORS by passing 'Origin' header to OAuthLib #1229

Fix CORS by passing 'Origin' header to OAuthLib #1229

Conversation

akanstantsinau commented Dec 11, 2022 • edited Loading

Description of the Change

Checklist

n2ygk left a comment

Choose a reason for hiding this comment

akanstantsinau commented Dec 16, 2022

codecov bot commented Feb 16, 2023 • edited Loading

Codecov Report

dopry commented May 20, 2023

akanstantsinau commented Jun 11, 2023

dopry left a comment • edited Loading

Choose a reason for hiding this comment

dopry commented Sep 26, 2023

akanstantsinau commented Sep 27, 2023

rcoup commented Sep 27, 2023

akanstantsinau commented Oct 1, 2023

dopry left a comment

Choose a reason for hiding this comment

dopry commented Oct 4, 2023

akanstantsinau commented Oct 4, 2023

rcoup commented Oct 4, 2023

dopry commented Oct 5, 2023

dopry commented Oct 5, 2023

dopry commented Oct 5, 2023

akanstantsinau commented Oct 5, 2023

dopry left a comment

Choose a reason for hiding this comment

akanstantsinau commented Oct 19, 2023

akanstantsinau commented Dec 11, 2022 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading

dopry left a comment •

edited

Loading