Skip to content

🐛 Ensure that HTTPDigest only raises an exception when auto_error is True #2939

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 27, 2025
Merged

🐛 Ensure that HTTPDigest only raises an exception when auto_error is True #2939

merged 5 commits into from
Feb 27, 2025

Conversation

arthurio
Copy link
Contributor

@arthurio arthurio commented Mar 12, 2021 •
edited
Loading

I believe HTTPDigest should follow the same pattern as other authentication mechanisms and raise an exception only if auto_error is True.

@codecov
Copy link

codecov bot commented Mar 12, 2021 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files 
@@            Coverage Diff            @@
##            master     #2939   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       540           
  Lines        13969     13948   -21     
=========================================
- Hits         13969     13948   -21

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

@github-actions
Copy link
Contributor

📝 Docs preview for commit 10bce4a at: https://604bacb90b626e35b36d5ef0--fastapi.netlify.app

juntatalor
juntatalor reviewed Mar 13, 2021
View reviewed changes
@arthurio
Copy link
Contributor Author

arthurio commented Jul 7, 2021

@tiangolo Is there anything I can do to get this PR moving forward?

@arthurio
Copy link
Contributor Author

@juntatalor @tiangolo Gentle nudge 👉🏻

@arthurio
Copy link
Contributor Author

I just rebased my branch with master.

cikay
cikay reviewed Jan 8, 2022
View reviewed changes
@arthurio arthurio force-pushed the digest-except branch 2 times, most recently from 66abd38 to 29abff8 Compare January 8, 2022 19:41
@arthurio
Copy link
Contributor Author

@juntatalor @tiangolo @cikay @yezz123 This is ready to go, not sure what's the hold up... No offense but I'd like to not have to rebase it for another year 🙃

@yezz123
Copy link
Contributor

yezz123 commented Mar 11, 2022

Hello, @arthurio I help just the opensource team if I see a PR that's really helpful to the FastAPI project I Review it, Mostly the Only Person that Maintain fastAPI is @tiangolo so wait until he will merge it

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

📝 Docs preview for commit 6328d1a at: https://624db921ff9a9a42fdb83026--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2022

📝 Docs preview for commit 582dafe at: https://62705ddda48eeb2f877c526e--fastapi.netlify.app

@arthurio
Copy link
Contributor Author

arthurio commented May 2, 2022

@tiangolo I know you are a busy person but it's been more than a year since I have opened this tiny PR... Could you at least respond to let me know if I should keep it up to date or not? Or give me pointers as to what to do to get it merged?
Again, I'm very aware that maintaining a project is time consuming and I'm not blaming you for anything, just trying to get things going ¯_(ツ)_/¯

@github-actions
Copy link
Contributor

📝 Docs preview for commit 69926bb at: https://628378da0fe4b865e5e5cc9c--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

📝 Docs preview for commit 4e201d4 at: https://62c60f000d2b5d0075c87a57--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 1d577cd at: https://630ec91c40bedd3307cf73e9--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 2262c94 at: https://634ddec3af32704c95e90354--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2022

📝 Docs preview for commit b3e2a5d at: https://636135f88e0a60005f6cef94--fastapi.netlify.app

@tiangolo tiangolo added bug Something isn't working p2 and removed investigate labels Jan 15, 2024
YuriiMotov
YuriiMotov approved these changes Mar 2, 2024
View reviewed changes
Copy link
Contributor

@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.

Problem is still relevant to FastAPI 0.110.0.
This PR fixes it

@arthurio arthurio force-pushed the digest-except branch 2 times, most recently from f8a70de to 1da302a Compare November 25, 2024 20:19
@svlandeg svlandeg changed the title Digest should raise exception only if auto_error is True 🐛 Ensure that HTTPDigest only raises an exception when auto_error is True Feb 18, 2025
@svlandeg
Copy link
Member

@arthurio: apologies for the super long delay in responding to this, and thanks for your patience and for keeping this PR up-to-date 🙏

I've moved it to my queue to review it in the next few days/weeks.

@svlandeg svlandeg self-assigned this Feb 18, 2025
svlandeg
svlandeg approved these changes Feb 26, 2025
View reviewed changes
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

This bug fix looks correct to me. It's a little puzzling why the test suite was specifically testing for a 403 in this case, as this does not seem to be the expected/documented behaviour, so I agree with changing the test as this PR does.

I'll run this by Tiangolo for a final review. Thanks again for your patience! 🙏

@svlandeg svlandeg removed their assignment Feb 26, 2025
tiangolo
tiangolo approved these changes Feb 27, 2025
View reviewed changes
Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Great, thank you @arthurio! 🚀 🍰

And thanks a lot for the patience. 😅

Thanks @svlandeg and everyone else for the help here. ☕

This will be available in the next release, 0.115.9, in the next few hours. 🤓

@tiangolo tiangolo merged commit ccc7c8f into fastapi:master Feb 27, 2025
28 checks passed
@arthurio
Copy link
Contributor Author

@tiangolo You are very welcome, thank you for everything you do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiangolo tiangolo tiangolo approved these changes

@svlandeg svlandeg svlandeg approved these changes

@YuriiMotov YuriiMotov YuriiMotov approved these changes

+4 more reviewers

@juntatalor juntatalor juntatalor left review comments

@cikay cikay cikay left review comments

@BilalAlpaslan BilalAlpaslan BilalAlpaslan approved these changes

@yezz123 yezz123 yezz123 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Something isn't working p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@arthurio @yezz123 @svlandeg @tiangolo @juntatalor @cikay @BilalAlpaslan @YuriiMotov