Skip to content

Commit

Permalink
Remove setting delta old values by mappings
Browse files Browse the repository at this point in the history 
The ProjectionMappingSetEvaluator is not the right place to store
estimated old values for the deltas produced. The reason is that the
values are based on what mapping thinks was there, which may or may not
correspond to the reality. Lens context does the same, and it should
know better.

This should resolve MID-9102.
  • Loading branch information
@mederly
mederly committed Sep 20, 2023
1 parent 1357480 commit 5934411
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 48 deletions.
20 changes: 6 additions & 14 deletions .../com/evolveum/midpoint/model/impl/lens/projector/focus/ProjectionMappingSetEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,17 +384,17 @@ Map<UniformItemPath, MappingOutputStruct<V>> evaluateMappingsToTriples(
}
targetItemDelta.setValuesToReplace(PrismValueCollectionsUtil.cloneCollection(valuesToReplace));

applyEstimatedOldValueInReplaceCase(targetItemDelta, outputTriple);

} else if (outputTriple.hasMinusSet()) {
LOGGER.trace("{} resulted in null or empty value for {} and there is a minus set, resetting it (replace with empty)", mappingDesc, targetContext);
targetItemDelta.setValueToReplace();
applyEstimatedOldValueInReplaceCase(targetItemDelta, outputTriple);

} else {
LOGGER.trace("{} resulted in null or empty value for {}, skipping", mappingDesc, targetContext);
}

// Here we intentionally do not set estimated old values in the delta. The reason is that the old values
// here would be determined by what the mapping THINKS was there. The reality may be different. The
// LensElementContext would know better when swallowing the delta.
}

