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: add federated identity #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abdurrahmanekr
Copy link

@abdurrahmanekr abdurrahmanekr commented Jun 14, 2023

The Google Identity provider is not supported, but the user is successfully migrated when the Google user sign-in plugin is available. But there is a problem with the web page the user is facing. The web page shows two options (edit profile and add existing account). Because there is a user from the legacy system, this option appears.

The purpose of this pull request is to solve this web page issue. The user should be redirected to the home page.

But... this doesn't quite solve the problem. This PR prevents the mentioned options from appearing if the user tries again log in with google (second time).

First time:
Screenshot 2023-06-14 at 4 14 09 PM

But as I said, for the second time, that page doesn't show up (which is the goal of this PR).

Issue #37 discusses this PR

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

36.2% 36.2% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@daniel-frak
Copy link
Owner

Hi and thanks for the contribution :)
Sorry it took me so long to get to this MR.

My thoughts:

  1. I think legacyFederatedIdentities should be made optional in the JSON (and therefore in the code), to not break compatibility with existing clients.
  2. It's not clear what values identityProvider can have or what a token is in the JSON, nor which properties of a legacyFederatedIdentities item are actually required. Could you add some info about those to the README?
  3. This seems important enough to test end-to-end. Could you write a Cypress test for it? You could modify one of the users in the test app's InMemoryUserRepository to have a federated identity and then check if it's properly migrated. Otherwise, I assume the test would be very similar to the should migrate user test. This would also prove that this MR works as intended.
  4. The code needs to have 100% code coverage (see: SonarCloud analysis).

Regards,
Daniel Frąk.

@daniel-frak
Copy link
Owner

I just took a longer look at the README.md on master and the hypocrisy is not lost on me in that none of the response fields are marked as required/optional or properly described XD So I guess you can skip point number 2 if you don't feel like doing it (though I think it would be really helpful for everyone if it was documented properly).

@daniel-frak daniel-frak added the requested changes Waiting for requested changes to be made to the code label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requested changes Waiting for requested changes to be made to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants