ilgrosso commented on code in PR #1226: URL: https://github.com/apache/syncope/pull/1226#discussion_r2489551263
########## ext/scimv2/logic/src/main/java/org/apache/syncope/core/logic/SCIMDataBinder.java: ########## @@ -26,11 +26,13 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; Review Comment: We are not using `AtomicReference` for this purpose anymore in `4_0_X` but `org.apache.commons.lang3.mutable.Mutable` and `org.apache.commons.lang3.mutable.MutableObject` see https://github.com/apache/syncope/blob/4_0_X/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/AbstractPropagationTaskExecutor.java#L551 ########## ext/scimv2/logic/src/main/java/org/apache/syncope/core/logic/SCIMDataBinder.java: ########## @@ -835,206 +1167,231 @@ protected void setAttribute( } } - public Pair<UserUR, StatusR> toUserUpdate( + public Pair<List<UserUR>, StatusR> toUserUpdate( final UserTO before, - final Collection<String> resources, - final SCIMPatchOperation op) { - StatusR statusR = null; - - if (op.getPath() == null && op.getOp() != PatchOp.remove - && !CollectionUtils.isEmpty(op.getValue()) - && op.getValue().getFirst() instanceof final SCIMUser after) { - - if (after.getActive() != null && before.isSuspended() == after.isActive()) { - statusR = new StatusR.Builder( - before.getKey(), - after.isActive() ? StatusRType.REACTIVATE : StatusRType.SUSPEND). - resources(resources). - build(); - } - - UserTO updated = toUserTO(after, false); - updated.setKey(before.getKey()); - return Pair.of(AnyOperations.diff(updated, before, true), statusR); - } - + final SCIMPatchOp patch) { + final AtomicReference<StatusR> statusR = new AtomicReference<>(); + List<UserUR> userURs = new ArrayList<>(); UserUR userUR = new UserUR.Builder(before.getKey()).build(); + userURs.add(userUR); + List<String> resources = new ArrayList<>(before.getResources()); + AtomicInteger numberUR = new AtomicInteger(); + + patch.getOperations().forEach(op -> { + if (op.getPath() == null && op.getOp() != PatchOp.remove && !CollectionUtils.isEmpty(op.getValue()) + && op.getValue().getFirst() instanceof final SCIMUser after) { + + if (after.getActive() != null && before.isSuspended() == after.isActive()) { + statusR.set(new StatusR.Builder(before.getKey(), + after.isActive() ? StatusRType.REACTIVATE : StatusRType.SUSPEND).resources(resources) + .build()); + } - SCIMConf conf = confManager.get(); - if (conf == null) { - return Pair.of(userUR, statusR); - } - - switch (op.getPath().getAttribute()) { - case "externalId" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getExternalId(), op); - - case "userName" -> { - if (op.getOp() != PatchOp.remove && !CollectionUtils.isEmpty(op.getValue())) { - userUR.setUsername( - new StringReplacePatchItem.Builder().value(op.getValue().getFirst().toString()).build()); + if (!after.getGroups().isEmpty()) { + String groupKey = after.getGroups().getFirst().getValue(); + org.apache.syncope.core.persistence.api.entity.group.Group group = + groupDAO.findById(groupKey).orElse(null); + if (group != null && before.getMembership(groupKey).isEmpty()) { + List<ExternalResource> filteredResources = group.getResources().stream() + .filter(resource -> resource.getProvisions().stream() + .anyMatch(provision -> AnyTypeKind.USER.name().equals(provision.getAnyType()))) + .collect(Collectors.toList()); + filteredResources.forEach(resource -> resources.add(resource.getKey())); + if (!filteredResources.isEmpty()) { + UserUR newUserUR = new UserUR.Builder(before.getKey()).build(); + userURs.add(newUserUR); + numberUR.getAndIncrement(); + } + } } + populateUserUR(userURs.get(numberUR.get()), before, after, resources, op); + return; } - case "password" -> { - if (op.getOp() != PatchOp.remove && !CollectionUtils.isEmpty(op.getValue())) { - userUR.setPassword(new PasswordPatch.Builder().value(op.getValue().getFirst().toString()).resources( - resources).build()); - } + SCIMConf conf = confManager.get(); + if (conf == null) { + return; } - case "active" -> { - if (!CollectionUtils.isEmpty(op.getValue())) { + switch (op.getPath().getAttribute()) { + case "externalId" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getExternalId(), + op); - // Workaround for Microsoft Entra being not SCIM compliant on PATCH requests - if (op.getValue().getFirst() instanceof String a) { - op.setValue(List.of(BooleanUtils.toBoolean(a))); + case "userName" -> { + if (op.getOp() != PatchOp.remove && !CollectionUtils.isEmpty(op.getValue())) { + userURs.get(numberUR.get()).setUsername( + new StringReplacePatchItem.Builder().value(op.getValue().getFirst().toString()) + .build()); } - - statusR = new StatusR.Builder(before.getKey(), - (boolean) op.getValue().getFirst() - ? StatusRType.REACTIVATE - : StatusRType.SUSPEND).resources(resources).build(); } - } - case "name" -> { - if (conf.getUserConf().getName() != null) { - if (op.getPath().getSub() == null || "familyName".equals(op.getPath().getSub())) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getName().getFamilyName(), op); - } - if (op.getPath().getSub() == null || "formatted".equals(op.getPath().getSub())) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getName().getFormatted(), op); - } - if (op.getPath().getSub() == null || "givenName".equals(op.getPath().getSub())) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getName().getGivenName(), op); + case "password" -> { + if (op.getOp() != PatchOp.remove && !CollectionUtils.isEmpty(op.getValue())) { + userURs.get(numberUR.get()).setPassword( + new PasswordPatch.Builder().value(op.getValue().getFirst().toString()) + .resources(resources).build()); } - if (op.getPath().getSub() == null || "honorificPrefix".equals(op.getPath().getSub())) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getName().getHonorificPrefix(), op); - } - if (op.getPath().getSub() == null || "honorificSuffix".equals(op.getPath().getSub())) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getName().getHonorificSuffix(), op); + } + + case "active" -> { + if (!CollectionUtils.isEmpty(op.getValue())) { + + // Workaround for Microsoft Entra being not SCIM compliant on PATCH requests + if (op.getValue().getFirst() instanceof String a) { + op.setValue(List.of(BooleanUtils.toBoolean(a))); + } + + statusR.set(new StatusR.Builder(before.getKey(), (boolean) op.getValue().getFirst() + ? StatusRType.REACTIVATE + : StatusRType.SUSPEND).resources(resources).build()); } - if (op.getPath().getSub() == null || "middleName".equals(op.getPath().getSub())) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getName().getMiddleName(), op); + } + + case "name" -> { + if (conf.getUserConf().getName() != null) { + if (op.getPath().getSub() == null || "familyName".equals(op.getPath().getSub())) { + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), + conf.getUserConf().getName().getFamilyName(), op); + } + if (op.getPath().getSub() == null || "formatted".equals(op.getPath().getSub())) { + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), + conf.getUserConf().getName().getFormatted(), op); + } + if (op.getPath().getSub() == null || "givenName".equals(op.getPath().getSub())) { + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), + conf.getUserConf().getName().getGivenName(), op); + } + if (op.getPath().getSub() == null || "honorificPrefix".equals(op.getPath().getSub())) { + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), + conf.getUserConf().getName().getHonorificPrefix(), op); + } + if (op.getPath().getSub() == null || "honorificSuffix".equals(op.getPath().getSub())) { + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), + conf.getUserConf().getName().getHonorificSuffix(), op); + } + if (op.getPath().getSub() == null || "middleName".equals(op.getPath().getSub())) { + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), + conf.getUserConf().getName().getMiddleName(), op); + } } } - } - case "displayName" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getDisplayName(), op); + case "displayName" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getDisplayName(), + op); - case "nickName" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getNickName(), op); + case "nickName" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getNickName(), op); - case "profileUrl" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getProfileUrl(), op); + case "profileUrl" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getProfileUrl(), + op); - case "title" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getTitle(), op); + case "title" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getTitle(), op); - case "userType" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getUserType(), op); + case "userType" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getUserType(), op); - case "preferredLanguage" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getPreferredLanguage(), op); + case "preferredLanguage" -> setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), + conf.getUserConf().getPreferredLanguage(), op); - case "locale" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getLocale(), op); + case "locale" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getLocale(), op); - case "timezone" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getTimezone(), op); + case "timezone" -> + setAttribute(userURs.get(numberUR.get()).getPlainAttrs(), conf.getUserConf().getTimezone(), op); - case "emails" -> { - if (!CollectionUtils.isEmpty(op.getValue()) && op.getValue().getFirst() instanceof SCIMUser) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getEmails(), - ((SCIMUser) op.getValue().getFirst()).getEmails(), op.getOp()); - } else if (op.getPath().getFilter() != null) { - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getEmails(), op); + case "emails" -> { + if (!CollectionUtils.isEmpty(op.getValue()) && op.getValue().getFirst() instanceof SCIMUser) { Review Comment: Replace with ``` op.getValue().getFirst() instanceof SCIMUser scimUser ``` to avoid type cast Please also check all other `instanceof` usages ########## ext/scimv2/logic/src/main/java/org/apache/syncope/core/logic/SCIMDataBinder.java: ########## @@ -835,206 +1167,231 @@ protected void setAttribute( } } - public Pair<UserUR, StatusR> toUserUpdate( + public Pair<List<UserUR>, StatusR> toUserUpdate( final UserTO before, - final Collection<String> resources, - final SCIMPatchOperation op) { - StatusR statusR = null; - - if (op.getPath() == null && op.getOp() != PatchOp.remove - && !CollectionUtils.isEmpty(op.getValue()) - && op.getValue().getFirst() instanceof final SCIMUser after) { - - if (after.getActive() != null && before.isSuspended() == after.isActive()) { - statusR = new StatusR.Builder( - before.getKey(), - after.isActive() ? StatusRType.REACTIVATE : StatusRType.SUSPEND). - resources(resources). - build(); - } - - UserTO updated = toUserTO(after, false); - updated.setKey(before.getKey()); - return Pair.of(AnyOperations.diff(updated, before, true), statusR); - } - + final SCIMPatchOp patch) { + final AtomicReference<StatusR> statusR = new AtomicReference<>(); + List<UserUR> userURs = new ArrayList<>(); UserUR userUR = new UserUR.Builder(before.getKey()).build(); + userURs.add(userUR); + List<String> resources = new ArrayList<>(before.getResources()); + AtomicInteger numberUR = new AtomicInteger(); + + patch.getOperations().forEach(op -> { + if (op.getPath() == null && op.getOp() != PatchOp.remove && !CollectionUtils.isEmpty(op.getValue()) + && op.getValue().getFirst() instanceof final SCIMUser after) { + + if (after.getActive() != null && before.isSuspended() == after.isActive()) { + statusR.set(new StatusR.Builder(before.getKey(), + after.isActive() ? StatusRType.REACTIVATE : StatusRType.SUSPEND).resources(resources) + .build()); + } - SCIMConf conf = confManager.get(); - if (conf == null) { - return Pair.of(userUR, statusR); - } - - switch (op.getPath().getAttribute()) { - case "externalId" -> - setAttribute(userUR.getPlainAttrs(), conf.getUserConf().getExternalId(), op); - - case "userName" -> { - if (op.getOp() != PatchOp.remove && !CollectionUtils.isEmpty(op.getValue())) { - userUR.setUsername( - new StringReplacePatchItem.Builder().value(op.getValue().getFirst().toString()).build()); + if (!after.getGroups().isEmpty()) { + String groupKey = after.getGroups().getFirst().getValue(); + org.apache.syncope.core.persistence.api.entity.group.Group group = + groupDAO.findById(groupKey).orElse(null); + if (group != null && before.getMembership(groupKey).isEmpty()) { + List<ExternalResource> filteredResources = group.getResources().stream() + .filter(resource -> resource.getProvisions().stream() + .anyMatch(provision -> AnyTypeKind.USER.name().equals(provision.getAnyType()))) + .collect(Collectors.toList()); Review Comment: Could `collect(Collectors.toList())` be replaced by `toList()` e.g. is it the returned list requested to be mutable? Same for all other instances -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
