Skip to content

Commit

Permalink
Do not allow remove a credential in account endpoint if provider mark…
Browse files Browse the repository at this point in the history 
…s it as not removable

Closes #25220

Signed-off-by: rmartinc <rmartinc@redhat.com>
(cherry picked from commit d004e92)
  • Loading branch information
@rmartinc @ahus1
rmartinc authored and ahus1 committed Dec 15, 2023
1 parent 496ca92 commit 3d16564
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 24 deletions.
41 changes: 25 additions & 16 deletions ...ices/src/main/java/org/keycloak/services/resources/account/AccountCredentialResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.keycloak.util.JsonSerialization;
import org.keycloak.utils.MediaType;

import jakarta.ws.rs.BadRequestException;
import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.DELETE;
import jakarta.ws.rs.GET;
Expand All @@ -41,7 +42,6 @@
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Predicate;
Expand Down Expand Up @@ -165,7 +165,7 @@ public Stream<CredentialContainer> credentialTypes(@QueryParam(TYPE) String type

boolean includeUserCredentials = userCredentials == null || userCredentials;

Set<String> enabledCredentialTypes = getEnabledCredentialTypes(getCredentialProviders());
Set<String> enabledCredentialTypes = getEnabledCredentialTypes();

Stream<CredentialModel> modelsStream = includeUserCredentials ? user.credentialManager().getStoredCredentialsStream() : Stream.empty();
List<CredentialModel> models = modelsStream.collect(Collectors.toList());
Expand Down Expand Up @@ -231,8 +231,8 @@ private Stream<CredentialProvider> getCredentialProviders() {

// Going through all authentication flows and their authentication executions to see if there is any authenticator of the corresponding
// credential type.
private Set<String> getEnabledCredentialTypes(Stream<CredentialProvider> credentialProviders) {
Stream<String> enabledCredentialTypes = realm.getAuthenticationFlowsStream()
private Set<String> getEnabledCredentialTypes() {
return realm.getAuthenticationFlowsStream()
.filter(((Predicate<AuthenticationFlowModel>) this::isFlowEffectivelyDisabled).negate())
.flatMap(flow ->
realm.getAuthenticationExecutionsStream(flow.getId())
Expand All @@ -242,13 +242,7 @@ private Set<String> getEnabledCredentialTypes(Stream<CredentialProvider> credent
.filter(Objects::nonNull)
.map(AuthenticatorFactory::getReferenceCategory)
.filter(Objects::nonNull)
);

Set<String> credentialTypes = credentialProviders
.map(CredentialProvider::getType)
.collect(Collectors.toSet());

return enabledCredentialTypes.filter(credentialTypes::contains).collect(Collectors.toSet());
).collect(Collectors.toSet());
}

// Returns true if flow is effectively disabled - either it's execution or some parent execution is disabled
Expand All @@ -267,6 +261,21 @@ private boolean isFlowEffectivelyDisabled(AuthenticationFlowModel flow) {
return false;
}

private void checkIfCanBeRemoved(String credentialType) {
Set<String> enabledCredentialTypes = getEnabledCredentialTypes();
CredentialProvider credentialProvider = getCredentialProviders()
.filter(p -> credentialType.equals(p.getType()) && enabledCredentialTypes.contains(p.getType()))
.findAny().orElse(null);
if (credentialProvider == null) {
throw new NotFoundException("Credential provider " + credentialType + " not found");
}
CredentialTypeMetadataContext ctx = CredentialTypeMetadataContext.builder().user(user).build(session);
CredentialTypeMetadata metadata = credentialProvider.getCredentialTypeMetadata(ctx);
if (!metadata.isRemoveable()) {
throw new BadRequestException("Credential type " + credentialType + " cannot be removed");
}
}

/**
* Remove a credential of current user
*
Expand All @@ -282,16 +291,16 @@ public void removeCredential(final @PathParam("credentialId") String credentialI
// Backwards compatibility with account console 1 - When stored credential is not found, it may be federated credential.
// In this case, it's ID needs to be something like "otp-id", which is returned by account REST GET endpoint as a placeholder
// for federated credentials (See CredentialHelper.createUserStorageCredentialRepresentation )
Optional<String> federatedCredentialType = getEnabledCredentialTypes(getCredentialProviders()).stream()
.filter(credentialType -> (credentialType + "-id").equals(credentialId))
.findFirst();
if (federatedCredentialType.isPresent()) {
user.credentialManager().disableCredentialType(federatedCredentialType.get());
if (credentialId.endsWith("-id")) {
String credentialType = credentialId.substring(0, credentialId.length() - 3);
checkIfCanBeRemoved(credentialType);
user.credentialManager().disableCredentialType(credentialType);
return;
}

throw new NotFoundException("Credential not found");
}
checkIfCanBeRemoved(credential.getType());
user.credentialManager().removeStoredCredentialById(credentialId);
}

Expand Down
36 changes: 28 additions & 8 deletions ...llian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import org.keycloak.representations.idm.ClientScopeRepresentation;
import org.keycloak.representations.idm.CredentialRepresentation;
import org.keycloak.representations.idm.ErrorRepresentation;
import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
import org.keycloak.representations.idm.ProtocolMapperRepresentation;
import org.keycloak.representations.idm.RealmRepresentation;
import org.keycloak.representations.idm.RequiredActionProviderRepresentation;
Expand Down Expand Up @@ -871,16 +872,21 @@ public void testCredentialsGetWithDisabledOtpRequiredAction() throws IOException
AccountCredentialResource.CredentialContainer otpCredential = credentials.get(1);
assertNull(otpCredential.getCreateAction());
assertNull(otpCredential.getUpdateAction());
assertTrue(otpCredential.isRemoveable());

String otpCredentialId = otpCredential.getUserCredentialMetadatas().get(0).getCredential().getId();

// remove credential using account console as otp is removable
try (SimpleHttp.Response response = SimpleHttp
.doDelete(getAccountUrl("credentials/" + otpCredentialId), httpClient)
.acceptJson()
.auth(tokenUtil.getToken())
.asResponse()) {
assertEquals(204, response.getStatus());
}

// Revert - re-enable requiredAction and remove OTP credential from the user
setRequiredActionEnabledStatus(UserModel.RequiredAction.CONFIGURE_TOTP.name(), true);

String otpCredentialId = adminUserResource.credentials().stream()
.filter(credential -> OTPCredentialModel.TYPE.equals(credential.getType()))
.findFirst()
.get()
.getId();
adminUserResource.removeCredential(otpCredentialId);
}

private void setRequiredActionEnabledStatus(String requiredActionProviderId, boolean enabled) {
Expand All @@ -904,6 +910,20 @@ public void testCredentialsForUserWithoutPassword() throws IOException {
// We won't be able to authenticate later as user won't have password
List<AccountCredentialResource.CredentialContainer> credentials = getCredentials();

// delete password should fail as it is not removable
AccountCredentialResource.CredentialContainer password = credentials.get(0);
assertCredentialContainerExpected(password, PasswordCredentialModel.TYPE, CredentialTypeMetadata.Category.BASIC_AUTHENTICATION.toString(),
"password-display-name", "password-help-text", "kcAuthenticatorPasswordClass",
null, UserModel.RequiredAction.UPDATE_PASSWORD.toString(), false, 1);
try (SimpleHttp.Response response = SimpleHttp
.doDelete(getAccountUrl("credentials/" + password.getUserCredentialMetadatas().get(0).getCredential().getId()), httpClient)
.acceptJson()
.auth(tokenUtil.getToken())
.asResponse()) {
assertEquals(400, response.getStatus());
Assert.assertEquals("Credential type password cannot be removed", response.asJson(OAuth2ErrorRepresentation.class).getError());
}

// Remove password from the user now
UserResource user = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost");
for (CredentialRepresentation credential : user.credentials()) {
Expand All @@ -914,7 +934,7 @@ public void testCredentialsForUserWithoutPassword() throws IOException {

// Get credentials. Ensure user doesn't have password credential and create action is UPDATE_PASSWORD
credentials = getCredentials();
AccountCredentialResource.CredentialContainer password = credentials.get(0);
password = credentials.get(0);
assertCredentialContainerExpected(password, PasswordCredentialModel.TYPE, CredentialTypeMetadata.Category.BASIC_AUTHENTICATION.toString(),
"password-display-name", "password-help-text", "kcAuthenticatorPasswordClass",
UserModel.RequiredAction.UPDATE_PASSWORD.toString(), null, false, 0);
Expand Down

0 comments on commit 3d16564

Please sign in to comment.