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

refactor(Social Login): Extract common code to abstract class #267

Merged

Conversation

hirokiterashima
Copy link
Member

@hirokiterashima hirokiterashima commented Apr 8, 2024

Changes

  • Extract common code in GoogleOpenIdConnectFilter and MicrosoftOpenIdConnectFilter to AbstractOpenIdConnectFilter
    • Note: there are still some duplicate code in attemptAuthentication() method. The functions behaves slightly differently in Google and Microsoft, and requires a change in application behavior to safely refactor. You can ignore these for now. I'll handle them in a separate task.

Test Prep

  • Test with login-with-microsoft client branch
  • In application-docker-dev.properties, add ?prompt=select_account at the end of microsoft.userAuthorizationUri property. This will make you select the Microsoft account each time you login. It adds an additional click, but it prevents issues when you're signed in as another MS user but want to sign in as a different MS user for WISE.

Test (with Google account and Microsoft account)

Verify that these work as before:

  • Sign in
  • Register user

@hirokiterashima hirokiterashima self-assigned this Apr 8, 2024
@hirokiterashima hirokiterashima marked this pull request as ready for review April 8, 2024 18:19
Copy link
Member

@geoffreykwan geoffreykwan left a comment

Choose a reason for hiding this comment

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

Looks good.

@hirokiterashima hirokiterashima merged commit d346c01 into login-with-microsoft Apr 9, 2024
0 of 2 checks passed
@hirokiterashima hirokiterashima deleted the refactor-open-id-connect-filter branch April 9, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants