DIRACGrid / diracx

The neXt DIRAC generation
GNU General Public License v3.0
8 stars 19 forks source link

AccessPolicy checks hide useful errors #275

Closed chaen closed 4 days ago

chaen commented 1 month ago

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 commented 1 month ago

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: https://github.com/tiangolo/fastapi/issues/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 https://github.com/tiangolo/fastapi/issues/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 commented 1 week ago

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