Skip to content

Fixes user to group syncing #160

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

Open
edwardofclt wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fixes user to group syncing #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9c20978
try this copilot fix
edwardofclt Mar 31, 2025
e6e18e2
store artifact
edwardofclt Mar 31, 2025
98f794a
log the group members
Mar 31, 2025
08b22e3
try this
edwardofclt Mar 31, 2025
aa581d1
log this no matter what
edwardofclt Mar 31, 2025
1210a2f
this seemingly works!
edwardofclt Apr 1, 2025
fff67fe
put workflow back
edwardofclt Apr 1, 2025
37829ce
put it back
edwardofclt Apr 1, 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
234 changes: 181 additions & 53 deletions src/main/java/sh/libre/scim/core/GroupAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ public void apply(GroupModel group) {
.getGroupMembersStream(session.getContext().getRealm(), group)
.map(x -> x.getId())
.collect(Collectors.toSet());
LOGGER.info(String.format("Collected %d members for group %s (id=%s)",
this.members.size(), group.getName(), group.getId()));
this.skip = StringUtils.equals(group.getFirstAttribute("scim-skip"), "true");
}

Expand All @@ -61,14 +63,30 @@ public void apply(Group group) {
setExternalId(group.getId().get());
setDisplayName(group.getDisplayName().get());
var groupMembers = group.getMembers();
this.members = new HashSet<String>();
if (groupMembers != null && groupMembers.size() > 0) {
this.members = new HashSet<String>();
LOGGER.info(String.format("Processing %d incoming members for group %s",
groupMembers.size(), getDisplayName()));
for (var groupMember : groupMembers) {
var userMapping = this.query("findByExternalId", groupMember.getValue().get(), "User")
.getSingleResult();
this.members.add(userMapping.getId());
try {
String memberValue = groupMember.getValue().get();
LOGGER.info(String.format("Looking up user with externalId: %s", memberValue));
var userMapping = this.query("findByExternalId", memberValue, "User")
.getSingleResult();
LOGGER.info(String.format("Found user mapping: id=%s, externalId=%s",
userMapping.getId(), userMapping.getExternalId()));
this.members.add(userMapping.getId());
} catch (NoResultException e) {
LOGGER.error(String.format("No user mapping found for externalId: %s",
groupMember.getValue().get()));
Comment on lines +79 to +81
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 1, 2025

Choose a reason for hiding this comment

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

medium

The NoResultException is caught, but only an error is logged. Consider adding more context to the log message, or potentially retrying the operation, or even skipping the user and logging a warning if the user is not found. Also, consider using a more specific exception type if possible.

} catch (Exception e) {
LOGGER.error(String.format("Failed to process incoming group member: %s - Error: %s",
groupMember.getValue().get(), e.getMessage()), e);
}
}
}
LOGGER.info(String.format("Processed incoming SCIM group %s with %d mapped members",
getDisplayName(), this.members.size()));
}

