[SYNCOPE-1166] Cleaning up the generation of propagation update tasks with mixed resources
Project: http://git-wip-us.apache.org/repos/asf/syncope/repo Commit: http://git-wip-us.apache.org/repos/asf/syncope/commit/aa85d990 Tree: http://git-wip-us.apache.org/repos/asf/syncope/tree/aa85d990 Diff: http://git-wip-us.apache.org/repos/asf/syncope/diff/aa85d990 Branch: refs/heads/master Commit: aa85d990087bc16c3496f273629fb187a488bcfe Parents: a9cc9e7 Author: Francesco Chicchiriccò <ilgro...@apache.org> Authored: Fri Jul 28 17:54:59 2017 +0200 Committer: Francesco Chicchiriccò <ilgro...@apache.org> Committed: Fri Jul 28 18:03:02 2017 +0200 ---------------------------------------------------------------------- .../java/DefaultGroupProvisioningManager.java | 2 +- .../java/data/AbstractAnyDataBinder.java | 15 -------- .../java/data/AnyObjectDataBinderImpl.java | 28 ++------------ .../java/data/UserDataBinderImpl.java | 21 +--------- .../propagation/PropagationManagerImpl.java | 40 ++++++++++---------- .../java/AbstractAnyObjectWorkflowAdapter.java | 7 +++- .../java/AbstractGroupWorkflowAdapter.java | 7 +++- .../java/AbstractUserWorkflowAdapter.java | 7 +++- .../camel/producer/ProvisionProducer.java | 4 +- .../syncope/fit/console/AnyTypesITCase.java | 1 - .../apache/syncope/fit/core/DynRealmITCase.java | 3 +- .../syncope/fit/core/UserIssuesITCase.java | 28 ++++++++++++++ 12 files changed, 75 insertions(+), 88 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultGroupProvisioningManager.java ---------------------------------------------------------------------- diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultGroupProvisioningManager.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultGroupProvisioningManager.java index 2e213f7..d794e61 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultGroupProvisioningManager.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultGroupProvisioningManager.java @@ -108,7 +108,7 @@ public class DefaultGroupProvisioningManager implements GroupProvisioningManager excludedResources); PropagationReporter propagationReporter = taskExecutor.execute(tasks, nullPriorityAsync); - return new ImmutablePair<>(created.getResult(), null); + return new ImmutablePair<>(created.getResult(), propagationReporter.getStatuses()); } @Override http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AbstractAnyDataBinder.java ---------------------------------------------------------------------- diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AbstractAnyDataBinder.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AbstractAnyDataBinder.java index 108b530..c950a6f 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AbstractAnyDataBinder.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AbstractAnyDataBinder.java @@ -26,7 +26,6 @@ import java.util.List; import java.util.Map; import java.util.Set; import org.apache.commons.collections4.CollectionUtils; -import org.apache.commons.collections4.Predicate; import org.apache.commons.lang3.StringUtils; import org.apache.syncope.common.lib.SyncopeClientCompositeException; import org.apache.syncope.common.lib.SyncopeClientException; @@ -94,20 +93,6 @@ abstract class AbstractAnyDataBinder { protected static final Logger LOG = LoggerFactory.getLogger(AbstractAnyDataBinder.class); - protected static class PropByResContains implements Predicate<String> { - - private final PropagationByResource propByRes; - - PropByResContains(final PropagationByResource propByRes) { - this.propByRes = propByRes; - } - - @Override - public boolean evaluate(final String resourceKey) { - return !propByRes.contains(resourceKey); - } - } - @Autowired protected SchemaDataBinder schemaDataBinder; http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AnyObjectDataBinderImpl.java ---------------------------------------------------------------------- diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AnyObjectDataBinderImpl.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AnyObjectDataBinderImpl.java index 2620718..523d165 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AnyObjectDataBinderImpl.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AnyObjectDataBinderImpl.java @@ -25,7 +25,6 @@ import java.util.List; import java.util.Map; import java.util.Set; import org.apache.commons.collections4.CollectionUtils; -import org.apache.commons.collections4.IterableUtils; import org.apache.commons.collections4.Transformer; import org.apache.commons.lang3.StringUtils; import org.apache.syncope.common.lib.SyncopeClientCompositeException; @@ -131,13 +130,13 @@ public class AnyObjectDataBinderImpl extends AbstractAnyDataBinder implements An // dynamic memberships CollectionUtils.collect(anyObjectDAO.findDynGroups(anyObject.getKey()), - new Transformer<Group, MembershipTO>() { + new Transformer<Group, MembershipTO>() { @Override public MembershipTO transform(final Group group) { MembershipTO membershipTO = new MembershipTO.Builder(). - group(group.getKey(), group.getName()). - build(); + group(group.getKey(), group.getName()). + build(); return membershipTO; } @@ -449,26 +448,7 @@ public class AnyObjectDataBinderImpl extends AbstractAnyDataBinder implements An } } - // finally double-check that there are no resources owned (after all changes above) that remain - // not considered for provisioning - anyObject = anyObjectDAO.save(anyObject); - PropByResContains propByResContains = new PropByResContains(propByRes); - Collection<String> prospectResources = anyObjectDAO.findAllResourceKeys(anyObject.getKey()); - for (String resourceKey : IterableUtils.filteredIterable( - CollectionUtils.intersection(currentResources, prospectResources), propByResContains)) { - - propByRes.add(ResourceOperation.DELETE, resourceKey); - } - for (String resourceKey : IterableUtils.filteredIterable( - CollectionUtils.intersection(prospectResources, currentResources), propByResContains)) { - - propByRes.add(ResourceOperation.CREATE, resourceKey); - } - for (String resourceKey : IterableUtils.filteredIterable( - CollectionUtils.intersection(prospectResources, currentResources), propByResContains)) { - - propByRes.add(ResourceOperation.UPDATE, resourceKey); - } + anyObjectDAO.save(anyObject); // Throw composite exception if there is at least one element set in the composing exceptions if (scce.hasExceptions()) { http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java ---------------------------------------------------------------------- diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java index 7c291c0..e9369c7 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java @@ -551,26 +551,7 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat } } - // finally double-check that there are no resources owned (after all changes above) that remain - // not considered for provisioning - user = userDAO.save(user); - PropByResContains propByResContains = new PropByResContains(propByRes); - Collection<String> prospectResources = userDAO.findAllResourceKeys(user.getKey()); - for (String resourceKey : IterableUtils.filteredIterable( - CollectionUtils.intersection(currentResources, prospectResources), propByResContains)) { - - propByRes.add(ResourceOperation.DELETE, resourceKey); - } - for (String resourceKey : IterableUtils.filteredIterable( - CollectionUtils.intersection(prospectResources, currentResources), propByResContains)) { - - propByRes.add(ResourceOperation.CREATE, resourceKey); - } - for (String resourceKey : IterableUtils.filteredIterable( - CollectionUtils.intersection(prospectResources, currentResources), propByResContains)) { - - propByRes.add(ResourceOperation.UPDATE, resourceKey); - } + userDAO.save(user); // Throw composite exception if there is at least one element set in the composing exceptions if (scce.hasExceptions()) { http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/PropagationManagerImpl.java ---------------------------------------------------------------------- diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/PropagationManagerImpl.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/PropagationManagerImpl.java index c826f6b..6b459d5 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/PropagationManagerImpl.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/PropagationManagerImpl.java @@ -199,7 +199,7 @@ public class PropagationManagerImpl implements PropagationManager { final Collection<String> noPropResourceKeys) { return getUpdateTasks( - userDAO.authFind(wfResult.getResult().getKey().getKey()), + userDAO.authFind(wfResult.getResult().getLeft().getKey()), wfResult.getResult().getKey().getPassword() == null ? null : wfResult.getResult().getKey().getPassword().getValue(), @@ -222,34 +222,36 @@ public class PropagationManagerImpl implements PropagationManager { } else { // b. generate the propagation task list in two phases: first the ones containing password, // the the rest (with no password) - PropagationByResource origPropByRes = new PropagationByResource(); - origPropByRes.merge(wfResult.getPropByRes()); + WorkflowResult<Pair<UserPatch, Boolean>> pwdWFResult = new WorkflowResult<>( + wfResult.getResult(), new PropagationByResource(), wfResult.getPerformedTasks()); Set<String> pwdResourceNames = new HashSet<>(userPatch.getPassword().getResources()); - Collection<String> currentResourceNames = userDAO.findAllResourceKeys(userPatch.getKey()); - pwdResourceNames.retainAll(currentResourceNames); - PropagationByResource pwdPropByRes = new PropagationByResource(); - pwdPropByRes.addAll(ResourceOperation.UPDATE, pwdResourceNames); - if (!pwdPropByRes.isEmpty()) { - Set<String> toBeExcluded = new HashSet<>(currentResourceNames); - toBeExcluded.addAll(CollectionUtils.collect(userPatch.getResources(), - new Transformer<StringPatchItem, String>() { + Collection<String> allResourceNames = userDAO.findAllResourceKeys(userPatch.getKey()); + pwdResourceNames.retainAll(allResourceNames); + + pwdWFResult.getPropByRes().addAll(ResourceOperation.UPDATE, pwdResourceNames); + if (!pwdWFResult.getPropByRes().isEmpty()) { + Set<String> toBeExcluded = new HashSet<>(allResourceNames); + CollectionUtils.collect(userPatch.getResources(), new Transformer<StringPatchItem, String>() { @Override public String transform(final StringPatchItem input) { return input.getValue(); } - })); + }, toBeExcluded); toBeExcluded.removeAll(pwdResourceNames); - tasks.addAll(getUserUpdateTasks(wfResult, true, toBeExcluded)); + + tasks.addAll(getUserUpdateTasks(pwdWFResult, true, toBeExcluded)); } - PropagationByResource nonPwdPropByRes = new PropagationByResource(); - nonPwdPropByRes.merge(origPropByRes); - nonPwdPropByRes.removeAll(pwdResourceNames); - nonPwdPropByRes.purge(); - if (!nonPwdPropByRes.isEmpty()) { - tasks.addAll(getUserUpdateTasks(wfResult, false, pwdResourceNames)); + WorkflowResult<Pair<UserPatch, Boolean>> noPwdWFResult = new WorkflowResult<>( + wfResult.getResult(), new PropagationByResource(), wfResult.getPerformedTasks()); + + noPwdWFResult.getPropByRes().merge(wfResult.getPropByRes()); + noPwdWFResult.getPropByRes().removeAll(pwdResourceNames); + noPwdWFResult.getPropByRes().purge(); + if (!noPwdWFResult.getPropByRes().isEmpty()) { + tasks.addAll(getUserUpdateTasks(noPwdWFResult, false, pwdResourceNames)); } } http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractAnyObjectWorkflowAdapter.java ---------------------------------------------------------------------- diff --git a/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractAnyObjectWorkflowAdapter.java b/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractAnyObjectWorkflowAdapter.java index 232470a..dcc2e82 100644 --- a/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractAnyObjectWorkflowAdapter.java +++ b/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractAnyObjectWorkflowAdapter.java @@ -60,7 +60,12 @@ public abstract class AbstractAnyObjectWorkflowAdapter @Override public WorkflowResult<String> update(final AnyObjectPatch anyObjectPatch) { - return doUpdate(anyObjectDAO.authFind(anyObjectPatch.getKey()), anyObjectPatch); + WorkflowResult<String> result = doUpdate(anyObjectDAO.authFind(anyObjectPatch.getKey()), anyObjectPatch); + + // re-read to ensure that requester's administration rights are still valid + anyObjectDAO.authFind(anyObjectPatch.getKey()); + + return result; } protected abstract void doDelete(AnyObject anyObject); http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractGroupWorkflowAdapter.java ---------------------------------------------------------------------- diff --git a/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractGroupWorkflowAdapter.java b/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractGroupWorkflowAdapter.java index 0689b94..3f5394b 100644 --- a/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractGroupWorkflowAdapter.java +++ b/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractGroupWorkflowAdapter.java @@ -59,7 +59,12 @@ public abstract class AbstractGroupWorkflowAdapter implements GroupWorkflowAdapt @Override public WorkflowResult<String> update(final GroupPatch groupPatch) { - return doUpdate(groupDAO.authFind(groupPatch.getKey()), groupPatch); + WorkflowResult<String> result = doUpdate(groupDAO.authFind(groupPatch.getKey()), groupPatch); + + // re-read to ensure that requester's administration rights are still valid + groupDAO.authFind(groupPatch.getKey()); + + return result; } protected abstract void doDelete(Group group); http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractUserWorkflowAdapter.java ---------------------------------------------------------------------- diff --git a/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractUserWorkflowAdapter.java b/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractUserWorkflowAdapter.java index d112934..8f3b9c7 100644 --- a/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractUserWorkflowAdapter.java +++ b/core/workflow-java/src/main/java/org/apache/syncope/core/workflow/java/AbstractUserWorkflowAdapter.java @@ -102,7 +102,12 @@ public abstract class AbstractUserWorkflowAdapter implements UserWorkflowAdapter @Override public WorkflowResult<Pair<UserPatch, Boolean>> update(final UserPatch userPatch) { - return doUpdate(userDAO.authFind(userPatch.getKey()), userPatch); + WorkflowResult<Pair<UserPatch, Boolean>> result = doUpdate(userDAO.authFind(userPatch.getKey()), userPatch); + + // re-read to ensure that requester's administration rights are still valid + userDAO.authFind(userPatch.getKey()); + + return result; } protected abstract WorkflowResult<String> doSuspend(User user); http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/ext/camel/provisioning-camel/src/main/java/org/apache/syncope/core/provisioning/camel/producer/ProvisionProducer.java ---------------------------------------------------------------------- diff --git a/ext/camel/provisioning-camel/src/main/java/org/apache/syncope/core/provisioning/camel/producer/ProvisionProducer.java b/ext/camel/provisioning-camel/src/main/java/org/apache/syncope/core/provisioning/camel/producer/ProvisionProducer.java index 182a459..bd58a36 100644 --- a/ext/camel/provisioning-camel/src/main/java/org/apache/syncope/core/provisioning/camel/producer/ProvisionProducer.java +++ b/ext/camel/provisioning-camel/src/main/java/org/apache/syncope/core/provisioning/camel/producer/ProvisionProducer.java @@ -71,9 +71,7 @@ public class ProvisionProducer extends AbstractProducer { } PropagationByResource propByRes = new PropagationByResource(); - for (String resource : resources) { - propByRes.add(ResourceOperation.UPDATE, resource); - } + propByRes.addAll(ResourceOperation.UPDATE, resources); WorkflowResult<Pair<UserPatch, Boolean>> wfResult = new WorkflowResult<Pair<UserPatch, Boolean>>( ImmutablePair.of(userPatch, (Boolean) null), propByRes, "update"); http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/fit/core-reference/src/test/java/org/apache/syncope/fit/console/AnyTypesITCase.java ---------------------------------------------------------------------- diff --git a/fit/core-reference/src/test/java/org/apache/syncope/fit/console/AnyTypesITCase.java b/fit/core-reference/src/test/java/org/apache/syncope/fit/console/AnyTypesITCase.java index 35f417d..3125d26 100644 --- a/fit/core-reference/src/test/java/org/apache/syncope/fit/console/AnyTypesITCase.java +++ b/fit/core-reference/src/test/java/org/apache/syncope/fit/console/AnyTypesITCase.java @@ -18,7 +18,6 @@ */ package org.apache.syncope.fit.console; -import static org.apache.syncope.fit.console.AbstractConsoleITCase.TESTER; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/fit/core-reference/src/test/java/org/apache/syncope/fit/core/DynRealmITCase.java ---------------------------------------------------------------------- diff --git a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/DynRealmITCase.java b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/DynRealmITCase.java index 88d08f9..7d745a7 100644 --- a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/DynRealmITCase.java +++ b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/DynRealmITCase.java @@ -185,8 +185,7 @@ public class DynRealmITCase extends AbstractITCase { userPatch.setKey(userKey); userPatch.getResources().add(new StringPatchItem.Builder(). value(RESOURCE_NAME_LDAP).operation(PatchOperation.DELETE).build()); - // this will fail because unassigning resource-ldap would result in removing the user - // from the dynamic realm + // this will fail because unassigning resource-ldap would result in removing the user from the dynamic realm try { delegatedUserService.update(userPatch); fail(); http://git-wip-us.apache.org/repos/asf/syncope/blob/aa85d990/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java ---------------------------------------------------------------------- diff --git a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java index 4e7988a..f60c4a6 100644 --- a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java +++ b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java @@ -1357,4 +1357,32 @@ public class UserIssuesITCase extends AbstractITCase { assertFalse(created.getPropagationStatuses().isEmpty()); assertEquals(RESOURCE_NAME_TESTDB, created.getPropagationStatuses().get(0).getResource()); } + + @Test + public void issueSYNCOPE1166() { + UserTO userTO = UserITCase.getUniqueSampleTO("syncope1...@apache.org"); + userTO = createUser(userTO).getEntity(); + assertNotNull(userTO); + + UserPatch userPatch = new UserPatch(); + userPatch.setKey(userTO.getKey()); + // resource-ldap has password mapped, resource-db-virattr does not + userPatch.setPassword(new PasswordPatch.Builder(). + onSyncope(true). + resource(RESOURCE_NAME_LDAP). + value("new2Password").build()); + + userPatch.getResources().add(new StringPatchItem.Builder(). + operation(PatchOperation.ADD_REPLACE).value(RESOURCE_NAME_LDAP).build()); + userPatch.getResources().add(new StringPatchItem.Builder(). + operation(PatchOperation.ADD_REPLACE).value(RESOURCE_NAME_DBVIRATTR).build()); + + ProvisioningResult<UserTO> result = updateUser(userPatch); + assertNotNull(result); + assertEquals(2, result.getPropagationStatuses().size()); + assertEquals(RESOURCE_NAME_LDAP, result.getPropagationStatuses().get(0).getResource()); + assertEquals(PropagationTaskExecStatus.SUCCESS, result.getPropagationStatuses().get(0).getStatus()); + assertEquals(RESOURCE_NAME_DBVIRATTR, result.getPropagationStatuses().get(1).getResource()); + assertEquals(PropagationTaskExecStatus.SUCCESS, result.getPropagationStatuses().get(1).getStatus()); + } }
