Skip to content
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

Extract oauth error information and fail early if it exists #3266

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

obydog002
Copy link
Contributor

Fix #3257

After deleting the CSRF cookie:

error

After explicitly denying the oauth authorization:

error2

Logging in normally still works as expected

@Brutus5000
Copy link
Member

This is nice. Why not go one step further and map some technical known errors into actual valuable user errors.

Ideas:
scope_denied => Seems like you did not accept the scopes on the login page.
csrf token expired (i'm not sure how to extract this) => You probably took too long to perform the login please try again.

@obydog002
Copy link
Contributor Author

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

@Brutus5000
Copy link
Member

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.

@obydog002
Copy link
Contributor Author

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

@obydog002
Copy link
Contributor Author

Wasnt sure how I wanted to go about this

Scopes:
1

No CSRF:
2

@Sheikah45
Copy link
Member

Only thing I would probably say is just if we can de URL encode the description when we print it

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 58.45%. Comparing base (5a15ded) to head (304e9a0).
Report is 12 commits behind head on develop.

Files with missing lines Patch % Lines
...om/faforever/client/login/OAuthValuesReceiver.java 80.00% 1 Missing and 1 partial ⚠️
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     
Files with missing lines Coverage Δ
...va/com/faforever/client/login/LoginController.java 88.13% <100.00%> (+0.71%) ⬆️
...om/faforever/client/login/OAuthValuesReceiver.java 81.08% <80.00%> (-3.30%) ⬇️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a53f5a9...304e9a0. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Catch OAuth login failures before extracting code and state
3 participants