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

feat(oauth): Allow to skip grant step for selected applications #49670

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Dec 5, 2024

Summary

Before After
request from application Copie d'écran_20241205_145140 request from application Copie d'écran_20241205_145140
Copie d'écran_20241209_145559 skipped
If not logged in, login screen Copie d'écran_20241209_145625 If not logged in, login screen Copie d'écran_20241209_145625
Copie d'écran_20241209_150703 skipped Copie d'écran_20241209_150703

To get the skipped version, I ran: occ config:app:set oauth2 skipAuthPickerApplications --type array --value '["me"]', with me being the name associated to my client in oc_oauth2_clients table.

TODO

  • Is client name the right thing to match against? (Especially, is that safe?) It should be good, clients (applications) are added and controlled by the admin.
  • Is it okay to store the list of bypassing apps in appconfig?
  • I suppose ClientFlowLoginV2Controller should be adapted as well? Why are there 2 of those? Not needed, oauth2 uses v1 only
  • Why is there a step before login screen? It is now skipped as well for configured applications.

Checklist

@come-nc come-nc added the 2. developing Work in progress label Dec 5, 2024
@come-nc come-nc added this to the Nextcloud 31 milestone Dec 5, 2024
@come-nc come-nc self-assigned this Dec 5, 2024
@come-nc come-nc requested a review from nickvergessen December 5, 2024 16:33
@come-nc come-nc changed the title feat(oauth): Allow bypass of grant step for selected applications feat(oauth): Allow to skip grant step for selected applications Dec 5, 2024
@come-nc come-nc force-pushed the feat/allow-oauth-grant-bypass branch from 05a5de3 to af256b9 Compare December 9, 2024 15:55
@come-nc come-nc force-pushed the feat/allow-oauth-grant-bypass branch from 040c8b3 to cdd4dec Compare December 19, 2024 09:33
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 19, 2024
@come-nc come-nc marked this pull request as ready for review December 19, 2024 14:42
@come-nc
Copy link
Contributor Author

come-nc commented Dec 19, 2024

I added back grant step because I am not comfortable enough with oauth2 to understand if it can be skipped.
So for now only adding an option to skip the first screen, for applications configured only. This has minimal impact and is an improvement already.

@come-nc come-nc requested review from a team, ArtificialOwl, nfebe, yemkareems and nickvergessen and removed request for a team December 19, 2024 14:43
@come-nc come-nc force-pushed the feat/allow-oauth-grant-bypass branch from 6817f55 to 75f8bb5 Compare January 7, 2025 09:34
@come-nc come-nc mentioned this pull request Jan 7, 2025
@@ -8,6 +8,7 @@
*/
namespace OCA\OAuth2\Controller;

use OC\Core\Controller\ClientFlowLoginController;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

@blizzz blizzz merged commit 1b0cb4a into master Jan 7, 2025
188 checks passed
@blizzz blizzz deleted the feat/allow-oauth-grant-bypass branch January 7, 2025 15:46
@julien-nc
Copy link
Member

I'm late to the party.

@come-nc

I added back grant step because I am not comfortable enough with oauth2 to understand if it can be skipped.

As long as this can only be enabled with a hidden config, we can also skip the grant step IMO, even if this does not fully respects the oauth specs. I mean, if the goal is to have a transparent oauth flow from a user perspective, the grant step can be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

Add an option to set oAuth2 applications as trusted, for own external apps which uses oAuth2 sessions
5 participants