fix(server): respond with forbidden if failed to authenticate#4200
Conversation
aljoshare
force-pushed
the
fix/unauthorized-if-failed-to-authenticate
branch
from
3c6a1c8 to
eafe47e
Compare
|
Could this path affect any of the RFC response paths? Is there an expectation of a 500 somewhere? |
|
Reasonable question, but I think there is no RFC because the callback endpoint is part of the relying party, and the RP can act as it wishes. My question is: can we distinguish these two errors from other possible errors?
|
|
Sorry, I was on vacation. I'll check ✌️ |
aljoshare
force-pushed
the
fix/unauthorized-if-failed-to-authenticate
branch
2 times, most recently
from
b7f671f to
61abe5e
Compare
|
@aljoshare looked through the changes, and the PR seems good. Feel free to undraft it. |
aljoshare
force-pushed
the
fix/unauthorized-if-failed-to-authenticate
branch
from
61abe5e to
c4fb0a3
Compare
aljoshare
changed the title
Thanks for the quick response! I updated the PR again and changed it to 403, because I think that 403 is more accurate. The user is authenticated but not authorized.
What do you think? |
…red groups Signed-off-by: Aljoscha Bollmann <aljoscha.bollmann@proton.me>
…e required groups Signed-off-by: Aljoscha Bollmann <aljoscha.bollmann@proton.me>
… not in any of the required groups Signed-off-by: Aljoscha Bollmann <aljoscha.bollmann@proton.me>
aljoshare
force-pushed
the
fix/unauthorized-if-failed-to-authenticate
branch
from
c4fb0a3 to
1ec99e6
Compare
|
Yeah that's even better, thanks. |
3 participants
Overview
It changes the response from 500 - Internal Server Error to 403 - Forbidden if the Dex is not able to authenticate a user, e.g. because the user is not in any of the required groups.
What this PR does / why we need it
This PR is necessary because 403 - Forbidden is imho a better response than 500 - Internal Server Error in this case. For example, this part of the code gets executed if a user is not in any of the required groups or if a refresh token is not valid, which is in my opinion a case where the user expects a 403 instead of a 500.
Special notes for your reviewer
I added a new generic custom error, that indicates that the user is authenticated but not in one of the required groups. It can be reused by other connectors if needed.