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

alternative fix: allow to change or delete a relyingPartySecret on IdP #2887

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.io.StringWriter;
import java.util.Date;
import java.util.List;
import java.util.Locale;
import java.util.Optional;

import org.cloudfoundry.identity.uaa.alias.EntityAliasFailedException;
Expand All @@ -48,6 +49,7 @@
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,6 +71,7 @@
import org.springframework.transaction.support.TransactionTemplate;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.PathVariable;
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 @@ -228,16 +231,14 @@ 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("IdentityProvider[origin=" + body.getOriginKey() + "; zone=" + body.getIdentityZoneId()
+ "] - Configuration validation error for update.", 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())
);
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 @@ -249,7 +250,11 @@ public ResponseEntity<IdentityProvider> updateIdentityProvider(@PathVariable Str
samlConfigurator.validateSamlIdentityProviderDefinition(definition);
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 @@ -308,6 +313,38 @@ public ResponseEntity<IdentityProviderStatus> updateIdentityProviderStatus(@Path
return new ResponseEntity<>(body, OK);
}

@PutMapping(value = "{id}/secret")
public ResponseEntity<IdentityProvider> changeSecret(@PathVariable String id, @RequestBody IdentityProviderSecretChangeRequest secretChangeRequest) {
String zoneId = identityZoneManager.getCurrentIdentityZoneId();
IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
if(secretChangeRequest.getChangeMode() == IdentityProviderSecretChangeRequest.ChangeMode.UPDATE
&& UaaStringUtils.isNullOrEmpty(secretChangeRequest.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(secretChangeRequest.getSecret());
return persistIdentityProviderChange(updated, false, zoneId, existing);
} else if(LDAP.equals(existing.getType())
&& secretChangeRequest.getChangeMode() == IdentityProviderSecretChangeRequest.ChangeMode.UPDATE
&& existing.getConfig() instanceof LdapIdentityProviderDefinition ldapIdentityProviderDefinition) {
ldapIdentityProviderDefinition.setBindPassword(secretChangeRequest.getSecret());
} else {
if (secretChangeRequest.getChangeMode() == IdentityProviderSecretChangeRequest.ChangeMode.DELETE) {
logger.info("Invalid operation. This operation is only supported on external IDP of type OAuth/OIDC");
} else {
logger.debug("Invalid operation. This operation has an invalid status");
}
return new ResponseEntity<>(UNPROCESSABLE_ENTITY);
}
logger.info("Secret {} for Identity Provider: {}", secretChangeRequest.getChangeMode().toString().toLowerCase(Locale.ROOT), existing.getId());
identityProviderProvisioning.update(existing, zoneId);
redactSensitiveData(existing);
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) {
boolean retrieveActiveOnly = Boolean.parseBoolean(activeOnly);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.cloudfoundry.identity.uaa.provider;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;

import static org.cloudfoundry.identity.uaa.provider.IdentityProviderSecretChangeRequest.ChangeMode.DELETE;
import static org.cloudfoundry.identity.uaa.provider.IdentityProviderSecretChangeRequest.ChangeMode.UPDATE;

@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)
public class IdentityProviderSecretChangeRequest {

public enum ChangeMode {
UPDATE,
DELETE
}

private String secret;
private ChangeMode changeMode;

public IdentityProviderSecretChangeRequest() {
changeMode = DELETE;
}

public IdentityProviderSecretChangeRequest(String secret) {
changeMode = UPDATE;
this.secret = secret;
}

public String getSecret() {
return secret;
}

public void setSecret(String secret) {
this.secret = secret;
}

public ChangeMode getChangeMode() {
return changeMode;
}

public void setChangeMode(ChangeMode changeMode) {
this.changeMode = changeMode;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.springframework.http.HttpStatus.UNPROCESSABLE_ENTITY;

import java.net.MalformedURLException;
import java.net.URL;
Expand Down Expand Up @@ -305,6 +306,66 @@ void retrieve_all_providers_redacts_data() {
assertNull(oauth.getConfig().getRelyingPartySecret());
}

@Test
void change_bindPassword_and_retrieve_by_origin_providers_redacts_data() {
IdentityProvider idp = getLdapDefinition();
when(mockIdentityProviderProvisioning.retrieve("puppyId", "uaa")).thenReturn(idp);
IdentityProviderSecretChangeRequest identityProviderSecretChangeRequest = new IdentityProviderSecretChangeRequest("newSecret");
ResponseEntity<IdentityProvider> oidcBody = identityProviderEndpoints.changeSecret("puppyId", identityProviderSecretChangeRequest);
IdentityProvider<?> oidc = oidcBody.getBody();
assertNotNull(oidc);
assertNotNull(oidc.getConfig());
assertTrue(oidc.getConfig() instanceof LdapIdentityProviderDefinition);
assertNull(((LdapIdentityProviderDefinition) oidc.getConfig()).getBindPassword());
}

@Test
void change_secret_and_retrieve_by_origin_providers_redacts_data() {
IdentityProvider idp = getExternalOAuthProvider();
when(mockIdentityProviderProvisioning.retrieve("puppyId", "uaa")).thenReturn(idp);
IdentityProviderSecretChangeRequest identityProviderSecretChangeRequest = new IdentityProviderSecretChangeRequest();
identityProviderSecretChangeRequest.setSecret("newSecret");
ResponseEntity<IdentityProvider> oidcBody = identityProviderEndpoints.changeSecret("puppyId", identityProviderSecretChangeRequest);
IdentityProvider<?> oidc = oidcBody.getBody();
assertNotNull(oidc);
assertNotNull(oidc.getConfig());
assertTrue(oidc.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition);
assertNotNull(((AbstractExternalOAuthIdentityProviderDefinition) oidc.getConfig()).getRelyingPartySecret());
}

@Test
void change_secret_invalid() {
IdentityProvider idp = getExternalOAuthProvider();
when(mockIdentityProviderProvisioning.retrieve("puppyId", "uaa")).thenReturn(idp);
IdentityProviderSecretChangeRequest identityProviderSecretChangeRequest = new IdentityProviderSecretChangeRequest("");
ResponseEntity<IdentityProvider> oidcBody = identityProviderEndpoints.changeSecret("puppyId", identityProviderSecretChangeRequest);
// secret must be set
assertEquals(UNPROCESSABLE_ENTITY, oidcBody.getStatusCode());
}

@Test
void change_secret_on_uaafails() {
IdentityProvider identityProvider = new IdentityProvider<>();
identityProvider.setConfig(new SamlIdentityProviderDefinition());
identityProvider.setName("my saml provider");
identityProvider.setIdentityZoneId(UAA);
identityProvider.setType(OriginKeys.SAML);
IdentityProviderSecretChangeRequest identityProviderSecretChangeRequest = new IdentityProviderSecretChangeRequest();
when(mockIdentityProviderProvisioning.retrieve("puppyId", "uaa")).thenReturn(identityProvider);
ResponseEntity<IdentityProvider> oidcBody = identityProviderEndpoints.changeSecret("puppyId", identityProviderSecretChangeRequest);
IdentityProvider<?> oidc = oidcBody.getBody();
// because not allowed for SAML IdP
assertEquals(UNPROCESSABLE_ENTITY, oidcBody.getStatusCode());
assertNull(oidc);
// because not possible for SAML IdP
identityProviderSecretChangeRequest.setSecret("newSecret");
identityProviderSecretChangeRequest.setChangeMode(IdentityProviderSecretChangeRequest.ChangeMode.UPDATE);
oidcBody = identityProviderEndpoints.changeSecret("puppyId", identityProviderSecretChangeRequest);
oidc = oidcBody.getBody();
assertEquals(UNPROCESSABLE_ENTITY, oidcBody.getStatusCode());
assertNull(oidc);
}

@Test
void update_ldap_provider_patches_password() throws Exception {
IdentityProvider<LdapIdentityProviderDefinition> provider = retrieve_ldap_provider_by_id("id");
Expand Down
40 changes: 40 additions & 0 deletions uaa/slateCustomizations/source/index.html.md.erb
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,46 @@ _Error Codes_
| 403 | Forbidden - Insufficient scope |
| 422 | Unprocessable Entity - Invalid config |

## Change Secret

<aside class="success">
Added in UAA 77.10.0
</aside>
<br/>
Change or delete a secret. Change is supported for LDAP, OAuth2 and OIDC IdP configuration.
Delete is supported for the OAuth2 / OIDC IdP configuration only, because these providers support usages without a secret.
<br/>

<%= render('IdentityProviderEndpointDocs/changeIdpSecret/curl-request.md') %>
<%= render('IdentityProviderEndpointDocs/changeIdpSecret/http-request.md') %>
<%= render('IdentityProviderEndpointDocs/changeIdpSecret/http-response.md') %>

_Path Parameters_

<%= render('IdentityProviderEndpointDocs/changeIdpSecret/path-parameters.md') %>

_Request Headers_

<%= render('IdentityProviderEndpointDocs/changeIdpSecret/request-headers.md') %>

<aside class="notice">
This example is a delete action for OAuth2 and OIDC identity providers <br/>
</aside>

_Request Fields_

<%= render('IdentityProviderEndpointDocs/changeIdpSecret/request-fields.md') %>

_Response Fields_

<%= render('IdentityProviderEndpointDocs/changeIdpSecret/response-fields.md') %>

_Error Codes_

| Error Code | Description |
|------------|-----------------------------------------------------------------------|
| 403 | Forbidden - Insufficient scope |
| 422 | Unprocessable Entity - Invalid config |

# Users

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import static org.cloudfoundry.identity.uaa.provider.IdentityProvider.FIELD_ALIAS_ID;
import static org.cloudfoundry.identity.uaa.provider.IdentityProvider.FIELD_ALIAS_ZID;
import static org.cloudfoundry.identity.uaa.provider.IdentityProvider.FIELD_IDENTITY_ZONE_ID;
import static org.cloudfoundry.identity.uaa.provider.IdentityProviderSecretChangeRequest.ChangeMode.DELETE;
import static org.cloudfoundry.identity.uaa.provider.IdentityProviderSecretChangeRequest.ChangeMode.UPDATE;
import static org.cloudfoundry.identity.uaa.provider.LdapIdentityProviderDefinition.MAIL;
import static org.cloudfoundry.identity.uaa.provider.SamlIdentityProviderDefinition.EMAIL_ATTRIBUTE_NAME;
import static org.cloudfoundry.identity.uaa.provider.SamlIdentityProviderDefinition.EMAIL_VERIFIED_ATTRIBUTE_NAME;
Expand Down Expand Up @@ -77,6 +79,7 @@
import org.cloudfoundry.identity.uaa.provider.AbstractExternalOAuthIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.IdentityProvider;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderSecretChangeRequest;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderStatus;
import org.cloudfoundry.identity.uaa.provider.JdbcIdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.provider.LdapIdentityProviderDefinition;
Expand Down Expand Up @@ -106,7 +109,6 @@
import org.springframework.restdocs.payload.FieldDescriptor;
import org.springframework.restdocs.snippet.Attributes;
import org.springframework.restdocs.snippet.Snippet;
import org.cloudfoundry.identity.uaa.client.UaaClientDetails;
import org.springframework.test.web.servlet.ResultActions;

class IdentityProviderEndpointDocs extends EndpointDocs {
Expand Down Expand Up @@ -1091,6 +1093,37 @@ void patchIdentityProviderStatus() throws Exception {

}

@Test
void changeIdpSecret() throws Exception {
IdentityProvider identityProvider = identityProviderProvisioning.retrieveByOrigin("my-oauth2-provider", IdentityZoneHolder.get().getId());

IdentityProviderSecretChangeRequest identityProviderSecretChange = new IdentityProviderSecretChangeRequest();

final FieldDescriptor[] secretChangeFields = new FieldDescriptor[]{
fieldWithPath("secret").constrained("Optional if changeMode is `"+DELETE+"`. Required otherwise.").type(STRING).description("The new secret and/or bindPassword for the external IdP."),
fieldWithPath("changeMode").constrained("Required, `"+DELETE+"` or `"+UPDATE+"`.").type(STRING).description("If the change mode is set to `"+DELETE+"`, the existing secret will be deleted to support other OAuth methods. If set to `"+UPDATE+"` requires a secret.")
};

mockMvc.perform(put("/identity-providers/{id}/secret", identityProvider.getId())
.header("Authorization", "Bearer " + adminToken)
.contentType(APPLICATION_JSON)
.accept(APPLICATION_JSON)
.content(serializeExcludingProperties(identityProviderSecretChange)))
.andExpect(status().isOk())
.andDo(document("{ClassName}/{methodName}",
preprocessRequest(prettyPrint()),
preprocessResponse(prettyPrint()),
pathParameters(parameterWithName("id").required().description(ID_DESC)
),
requestHeaders(
headerWithName("Authorization").description("Bearer token containing `zones.<zone id>.admin` or `uaa.admin` or `idps.write` (only in the same zone that you are a user of)"),
IDENTITY_ZONE_ID_HEADER,
IDENTITY_ZONE_SUBDOMAIN_HEADER
),
requestFields(secretChangeFields),
responseFields(getCommonProviderFieldsAnyType())));
}

@Test
void deleteIdentityProvider() throws Exception {
IdentityProvider identityProvider = JsonUtils.readValue(mockMvc.perform(post("/identity-providers")
Expand Down
Loading