8000 [admin-api-v2] Incorrect DTO/DAO mapping by mabartos · Pull Request #44587 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

[admin-api-v2] Incorrect DTO/DAO mapping #44587

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

Merged
vmuzikar merged 2 commits into from
Dec 3, 2025
Merged

[admin-api-v2] Incorrect DTO/DAO mapping #44587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83f30f4
[admin-api-v2] Incorrect DTO/DAO mapping
mabartos Dec 1, 2025
5b20be5
Handle roles and service account operations, cleanup service contract
mabartos Dec 2, 2025
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
8 changes: 6 additions & 2 deletions rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/ClientModelMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
package org.keycloak.models.mapper;

import org.keycloak.models.ClientModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.RealmModel;
import org.keycloak.representations.admin.v2.ClientRepresentation;
import org.keycloak.services.ServiceException;

public interface ClientModelMapper {

ClientRepresentation fromModel(ClientModel model);
ClientRepresentation fromModel(KeycloakSession session, ClientModel model);

void toModel(ClientModel model, ClientRepresentation rep, RealmModel realm);
ClientModel toModel(KeycloakSession session, RealmModel realm, ClientModel existingModel, ClientRepresentation rep) throws ServiceException;

ClientModel toModel(KeycloakSession session, RealmModel realm, ClientRepresentation rep) throws ServiceException;
}
79 changes: 68 additions & 11 deletions rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/MapStructClientModelMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,103 @@
package org.keycloak.models.mapper;

import java.util.Collections;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.keycloak.models.ClientModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RoleModel;
import org.keycloak.representations.admin.v2.ClientRepresentation;
import org.keycloak.services.ServiceException;

import org.mapstruct.Context;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.MappingTarget;
import org.mapstruct.Named;
import org.mapstruct.ObjectFactory;

@Mapper
public interface MapStructClientModelMapper extends ClientModelMapper {

@Override
@ModelToRep
ClientRepresentation fromModel(@Context KeycloakSession session, ClientModel model);

// we don't want to ignore nulls so that we completely overwrite the state
@Override
@RepToModel
ClientModel toModel(@Context KeycloakSession session, @Context RealmModel realm, @MappingTarget ClientModel existingModel, ClientRepresentation rep) throws ServiceException;

@Override
@RepToModel
ClientModel toModel(@Context KeycloakSession session, @Context RealmModel realm, ClientRepresentation rep) throws ServiceException;

/*-------------------------------------*
* MAPPERS *
*-------------------------------------*/
@Mapping(target = "name", source = "displayName")
@Mapping(target = "baseUrl", source = "appUrl")
@Mapping(target = "redirectUris", source = "appRedirectUrls")
@Mapping(target = "authenticationFlowBindingOverrides", source = "loginFlows", ignore = true) // TODO
@Mapping(target = "publicClient", source = "auth.enabled", qualifiedByName = "isPublicClientPrimitive")
@Mapping(target = "clientAuthenticatorType", source = "auth.method")
@Mapping(target = "secret", source = "auth.secret")
@Mapping(target = "serviceAccountsEnabled", source = "serviceAccount.enabled")
@interface RepToModel {
}

@Mapping(target = "displayName", source = "name")
@Mapping(target = "appUrl", source = "baseUrl")
@Mapping(target = "appRedirectUrls", source = "redirectUris")
@Mapping(target = "loginFlows", source = "authenticationFlowBindingOverrides", ignore = true)
@Mapping(target = "auth", ignore = true) // TODO
@Mapping(target = "auth.enabled", source = "publicClient", qualifiedByName = "isPublicClient")
@Mapping(target = "auth.method", source = "clientAuthenticatorType")
@Mapping(target = "auth.secret", source = "secret")
@Mapping(target = "auth.certificate", ignore = true) // no cert in the representation
@Mapping(target = "roles", source = "rolesStream", qualifiedByName = "getRoleStrings")
@Mapping(target = "serviceAccount.enabled", source = "serviceAccountsEnabled")
@Mapping(target = "serviceAccount.roles", source = "rolesStream", qualifiedByName = "getServiceAccountRoles")
@Override
ClientRepresentation fromModel(ClientModel model);
@Mapping(target = "serviceAccount.roles", source = ".", qualifiedByName = "getServiceAccountRoles")
@interface ModelToRep {
}

// we don't want to ignore nulls so that we completely overwrite the state
@Override
void toModel(@MappingTarget ClientModel model, ClientRepresentation rep, @Context RealmModel realm);
/*-------------------------------------*
* HELPER METHODS *
*-------------------------------------*/
@ObjectFactory
default ClientModel createClientModel(@Context RealmModel realm, ClientRepresentation rep) {
// dummy add/remove to obtain a detached model
var model = realm.addClient(rep.getClientId());
realm.removeClient(model.getId());
Comment thread
vmuzikar marked this conversation as resolved.
Resolved
return model;
}

@Named("isPublicClientPrimitive")
default boolean isPublicClientPrimitive(Boolean authEnabled) {
var result = isPublicClient(authEnabled);
return result != null ? result : false;
}

@Named("isPublicClient")
default Boolean isPublicClient(Boolean authEnabled) {
return authEnabled != null ? !authEnabled : null;
}

@Named("getRoleStrings")
default Set<String> getRoleStrings(Stream<RoleModel> stream) {
return stream.map(RoleModel::getName).collect(Collectors.toSet());
}

@Named("getServiceAccountRoles")
default Set<String> getServiceAccountRoles(Stream<RoleModel> stream) {
return stream.filter(f -> true) //TODO check roles for SA
.map(RoleModel::getName)
.collect(Collectors.toSet());
default Set<String> getServiceAccountRoles(@Context KeycloakSession session, ClientModel client) {
if (client.isServiceAccountsEnabled()) {
return session.users().getServiceAccount(client)
.getRoleMappingsStream()
.map(RoleModel::getName)
.collect(Collectors.toSet());
}
return Collections.emptySet();
}
}
55 changes: 55 additions & 0 deletions ...dmin-v2/api/src/main/java/org/keycloak/representations/admin/v2/ClientRepresentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.keycloak.representations.admin.v2;

import java.util.LinkedHashSet;
import java.util.Objects;
import java.util.Set;

import jakarta.validation.Valid;
Expand Down Expand Up @@ -211,6 +212,21 @@ public String getCertificate() {
public void setCertificate(String certificate) {
this.certificate = certificate;
}

@Override
public boolean equals(Object o) {
@shawkins shawkins Dec 2, 2025
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally we could use lombok for default equals and hashcode implementations.

@vmuzikar is there anything preventing us from doing that?

@vmuzikar vmuzikar Dec 3, 2025
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can do as a follow-up but not sure about introducing this dependency (if it's not there already as a transitive one).

@mabartos mabartos Dec 3, 2025
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I had the same idea :)

I'll try to check it.

@mabartos mabartos Dec 3, 2025 •
edited
Loading
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@vmuzikar @shawkins The Lombok dependency is not included, not even as a transitive one. Lombok does not have the Apache 2.0 license, and no license exception on the CNCF side either.

So, probably not a way forward.

@shawkins shawkins Dec 3, 2025
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The way we'd use Lombok is as a provided dependency only for source processing. It will not be included in the runtime.

@mabartos mabartos Dec 3, 2025 •
edited
Loading
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@shawkins Ok, that's right. AFAIK, CNCF might probably not restrict this.

if (this == o) return true;
if (!(o instanceof Auth auth)) return false;
return Objects.equals(enabled, auth.enabled)
&& Objects.equals(method, auth.method)
&& Objects.equals(secret, auth.secret)
&& Objects.equals(certificate, auth.certificate);
}

@Override
public int hashCode() {
return Objects.hash(enabled, method, secret, certificate);
}
}

@JsonInclude(JsonInclude.Include.NON_ABSENT)
Expand Down Expand Up @@ -238,5 +254,44 @@ public Set<String> getRoles() {
public void setRoles(Set<String> roles) {
this.roles = roles;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof ServiceAccount that)) return false;
return Objects.equals(enabled, that.enabled)
&& Objects.equals(roles, that.roles);
}

