Alias ID and Alias ZID for Identity Providers #2637
Conversation
…umns to identity_provider table
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/186629962 The labels on this github issue will be updated when the story is started. |
|
If alias_zid is set, replicate entry into alias identity zone. From default zone "uaa" you may replicate into any custom zone. From any custom zone, you may replicate into default zone "uaa" only. |
model/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProvider.java
Show resolved
Hide resolved
model/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProvider.java
Show resolved
Hide resolved
…d IdP cannot be found
...r/src/main/java/org/cloudfoundry/identity/uaa/provider/JdbcIdentityProviderProvisioning.java
Show resolved
Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java
Outdated
Show resolved
Hide resolved
…eature-flag' into feature/alias-id-and-alias-zid-for-identity-providers
...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java
Show resolved
Hide resolved
| throw new IllegalArgumentException("IdP type not supported."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There appears to be good coverage here, though I wonder if something is missed. I would prefer if there was a doc or matrix indicating conditions that maps to the tests to validate if any gaps exist.
There was a problem hiding this comment.
There is a way to see the test coverage on the new code in SonarCloud. Overall coverage on new code is reported as 80.7% currently, just over the minimum threshold.
If I drill into IdentityProviderEndpoints.java, I see some if statement bodies with non-trivial code that would be nice to get test coverage for, plus some exceptions that aren't tested.
Also, Sonar indicates that there is one new code smell introduced in the PR that I'd like to see addressed.
...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java
Show resolved
Hide resolved
…assertOtherPropertiesAreEqual
…king the alias reference
| } | ||
|
|
||
| private static String buildMessagePrefix(@NonNull final IdentityProvider<?> idp) { | ||
| private static String buildMessagePrefix( |
There was a problem hiding this comment.
I'm not sure what code smell this particular change is addressing, but I think having 4 parameters vs. a single parameter class is also a code smell.
There was a problem hiding this comment.
The reason I changed this is the new method createNewAliasIdp in IdentityProviderEndpoints. With just the other constructor of IdpAliasFailedException being present, we would have to pass the original IdP as a parameter just for the exception case (e.g., e5f5791#diff-90da5e48423637b46ed07b703063b4fca4a0ee47cca8286548c91941b532fd07R566), even though all of the information for the exception message is also present in the alias IdP.
| @@ -541,27 +541,43 @@ private <T extends AbstractIdentityProviderDefinition> IdentityProvider<?> ensur | |||
| return originalIdp; | |||
| } | |||
|
|
|||
| final IdentityProvider<?> persistedAliasIdp = createNewAliasIdp(aliasIdp, originalIdp.getAliasZid()); | |||
There was a problem hiding this comment.
I like the change reducing the length of a long method. I'm curious what the Sonar smells are that all of these changes are addressing.
There was a problem hiding this comment.
This is the fix to the Sonar warning about the "Cognitive Complexity" being too high in this method.
adrianhoelzl-sap
deleted the
feature/alias-id-and-alias-zid-for-identity-providers
branch
Issue #2505
Identity providers may be copied from the custom identity zone to the default identity zone (or vice versa) to enable customers to do user management in own identity zones