keycloak-user-migration icon indicating copy to clipboard operation
keycloak-user-migration copied to clipboard

feat: add federated identity

Open abdurrahmanekr opened this issue 2 years ago • 3 comments

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

abdurrahmanekr avatar Jun 14 '23 13:06 abdurrahmanekr

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

sonarqubecloud[bot] avatar Jun 22 '23 14:06 sonarqubecloud[bot]

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 avatar Jun 22 '23 14:06 daniel-frak

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 avatar Jun 22 '23 15:06 daniel-frak