@Override
public int hashCode() {
return Objects.hash(enabled, roles);
}
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ClientRepresentation that = (ClientRepresentation) o;
return Objects.equals(clientId, that.clientId)
&& Objects.equals(displayName, that.displayName)
&& Objects.equals(description, that.description)
&& Objects.equals(protocol, that.protocol)
&& Objects.equals(enabled, that.enabled)
&& Objects.equals(appUrl, that.appUrl)
&& Objects.equals(appRedirectUrls, that.appRedirectUrls)
&& Objects.equals(loginFlows, that.loginFlows)
&& Objects.equals(auth, that.auth)
&& Objects.equals(webOrigins, that.webOrigins)
&& Objects.equals(roles, that.roles)
&& Objects.equals(serviceAccount, that.serviceAccount);
}

@Override
public int hashCode() {
return Objects.hash(clientId, displayName, description, protocol, enabled, appUrl, appRedirectUrls,
loginFlows, auth, webOrigins, roles, serviceAccount
);
}
}
8 changes: 3 additions & 5 deletions rest/admin-v2/api/src/main/java/org/keycloak/services/client/ClientService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
import org.keycloak.representations.admin.v2.ClientRepresentation;
import org.keycloak.services.Service;
import org.keycloak.services.ServiceException;
import org.keycloak.services.resources.admin.ClientResource;
import org.keycloak.services.resources.admin.ClientsResource;

public interface ClientService extends Service {

Expand All @@ -29,12 +27,12 @@ class ClientSortAndSliceOptions {

record CreateOrUpdateResult(ClientRepresentation representation, boolean created) {}

Optional<ClientRepresentation> getClient(ClientResource clientResource, RealmModel realm, String clientId, ClientProjectionOptions projectionOptions);
Optional<ClientRepresentation> getClient(RealmModel realm, String clientId, ClientProjectionOptions projectionOptions);

Stream<ClientRepresentation> getClients(ClientsResource clientsResource, RealmModel realm, ClientProjectionOptions projectionOptions, ClientSearchOptions searchOptions, ClientSortAndSliceOptions sortAndSliceOptions);
Stream<ClientRepresentation> getClients(RealmModel realm, ClientProjectionOptions projectionOptions, ClientSearchOptions searchOptions, ClientSortAndSliceOptions sortAndSliceOptions);

Stream<ClientRepresentation> deleteClients(RealmModel realm, ClientSearchOptions searchOptions);

CreateOrUpdateResult createOrUpdate(ClientsResource clientsResource, ClientResource clientResource, RealmModel realm, ClientRepresentation client, boolean allowUpdate) throws ServiceException;
CreateOrUpdateResult createOrUpdate(RealmModel realm, ClientRepresentation client, boolean allowUpdate) throws ServiceException;

}
Loading
Loading
0