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

Feature request: OAuth redirect should gracefully handle user explicit cancellation #4177

Closed
pnmcosta opened this issue Jan 18, 2024 · 7 comments

Comments

@pnmcosta
Copy link
Contributor

Hi,

I'm not sure how this works with other providers, but with Facebook, if the user presses Cancel instead of continuing:

image

The oauth2-redirect throws this error:

image

Which causes some confusion to users, they're expecting to be able to reauth and try again.

The params passed by Facebook to the redirect do include error=access_denied&error_code=200&error_description=Permissions+error&error_reason=user_denied

Would it be reasonable for the redirect handler:

func (api *recordAuthApi) oauth2SubscriptionRedirect(c echo.Context) error {

to check for error_reason=user_denied and emit a close event of the modal via the realtime subscription?

Happy to research more and observe how other providers do user cancellation to have a generic solution for the handler.

Let me know your thoughts?

Thank you,

@ganigeorgiev
Copy link
Member

I don't think error_reason is part of the OAuth2 spec and will differ for the different provider APIs.

I'm also not sure that I understand what is the desired behavior here. Could you elaborate a little more with your expected flow?

@ganigeorgiev
Copy link
Member

Will have to be researched better but based on OAuth2 possible errors it seems that in case of an error there should be error and optionally error_description and error_uri.

@pnmcosta
Copy link
Contributor Author

pnmcosta commented Jan 18, 2024

Hi,

Thanks for looking into this in such short notice

I'm also not sure that I understand what is the desired behavior here. Could you elaborate a little more with your expected flow?

It's mainly an UX thing, currently the user is shown that error message on the popup, and they get confused (YKWIM!). Me an you know that we can close that popup and press the login button again, in fact even not closing it and just pressing the button would refresh the popup to start over.

Ah you are right, but error=access_denied is part of the spec and as per the link:

The user denies the request
If the user denies the authorization request, the server will redirect the user back to the redirect URL with error=access_denied in the query string, and no code will be present. It is up to the app to decide what to display to the user at this point.

Is exactly what I would be looking for here.

My follow up question is, would closing the modal and focus back onto the main window from the subscription be viable?

Happy to test this if you'd like.

@ganigeorgiev
Copy link
Member

Hm, on second read I think I understand what do you mean.

Yes, it should be doable to use the realtime subscription as long as the error request has also the state query parameter because that is the realtime OAuth2 client id.
I'm not sure at the moment about the format of the event data but I guess we can also pass the error field and check it from within the client-side and trigger the cleanup procedures (both JS and Dart SDKs will have to be updated).

I think it maybe also a good idea to have a generic OAuth2 failure HTML screen and redirect to it instead of the json response (similar to the current success one) because there are some situations where calling window.close may not be allowed.

I'll leave it for tomorrow to think a little more on it.

@pnmcosta
Copy link
Contributor Author

Thank you, it's not urgent at all, but a really good nice to have.

@ganigeorgiev
Copy link
Member

The OAuth2 redirect error handling was updated in the develop branch and the changes will be available with the next v0.21.0 release.

Now instead of returning a json response we are redirecting to a generic error HTML page that will attempt to autoclose the OAuth2 window. If it fails (eg. doesn't satisfy the window.close requirement or is not a native browser window), the user will see a generic text like:

Auth failed.
You can close this window and go back to the app to try again.

The SDKs will be also updated sometime later this weekend to handle the error key in the @oauth2 realtime event and throw an error to reject the Promise/Future of the authWithOAuth2() call.

@pnmcosta
Copy link
Contributor Author

Thank you @ganigeorgiev much appreciated 🙇

If you'd like me to address the SDK's let me know. I'd be honoured to contribute with this, or any other little bit, to take some workload of you, especially these least important features.

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

No branches or pull requests

2 participants