You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@syncope.apache.org by il...@apache.org on 2012/08/29 17:06:35 UTC
svn commit: r1378601 - in /incubator/syncope/branches/1_0_X/core/src:
main/java/org/apache/syncope/core/persistence/validation/entity/
main/java/org/apache/syncope/core/propagation/
test/java/org/apache/syncope/core/rest/
Author: ilgrosso
Date: Wed Aug 29 15:06:35 2012
New Revision: 1378601
URL: http://svn.apache.org/viewvc?rev=1378601&view=rev
Log:
[SYNCOPE-185] Cleaning up the conversion from a DELETE PropagationTask to an actual delete() or update() on the external resource
Modified:
incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/persistence/validation/entity/PropagationTaskValidator.java
incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationByResource.java
incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationManager.java
incubator/syncope/branches/1_0_X/core/src/test/java/org/apache/syncope/core/rest/UserTestITCase.java
Modified: incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/persistence/validation/entity/PropagationTaskValidator.java
URL: http://svn.apache.org/viewvc/incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/persistence/validation/entity/PropagationTaskValidator.java?rev=1378601&r1=1378600&r2=1378601&view=diff
==============================================================================
--- incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/persistence/validation/entity/PropagationTaskValidator.java (original)
+++ incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/persistence/validation/entity/PropagationTaskValidator.java Wed Aug 29 15:06:35 2012
@@ -24,7 +24,6 @@ import javax.validation.ConstraintValida
import org.apache.syncope.core.persistence.beans.PropagationTask;
import org.apache.syncope.core.persistence.beans.TaskExec;
import org.apache.syncope.types.EntityViolationType;
-import org.apache.syncope.types.PropagationOperation;
import org.apache.syncope.types.PropagationTaskExecStatus;
public class PropagationTaskValidator extends AbstractValidator implements
@@ -45,8 +44,7 @@ public class PropagationTaskValidator ex
isValid = object.getPropagationMode() != null
&& object.getPropagationOperation() != null
&& !object.getAttributes().isEmpty()
- && object.getResource() != null
- && (PropagationOperation.DELETE == object.getPropagationOperation() || object.getSyncopeUser() != null);
+ && object.getResource() != null;
if (isValid) {
List<TaskExec> executions = object.getExecs();
Modified: incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationByResource.java
URL: http://svn.apache.org/viewvc/incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationByResource.java?rev=1378601&r1=1378600&r2=1378601&view=diff
==============================================================================
--- incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationByResource.java (original)
+++ incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationByResource.java Wed Aug 29 15:06:35 2012
@@ -27,8 +27,7 @@ import java.util.Set;
import org.apache.syncope.types.PropagationOperation;
/**
- * Utility class for encapsulating operations to be performed on various
- * resources.
+ * Utility class for encapsulating operations to be performed on various resources.
*/
public class PropagationByResource implements Serializable {
@@ -66,9 +65,8 @@ public class PropagationByResource imple
}
/**
- * Avoid potential conflicts by not doing create or update on any
- * resource for which a delete is requested, and by not doing any create
- * on any resource for which an update is requested.
+ * Avoid potential conflicts by not doing create or update on any resource for which a delete is requested, and by
+ * not doing any create on any resource for which an update is requested.
*/
public final void purge() {
toBeCreated.removeAll(toBeDeleted);
@@ -85,7 +83,6 @@ public class PropagationByResource imple
* @return whether the operation was successful or not
*/
public final boolean add(final PropagationOperation type, final String resourceName) {
-
Set<String> set;
switch (type) {
case CREATE:
@@ -113,7 +110,6 @@ public class PropagationByResource imple
* @return whether the operation was successful or not
*/
public boolean addAll(final PropagationOperation type, final Set<String> resourceNames) {
-
Set<String> set;
switch (type) {
case CREATE:
@@ -141,7 +137,6 @@ public class PropagationByResource imple
* @return whether the operation was successful or not
*/
public final boolean remove(final PropagationOperation type, final String resourceName) {
-
boolean result = false;
switch (type) {
@@ -200,7 +195,6 @@ public class PropagationByResource imple
public final void set(final PropagationOperation type, final Set<String> resourceNames) {
switch (type) {
-
case CREATE:
toBeCreated.clear();
toBeCreated.addAll(resourceNames);
@@ -235,8 +229,7 @@ public class PropagationByResource imple
/**
* whether no operations are present.
*
- * @return true if no operations (create / update / delete) and no
- * old account ids are present
+ * @return true if no operations (create / update / delete) and no old account ids are present
*/
public final boolean isEmpty() {
return toBeCreated.isEmpty() && toBeUpdated.isEmpty() && toBeDeleted.isEmpty() && oldAccountIds.isEmpty();
@@ -268,7 +261,6 @@ public class PropagationByResource imple
* @param oldAccountId old account id
*/
public void addOldAccountId(final String resourceName, final String oldAccountId) {
-
if (resourceName != null && oldAccountId != null) {
oldAccountIds.put(resourceName, oldAccountId);
}
@@ -276,7 +268,9 @@ public class PropagationByResource imple
@Override
public String toString() {
- return "To be Created: " + toBeCreated + ";\n" + "To be Updated: " + toBeUpdated + ";\n" + "To be Deleted: "
- + toBeDeleted + ";\n" + "Old account Ids: " + oldAccountIds;
+ return "To be Created: " + toBeCreated + ";\n"
+ + "To be Updated: " + toBeUpdated + ";\n"
+ + "To be Deleted: " + toBeDeleted + ";\n"
+ + "Old account Ids: " + oldAccountIds;
}
}
Modified: incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationManager.java
URL: http://svn.apache.org/viewvc/incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationManager.java?rev=1378601&r1=1378600&r2=1378601&view=diff
==============================================================================
--- incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationManager.java (original)
+++ incubator/syncope/branches/1_0_X/core/src/main/java/org/apache/syncope/core/propagation/PropagationManager.java Wed Aug 29 15:06:35 2012
@@ -36,7 +36,6 @@ import org.apache.syncope.core.init.Conn
import org.apache.syncope.core.persistence.beans.AbstractAttrValue;
import org.apache.syncope.core.persistence.beans.AbstractAttributable;
import org.apache.syncope.core.persistence.beans.AbstractSchema;
-import org.apache.syncope.core.persistence.beans.ConnInstance;
import org.apache.syncope.core.persistence.beans.ExternalResource;
import org.apache.syncope.core.persistence.beans.PropagationTask;
import org.apache.syncope.core.persistence.beans.SchemaMapping;
@@ -183,7 +182,7 @@ public class PropagationManager {
propByRes.get(PropagationOperation.CREATE).removeAll(syncResourceNames);
}
- return provision(user, password, wfResult.getResult().getValue(), propByRes);
+ return provision(user, password, wfResult.getResult().getValue(), false, propByRes);
}
/**
@@ -285,7 +284,7 @@ public class PropagationManager {
localPropByRes.get(PropagationOperation.DELETE).removeAll(syncResourceNames);
}
- return provision(user, password, enable, localPropByRes);
+ return provision(user, password, enable, false, localPropByRes);
}
/**
@@ -324,7 +323,7 @@ public class PropagationManager {
propByRes.get(PropagationOperation.DELETE).remove(syncResourceName);
}
- return provision(user, null, false, propByRes);
+ return provision(user, null, false, true, propByRes);
}
/**
@@ -367,12 +366,16 @@ public class PropagationManager {
final String extAttrName = SchemaMappingUtil.getExtAttrName(mapping);
- LOG.debug("Define mapping for: " + "\n* ExtAttrName " + extAttrName + "\n* is accountId "
- + mapping.isAccountid() + "\n* is password "
- + (mapping.isPassword() || mapping.getIntMappingType().equals(IntMappingType.Password))
- + "\n* mandatory condition " + mapping.getMandatoryCondition() + "\n* Schema "
- + mapping.getIntAttrName() + "\n* IntMappingType " + mapping.getIntMappingType().toString()
- + "\n* ClassType " + schemaType.getClassName() + "\n* Values " + values);
+ LOG.debug("Define mapping for: "
+ + "\n* ExtAttrName " + extAttrName
+ + "\n* is accountId " + mapping.isAccountid()
+ + "\n* is password " + (mapping.isPassword() || mapping.getIntMappingType().equals(
+ IntMappingType.Password))
+ + "\n* mandatory condition " + mapping.getMandatoryCondition()
+ + "\n* Schema " + mapping.getIntAttrName()
+ + "\n* IntMappingType " + mapping.getIntMappingType().toString()
+ + "\n* ClassType " + schemaType.getClassName()
+ + "\n* Values " + values);
List<Object> objValues = new ArrayList<Object>();
@@ -387,18 +390,13 @@ public class PropagationManager {
Map.Entry<String, Attribute> res;
if (mapping.isAccountid()) {
-
res = new DefaultMapEntry(objValues.iterator().next().toString(), null);
-
} else if (mapping.isPassword()) {
-
- res = new DefaultMapEntry(null, AttributeBuilder.buildPassword(objValues.iterator().next().toString().
- toCharArray()));
-
+ res = new DefaultMapEntry(null,
+ AttributeBuilder.buildPassword(objValues.iterator().next().toString().toCharArray()));
} else {
if (schema != null && schema.isMultivalue()) {
res = new DefaultMapEntry(null, AttributeBuilder.build(extAttrName, objValues));
-
} else {
res = new DefaultMapEntry(null, objValues.isEmpty()
? AttributeBuilder.build(extAttrName)
@@ -421,8 +419,8 @@ public class PropagationManager {
private Map.Entry<String, Set<Attribute>> prepareAttributes(final SyncopeUser user, final String password,
final Boolean enable, final ExternalResource resource) {
- LOG.debug("Preparing resource attributes for {}" + " on resource {}" + " with attributes {}", new Object[]{
- user, resource, user.getAttributes()});
+ LOG.debug("Preparing resource attributes for {} on resource {} with attributes {}",
+ new Object[]{user, resource, user.getAttributes()});
Set<Attribute> attributes = new HashSet<Attribute>();
String accountId = null;
@@ -496,11 +494,12 @@ public class PropagationManager {
* @param user user to be provisioned
* @param password cleartext password to be provisioned
* @param enable whether user must be enabled or not
+ * @param deleteOnResource whether user must be deleted anyway from external resource or not
* @param propByRes operation to be performed per resource
* @return list of propagation tasks created
*/
protected List<PropagationTask> provision(final SyncopeUser user, final String password, final Boolean enable,
- final PropagationByResource propByRes) {
+ final boolean deleteOnResource, final PropagationByResource propByRes) {
LOG.debug("Provisioning with user {}:\n{}", user, propByRes);
@@ -519,16 +518,16 @@ public class PropagationManager {
}
for (ExternalResource resource : resourcesByPriority) {
-
PropagationTask task = new PropagationTask();
task.setResource(resource);
- task.setSyncopeUser(user);
+ if (!deleteOnResource) {
+ task.setSyncopeUser(user);
+ }
task.setPropagationOperation(operation);
task.setPropagationMode(resource.getPropagationMode());
task.setOldAccountId(propByRes.getOldAccountId(resource.getName()));
Map.Entry<String, Set<Attribute>> preparedAttrs = prepareAttributes(user, password, enable, resource);
-
task.setAccountId(preparedAttrs.getKey());
task.setAttributes(preparedAttrs.getValue());
@@ -550,6 +549,7 @@ public class PropagationManager {
* Execute a list of PropagationTask, in given order.
*
* @param tasks to be execute, in given order
+ * @param handler propagation handler
* @throws PropagationException if propagation goes wrong: propagation is interrupted as soon as the result of the
* communication with a primary resource is in error
*/
@@ -573,7 +573,6 @@ public class PropagationManager {
execStatus = PropagationTaskExecStatus.FAILURE;
}
if (task.getResource().isPropagationPrimary() && !execStatus.isSuccessful()) {
-
throw new PropagationException(task.getResource().getName(), execution.getMessage());
}
}
@@ -648,15 +647,10 @@ public class PropagationManager {
ConnectorObject after = null;
try {
- final ConnInstance connInstance = task.getResource().getConnector();
-
final ConnectorFacadeProxy connector = connLoader.getConnector(task.getResource());
-
if (connector == null) {
- final String msg = String.format("Connector instance bean for resource %s and "
- + "connInstance %s not found", task.getResource(), connInstance);
-
- throw new NoSuchBeanDefinitionException(msg);
+ throw new NoSuchBeanDefinitionException(String.format(
+ "Connector instance bean for resource %s not found", task.getResource()));
}
// Try to read user BEFORE any actual operation
@@ -669,26 +663,7 @@ public class PropagationManager {
// set of attributes to be propagated
final Set<Attribute> attributes = new HashSet<Attribute>(task.getAttributes());
- if (before != null) {
-
- // 1. check if rename is really required
- final Name newName = (Name) AttributeUtil.find(Name.NAME, attributes);
-
- LOG.debug("Rename required with value {}", newName);
-
- if (newName != null && newName.equals(before.getName())
- && !before.getUid().getUidValue().equals(newName.getNameValue())) {
-
- LOG.debug("Remote object name unchanged");
- attributes.remove(newName);
- }
-
- LOG.debug("Attributes to be replaced {}", attributes);
-
- // 2. update with a new "normalized" attribute set
- connector.update(task.getPropagationMode(), ObjectClass.ACCOUNT, before.getUid(),
- attributes, null, propagationAttempted);
- } else {
+ if (before == null) {
// 1. get accountId
final String accountId = task.getAccountId();
@@ -713,17 +688,59 @@ public class PropagationManager {
// 4. provision entry
connector.create(task.getPropagationMode(), ObjectClass.ACCOUNT, attributes, null,
propagationAttempted);
+ } else {
+
+ // 1. check if rename is really required
+ final Name newName = (Name) AttributeUtil.find(Name.NAME, attributes);
+
+ LOG.debug("Rename required with value {}", newName);
+
+ if (newName != null && newName.equals(before.getName())
+ && !before.getUid().getUidValue().equals(newName.getNameValue())) {
+
+ LOG.debug("Remote object name unchanged");
+ attributes.remove(newName);
+ }
+
+ LOG.debug("Attributes to be replaced {}", attributes);
+
+ // 2. update with a new "normalized" attribute set
+ connector.update(task.getPropagationMode(), ObjectClass.ACCOUNT, before.getUid(),
+ attributes, null, propagationAttempted);
}
break;
case DELETE:
if (before == null) {
- LOG.debug("{} not found on external resource:" + " ignoring delete", task.getAccountId());
+ LOG.debug("{} not found on external resource: ignoring delete", task.getAccountId());
} else {
- final SyncopeUser user = getSyncopeUser(task.getSyncopeUser().getId());
+ /*
+ * We must choose here whether to
+ * a. actually delete the provided user from the external resource
+ * b. just update the provided user data onto the external resource
+ *
+ * (a) happens when either there is no user associated with the PropagationTask (this takes
+ * place when the task is generated via UserController.delete()) or the provided updated
+ * user hasn't the current resource assigned (when the task is generated via
+ * UserController.update()).
+ *
+ * (b) happens when the provided updated user does have the current resource assigned
+ * (when the task is generated via UserController.update()): this basically means that
+ * before such update, this user used to have the current resource assigned by more than
+ * one mean (for example, two different memberships with the same resource).
+ */
+
+ SyncopeUser user = null;
+ if (task.getSyncopeUser() != null) {
+ try {
+ user = getSyncopeUser(task.getSyncopeUser().getId());
+ } catch (NotFoundException e) {
+ LOG.warn("Requesting to delete a non-existing user from {}",
+ task.getResource().getName(), e);
+ }
+ }
if (user == null || !user.getResourceNames().contains(task.getResource().getName())) {
- // perform de-provisioning
LOG.debug("Perform deprovisioning on {}", task.getResource().getName());
connector.delete(
@@ -733,10 +750,6 @@ public class PropagationManager {
null,
propagationAttempted);
} else {
- // Update remote profile.
- // This is absolutely needed because otherwise the resource won't be updated:
- // resources to be deleted won't be considered by UserDataBinder.update() for the update
- // but, often, this have to be done.
LOG.debug("Update remote object on {}", task.getResource().getName());
connector.update(
@@ -762,12 +775,10 @@ public class PropagationManager {
// Try to read user AFTER any actual operation
after = getRemoteObject(connector, task, true);
-
} catch (Exception e) {
after = getRemoteObject(connector, task, false);
throw e;
}
-
} catch (Exception e) {
LOG.error("Exception during provision on resource " + task.getResource().getName(), e);
@@ -808,12 +819,11 @@ public class PropagationManager {
taskDAO.save(task);
- // Flush call is needed to value the id field of execution (used by deal test of TaskTestITCase).
+ // This flush call is needed to generate a value for the execution id
+ // An alternative to this would be the following statement that might cause troubles with
+ // concurrent calls.
+ // taskExecDAO.findLatestStarted(task);
taskDAO.flush();
-
- // An alternative to the flush call could be the following statement but we should accept the risk to
- // have a not so probable trouble coming from concurrent calls.
- //final TaskExec latestExec = taskExecDAO.findLatestStarted(taskDAO.save(task));
}
}
Modified: incubator/syncope/branches/1_0_X/core/src/test/java/org/apache/syncope/core/rest/UserTestITCase.java
URL: http://svn.apache.org/viewvc/incubator/syncope/branches/1_0_X/core/src/test/java/org/apache/syncope/core/rest/UserTestITCase.java?rev=1378601&r1=1378600&r2=1378601&view=diff
==============================================================================
--- incubator/syncope/branches/1_0_X/core/src/test/java/org/apache/syncope/core/rest/UserTestITCase.java (original)
+++ incubator/syncope/branches/1_0_X/core/src/test/java/org/apache/syncope/core/rest/UserTestITCase.java Wed Aug 29 15:06:35 2012
@@ -308,13 +308,13 @@ public class UserTestITCase extends Abst
}
assertNull(sce);
}
-
+
@Test
public void testMandatoryContraintsUserCreation() {
UserTO userTO = getSampleTO("issue183@apache.org");
userTO.addResource("ws-target-resource-2");
userTO.setPassword("newPassword");
-
+
AttributeTO type = null;
for (AttributeTO attr : userTO.getAttributes()) {
if ("type".equals(attr.getSchema())) {
@@ -331,9 +331,9 @@ public class UserTestITCase extends Abst
sce = scce.getException(SyncopeClientExceptionType.RequiredValuesMissing);
}
assertNotNull(sce);
-
+
userTO.addAttribute(type);
-
+
userTO = restTemplate.postForObject(BASE_URL + "user/create", userTO, UserTO.class);
assertNotNull(userTO);
}
@@ -552,7 +552,8 @@ public class UserTestITCase extends Abst
assertEquals(maxTaskExecutions, taskTO.getExecutions().size());
// 3. verify password
- Boolean verify = restTemplate.getForObject(BASE_URL + "user/verifyPassword/{username}.json?password=password123",
+ Boolean verify = restTemplate.
+ getForObject(BASE_URL + "user/verifyPassword/{username}.json?password=password123",
Boolean.class, newUserTO.getUsername());
assertTrue(verify);
@@ -868,7 +869,8 @@ public class UserTestITCase extends Abst
assertNotNull(user);
}
- users = Arrays.asList(restTemplate.getForObject(BASE_URL + "user/list/{page}/{size}.json", UserTO[].class, 2, 2));
+ users = Arrays.
+ asList(restTemplate.getForObject(BASE_URL + "user/list/{page}/{size}.json", UserTO[].class, 2, 2));
assertNotNull(users);
assertFalse(users.isEmpty());
@@ -1435,7 +1437,8 @@ public class UserTestITCase extends Abst
ConnObjectTO connObjectTO = restTemplate.getForObject(BASE_URL
+ "/resource/{resourceName}/read/{objectId}.json", ConnObjectTO.class, dbTable.getName(), dbTableUID);
- assertFalse(Boolean.parseBoolean(connObjectTO.getAttributeMap().get(OperationalAttributes.ENABLE_NAME).getValues().
+ assertFalse(Boolean.parseBoolean(connObjectTO.getAttributeMap().get(OperationalAttributes.ENABLE_NAME).
+ getValues().
get(0)));
String ldapUID = userTO.getUsername();
@@ -1456,7 +1459,8 @@ public class UserTestITCase extends Abst
connObjectTO = restTemplate.getForObject(BASE_URL + "/resource/{resourceName}/read/{objectId}.json",
ConnObjectTO.class, dbTable.getName(), dbTableUID);
- assertFalse(Boolean.parseBoolean(connObjectTO.getAttributeMap().get(OperationalAttributes.ENABLE_NAME).getValues().
+ assertFalse(Boolean.parseBoolean(connObjectTO.getAttributeMap().get(OperationalAttributes.ENABLE_NAME).
+ getValues().
get(0)));
query = "?resourceNames=" + dbTable.getName() + "&performLocally=true"; // check also performLocally
@@ -1469,7 +1473,8 @@ public class UserTestITCase extends Abst
connObjectTO = restTemplate.getForObject(BASE_URL + "/resource/{resourceName}/read/{objectId}.json",
ConnObjectTO.class, dbTable.getName(), dbTableUID);
- assertTrue(Boolean.parseBoolean(connObjectTO.getAttributeMap().get(OperationalAttributes.ENABLE_NAME).getValues().
+ assertTrue(Boolean.parseBoolean(connObjectTO.getAttributeMap().get(OperationalAttributes.ENABLE_NAME).
+ getValues().
get(0)));
}
@@ -1911,7 +1916,6 @@ public class UserTestITCase extends Abst
userTO.addResource("resource-ldap");
UserTO actual = restTemplate.postForObject(BASE_URL + "user/create", userTO, UserTO.class);
-
assertNotNull(actual);
assertEquals(2, actual.getMemberships().size());
@@ -1966,4 +1970,34 @@ public class UserTestITCase extends Abst
assertTrue(title.getValues().contains("r13"));
// -----------------------------------
}
+
+ @Test
+ public void issueSYNCOPE185() {
+ // 1. create user with LDAP resource, succesfully propagated
+ UserTO userTO = getSampleTO("syncope185@syncope.apache.org");
+ userTO.getVirtualAttributes().clear();
+ userTO.addResource("resource-ldap");
+
+ userTO = restTemplate.postForObject(BASE_URL + "user/create", userTO, UserTO.class);
+ assertNotNull(userTO);
+ assertFalse(userTO.getPropagationTOs().isEmpty());
+ assertEquals("resource-ldap", userTO.getPropagationTOs().get(0).getResourceName());
+ assertEquals(PropagationTaskExecStatus.SUCCESS, userTO.getPropagationTOs().get(0).getStatus());
+
+ // 2. delete this user
+ restTemplate.getForObject(BASE_URL + "user/delete/{userId}", UserTO.class, userTO.getId());
+
+ // 3. try (and fail) to find this user on the external LDAP resource
+ SyncopeClientException sce = null;
+ try {
+ ConnObjectTO connObjectTO = restTemplate.getForObject(
+ BASE_URL + "/resource/{resourceName}/read/{objectId}.json",
+ ConnObjectTO.class, "resource-ldap", userTO.getUsername());
+ fail("This entry should not be present on this resource");
+ } catch (SyncopeClientCompositeErrorException sccee) {
+ sce = sccee.getException(SyncopeClientExceptionType.NotFound);
+
+ }
+ assertNotNull(sce);
+ }
}