@Override
Expand All @@ -77,28 +95,42 @@ public Group toSCIM(Boolean addMeta) {
group.setId(externalId);
group.setExternalId(id);
group.setDisplayName(displayName);
if (members.size() > 0) {
var groupMembers = new ArrayList<Member>();
for (var member : members) {
var groupMember = new Member();
try {
var userMapping = this.query("findById", member, "User").getSingleResult();
groupMember.setValue(userMapping.getExternalId());
var ref = new URI(String.format("Users/%s", userMapping.getExternalId()));
groupMember.setRef(ref.toString());
groupMembers.add(groupMember);
} catch (Exception e) {
LOGGER.error(e);

List<Member> groupMembers = new ArrayList<>();
LOGGER.info(String.format("Processing %d members for SCIM group %s", members.size(), displayName));

for (String memberId : members) {
try {
// Try to ensure the user has a mapping
String externalId = ensureUserMapping(memberId);

if (externalId == null) {
LOGGER.error(String.format("Could not get or create mapping for user %s, skipping", memberId));
continue;
Comment on lines +107 to +109
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 1, 2025

Choose a reason for hiding this comment

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

high

If externalId is null, the code logs an error and continues. It might be better to throw an exception or return an error to indicate that the mapping failed. This would allow the calling method to handle the error appropriately.

}

LOGGER.info(String.format("Adding member with externalId %s to group %s", externalId, displayName));
var groupMember = new Member();
groupMember.setValue(externalId);
groupMember.setType("User");
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 1, 2025

Choose a reason for hiding this comment

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

medium

Why is the type hardcoded to User? Should this be configurable, or is it always the case that the members are users?

var ref = new URI(String.format("Users/%s", externalId));
groupMember.setRef(ref.toString());
groupMembers.add(groupMember);
} catch (Exception e) {
LOGGER.error("Failed to process group member: " + memberId, e);
}
group.setMembers(groupMembers);
}

group.setMembers(groupMembers);
LOGGER.info(String.format("Final SCIM group %s has %d members", displayName, groupMembers.size()));

if (addMeta) {
var meta = new Meta();
try {
var uri = new URI("Groups/" + externalId);
meta.setLocation(uri.toString());
} catch (URISyntaxException e) {
LOGGER.error("Failed to create meta URI", e);
}
group.setMeta(meta);
}
Expand Down Expand Up @@ -131,15 +163,20 @@ public Boolean tryToMap() {
public void createEntity() {
var group = session.groups().createGroup(realm, displayName);
this.id = group.getId();
LOGGER.info(String.format("Created new group: %s (id=%s)", displayName, this.id));

for (String mId : members) {
try {
LOGGER.info(String.format("Attempting to add user with id=%s to group %s", mId, displayName));
var user = session.users().getUserById(realm, mId);
if (user == null) {
throw new NoResultException();
LOGGER.warn(String.format("User with id=%s not found in Keycloak", mId));
continue;
}
LOGGER.info(String.format("Adding user %s to group %s", user.getUsername(), displayName));
user.joinGroup(group);
} catch (Exception e) {
LOGGER.warn(e);
LOGGER.warn(String.format("Failed to add user with id=%s to group: %s", mId, e.getMessage()), e);
}
}
}
Expand All @@ -158,42 +195,133 @@ public Boolean skipRefresh() {
public PatchBuilder<Group> toPatchBuilder(ScimRequestBuilder scimRequestBuilder, String url) {
List<Member> groupMembers = new ArrayList<>();
PatchBuilder<Group> patchBuilder;
patchBuilder = scimRequestBuilder.patch(url, Group.class);
if (members.size() > 0) {
for (String member : members) {
var userMapping = this.query("findById", member, "User").getSingleResult();
groupMembers.add(Member.builder().value(userMapping.getExternalId()).build());
try {
LOGGER.info(String.format("Creating PATCH request to URL: %s", url));
patchBuilder = scimRequestBuilder.patch(url, Group.class);

if (members.size() > 0) {
for (String member : members) {
try {
LOGGER.info(String.format("Looking for member with id: %s", member));

// Try to ensure the user has a mapping
String externalId = ensureUserMapping(member);

if (externalId == null) {
LOGGER.error(
String.format("Could not get or create mapping for user %s, skipping", member));
continue;
Comment on lines +210 to +213
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 1, 2025

Choose a reason for hiding this comment

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

high

If externalId is null, the code logs an error and continues. It might be better to throw an exception or return an error to indicate that the mapping failed. This would allow the calling method to handle the error appropriately.

}

LOGGER.info(String.format("Using externalId %s for user %s", externalId, member));

Member memberNode = Member.builder()
.value(externalId)
.type("User")
.build();
groupMembers.add(memberNode);
LOGGER.info(String.format("Added member %s to PATCH request", externalId));
} catch (Exception e) {
LOGGER.error(String.format("Failed to process member %s: %s - Stack trace: ",
member, e.getMessage()), e);
}
}

// Debug the members being sent
LOGGER.info(String.format("Adding %d members to PATCH request", groupMembers.size()));

patchBuilder.addOperation()
.path("members")
.op(PatchOp.REPLACE)
.valueNodes(groupMembers)
.next()
.op(PatchOp.REPLACE)
.path("displayName")
.value(displayName)
.next()
.op(PatchOp.REPLACE)
.path("externalId")
.value(id)
.build();
} else {
LOGGER.info("No members to add, using REMOVE operation");
patchBuilder.addOperation()
.path("members")
.op(PatchOp.REMOVE)
.value(null)
.next()
.op(PatchOp.REPLACE)
.path("displayName")
.value(displayName)
.next()
.op(PatchOp.REPLACE)
.path("externalId")
.value(id)
.build();
}
patchBuilder.addOperation()
.path("members")
.op(PatchOp.REPLACE)
.valueNodes(groupMembers)
.next()
.op(PatchOp.REPLACE)
.path("displayName")
.value(displayName)
.next()
.op(PatchOp.REPLACE)
.path("externalId")
.value(id)
.build();
} else {
patchBuilder.addOperation()
.path("members")
.op(PatchOp.REMOVE)
.value(null)
.next()
.op(PatchOp.REPLACE)
.path("displayName")
.value(displayName)
.next()
.op(PatchOp.REPLACE)
.path("externalId")
.value(id)
.build();

// Log the entire request for debugging
LOGGER.info("Final PATCH request payload: " + patchBuilder.getResource());
return patchBuilder;

} catch (Exception e) {
LOGGER.error(String.format("Failed to create patch request to %s: %s", url, e.getMessage()), e);
throw e;
}
}

/**
* Ensures a user has a SCIM mapping, creating one if needed
*
* @param userId The Keycloak user ID
* @return The external SCIM ID for the user, or null if mapping failed
*/
private String ensureUserMapping(String userId) {
try {
// First try to get the existing mapping
LOGGER.info(String.format("Checking if user %s already has a SCIM mapping", userId));
var userMappingQuery = this.query("findById", userId, "User");
try {
var existingMapping = userMappingQuery.getSingleResult();
LOGGER.info(String.format("Found existing user mapping: id=%s, externalId=%s",
existingMapping.getId(), existingMapping.getExternalId()));
return existingMapping.getExternalId();
} catch (NoResultException e) {
// No mapping found, need to create one
LOGGER.info(String.format("No SCIM mapping found for user %s, creating one", userId));

// Get the user from Keycloak
var user = session.users().getUserById(realm, userId);
if (user == null) {
LOGGER.error(String.format("Cannot create mapping: User %s not found in Keycloak", userId));
return null;
}

// Create a new UserAdapter to handle the mapping
LOGGER.info(String.format("Creating UserAdapter for user %s (%s)", userId, user.getUsername()));
try {
// Create a new User adapter and generate a mapping
var userAdapter = new UserAdapter(session, this.componentId);
userAdapter.setId(userId);

// Generate a new externalId (UUID) for this user
String externalId = java.util.UUID.randomUUID().toString();
userAdapter.setExternalId(externalId);

// Persist the mapping
LOGGER.info(String.format("Persisting new user mapping: id=%s, externalId=%s", userId, externalId));
userAdapter.saveMapping();

return externalId;
} catch (Exception ex) {
LOGGER.error(String.format("Failed to create user mapping for %s: %s", userId, ex.getMessage()),
ex);
return null;
}
}
LOGGER.info(patchBuilder.getResource());
return patchBuilder;
} catch (Exception e) {
LOGGER.error(String.format("Error ensuring user mapping for %s: %s", userId, e.getMessage()), e);
return null;
}
}
}
5 changes: 4 additions & 1 deletion src/main/java/sh/libre/scim/core/ScimClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,9 @@ public <M extends RoleMapperModel, S extends ResourceNode, A extends Adapter<M,
LOGGER.info("Import");
try {
var adapter = getAdapter(aClass);
ServerResponse<ListResponse<S>> response = scimRequestBuilder.list("url", adapter.getResourceClass()).get().sendRequest();
String endpointPath = "/" + adapter.getSCIMEndpoint();
LOGGER.infof("Importing resources from %s", endpointPath);
ServerResponse<ListResponse<S>> response = scimRequestBuilder.list(endpointPath, adapter.getResourceClass()).get().sendRequest();
ListResponse<S> resourceTypeListResponse = response.getResource();

for (var resource : resourceTypeListResponse.getListedResources()) {
Expand Down Expand Up @@ -311,6 +313,7 @@ public <M extends RoleMapperModel, S extends ResourceNode, A extends Adapter<M,
}
}
} catch (ResponseException e) {
LOGGER.error("Error during import: " + e.getMessage(), e);
throw new RuntimeException(e);
}
}
Expand Down
35 changes: 18 additions & 17 deletions src/main/java/sh/libre/scim/core/UserAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import de.captaingoldfish.scim.sdk.common.resources.multicomplex.PersonRole;
import de.captaingoldfish.scim.sdk.common.resources.complex.Meta;


import org.apache.commons.lang3.StringUtils;
import org.jboss.logging.Logger;
import org.keycloak.models.KeycloakSession;
Expand Down Expand Up @@ -164,15 +163,15 @@ public User toSCIM(Boolean addMeta) {
var emails = new ArrayList<Email>();
if (email != null) {
emails.add(
Email.builder().value(getEmail()).build());
Email.builder().value(getEmail()).build());
}
user.setEmails(emails);
user.setActive(active);
if (addMeta) {
var meta = new Meta();
try {
var uri = new URI("Users/" + externalId);
meta.setLocation(uri.toString());
meta.setLocation(uri.toString());
} catch (URISyntaxException e) {
}
user.setMeta(meta);
Expand Down Expand Up @@ -238,35 +237,37 @@ public Boolean tryToMap() {

@Override
public Stream<UserModel> getResourceStream() {
return this.session.users().searchForUserStream(this.session.getContext().getRealm(), Map.of(UserModel.ENABLED, "true"));
return this.session.users().searchForUserStream(this.session.getContext().getRealm(),
Map.of(UserModel.ENABLED, "true"));
}

@Override
public Boolean skipRefresh() {
return "admin".equals(getUsername());
}

@Override
public PatchBuilder<User> toPatchBuilder(ScimRequestBuilder scimRequestBuilder, String url) {
var emails = new ArrayList<Email>();
if (email != null) {
emails.add(
Email.builder().value(getEmail()).build());
Email.builder().value(getEmail()).build());
}
PatchBuilder<User> patchBuilder;
patchBuilder = scimRequestBuilder.patch(url, User.class);
patchBuilder.addOperation()
.path("active")
.op(PatchOp.REPLACE)
.value(active.toString())
.next()
.path("userName")
.op(PatchOp.REPLACE)
.value(username)
.next()
.path("displayName")
.op(PatchOp.REPLACE)
.value(displayName)
.build();
.path("active")
.op(PatchOp.REPLACE)
.value(active.toString())
.next()
.path("userName")
.op(PatchOp.REPLACE)
.value(username)
.next()
.path("displayName")
.op(PatchOp.REPLACE)
.value(displayName)
.build();

return patchBuilder;
}
Expand Down