Skip to content

Commit

Permalink
Tidy up AssignmentTripleEvaluator a bit
Browse files Browse the repository at this point in the history
Before diagnosing an issue with idempotent role assignments, let us
clean up the code a little. TestAssignmentProcessor was cleanup
and documented as well.

Some minor fixes were introduced as part of this:

- Fixed a condition that checks for phantom assignment adds.
- Fixed freezing of AssignmentOrigin (originally executed only
if a delta was present).

Related to MID-7382.
  • Loading branch information
mederly committed Nov 2, 2021
1 parent d1609c7 commit e79c946
Show file tree
Hide file tree
Showing 16 changed files with 610 additions and 530 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,9 @@ private <F extends ObjectType, O extends ObjectType> ObjectSecurityConstraints a
}
} else {
// modify
Collection<? extends ItemDelta<?, ?>> credentialChanges = primaryDeltaClone.findItemDeltasSubPath(UserType.F_CREDENTIALS);
for (ItemDelta credentialChange: credentialChanges) {
Collection<? extends ItemDelta<?, ?>> credentialChanges =
primaryDeltaClone.findItemDeltasSubPath(UserType.F_CREDENTIALS);
for (ItemDelta<?, ?> credentialChange : credentialChanges) {
AuthorizationDecisionType cdecision = evaluateCredentialDecision(context, securityConstraints, credentialChange);
LOGGER.trace("AUTZ: credential delta {} decision: {}", credentialChange.getPath(), cdecision);
if (cdecision == AuthorizationDecisionType.ALLOW) {
Expand All @@ -193,7 +194,7 @@ private <F extends ObjectType, O extends ObjectType> ObjectSecurityConstraints a
}
}

if (primaryDeltaClone != null && !primaryDeltaClone.isEmpty()) {
if (!primaryDeltaClone.isEmpty()) {
// TODO: optimize, avoid evaluating the constraints twice
securityEnforcer.authorize(deltaOperationUrl, getRequestAuthorizationPhase(context) , AuthorizationParameters.Builder.buildObjectDelta(object, primaryDeltaClone), ownerResolver, task, result);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ private <P extends ObjectType> boolean isShadowDeltaSignificant(ObjectDelta<P> d
return true;
} else {
Collection<? extends ItemDelta<?, ?>> attrDeltas = delta.findItemDeltasSubPath(ShadowType.F_ATTRIBUTES);
return attrDeltas != null && !attrDeltas.isEmpty();
return !attrDeltas.isEmpty();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ private <F extends FocusType> void processProjectionValues(LensContext<F> contex

context.checkConsistenceIfNeeded();

// Re-evaluates the values in the account constructions (including roles)
// Re-evaluates the values in the account constructions (including roles) - currently no-op!
assignmentProcessor.processAssignmentsAccountValues(projContext, iterationResult);

context.recompute();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,12 @@ public void collectAndFreeze(PrismObject<F> objectCurrent, PrismObject<F> object
// TODO what if assignment is both virtual and in delta? It will have virtual flag set to true... MID-6404
collectVirtualAssignments(virtualAssignments);

collectDeltaValuesAndFreeze(assignmentContainerCurrent, currentAssignmentDelta, prismContext);
collectDeltaValues(assignmentContainerCurrent, currentAssignmentDelta, prismContext);

freezeOrigins();
}

private void collectDeltaValuesAndFreeze(PrismContainer<AssignmentType> assignmentContainerCurrent, ContainerDelta<AssignmentType> currentAssignmentDelta, PrismContext prismContext) throws SchemaException {
private void collectDeltaValues(PrismContainer<AssignmentType> assignmentContainerCurrent, ContainerDelta<AssignmentType> currentAssignmentDelta, PrismContext prismContext) throws SchemaException {
if (currentAssignmentDelta != null) {
if (currentAssignmentDelta.isReplace()) {
allValues().forEach(v -> v.getOrigin().setNew(false));
Expand All @@ -92,7 +94,6 @@ private void collectDeltaValuesAndFreeze(PrismContainer<AssignmentType> assignme
// the whole new assignment set (that can have hundreds of assignments)
collectAssignmentsFromAddDeleteDelta(currentAssignmentDelta);
}
freezeOrigins();
}
}

Expand Down Expand Up @@ -244,7 +245,7 @@ public String toString() {
return "SmartAssignmentCollection(" + allValues() + ")";
}

public void freezeOrigins() {
private void freezeOrigins() {
allValues().forEach(v -> v.getOrigin().freeze());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ public PrismContainerValue<AssignmentType> getAssignmentCVal() {
return assignmentCVal;
}

public Long getAssignmentId() {
return assignmentCVal.getId();
}

public SmartAssignmentKey getKey() {
return new SmartAssignmentKey(assignmentCVal);
}
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@

@ContextConfiguration(locations = { "classpath:ctx-model-test-main.xml" })
@DirtiesContext(classMode = ClassMode.AFTER_CLASS)
public abstract class TestAbstractAssignmentEvaluator extends AbstractLensTest {
public abstract class AbstractAssignmentEvaluatorTest extends AbstractLensTest {

@Autowired private ReferenceResolver referenceResolver;
@Autowired @Qualifier("modelObjectResolver") private ObjectResolver objectResolver;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ public abstract class AbstractLensTest extends AbstractInternalModelIntegrationT
protected static final File ROLE_CORP_AUTH_FILE = new File(TEST_DIR, "role-corp-auth.xml");
protected static final String ROLE_CORP_AUTH_OID = "12345678-d34d-b33f-f00d-55555555aaaa";

protected static final TestResource USER_LOCALIZED = new TestResource(TEST_DIR, "user-localized.xml", "c46f4b09-2200-4977-88bc-da1f3ffd0b42");
protected static final TestResource ROLE_LOCALIZED = new TestResource(TEST_DIR, "role-localized.xml", "25294519-5e0e-44d4-bebc-ea549d850ed9");
protected static final TestResource<UserType> USER_LOCALIZED = new TestResource<>(TEST_DIR, "user-localized.xml", "c46f4b09-2200-4977-88bc-da1f3ffd0b42");
protected static final TestResource<RoleType> ROLE_LOCALIZED = new TestResource<>(TEST_DIR, "role-localized.xml", "25294519-5e0e-44d4-bebc-ea549d850ed9");

protected static final File[] ROLE_CORP_FILES = {
ROLE_METAROLE_SOD_NOTIFICATION_FILE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
*/
@ContextConfiguration(locations = { "classpath:ctx-model-test-main.xml" })
@DirtiesContext(classMode = ClassMode.AFTER_CLASS)
public class TestAssignmentEvaluator extends TestAbstractAssignmentEvaluator {
public class TestAssignmentEvaluator extends AbstractAssignmentEvaluatorTest {

protected static final File[] ROLE_CORP_FILES = {
private static final File[] ROLE_CORP_FILES = {
ROLE_METAROLE_SOD_NOTIFICATION_FILE,
ROLE_CORP_AUTH_FILE,
ROLE_CORP_GENERIC_METAROLE_FILE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

@ContextConfiguration(locations = { "classpath:ctx-model-test-main.xml" })
@DirtiesContext(classMode = ClassMode.AFTER_CLASS)
public class TestAssignmentEvaluatorDynamic extends TestAbstractAssignmentEvaluator {
public class TestAssignmentEvaluatorDynamic extends AbstractAssignmentEvaluatorTest {

protected static final File ROLE_CORP_GENERIC_METAROLE_DYNAMIC_FILE = new File(TEST_DIR, "role-corp-generic-metarole-dynamic.xml");

Expand Down

0 comments on commit e79c946

Please sign in to comment.