if (targetItemDelta.isEmpty()) {
Expand Down Expand Up @@ -428,16 +428,6 @@ Map<UniformItemPath, MappingOutputStruct<V>> evaluateMappingsToTriples(
return outputTripleMap;
}


private <V extends PrismValue, D extends ItemDefinition<?>> void applyEstimatedOldValueInReplaceCase(
ItemDelta<V, D> targetItemDelta, PrismValueDeltaSetTriple<V> outputTriple) {
Collection<V> nonPositiveValues = outputTriple.getNonPositiveValues();
if (nonPositiveValues.isEmpty()) {
return;
}
targetItemDelta.setEstimatedOldValues(PrismValueCollectionsUtil.cloneCollection(nonPositiveValues));
}

private <V extends PrismValue> boolean isMeaningful(PrismValueDeltaSetTriple<V> mappingOutputTriple) {
if (mappingOutputTriple == null) {
// this means: mapping not applicable
Expand All @@ -453,7 +443,9 @@ private <V extends PrismValue> boolean isMeaningful(PrismValueDeltaSetTriple<V>
return true;
}
//noinspection RedundantIfStatement
if (hasNoOrHashedValuesOnly(mappingOutputTriple.getMinusSet()) && hasNoOrHashedValuesOnly(mappingOutputTriple.getZeroSet()) && hasNoOrHashedValuesOnly(mappingOutputTriple.getPlusSet())) {
if (hasNoOrHashedValuesOnly(mappingOutputTriple.getMinusSet())
&& hasNoOrHashedValuesOnly(mappingOutputTriple.getZeroSet())
&& hasNoOrHashedValuesOnly(mappingOutputTriple.getPlusSet())) {
// Used to skip application of mapping that produces only hashed protected values.
// Those values are useless, e.g. to set new password. If we would consider them as
// meaningful then a normal mapping with such values may prohibit application of
Expand Down
121 changes: 89 additions & 32 deletions model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/TestActivation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,19 @@

import static com.evolveum.midpoint.schema.constants.SchemaConstants.*;

import static com.evolveum.midpoint.test.DummyResourceContoller.DUMMY_ACCOUNT_ATTRIBUTE_GOSSIP_NAME;

import static org.assertj.core.api.Assertions.assertThat;
import static org.testng.AssertJUnit.*;

import static com.evolveum.midpoint.schema.util.ObjectTypeUtil.cast;

import java.io.File;
import java.util.*;

import com.evolveum.midpoint.prism.PrismValueCollectionsUtil;
import com.evolveum.midpoint.schema.processor.ResourceObjectTypeIdentification;

import jakarta.xml.bind.JAXBElement;
import javax.xml.datatype.XMLGregorianCalendar;

Expand Down Expand Up @@ -65,9 +71,9 @@ public class TestActivation extends AbstractInitializedModelIntegrationTest {

// This resource does not support native activation. It has simulated activation instead.
// + unusual validTo and validFrom mappings
private static final File RESOURCE_DUMMY_KHAKI_FILE = new File(TEST_DIR, "resource-dummy-khaki.xml");
private static final String RESOURCE_DUMMY_KHAKI_OID = "10000000-0000-0000-0000-0000000a1004";
private static final String RESOURCE_DUMMY_KHAKI_NAME = "khaki";
private static final DummyTestResource RESOURCE_DUMMY_KHAKI = new DummyTestResource(TEST_DIR,
"resource-dummy-khaki.xml", "10000000-0000-0000-0000-0000000a1004", "khaki",
c -> c.extendSchemaPirate());

// This resource does not support native activation. It has simulated activation instead.
// + unusual validTo and validFrom mappings
Expand All @@ -93,10 +99,7 @@ public class TestActivation extends AbstractInitializedModelIntegrationTest {
private String userMancombOid;
private XMLGregorianCalendar manana;

private DummyResource dummyResourceKhaki;
private DummyResourceContoller dummyResourceCtlKhaki;
private ResourceType resourceDummyKhakiType;
private PrismObject<ResourceType> resourceDummyKhaki;
private ResourceType resourceDummyKhaki;

private DummyResource dummyResourceCoral;
private DummyResourceContoller dummyResourceCtlCoral;
Expand All @@ -108,14 +111,6 @@ public void initSystem(Task initTask, OperationResult initResult)
throws Exception {
super.initSystem(initTask, initResult);

dummyResourceCtlKhaki = DummyResourceContoller.create(RESOURCE_DUMMY_KHAKI_NAME, resourceDummyKhaki);
dummyResourceCtlKhaki.extendSchemaPirate();
dummyResourceKhaki = dummyResourceCtlKhaki.getDummyResource();
resourceDummyKhaki = importAndGetObjectFromFile(ResourceType.class, RESOURCE_DUMMY_KHAKI_FILE, RESOURCE_DUMMY_KHAKI_OID, initTask, initResult);
resourceDummyKhakiType = resourceDummyKhaki.asObjectable();
dummyResourceCtlKhaki.setResource(resourceDummyKhaki);
dummyResourceCollection.initDummyResource(RESOURCE_DUMMY_KHAKI_NAME, dummyResourceCtlKhaki);

dummyResourceCtlCoral = DummyResourceContoller.create(RESOURCE_DUMMY_CORAL_NAME, resourceDummyCoral);
DummyObjectClass accountObjectClass = dummyResourceCtlCoral.getDummyResource().getAccountObjectClass();
dummyResourceCtlCoral.addAttrDef(accountObjectClass, SUSPENDED_ATTRIBUTE_NAME, Boolean.class, false, false);
Expand All @@ -125,16 +120,23 @@ public void initSystem(Task initTask, OperationResult initResult)
dummyResourceCtlCoral.setResource(resourceDummyCoral);
dummyResourceCollection.initDummyResource(RESOURCE_DUMMY_CORAL_NAME, dummyResourceCtlCoral);

RESOURCE_DUMMY_KHAKI.init(this, initTask, initResult);
RESOURCE_DUMMY_PRECREATE.init(this, initTask, initResult);
RESOURCE_DUMMY_FULL_VALIDITY.init(this, initTask, initResult);

resourceDummyKhaki = modelService
.getObject(ResourceType.class, RESOURCE_DUMMY_KHAKI.oid, null, initTask, initResult)
.asObjectable();
}

@Test
public void test000Sanity() {
// MID-6609
// 1. Correct serialization and parsing of <path> expression

PrismProperty<Object> uselessStringProp = resourceDummyKhaki.findProperty(ItemPath.create(ResourceType.F_CONNECTOR_CONFIGURATION, "configurationProperties", "uselessString"));
PrismProperty<Object> uselessStringProp =
resourceDummyKhaki.asPrismObject().findProperty(
ItemPath.create(ResourceType.F_CONNECTOR_CONFIGURATION, "configurationProperties", "uselessString"));
ExpressionType expression = (ExpressionType) Objects.requireNonNull(uselessStringProp.getValue().getExpression())
.getExpression();
assertThat(expression.getExpressionEvaluator()).hasSize(1);
Expand All @@ -143,7 +145,9 @@ public void test000Sanity() {
assertThat(evaluatorJaxb.getValue().toString()).isEqualTo("$configuration/name");

// 2. Correct evaluation of <path> expression: $configuration/name = 'SystemConfiguration'
assertThat(dummyResourceKhaki.getUselessString()).as("useless string").isEqualTo("SystemConfiguration");
assertThat(RESOURCE_DUMMY_KHAKI.getDummyResource().getUselessString())
.as("useless string")
.isEqualTo("SystemConfiguration");
}

@Test
Expand Down Expand Up @@ -1331,7 +1335,7 @@ public void test160ModifyUserJackAssignAccountKhaki() throws Exception {

// WHEN
when();
assignAccountToUser(USER_JACK_OID, RESOURCE_DUMMY_KHAKI_OID, null, task, result);
assignAccountToUser(USER_JACK_OID, RESOURCE_DUMMY_KHAKI.oid, null, task, result);

// THEN
then();
Expand All @@ -1342,26 +1346,26 @@ public void test160ModifyUserJackAssignAccountKhaki() throws Exception {
PrismObject<UserType> userAfter = getUser(USER_JACK_OID);
display("User after", userAfter);
assertUserJack(userAfter);
String accountKhakiOid = getLiveLinkRefOid(userAfter, RESOURCE_DUMMY_KHAKI_OID);
String accountKhakiOid = getLiveLinkRefOid(userAfter, RESOURCE_DUMMY_KHAKI.oid);

// Check shadow
PrismObject<ShadowType> accountShadow = repositoryService.getObject(ShadowType.class, accountKhakiOid, null, result);
display("Shadow (repo)", accountShadow);
assertAccountShadowRepo(accountShadow, accountKhakiOid, "jack", resourceDummyKhakiType);
assertAccountShadowRepo(accountShadow, accountKhakiOid, "jack", resourceDummyKhaki);
TestUtil.assertCreateTimestamp(accountShadow, start, end);
assertEnableTimestampShadow(accountShadow, start, end);

// Check account
PrismObject<ShadowType> accountModel = modelService.getObject(ShadowType.class, accountKhakiOid, null, task, result);
display("Shadow (model)", accountModel);
assertAccountShadowModel(accountModel, accountKhakiOid, "jack", resourceDummyKhakiType);
assertAccountShadowModel(accountModel, accountKhakiOid, "jack", resourceDummyKhaki);
TestUtil.assertCreateTimestamp(accountModel, start, end);
assertEnableTimestampShadow(accountModel, start, end);

// Check account in dummy resource
assertDummyAccount(RESOURCE_DUMMY_KHAKI_NAME, "jack", "Jack Sparrow", true);
assertDummyAccount(RESOURCE_DUMMY_KHAKI.name, "jack", "Jack Sparrow", true);

assertDummyEnabled(RESOURCE_DUMMY_KHAKI_NAME, "jack");
assertDummyEnabled(RESOURCE_DUMMY_KHAKI.name, "jack");

TestUtil.assertModifyTimestamp(userAfter, start, end);

Expand Down Expand Up @@ -1410,13 +1414,13 @@ public void test165AddWithArchived() throws Exception {
.administrativeStatus(ActivationStatusType.ARCHIVED))
.assignment(new AssignmentType()
.construction(new ConstructionType()
.resourceRef(RESOURCE_DUMMY_KHAKI_OID, ResourceType.COMPLEX_TYPE)))
.resourceRef(RESOURCE_DUMMY_KHAKI.oid, ResourceType.COMPLEX_TYPE)))
.assignment(new AssignmentType()
.construction(RESOURCE_DUMMY_FULL_VALIDITY.construction(null, null)));
.construction(RESOURCE_DUMMY_FULL_VALIDITY.defaultConstruction()));
addObject(user, task, result);

then("the khaki account simulation attribute indicates it's disabled");
assertDummyAccountByUsername(RESOURCE_DUMMY_KHAKI_NAME, userName)
assertDummyAccountByUsername(RESOURCE_DUMMY_KHAKI.name, userName)
.display()
.assertAttribute("gossip", "dead");

Expand Down Expand Up @@ -1446,11 +1450,11 @@ public void test167ModifyWithArchived() throws Exception {
.name(userName)
.assignment(new AssignmentType()
.construction(new ConstructionType()
.resourceRef(RESOURCE_DUMMY_KHAKI_OID, ResourceType.COMPLEX_TYPE)))
.resourceRef(RESOURCE_DUMMY_KHAKI.oid, ResourceType.COMPLEX_TYPE)))
.assignment(new AssignmentType()
.construction(RESOURCE_DUMMY_FULL_VALIDITY.construction(null, null)));
.construction(RESOURCE_DUMMY_FULL_VALIDITY.defaultConstruction()));
addObject(user, task, result);
assertDummyAccountByUsername(RESOURCE_DUMMY_KHAKI_NAME, userName);
assertDummyAccountByUsername(RESOURCE_DUMMY_KHAKI.name, userName);
assertDummyAccountByUsername(RESOURCE_DUMMY_FULL_VALIDITY.name, userName);

when("the administrativeStatus is changed to ARCHIVED");
Expand All @@ -1462,7 +1466,7 @@ public void test167ModifyWithArchived() throws Exception {
null, task, result);

then("the khaki account simulation attribute indicates it's disabled");
assertDummyAccountByUsername(RESOURCE_DUMMY_KHAKI_NAME, userName)
assertDummyAccountByUsername(RESOURCE_DUMMY_KHAKI.name, userName)
.display()
.assertAttribute("gossip", "dead");

Expand Down Expand Up @@ -2489,7 +2493,7 @@ public void test410AssignHermanKhakiAccount() throws Exception {
OperationResult result = task.getResult();

// WHEN
assignAccountToUser(USER_HERMAN_OID, RESOURCE_DUMMY_KHAKI_OID, null, task, result);
assignAccountToUser(USER_HERMAN_OID, RESOURCE_DUMMY_KHAKI.oid, null, task, result);

// THEN
result.computeStatus();
Expand All @@ -2499,7 +2503,7 @@ public void test410AssignHermanKhakiAccount() throws Exception {
display("User after change execution", user);
assertLiveLinks(user, 1);

DummyAccount khakiAccount = getDummyAccount(RESOURCE_DUMMY_KHAKI_NAME, USER_HERMAN_USERNAME);
DummyAccount khakiAccount = getDummyAccount(RESOURCE_DUMMY_KHAKI.name, USER_HERMAN_USERNAME);
assertNotNull("No khaki account", khakiAccount);
assertTrue("khaki account not enabled", khakiAccount.isEnabled());
assertEquals("Wrong quote (validFrom) in khaki account", "from: 1700-05-30T11:00:00Z",
Expand Down Expand Up @@ -3004,6 +3008,59 @@ public void test800DeleteInvalidAssignment() throws Exception {
.assertAssignments(0);
}

/**
* When an account is disabled, the "estimated old" value should be correctly set.
*
* MID-9102
*/
@Test
public void test810AuditAccountDisable() throws Exception {
var task = getTestTask();
var result = task.getResult();

given("user (suspended) with an account (enabled)");
var userName = getTestNameShort();
var user = new UserType()
.name(userName)
.assignment(new AssignmentType()
.construction(RESOURCE_DUMMY_KHAKI.defaultConstruction()));
addObject(user, task, result);

and("suspending the user after the account is created (otherwise it would not get created at all)");
modifyUserReplace(
user.getOid(),
UserType.F_LIFECYCLE_STATE,
task, result,
LIFECYCLE_SUSPENDED);

and("account is manually enabled");
var account = RESOURCE_DUMMY_KHAKI.getDummyResource().getAccountByUsername(userName);
account.replaceAttributeValue(DUMMY_ACCOUNT_ATTRIBUTE_GOSSIP_NAME, "alive");

when("account is imported");
dummyAuditService.clear();
importAccountsRequest()
.withResourceOid(RESOURCE_DUMMY_KHAKI.oid)
.withTypeIdentification(ResourceObjectTypeIdentification.of(ShadowKindType.ACCOUNT, INTENT_DEFAULT))
.withNameValue(userName)
.executeOnForeground(result);

then("the account is disabled");
assertDummyAccountByUsername(RESOURCE_DUMMY_KHAKI.name, userName)
.display()
.assertAttribute(DUMMY_ACCOUNT_ATTRIBUTE_GOSSIP_NAME, "dead");

and("audit has status old value of ENABLED");
displayDumpable("audit", dummyAuditService);
dummyAuditService.assertSimpleRecordSanity();
ObjectDeltaOperation<ShadowType> odo = dummyAuditService.getExecutionDelta(0, ChangeType.MODIFY, ShadowType.class);
var delta = odo.getObjectDelta();
PropertyDelta<Object> statusDelta = delta.findPropertyDelta(PATH_ACTIVATION_ADMINISTRATIVE_STATUS);
assertThat(PrismValueCollectionsUtil.getRealValuesOfCollection(statusDelta.getEstimatedOldValues()))
.as("status estimated old values")
.containsExactlyInAnyOrder(ActivationStatusType.ENABLED);
}

private void assertDummyActivationEnabledState(String userId, Boolean expectedEnabled) throws SchemaViolationException, ConflictException, InterruptedException {
assertDummyActivationEnabledState(null, userId, expectedEnabled);
}
Expand Down
14 changes: 13 additions & 1 deletion model/model-intest/src/test/resources/activation/resource-dummy-khaki.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,10 @@
<outbound/>
</existence>
<administrativeStatus>
<outbound/>
<outbound>
<!-- Strong because of TestActivation.test810 -->
<strength>strong</strength>
</outbound>
</administrativeStatus>
<validFrom>
<outbound>
Expand Down Expand Up @@ -140,6 +143,15 @@
</password>
</credentials>

<synchronization>
<reaction>
<situation>linked</situation>
<actions>
<synchronize/>
</actions>
</reaction>
</synchronization>

</objectType>

</schemaHandling>
Expand Down
2 changes: 1 addition & 1 deletion repo/repo-test-util/src/main/java/com/evolveum/midpoint/test/DummyAuditService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public synchronized void assertSimpleRecordSanity() {
while (iterator.hasNext()) {
AuditEventRecord record = iterator.next();
num++;
assertRecordSanity("" + num + "th record", record);
assertRecordSanity(num + "th record", record);

if (record.getEventStage() == AuditEventStage.REQUEST) {
numRequests++;
Expand Down
4 changes: 4 additions & 0 deletions repo/repo-test-util/src/main/java/com/evolveum/midpoint/test/TestResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ public void initAndTest(ResourceTester tester, Task task, OperationResult result
reload(tester.getResourceReloader(), result);
}

public @NotNull ConstructionType defaultConstruction() {
return construction(null, null);
}

public @NotNull ConstructionType construction(ShadowKindType kind, String intent) {
return new ConstructionType()
.resourceRef(oid, ResourceType.COMPLEX_TYPE)
Expand Down

0 comments on commit 5934411

Please sign in to comment.