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

Fix OIDC response code when authorization header is missing #5332

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

skion
Copy link

@skion skion commented Sep 1, 2022

The OIDC dependency currently returns a 403 response if an Authorization header is missing.

Using a 401 instead of 403 aligns with the HTTP standard when authentication is missing and with the existing OAuth2 dependency.

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (cf73051) to head (b515950).
Report is 1328 commits behind head on master.

❗ Current head b515950 differs from pull request most recent head 586938e. Consider uploading reports for the commit 586938e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            master     #5332    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          540       535     -5     
  Lines        13969     13838   -131     
==========================================
- Hits         13969     13838   -131     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skion skion marked this pull request as ready for review September 1, 2022 09:32
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

📝 Docs preview for commit b515950 at: https://63107c7d01ad3a4be20ed7ce--fastapi.netlify.app

Using a 401 instead of 403 aligns with the HTTP standard when authentication is missing and with the existing OAuth2 dependency.
@github-actions
Copy link
Contributor

📝 Docs preview for commit f98ebda at: https://63832321d901c53c90138745--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 9778525 at: https://6383634bcb239267a6f8ad2f--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 1c44941 at: https://638370a0acf33a7e83a9c71c--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 3195cf8 at: https://639ce73e2c118d080c47f755--fastapi.netlify.app

Copy link

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

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

The issue is still relevant to current version of FastAPI (0.110.0).

It was said in this comment to the similar issue, that it should be fixed, but we have to make sure that new solution follows the specification.

OIDC follows the OAuth2 specification here. And in the current implementation of OAuth2AuthorizationCodeBearer and OAuth2PasswordBearer status_code and headers are exactly the same.

tests/test_security_openid_connect.py Show resolved Hide resolved
@skion
Copy link
Author

skion commented Mar 3, 2024

Thanks for the suggestion @YuriiMotov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants