-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Include AuthenticationCompletionException messages in HTTP response in devmode #42917
Include AuthenticationCompletionException messages in HTTP response in devmode #42917
Conversation
50e553a
to
74ee216
Compare
Status for workflow
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
first of all, I think returning error message in DEV mode is right step and it will help debugging. As far as AuthenticationCompletionException
changes are concerned LGTM.
As you are changing logging level to error and some error messages are verbose, I'd like to check all the logged messages are actionable (provides information on which normally admin will act, but does not describe regular situation). I'll comment on individual changes.
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
Show resolved
Hide resolved
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
Show resolved
Hide resolved
I'll ask for more reviews tomorrow, thanks @michalvavrik |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a great improvement, thanks!
looks good - would be great if you showed a before and after example :) p.s. I don't have a easy setup atm to try it out atm but let me know if need to and i can try redo it. |
Hi @maxandersen, before, in your particular setup, I believe it was an empty screen, due to
But this error code and description were just lost for the purpose of the logs and all you got was an empty screen. And in addition, as you proposed, we'll show the info in the browser in devmode, so it will be much clearer what to do next. As far as error code and descriptions are concerned, they are standardized, so, in case of errors, users would see something from: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-10#section-4.1.2.1 This is a newer OAuth 2.1 spec, but AFAIK, the same codes would be available in the core OAuth 2.0. |
There is also an optional |
Let me merge it now |
Fixes #41890.
It really fixes a bug rather than a planned enhancement.
Sometimes, when the user is redirected to the OIDC provider and something goes wrong at the OIDC provider's side, an error code and optional error description is returned.
Awhile back, @FroMage asked about an option to customize such error responses so now users can register error handlers and refer to them with
quarkus.oidc.authentication.error-path
property.But if they haven't registered it, they will get a rather obscure warning and 401 in the browser without any further clues.
@maxandersen was concerned about it the other day, but also @Sanne and indeed @FroMage have also been concerned.
So first of all, this PR improves a log message reported when the user is redirected back to Quarkus with the error code.
And it goes over all/most of
AuthenticationCompletionException
cases, initializesAuthenticationCompletionException
with the error message, and allows this error message be reported back to the user, but in devmode only, to avoid accidentally leaking too many details.