Extract oauth error information and fail early if it exists #3266
Conversation
|
This is nice. Why not go one step further and map some technical known errors into actual valuable user errors. Ideas: |
|
Do we want to display that in the error details view? It may be nice also to have some of that information populate instead of the generic "If this error continues..." message. That would be nicer for users as I imagine not everyone reads the stack trace, though I dont know much work it would be to do this |
|
It's a question of confidence. Are we 100% sure that our extracted errors are really the error cause? I am not sure if there is ambiguity and we might still need the original stacktrace. |
|
How about we leave the stack trace for exactly whatever the error is with as much detail as possible, but we allow the generic message to change based on the cases you describe. For a start anyway We can also always leave in a general "If this does not resolve..." paragraph at the end for the specific cases |
…nto bugfix/#3257_improve_oauth_failures
…nto bugfix/#3257_improve_oauth_failures
|
Wasnt sure how I wanted to go about this Scopes: No CSRF: |
|
Only thing I would probably say is just if we can de URL encode the description when we print it |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3266 +/- ##
=============================================
+ Coverage 58.40% 58.45% +0.04%
- Complexity 3900 3913 +13
=============================================
Files 574 574
Lines 19158 19163 +5
Branches 1020 1025 +5
=============================================
+ Hits 11190 11202 +12
+ Misses 7458 7455 -3
+ Partials 510 506 -4
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -147,5 +151,19 @@ private String extractValue(String request, Pattern pattern) { | |||
| return matcher.group(1); | |||
| } | |||
|
|
|||
| private void checkForError(String request) { | |||
| String decodedRequest = URLDecoder.decode(request, StandardCharsets.UTF_8); | |||
There was a problem hiding this comment.
Instead of decoding twice I was thinking we can decode the request string once and change the regex patterns, thoughts @Sheikah45 ?
There was a problem hiding this comment.
Yes I think that would be best
|
@Sheikah45 this is ready for review. I decided against changing the regex as matching on the un-escaped request is easier. Instead we can just format the request string when we throw. As it is also easier to read the error description if its present I decided not to print it separately I guess we need to confirm the exact wording of the i18n keys and translate? |
| private String getLoginErrorSpecificCause(Throwable throwable) { | ||
| String cause = throwable.getMessage(); | ||
| if (ERROR_SCOPE_DENIED.matcher(cause).find()) { | ||
| return "login.scopeDenied"; | ||
| } | ||
| if (ERROR_NO_CSRF.matcher(cause).find()) { | ||
| return "login.noCSRF"; | ||
| } | ||
| return "login.failed"; | ||
| } | ||
|
|
There was a problem hiding this comment.
The only thing I might say is instead of matching based off the string of the message if instead we just throw a specific type of exception for scope denied and no csrf. Then we just just use pattern matchiing
There was a problem hiding this comment.
Can you expand a bit? Where should these be throwing from? and then Ideally we catch them here and put the keys in that way?
I experimented a bit with exceptions in the OAuthReceivers file
There was a problem hiding this comment.
This is already in a catch statement so these exception messages are being created in the oauthValuesRetriever so instead of crafting a custom exception message we should instead create an instance of a custom exception type.
So more specifically it should be done in the checkForError function.
That is better because there it is easier to see how the error string has a limited set of options that can be matched against.
While here we are just checking a generic string which is more fragile and error prone.
Fix #3257
After deleting the CSRF cookie:
After explicitly denying the oauth authorization:
Logging in normally still works as expected