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.

Sign up for GitHub

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

Jump to bottom

[FEATURE REQUEST] [OIDC] Branding option to remove login_hint and user parameter from re-login URL #4291

Merged
merged 6 commits into from
Jan 24, 2024
Merged

[FEATURE REQUEST] [OIDC] Branding option to remove login_hint and user parameter from re-login URL #4291

merged 6 commits into from
Jan 24, 2024

Conversation

JuancaG05
Copy link
Collaborator

@JuancaG05 JuancaG05 commented Jan 22, 2024 •
edited by jesmrec
Loading

Related Issues

App: #4288

  • Added changelog files for the fixed issues in folder changelog/unreleased. More info here

QA

#4291 (comment)

@JuancaG05 JuancaG05 self-assigned this Jan 22, 2024
@JuancaG05 JuancaG05 linked an issue Jan 22, 2024 that may be closed by this pull request
[FEATURE REQUEST] [OIDC] Branding option to remove login_hint and user parameter from re-login URL #4288
Closed
10 tasks
@JuancaG05 JuancaG05 force-pushed the feature/login_hint_and_user_brandables branch from b2c6cf6 to 719190a Compare January 22, 2024 17:21
@JuancaG05
New brandable parameter to indicate whether sending or not `login_hin…
c5e8b5a 
…t` and `user` parameters in login request URL
@JuancaG05 JuancaG05 force-pushed the feature/login_hint_and_user_brandables branch from 719190a to c5e8b5a Compare January 22, 2024 17:23
@JuancaG05 JuancaG05 requested a review from Aitorbp January 23, 2024 09:21
@JuancaG05 JuancaG05 marked this pull request as ready for review January 23, 2024 09:21
Aitorbp
Aitorbp suggested changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@Aitorbp Aitorbp left a comment

Choose a reason for hiding this comment

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

Look good for me 👍 just one doubt in comments and Bitrise is returning an error. I think you have to review the unit tests.

owncloudApp/src/main/java/com/owncloud/android/presentation/authentication/oauth/OAuthUtils.kt Show resolved Hide resolved
@JuancaG05 JuancaG05 requested a review from Aitorbp January 23, 2024 13:58
Aitorbp
Aitorbp approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@Aitorbp Aitorbp left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

@jesmrec
Copy link
Collaborator

jesmrec commented Jan 24, 2024 •
edited
Loading

QA Checks over authorize request in login and re-login processes

Branding parameter

send_login_hint_and_user = true

  • OAuth2 login -> no user, no login_hint
  • OAuth2 re-login -> user and login hint
  • OIDC login -> no user, no login_hint
  • OIDC re-login -> user and login hint

send_login_hint_and_user = false

  • OAuth2 login -> no user, no login_hint
  • OAuth2 re-login -> no user, no login_hint
  • OIDC login -> no user, no login_hint
  • OIDC re-login -> no user, no login_hint

MDM parameter

send_login_hint_and_user = true

  • OAuth2 login -> no user, no login_hint
  • OAuth2 re-login -> user and login hint
  • OIDC login -> no user, no login_hint
  • OIDC re-login -> user and login hint

send_login_hint_and_user = false

  • OAuth2 login -> no user, no login_hint
  • OAuth2 re-login -> no user, no login_hint
  • OIDC login -> no user, no login_hint
  • OIDC re-login -> no user, no login_hint

@jesmrec
Copy link
Collaborator

jesmrec commented Jan 24, 2024

One consideration:

I noticed this is affecting the legacy webfinger flow somehow: if send_login_hint_and_user is true, the username is propagated from the login view to the idP. If if send_login_hint_and_user is false, it is not. Since the feature and the new parameter should only affect the re-login process as defined, this might need a look.

@jesmrec
Copy link
Collaborator

jesmrec commented Jan 24, 2024

I also noticed that scope and prompt are sent empty to OAuth2, because they are OIDC parameters. This is not in the scope of this PR, i will check and open issue in order to discuss whether that behaviour could be improved.

@jesmrec
Copy link
Collaborator

jesmrec commented Jan 24, 2024

I noticed this is affecting the legacy webfinger flow somehow: if send_login_hint_and_user is true, the username is propagated from the login view to the idP. If if send_login_hint_and_user is false, it is not. Since the feature and the new parameter should only affect the re-login process as defined, this might need a look.

this is the expected behaviour since the login and relogin calls are the same. We can not prevent the side-effect is attached to the branding decision regarding the send_login_hint_and_user parameter.

@jesmrec
Copy link
Collaborator

jesmrec commented Jan 24, 2024

Let's move this forward

@JuancaG05 JuancaG05 merged commit c6b2fed into master Jan 24, 2024
3 checks passed
@JuancaG05 JuancaG05 deleted the feature/login_hint_and_user_brandables branch January 24, 2024 14:14
Aitorbp pushed a commit that referenced this pull request Feb 5, 2024
@JuancaG05 @Aitorbp
Merge pull request #4291 from owncloud/feature/login_hint_and_user_br…
e2bef92 
…andables

[FEATURE REQUEST] [OIDC] Branding option to remove `login_hint` and `user` parameter from re-login URL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aitorbp Aitorbp Aitorbp approved these changes

Assignees

@JuancaG05 JuancaG05

Labels
Approved by qa
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] [OIDC] Branding option to remove login_hint and user parameter from re-login URL
3 participants
@JuancaG05 @jesmrec @Aitorbp