Skip to content

Commit

Permalink
2nd alternative fix: allow to change or delete a relyingPartySecret o…
Browse files Browse the repository at this point in the history 
…n IdP

Alternative to PR: #2882, #2887
and PR: #2885

Only one call in backend, but more input on client side.

authMethod is used to make this change non-breakable.
Because before the update prevented the removal of the relyingPartySecret.
No if authMethod is not client_secret_basic or client_secret_post, then the relyingPartySecret can be overwritten with null.
  • Loading branch information
@strehle
strehle committed May 16, 2024
1 parent 2820f35 commit 3cea251
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 368 deletions.
21 changes: 21 additions & 0 deletions model/src/main/java/org/cloudfoundry/identity/uaa/constants/ClientAuthentication.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package org.cloudfoundry.identity.uaa.constants;

import java.util.List;
import java.util.Optional;

/**
* ClientAuthentication constants are defined in OIDC core and discovery standard, e.g. https://openid.net/specs/openid-connect-registration-1_0.html
* OIDC possible values are: client_secret_post, client_secret_basic, client_secret_jwt, private_key_jwt, and none
Expand All @@ -15,4 +18,22 @@ private ClientAuthentication() {}
public static final String CLIENT_SECRET_POST = "client_secret_post";
public static final String PRIVATE_KEY_JWT = "private_key_jwt";
public static final String NONE = "none";

public static final List<String> UAA_SUPPORTED_METHODS = List.of(CLIENT_SECRET_BASIC, CLIENT_SECRET_POST, NONE, PRIVATE_KEY_JWT);

public static boolean secretNeeded(String method) {
return method == null || CLIENT_SECRET_POST.equals(method) || CLIENT_SECRET_BASIC.equals(method);
}

public static boolean isMethodSupported(String method) {
return Optional.ofNullable(method).map(UAA_SUPPORTED_METHODS::contains).orElse(true);
}

public static boolean isValidMethod(String method, String secret) {
if (isMethodSupported(method) && secretNeeded(method) && secret != null || (method == null && secret == null)) {
return true;
} else {
return false;
}
}
}
2 changes: 0 additions & 2 deletions ...g/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Getter;

import java.lang.reflect.ParameterizedType;
Expand Down Expand Up @@ -53,7 +52,6 @@ public enum OAuthGroupMappingMode {
private boolean pkce = true;
private boolean performRpInitiatedLogout = true;
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonProperty(value = "auth_method", access = JsonProperty.Access.READ_ONLY)
private String authMethod;

public T setAuthUrl(URL authUrl) {
Expand Down
17 changes: 0 additions & 17 deletions model/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderSecretChange.java
View file

This file was deleted.

78 changes: 13 additions & 65 deletions server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import org.cloudfoundry.identity.uaa.scim.ScimGroupProvisioning;
import org.cloudfoundry.identity.uaa.util.JsonUtils;
import org.cloudfoundry.identity.uaa.util.ObjectUtils;
import org.cloudfoundry.identity.uaa.util.UaaStringUtils;
import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
import org.opensaml.saml2.metadata.provider.MetadataProviderException;
import org.slf4j.Logger;
Expand All @@ -69,9 +68,7 @@
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.PatchMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
Expand Down Expand Up @@ -230,7 +227,6 @@ public ResponseEntity<IdentityProvider> updateIdentityProvider(@PathVariable Str
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
body.setId(id);
body.setIdentityZoneId(zoneId);
setAuthMethod(existing);
patchSensitiveData(id, body);
try {
configValidator.validate(body);
Expand All @@ -257,11 +253,6 @@ public ResponseEntity<IdentityProvider> updateIdentityProvider(@PathVariable Str
body.setConfig(definition);
}

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

private ResponseEntity<IdentityProvider> persistIdentityProviderChange(IdentityProvider body, boolean rawConfig, String zoneId,
IdentityProvider existing) {
final IdentityProvider<?> updatedIdp;
try {
updatedIdp = transactionTemplate.execute(txStatus -> {
Expand Down Expand Up @@ -321,63 +312,11 @@ public ResponseEntity<IdentityProviderStatus> updateIdentityProviderStatus(@Path
return new ResponseEntity<>(body, OK);
}

@DeleteMapping(value = "{id}/secret")
public ResponseEntity<IdentityProvider> deleteSecret(@PathVariable String id) {
String zoneId = identityZoneManager.getCurrentIdentityZoneId();
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
if((OIDC10.equals(existing.getType()) || OAUTH20.equals(existing.getType()))
&& existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition<?>) {
IdentityProvider updated = existing;
((AbstractExternalOAuthIdentityProviderDefinition) updated.getConfig()).setRelyingPartySecret(null);
logger.info("Delete secret for Identity Provider: {}", existing.getId());
return persistIdentityProviderChange(updated, false, zoneId, existing);
} else {
logger.debug("Invalid operation. This operation is only supported on external IDP of type OAuth/OIDC");
return new ResponseEntity<>(UNPROCESSABLE_ENTITY);
}
}

@PatchMapping(value = "{id}/secret")
public ResponseEntity<IdentityProvider> changeSecret(@PathVariable String id, @RequestBody IdentityProviderSecretChange secretChange) {
String zoneId = identityZoneManager.getCurrentIdentityZoneId();
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
if(UaaStringUtils.isNullOrEmpty(secretChange.getSecret())) {
logger.debug("Invalid payload. The property secret needs to be set");
return new ResponseEntity<>(UNPROCESSABLE_ENTITY);
}
if((OIDC10.equals(existing.getType()) || OAUTH20.equals(existing.getType()))
&& existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition<?>) {
IdentityProvider updated = existing;
((AbstractExternalOAuthIdentityProviderDefinition) updated.getConfig()).setRelyingPartySecret(secretChange.getSecret());
logger.info("Change secret for Identity Provider: {}", existing.getId());
return persistIdentityProviderChange(updated, false, zoneId, existing);
} else if(LDAP.equals(existing.getType()) && existing.getConfig() instanceof LdapIdentityProviderDefinition ldapIdentityProviderDefinition) {
ldapIdentityProviderDefinition.setBindPassword(secretChange.getSecret());
} else {
logger.debug("Invalid operation. This operation is only supported on external IDP of type LDAP/OAuth/OIDC");
return new ResponseEntity<>(UNPROCESSABLE_ENTITY);
}
identityProviderProvisioning.update(existing, zoneId);
setAuthMethod(existing);
redactSensitiveData(existing);
logger.info("Secret changed for Identity Provider: {}", existing.getId());
return new ResponseEntity<>(existing, OK);
}

@RequestMapping(method = GET)
public ResponseEntity<List<IdentityProvider>> retrieveIdentityProviders(
@RequestParam(value = "active_only", required = false) String activeOnly,
@RequestParam(required = false, defaultValue = "false") boolean rawConfig,
@RequestParam(required = false, defaultValue = "") String origin)
{
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;
if (UaaStringUtils.isNotEmpty(origin)) {
identityProviderList = List.of(identityProviderProvisioning.retrieveByOrigin(origin, identityZoneManager.getCurrentIdentityZoneId()));
} else {
identityProviderList = identityProviderProvisioning.retrieveAll(retrieveActiveOnly, identityZoneManager.getCurrentIdentityZoneId());
}
for(IdentityProvider<?> idp : identityProviderList) {
List<IdentityProvider> identityProviderList = identityProviderProvisioning.retrieveAll(retrieveActiveOnly, identityZoneManager.getCurrentIdentityZoneId());
for(IdentityProvider idp : identityProviderList) {
idp.setSerializeConfigRaw(rawConfig);
setAuthMethod(idp);
redactSensitiveData(idp);
Expand Down Expand Up @@ -495,7 +434,8 @@ protected void patchSensitiveData(String id, IdentityProvider provider) {
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
if (existing!=null &&
existing.getConfig()!=null &&
existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition) {
existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition &&
secretNeeded(definition)) {
AbstractExternalOAuthIdentityProviderDefinition existingDefinition = (AbstractExternalOAuthIdentityProviderDefinition)existing.getConfig();
definition.setRelyingPartySecret(existingDefinition.getRelyingPartySecret());
}
Expand Down Expand Up @@ -537,6 +477,14 @@ protected void redactSensitiveData(IdentityProvider provider) {
}
}

protected boolean secretNeeded(AbstractExternalOAuthIdentityProviderDefinition abstractExternalOAuthIdentityProviderDefinition) {
boolean needSecret = true;
if (abstractExternalOAuthIdentityProviderDefinition.getAuthMethod() != null) {
return ClientAuthentication.secretNeeded(abstractExternalOAuthIdentityProviderDefinition.getAuthMethod());
}
return needSecret;
}

protected void setAuthMethod(IdentityProvider<?> provider) {
if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition<?> abstractExternalOAuthIdentityProviderDefinition) {
if (abstractExternalOAuthIdentityProviderDefinition.getRelyingPartySecret() == null) {
Expand Down
9 changes: 9 additions & 0 deletions ...loudfoundry/identity/uaa/provider/oauth/ExternalOAuthIdentityProviderConfigValidator.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.cloudfoundry.identity.uaa.provider.oauth;

import org.cloudfoundry.identity.uaa.constants.ClientAuthentication;
import org.cloudfoundry.identity.uaa.provider.AbstractExternalOAuthIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.AbstractIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.BaseIdentityProviderValidator;
Expand Down Expand Up @@ -61,6 +62,14 @@ public void validate(AbstractIdentityProviderDefinition definition) {
errors.add("Relying Party Id must be the client-id for the UAA that is registered with the external IDP");
}

if (hasText(def.getAuthMethod()) && !ClientAuthentication.isMethodSupported(def.getAuthMethod())) {
errors.add("Relying Party Authentication Method must be set to either " + String.join(" or ", ClientAuthentication.UAA_SUPPORTED_METHODS));
} else {
if (!ClientAuthentication.isValidMethod(def.getAuthMethod(), def.getRelyingPartySecret())) {
errors.add("Relying Party Authentication has invalid requirements for the secret.");
}
}

if (def.isShowLinkText() && !hasText(def.getLinkText())) {
errors.add("Link Text must be specified because showLinkText is true");
}
Expand Down
Loading

0 comments on commit 3cea251

Please sign in to comment.