fix: show OAuth2 error details on error screen #277
Conversation
jonas-jonas
requested review from
Benehiko and
aeneasr
and removed request for
Benehiko
src/routes/error.ts
Outdated
| } | ||
|
|
||
| function isOAuth2Error(query: qs.ParsedQs): query is OAuth2Error { | ||
| return query.error !== undefined && query.error_description !== undefined |
There was a problem hiding this comment.
Can there be other parameters, as well? Is there a case where one of these wouldn't be present?
There was a problem hiding this comment.
unsure, might need to refer to hydra docs
There was a problem hiding this comment.
I don't know if we always have error_description
There was a problem hiding this comment.
https://github.com/ory/fosite/blob/master/errors.go#L460
error_hint is super useful
There was a problem hiding this comment.
Nice, I removed the error_description requirement and added an option error_hint. We'll need to observe if anything else should be in there.
src/routes/error.ts
Outdated
| } | ||
|
|
||
| function isOAuth2Error(query: qs.ParsedQs): query is OAuth2Error { | ||
| return query.error !== undefined && query.error_description !== undefined |
There was a problem hiding this comment.
unsure, might need to refer to hydra docs
src/routes/error.ts
Outdated
| }), | ||
| }) | ||
| } catch (err) { | ||
| // The error could not be found or there was an error fetching it, redirect back to home. | ||
| res.redirect("welcome") |
There was a problem hiding this comment.
what happens if the welcome page is disabled? Could we not have a redirect back to req.header("Referer") instead here?
There was a problem hiding this comment.
Wouldn't that be the flow that the error occurred in? Might lead to infinite redirects, right?
But I agree that we need more sophisticated handling here. But now sure where to...
There was a problem hiding this comment.
hmm, true. Maybe to the project error page? I wonder if we could get these URLs from the backoffice somehow.
There was a problem hiding this comment.
Maybe to the project error page
Isn't this the project error page?
I wonder if we could get these URLs from the backoffice somehow.
Also a possibility.
There was a problem hiding this comment.
Show an error that describes what happened and what the user can do
There was a problem hiding this comment.
Show an error that describes what happened and what the user can do
Lol, yeah, sometimes the best solution is the most intuitive. Didn't even think of that. I added some handling around that.
|
@Benehiko would be great to increase the width of the error box, as the JSON can get quite long. Would that be easy to do within elements? |
Yes, can be done in elements |
|
Can also override the styling for this in hostedui if you want. I think you should be able to |
| ...(query.error_hint | ||
| ? { hint: decodeURIComponent(query.error_hint.toString()) } | ||
| : {}), | ||
| code: 599, // Dummy code to trigger the full error screen |
There was a problem hiding this comment.
why do we need a dummy code? why not just make it 400?
There was a problem hiding this comment.
to trigger the full error screen
Otherwise, we only show the message, bot the id, etc.
https://github.com/ory/elements/blob/main/src/react-components/ory/user-error-card.tsx#L37
Also, this is most likely a 5XX error, as there is likely a misconfiguration.
There was a problem hiding this comment.
ah i see, yeah we will then need to adjust ory/elements and move the logic at some point to the hosted ui rather.
elements should just render what it is given.
Related to ory/network#325
Related Issue or Design Document
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
Further comments