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

AccessPolicy checks hide useful errors #275

Closed
chaen opened this issue Jul 12, 2024 · 2 comments · Fixed by #285
Closed

AccessPolicy checks hide useful errors #275

chaen opened this issue Jul 12, 2024 · 2 comments · Fixed by #285
Assignees
Labels
bug Something isn't working

Comments

@chaen
Copy link
Contributor

chaen commented Jul 12, 2024

If you are calling a method with wrong arguments, you are supposed to see an error 402 with what is wrong.
Instead, you get nothing because the server crashes with the infamous "THIS SHOULD NOT HAPPEN, ALWAYS VERIFY PERMISSION",

@chaen chaen added the bug Something isn't working label Jul 12, 2024
@chaen chaen self-assigned this Jul 12, 2024
@chaen
Copy link
Contributor Author

chaen commented Jul 12, 2024

What I wanted to do to fix this was to check in the check_permission if a response had already been returned, or if it was the infamous 402 or something like that. Problem is, it's impossible as of now: fastapi/fastapi#3500

Then, looking a bit more at the dependency doc (https://fastapi.tiangolo.com/tutorial/dependencies/dependencies-with-yield/#a-database-dependency-with-yield) it states

The code following the yield statement is executed after the response has been delivered:

The problem is that check_permissions is not async, so this statement is not true (verified here though fastapi/fastapi#5068).
One thing to try is to make check_permissions async, so the client will get the response and then we will crash.
Problem is: it would still crash ! And you can expect to have robots (or users...) making malformed request. A crash in that case is obviously unacceptable.

The other option I see is to disable the crash altogether, except when running in the CI, but that isn't so nice, as the idea was that the check_permissions protects you in prod. But well...

@chaen
Copy link
Contributor Author

chaen commented Aug 23, 2024

Conclusion after discussion:
Create a Setting objects for all the tests options, and indeed only run that specific check in the CI

chaen added a commit to chaen/diracx that referenced this issue Aug 28, 2024
chrisburr pushed a commit to chaen/diracx that referenced this issue Aug 28, 2024
chaen added a commit to chaen/diracx that referenced this issue Aug 29, 2024
chaen added a commit to chaen/diracx that referenced this issue Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant