Skip to content

Commit

Permalink
Pre-generate focus OID for all "add" operations
Browse files Browse the repository at this point in the history
In order to fix "extra audit record" issue completely, we have to
know focus OID right at the start of the clockwork execution. Hence,
now we generate one.

This should fix MID-8659.
  • Loading branch information
mederly committed Sep 5, 2023
1 parent d5637c8 commit 8aa73f9
Show file tree
Hide file tree
Showing 9 changed files with 213 additions and 90 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ public class ModelController implements ModelService, TaskService, CaseService,
private static final String OP_APPLY_PROVISIONING_DEFINITION = CLASS_NAME_WITH_DOT + "applyProvisioningDefinition";
static final String OP_REEVALUATE_SEARCH_FILTERS = CLASS_NAME_WITH_DOT + "reevaluateSearchFilters";

private static final int OID_GENERATION_ATTEMPTS = 5;

private static final Trace LOGGER = TraceManager.getTrace(ModelController.class);

private static final Trace OP_LOGGER = TraceManager.getTrace(ModelService.OPERATION_LOGGER_NAME);
Expand Down Expand Up @@ -344,6 +346,10 @@ private Collection<ObjectDeltaOperation<? extends ObjectType>> executeChangesNon
context.setProgressListeners(statusListeners);
// Note: Request authorization happens inside clockwork

// We generate focus OID even for generic repo, and when access metadata are not concerned.
// It is maybe not strictly needed for these cases, but this allows us to rely on the fact that the OID is always there.
generateFocusOidIfNeeded(context, result);

clockwork.run(context, task, result);

// prepare return value
Expand Down Expand Up @@ -372,6 +378,40 @@ private Collection<ObjectDeltaOperation<? extends ObjectType>> executeChangesNon
return executedDeltas;
}

private void generateFocusOidIfNeeded(LensContext<? extends ObjectType> context, OperationResult result)
throws SchemaException {
LensFocusContext<? extends ObjectType> focusContext = context.getFocusContext();
if (focusContext == null) {
return;
}
var primaryDelta = focusContext.getPrimaryDelta();
if (primaryDelta == null || !primaryDelta.isAdd()) {
return;
}
var objectToAdd = primaryDelta.getObjectToAdd().asObjectable();
if (objectToAdd.getOid() != null) {
return;
}
String oid = getNewOid(objectToAdd.getClass(), result);
focusContext.modifyPrimaryDelta(d -> d.setOid(oid));
}

private String getNewOid(Class<? extends ObjectType> type, OperationResult result) {
for (int attempt = 1; attempt <= OID_GENERATION_ATTEMPTS; attempt++) {
var randomOid = UUID.randomUUID().toString();
try {
cacheRepositoryService.getObject(type, randomOid, GetOperationOptions.createAllowNotFoundCollection(), result);
LOGGER.info("Random UUID is not that random? Attempt {} of {}: {}", attempt, OID_GENERATION_ATTEMPTS, randomOid);
} catch (ObjectNotFoundException e) {
result.clearLastSubresultError(); // e.g. because of tests
return randomOid; // This is the good case
} catch (SchemaException e) {
LoggingUtils.logException(LOGGER, "Couldn't check for existence of object with OID {}", e, randomOid);
}
}
throw new IllegalStateException("Couldn't generate random OID even after " + OID_GENERATION_ATTEMPTS + " attempts");
}

static void checkIndestructible(ObjectType object)
throws IndestructibilityViolationException {
if (object != null && Boolean.TRUE.equals(object.isIndestructible())) {
Expand Down Expand Up @@ -760,8 +800,8 @@ public <T extends Containerable> SearchResultList<T> searchContainers(
}
}


public <T extends Containerable> SearchResultMetadata searchContainersIterative(Class<T> type, ObjectQuery origQuery,
public <T extends Containerable> SearchResultMetadata searchContainersIterative(
Class<T> type, ObjectQuery origQuery,
ObjectHandler<T> handler, Collection<SelectorOptions<GetOperationOptions>> rawOptions,
Task task, OperationResult parentResult)
throws SchemaException, ObjectNotFoundException, CommunicationException, ConfigurationException,
Expand Down Expand Up @@ -803,7 +843,6 @@ public <T extends Containerable> SearchResultMetadata searchContainersIterative(
if (ctx.skipSecurityPostProcessing) {
LOGGER.debug("Objects of type '{}' do not have security constraints applied yet", type.getSimpleName());
} else {

SearchResultList<Containerable> list = new SearchResultList<>();
list.add(object);
schemaTransformer.applySchemasAndSecurityToContainerValues(list, parsedOptions, task, result);
Expand All @@ -813,9 +852,11 @@ public <T extends Containerable> SearchResultMetadata searchContainersIterative(
throw new SystemException(ex.getMessage(), ex);
}

OP_LOGGER.debug("MODEL OP handle searchObjects({},{},{}): {}", type.getSimpleName(), query, rawOptions, object);
OP_LOGGER.debug("MODEL OP handle searchContainersIterative({},{},{}): {}",
type.getSimpleName(), query, rawOptions, object);
if (OP_LOGGER.isTraceEnabled()) {
OP_LOGGER.trace("MODEL OP handle searchObjects({},{},{}):\n{}", type.getSimpleName(), query, rawOptions, object.debugDump(1));
OP_LOGGER.trace("MODEL OP handle searchContainersIterative({},{},{}):\n{}",
type.getSimpleName(), query, rawOptions, DebugUtil.debugDump(object,1));
}

return handler.handle(prismContainer.getRealValue(), lResult);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,14 @@

package com.evolveum.midpoint.model.impl.lens;

import com.evolveum.midpoint.model.impl.ModelBeans;
import com.evolveum.midpoint.model.impl.lens.assignments.EvaluatedAssignmentImpl;
import com.evolveum.midpoint.schema.result.OperationResult;
import com.evolveum.midpoint.xml.ns._public.common.common_3.AssignmentType;
import java.util.HashMap;
import java.util.Map;

import org.jetbrains.annotations.NotNull;

import java.util.HashMap;
import java.util.Map;
import com.evolveum.midpoint.model.impl.lens.assignments.EvaluatedAssignmentImpl;
import com.evolveum.midpoint.util.ShortDumpable;
import com.evolveum.midpoint.xml.ns._public.common.common_3.AssignmentType;

/**
* When new assignments are being created (either as part of focus "add" or "modify" operation), we need to know their PCV IDs
Expand All @@ -34,7 +33,7 @@
* NOTE: {@link TemporaryContainerIdStore} addresses a similar problem. However, in that case, IDs are not really important,
* and they can be safely discarded when the delta is executed. (Unlike this case, where we need to keep them.)
*/
public class AssignmentIdStore {
public class AssignmentIdStore implements ShortDumpable {

/** The map keys are parent-less assignments, immutable (to avoid hashCode being inadvertently changed). */
@NotNull private final Map<AssignmentType, Long> generatedIdsMap = new HashMap<>();
Expand All @@ -52,4 +51,11 @@ public void put(AssignmentType assignment, Long newId) {
public boolean isEmpty() {
return generatedIdsMap.isEmpty();
}

@Override
public void shortDump(StringBuilder sb) {
sb.append("AssignmentIdStore(");
sb.append(generatedIdsMap.values());
sb.append(")");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,9 @@ public String debugDump(int indent) {
sb.append("\n");

DebugUtil.debugDumpWithLabelLn(sb, getDebugDumpTitle("executed deltas"), getExecutedDeltas(), indent+1);
DebugUtil.debugDumpWithLabel(sb, "Policy rules context", policyRulesContext, indent + 1);
DebugUtil.debugDumpWithLabelLn(sb, "Policy rules context", policyRulesContext, indent + 1);
DebugUtil.debugDumpWithLabel(sb, "Assignment ID store",
assignmentIdStore != null ? assignmentIdStore.shortDump() : null, indent + 1);
return sb.toString();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,11 @@ public String toString() {
public String debugDump(int indent) {
StringBuilder sb = new StringBuilder();
DebugUtil.indentDebugDump(sb, indent);
sb.append("SmartAssignmentElement: ").append(origin).append("\n");
sb.append("SmartAssignmentElement: ").append(origin);
if (externalId != null) {
sb.append(" (externalId=").append(externalId).append(")");
}
sb.append("\n");
sb.append(assignmentCVal.debugDump(indent + 1));
return sb.toString();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,7 @@ public void test230ChuckAssign2a2b() throws Exception {
assertEquals("Wrong # of assignments", 2, chuck.getAssignment().size());

displayDumpable("Audit", dummyAuditService);
// 2 because sourceRef.oid is null on the first iteration (object OID is unknown before actual creation in repo)
dummyAuditService.assertExecutionRecords(2);
dummyAuditService.assertExecutionRecords(1);

for (AssignmentType assignment : chuck.getAssignment()) {
assertExclusionViolationState(assignment, 1);
Expand Down

0 comments on commit 8aa73f9

Please sign in to comment.