Skip to content

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

Merged
JuancaG05 merged 6 commits into
masterfrom
feature/login_hint_and_user_brandables
Jan 24, 2024
Merged

[FEATURE REQUEST] [OIDC] Branding option to remove login_hint and user parameter from re-login URL#4291
JuancaG05 merged 6 commits into
masterfrom
feature/login_hint_and_user_brandables

Conversation

@JuancaG05

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

Copy link
Copy Markdown
Contributor

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

@Aitorbp Aitorbp left a comment

Copy link
Copy Markdown
Contributor

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.

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

@Aitorbp Aitorbp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 🥇

@jesmrec

jesmrec commented Jan 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

jesmrec commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

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

jesmrec commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

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

jesmrec commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

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

jesmrec commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Let's move this forward

@JuancaG05 JuancaG05 merged commit c6b2fed into master Jan 24, 2024
@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

1 more reviewer

@Aitorbp Aitorbp Aitorbp approved these changes

Reviewers whose approvals may not affect merge requirements

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