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

AuthPicker: redirect oauth client to grant page #17136

Closed
wants to merge 3 commits into from

Conversation

kinolaev
Copy link
Member

@kinolaev kinolaev commented Sep 13, 2019

Hello!

This PR skips auth picker page for oauth clients.

For now nextcloud redirects user from /apps/oauth2/authorize to /login/flow where user sees only login button that redirects user to /login/flow/grant where user allows access to account. In other cases /login/flow page is used to suggest authorization by application password but this is hidden for oauth clients, so this page is actually useless. With this PR ClientFlowLogin#showAuthPickerPage returns redirect to /login/flow/grant instead of auth picker page and oauthState removed from auth picker template because it will always be empty.

@kinolaev kinolaev force-pushed the oauth2-skip-authpicker branch from e6c258c to 4d3dbac Compare September 13, 2019 16:05
@rullzer rullzer self-requested a review September 13, 2019 22:54
@rullzer rullzer self-assigned this Sep 13, 2019
@antonkarliner
Copy link

Great idea! Completely agree.

@skjnldsv skjnldsv added 3. to review Waiting for reviews enhancement labels Sep 28, 2019
@skjnldsv skjnldsv added this to the Nextcloud 18 milestone Sep 28, 2019
@kinolaev kinolaev force-pushed the oauth2-skip-authpicker branch from 4d3dbac to 127a1e6 Compare October 7, 2019 10:46
@kinolaev
Copy link
Member Author

kinolaev commented Oct 7, 2019

Conflict resolved.
@rullzer and @skjnldsv, have you any idea why authpicker page must be shown to user?

@rullzer
Copy link
Member

rullzer commented Oct 7, 2019

@kinolaev so it is a leftover from an older time ;)
The reason we show it is legacy. We had/have no way to show the message on the login page. And I like that the users know why they have to login before just entering their username and password ;)

@kinolaev
Copy link
Member Author

kinolaev commented Oct 7, 2019

Sorry, @rullzer, I didn't understand your position. Do you still like that the users see that message or do you want to simplify oauth login flow now?
Need I move this message directly to login form?
Personally, I did not even notice message on authpicker page) So, I think it can be safely removed.

@rullzer
Copy link
Member

rullzer commented Oct 14, 2019

@kinolaev so after some thinking I'm not a huge fan of this. I'd rather make sure that we fix it for all properly. (so also for the normal flow). Let me discuss a bit with @ChristophWurst as well as he moved the login page over to a webpacked thing so maybe we can do something more fancy these days.

@jancborchardt
Copy link
Member

Just for reference, design-wise I totally agree with @kinolaev that this needs to be simplified.

  • The first page duplicates the 3rd page. If we need some sort of notice, we could put it above the log in fields, if at all.
  • 3rd page only needs to be shown once indeed, as it does for other "Sign in with" systems.

Since this is sign in and also connecting to mobile and desktop clients – thus happening a lot, I’d say we should look into this for 19?

@rullzer
Copy link
Member

rullzer commented Dec 10, 2019

Yeah and witht he improved login page to vue we might now finally be able to tackle it properly.

This was referenced Apr 4, 2020
This was referenced Apr 15, 2020
@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
@rullzer rullzer removed this from the Nextcloud 19 milestone Apr 24, 2020
@kinolaev kinolaev force-pushed the oauth2-skip-authpicker branch 2 times, most recently from b47156a to 46adb7d Compare September 14, 2020 11:15
@kinolaev kinolaev force-pushed the oauth2-skip-authpicker branch 3 times, most recently from e268958 to 203f744 Compare September 19, 2020 21:15
@kinolaev kinolaev force-pushed the oauth2-skip-authpicker branch 3 times, most recently from e293ad9 to feee428 Compare September 19, 2020 22:04
Signed-off-by: Sergej Nikolaev <kinolaev@gmail.com>
Signed-off-by: Sergey Nikolaev <kinolaev@gmail.com>
Signed-off-by: Sergey Nikolaev <kinolaev@gmail.com>
@kinolaev kinolaev force-pushed the oauth2-skip-authpicker branch from feee428 to 1c12f6d Compare September 20, 2020 07:27
@kinolaev
Copy link
Member Author

kinolaev commented Sep 20, 2020

Hello @rullzer,
I also moved login by app password to login page and completely removed authPickerPage (v1 and v2)!
Now LoginController checks redirect_url and if it is grant page url (v1 or v2) then it adds client name (oauth client name for oauth flow, browser user agent for v1 flow, app user-agent for v2 flow) and instance name to initial state, so Login.vue can show “login to grant” message. Login.vue also checks redirect_url and shows link to AppTokenLoginForm if url is login/flow and has no clientIdentifier.
What do you think about it?

@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this ticket.
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to re-open and solve the various conflicts.

Thanks for the interest in Nextcloud and the effort put into this! 🙇

@skjnldsv skjnldsv closed this Feb 27, 2024
@skjnldsv skjnldsv removed the 3. to review Waiting for reviews label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants