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.

Sign up for GitHub

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

Jump to bottom

Sonar refactoring - IdentityProviderEndpoints class #2890

Merged
merged 12 commits into from
May 29, 2024
Merged

Sonar refactoring - IdentityProviderEndpoints class #2890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ede8431
Sonar refactorings
strehle May 14, 2024
c7774ea
remove dead code
strehle May 14, 2024
d1e706e
more sonar smell fixes
strehle May 14, 2024
672b8d3
no log change
strehle May 14, 2024
771bc91
revert and allow test endpoint again
strehle May 14, 2024
2e2f04d
warnings
strehle May 14, 2024
6d6b5c3
Merge remote-tracking branch 'refs/remotes/origin/develop' into refac…
strehle May 23, 2024
441c13d
rebase
strehle May 23, 2024
a07632e
Merge remote-tracking branch 'refs/remotes/origin/develop' into refac…
strehle May 24, 2024
d7a1c35
rebase
strehle May 24, 2024
ae4a539
test setup
strehle May 24, 2024
3d087dc
Merge remote-tracking branch 'refs/remotes/origin/develop' into refac…
strehle May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 51 additions & 79 deletions server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@
import static org.springframework.http.HttpStatus.OK;
import static org.springframework.http.HttpStatus.UNPROCESSABLE_ENTITY;
import static org.springframework.util.StringUtils.hasText;
import static org.springframework.web.bind.annotation.RequestMethod.DELETE;
import static org.springframework.web.bind.annotation.RequestMethod.GET;
import static org.springframework.web.bind.annotation.RequestMethod.PATCH;
import static org.springframework.web.bind.annotation.RequestMethod.POST;
import static org.springframework.web.bind.annotation.RequestMethod.PUT;

import java.io.PrintWriter;
import java.io.StringWriter;
Expand Down Expand Up @@ -69,8 +65,13 @@
import org.springframework.transaction.PlatformTransactionManager;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.support.TransactionTemplate;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PatchMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
Expand Down Expand Up @@ -121,15 +122,15 @@ public IdentityProviderEndpoints(
this.idpAliasHandler = idpAliasHandler;
}

@RequestMapping(method = POST)
@PostMapping()
public ResponseEntity<IdentityProvider> createIdentityProvider(@RequestBody IdentityProvider body, @RequestParam(required = false, defaultValue = "false") boolean rawConfig) throws MetadataProviderException{
body.setSerializeConfigRaw(rawConfig);
String zoneId = identityZoneManager.getCurrentIdentityZoneId();
body.setIdentityZoneId(zoneId);
try {
configValidator.validate(body);
} catch (IllegalArgumentException e) {
logger.debug("IdentityProvider[origin="+body.getOriginKey()+"; zone="+body.getIdentityZoneId()+"] - Configuration validation error.", e);
logger.debug(String.format("IdentityProvider[origin=%s; zone=%s] - Configuration validation error.", body.getOriginKey(), body.getIdentityZoneId()), e);
return new ResponseEntity<>(body, UNPROCESSABLE_ENTITY);
}
if (SAML.equals(body.getType())) {
Expand All @@ -145,38 +146,10 @@ public ResponseEntity<IdentityProvider> createIdentityProvider(@RequestBody Iden
}

// persist IdP and create alias if necessary
final IdentityProvider<?> createdIdp;
try {
createdIdp = transactionTemplate.execute(txStatus -> {
final IdentityProvider<?> createdOriginalIdp = identityProviderProvisioning.create(body, zoneId);
return idpAliasHandler.ensureConsistencyOfAliasEntity(createdOriginalIdp, null);
});
} catch (final IdpAlreadyExistsException e) {
return new ResponseEntity<>(body, CONFLICT);
} catch (final EntityAliasFailedException e) {
logger.warn("Could not create alias for {}", e.getMessage());
final HttpStatus responseCode = Optional.ofNullable(HttpStatus.resolve(e.getHttpStatus())).orElse(INTERNAL_SERVER_ERROR);
return new ResponseEntity<>(body, responseCode);
} catch (final Exception e) {
logger.warn("Unable to create IdentityProvider[origin=" + body.getOriginKey() + "; zone=" + body.getIdentityZoneId() + "]", e);
return new ResponseEntity<>(body, INTERNAL_SERVER_ERROR);
}
if (createdIdp == null) {
logger.warn(
"IdentityProvider[origin={}; zone={}] - Transaction creating IdP (and alias IdP, if applicable) was not successful, but no exception was thrown.",
getCleanedUserControlString(body.getOriginKey()),
getCleanedUserControlString(body.getIdentityZoneId())
);
return new ResponseEntity<>(body, UNPROCESSABLE_ENTITY);
}
createdIdp.setSerializeConfigRaw(rawConfig);
setAuthMethod(createdIdp);
redactSensitiveData(createdIdp);

return new ResponseEntity<>(createdIdp, CREATED);
return persistIdentityProviderChange(body, rawConfig, zoneId, null, CREATED);
}

@RequestMapping(value = "{id}", method = DELETE)
@DeleteMapping(value = "{id}")
@Transactional
public ResponseEntity<IdentityProvider> deleteIdentityProvider(@PathVariable String id, @RequestParam(required = false, defaultValue = "false") boolean rawConfig) {
String identityZoneId = identityZoneManager.getCurrentIdentityZoneId();
Expand Down Expand Up @@ -221,7 +194,7 @@ public ResponseEntity<IdentityProvider> deleteIdentityProvider(@PathVariable Str
return new ResponseEntity<>(existing, OK);
}

@RequestMapping(value = "{id}", method = PUT)
@PutMapping(value = "{id}")
public ResponseEntity<IdentityProvider> updateIdentityProvider(@PathVariable String id, @RequestBody IdentityProvider body, @RequestParam(required = false, defaultValue = "false") boolean rawConfig) throws MetadataProviderException {
body.setSerializeConfigRaw(rawConfig);
String zoneId = identityZoneManager.getCurrentIdentityZoneId();
Expand All @@ -232,16 +205,15 @@ public ResponseEntity<IdentityProvider> updateIdentityProvider(@PathVariable Str
try {
configValidator.validate(body);
} catch (IllegalArgumentException e) {
logger.debug("IdentityProvider[origin="+body.getOriginKey()+"; zone="+body.getIdentityZoneId()+"] - Configuration validation error for update.", e);
logger.debug(String.format("IdentityProvider[origin=%s; zone=%s] - Configuration validation error for update.", body.getOriginKey(), body.getIdentityZoneId()), e);
return new ResponseEntity<>(body, UNPROCESSABLE_ENTITY);
}

if (!idpAliasHandler.aliasPropertiesAreValid(body, existing)) {
logger.warn(
"IdentityProvider[origin={}; zone={}] - Alias ID and/or ZID changed during update of IdP with alias.",
getCleanedUserControlString(body.getOriginKey()),
getCleanedUserControlString(body.getIdentityZoneId())
);
if (logger.isWarnEnabled()) {
logger.warn("IdentityProvider[origin={}; zone={}] - Alias ID and/or ZID changed during update of IdP with alias.",
getCleanedUserControlString(body.getOriginKey()), getCleanedUserControlString(body.getIdentityZoneId()));
}
return new ResponseEntity<>(body, UNPROCESSABLE_ENTITY);
}

Expand All @@ -254,38 +226,45 @@ public ResponseEntity<IdentityProvider> updateIdentityProvider(@PathVariable Str
body.setConfig(definition);
}

return persistIdentityProviderChange(body, rawConfig, zoneId, existing, OK);
}

private ResponseEntity<IdentityProvider> persistIdentityProviderChange(IdentityProvider body, boolean rawConfig, String zoneId,
IdentityProvider existing, HttpStatus status) {
final IdentityProvider<?> updatedIdp;
try {
updatedIdp = transactionTemplate.execute(txStatus -> {
final IdentityProvider<?> updatedOriginalIdp = identityProviderProvisioning.update(body, zoneId);
final IdentityProvider<?> updatedOriginalIdp = status == CREATED ? identityProviderProvisioning.create(body, zoneId) : identityProviderProvisioning.update(body, zoneId);
return idpAliasHandler.ensureConsistencyOfAliasEntity(updatedOriginalIdp, existing);
});
} catch (final IdpAlreadyExistsException e) {
return new ResponseEntity<>(body, CONFLICT);
} catch (final EntityAliasFailedException e) {
logger.warn("Could not create alias for {}", e.getMessage());
logger.warn(String.format("Could not create alias for %s", e.getMessage()));
final HttpStatus responseCode = Optional.ofNullable(HttpStatus.resolve(e.getHttpStatus())).orElse(INTERNAL_SERVER_ERROR);
return new ResponseEntity<>(body, responseCode);
} catch (final Exception e) {
logger.warn("Unable to update IdentityProvider[origin=" + body.getOriginKey() + "; zone=" + body.getIdentityZoneId() + "]", e);
logger.warn(String.format("Unable to %s IdentityProvider[origin=%s; zone=%s]",
status == CREATED ? "create" : "update", body.getOriginKey(), body.getIdentityZoneId()), e);
return new ResponseEntity<>(body, INTERNAL_SERVER_ERROR);
}
if (updatedIdp == null) {
logger.warn(
"IdentityProvider[origin={}; zone={}] - Transaction updating IdP (and alias IdP, if applicable) was not successful, but no exception was thrown.",
getCleanedUserControlString(body.getOriginKey()),
getCleanedUserControlString(body.getIdentityZoneId())
);
if (logger.isWarnEnabled()) {
logger.warn(
"IdentityProvider[origin={}; zone={}] - Transaction {} IdP (and alias IdP, if applicable) was not successful, but no exception was thrown.",
getCleanedUserControlString(body.getOriginKey()), getCleanedUserControlString(body.getIdentityZoneId()),
status == CREATED ? "creating" : "updating");
}
return new ResponseEntity<>(body, UNPROCESSABLE_ENTITY);
}
updatedIdp.setSerializeConfigRaw(rawConfig);
setAuthMethod(updatedIdp);
redactSensitiveData(updatedIdp);

return new ResponseEntity<>(updatedIdp, OK);
return new ResponseEntity<>(updatedIdp, status);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not end up returning different status for the same result after this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

this part was a duplicated one, see
image

now an extra method persistIdentityProviderChange for create and update

}

@RequestMapping (value = "{id}/status", method = PATCH)
@PatchMapping(value = "{id}/status")
public ResponseEntity<IdentityProviderStatus> updateIdentityProviderStatus(@PathVariable String id, @RequestBody IdentityProviderStatus body) {
String zoneId = identityZoneManager.getCurrentIdentityZoneId();
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
Expand All @@ -304,7 +283,7 @@ public ResponseEntity<IdentityProviderStatus> updateIdentityProviderStatus(@Path
}
uaaIdentityProviderDefinition.getPasswordPolicy().setPasswordNewerThan(new Date(System.currentTimeMillis()));
identityProviderProvisioning.update(existing, zoneId);
logger.info("PasswordChangeRequired property set for Identity Provider: " + existing.getId());
logger.info("PasswordChangeRequired property set for Identity Provider: {}", existing.getId());

/* since this operation is only allowed for IdPs of type "UAA" and aliases are not supported for "UAA" IdPs,
* we do not need to propagate the changes to an alias IdP here. */
Expand All @@ -313,7 +292,7 @@ public ResponseEntity<IdentityProviderStatus> updateIdentityProviderStatus(@Path
return new ResponseEntity<>(body, OK);
}

@RequestMapping(method = GET)
@GetMapping()
public ResponseEntity<List<IdentityProvider>> retrieveIdentityProviders(@RequestParam(value = "active_only", required = false) String activeOnly, @RequestParam(required = false, defaultValue = "false") boolean rawConfig) {
boolean retrieveActiveOnly = Boolean.parseBoolean(activeOnly);
List<IdentityProvider> identityProviderList = identityProviderProvisioning.retrieveAll(retrieveActiveOnly, identityZoneManager.getCurrentIdentityZoneId());
Expand All @@ -325,7 +304,7 @@ public ResponseEntity<List<IdentityProvider>> retrieveIdentityProviders(@Request
return new ResponseEntity<>(identityProviderList, OK);
}

@RequestMapping(value = "{id}", method = GET)
@GetMapping(value = "{id}")
public ResponseEntity<IdentityProvider> retrieveIdentityProvider(@PathVariable String id, @RequestParam(required = false, defaultValue = "false") boolean rawConfig) {
IdentityProvider identityProvider = identityProviderProvisioning.retrieve(id, identityZoneManager.getCurrentIdentityZoneId());
identityProvider.setSerializeConfigRaw(rawConfig);
Expand Down Expand Up @@ -413,28 +392,24 @@ protected void patchSensitiveData(String id, IdentityProvider provider) {
}
switch (provider.getType()) {
case LDAP: {
if (provider.getConfig() instanceof LdapIdentityProviderDefinition) {
LdapIdentityProviderDefinition definition = (LdapIdentityProviderDefinition) provider.getConfig();
if (definition.getBindPassword() == null) {
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
if (existing!=null &&
existing.getConfig()!=null &&
existing.getConfig() instanceof LdapIdentityProviderDefinition) {
LdapIdentityProviderDefinition existingDefinition = (LdapIdentityProviderDefinition)existing.getConfig();
definition.setBindPassword(existingDefinition.getBindPassword());
}
if (provider.getConfig() instanceof LdapIdentityProviderDefinition definition && definition.getBindPassword() == null) {
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
if (existing!=null &&
existing.getConfig()!=null &&
existing.getConfig() instanceof LdapIdentityProviderDefinition existingDefinition) {
definition.setBindPassword(existingDefinition.getBindPassword());
}
}
break;
}
case OAUTH20 :
case OIDC10 : {
if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition definition && definition.getRelyingPartySecret() == null) {
case OAUTH20, OIDC10 : {
if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition definition &&
definition.getRelyingPartySecret() == null &&
secretNeeded(definition)) {
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
if (existing!=null &&
existing.getConfig()!=null &&
existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition existingDefinition &&
secretNeeded(definition)) {
existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition existingDefinition) {
definition.setRelyingPartySecret(existingDefinition.getRelyingPartySecret());
}
}
Expand All @@ -452,18 +427,15 @@ protected void redactSensitiveData(IdentityProvider provider) {
}
switch (provider.getType()) {
case LDAP: {
if (provider.getConfig() instanceof LdapIdentityProviderDefinition) {
logger.debug("Removing bind password from LDAP provider id:"+provider.getId());
LdapIdentityProviderDefinition definition = (LdapIdentityProviderDefinition) provider.getConfig();
if (provider.getConfig() instanceof LdapIdentityProviderDefinition definition) {
logger.debug("Removing bind password from LDAP provider id: {}", provider.getId());
definition.setBindPassword(null);
}
break;
}
case OAUTH20 :
case OIDC10 : {
if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition) {
logger.debug("Removing relying secret from OAuth/OIDC provider id:"+provider.getId());
AbstractExternalOAuthIdentityProviderDefinition definition = (AbstractExternalOAuthIdentityProviderDefinition) provider.getConfig();
case OAUTH20, OIDC10 : {
if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition<?> definition) {
logger.debug("Removing relying secret from OAuth/OIDC provider id: {}", provider.getId());
hsinn0 marked this conversation as resolved.
Show resolved Hide resolved
definition.setRelyingPartySecret(null);
}
break;
Expand Down
Loading
Loading