Skip to content

Commit

Permalink
First round of real AssignmentEvaluator fixes; with some new tests ad…
Browse files Browse the repository at this point in the history 
…ded.

MID-3679 (roleMembershipRef / parentOrgRef contains also roles/orgs from disabled assignments)
NPE in construction evaluation with mode of null
gathering focus mappings, authorizations, GUI config and policy rules when mode is ZERO or null
thrown out outdated algorithm for parentOrgRef maintenance
  • Loading branch information
@mederly
mederly committed Mar 13, 2017
1 parent 4494079 commit 87328ee
Show file tree
Hide file tree
Showing 7 changed files with 501 additions and 388 deletions.
14 changes: 9 additions & 5 deletions infra/prism/src/main/java/com/evolveum/midpoint/prism/PrismReferenceValue.java
View file
Expand Up @@ -363,10 +363,10 @@ public boolean isEmpty() {
}

/**
* Returns a version of this value that is cannonical, that means it has the minimal form.
* Returns a version of this value that is canonical, that means it has the minimal form.
* E.g. it will have only OID and no object.
*/
public PrismReferenceValue toCannonical() {
public PrismReferenceValue toCanonical() {
PrismReferenceValue can = new PrismReferenceValue();
can.setOid(getOid());
// do NOT copy object
Expand Down Expand Up @@ -597,15 +597,19 @@ public String debugDump(int indent, boolean expandObject) {

@Override
public PrismReferenceValue clone() {
return clone(true);
}

public PrismReferenceValue clone(boolean copyFullObject) {
PrismReferenceValue clone = new PrismReferenceValue(getOid(), getOriginType(), getOriginObject());
copyValues(clone);
copyValues(clone, copyFullObject);
return clone;
}

protected void copyValues(PrismReferenceValue clone) {
protected void copyValues(PrismReferenceValue clone, boolean copyFullObject) {
super.copyValues(clone);
clone.targetType = this.targetType;
if (this.object != null) {
if (this.object != null && copyFullObject) {
clone.object = this.object.clone();
}
clone.description = this.description;
Expand Down
2 changes: 2 additions & 0 deletions ...-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignmentTarget.java
View file
Expand Up @@ -47,4 +47,6 @@ public interface EvaluatedAssignmentTarget extends DebugDumpable {
AssignmentType getAssignment();

AssignmentPath getAssignmentPath();

boolean isValid();
}
46 changes: 25 additions & 21 deletions ...l/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentEvaluator.java
View file
Expand Up @@ -326,21 +326,19 @@ private <O extends ObjectType> boolean evaluateSegmentContent(AssignmentPathSegm
// Here we ignore "reallyValid". It is OK, because reallyValid can be false here only when
// evaluating direct assignments; and invalid ones are marked as such via EvaluatedAssignment.isValid.
// (This is currently ignored by downstream processing, but that's another story. Will be fixed soon.)
// ---
// But we also ignore "mode". This is because focus mappings are not categorized into PLUS/MINUS/ZERO sets.
// They are simply evaluated as they are: skipped only if both condOld and condNew is false.
// This is less sophisticated than constructions, but for the time being it is perhaps OK.
// TODO But shouldn't we skip focus mapping with mode==null? And with mode=MINUS?
evaluateFocusMappings(segment, ctx);
if (isNonNegative(mode)) {
evaluateFocusMappings(segment, ctx);
}
}
if (assignmentType.getPolicyRule() != null && !loginMode) {
// We can ignore "reallyValid" for the same reason as for focus mappings.
// TODO but what if mode is null or MINUS?
if (segment.isMatchingOrder()) {
collectPolicyRule(true, segment, ctx);
}
if (segment.isMatchingOrderPlusOne()) {
collectPolicyRule(false, segment, ctx);
if (isNonNegative(mode)) {
if (segment.isMatchingOrder()) {
collectPolicyRule(true, segment, ctx);
}
if (segment.isMatchingOrderPlusOne()) {
collectPolicyRule(false, segment, ctx);
}
}
}
if (assignmentType.getTarget() != null || assignmentType.getTargetRef() != null) {
Expand Down Expand Up @@ -422,6 +420,9 @@ private void collectConstruction(AssignmentPathSegmentImpl segment, PlusMinusZer
construction.setValid(isValid);

// Do not evaluate the construction here. We will do it in the second pass. Just prepare everything to be evaluated.
if (mode == null) {
return; // null mode (i.e. plus + minus) means 'ignore the payload'
}
switch (mode) {
case PLUS:
ctx.evalAssignment.addConstructionPlus(construction);
Expand Down Expand Up @@ -625,15 +626,12 @@ private void evaluateSegmentTarget(AssignmentPathSegmentImpl segment,
}
}

EvaluatedAssignmentTargetImpl evalAssignmentTarget = new EvaluatedAssignmentTargetImpl();
evalAssignmentTarget.setTarget(targetType.asPrismObject());
evalAssignmentTarget.setEvaluateConstructions(segment.isMatchingOrder());
evalAssignmentTarget.setAssignment(segment.getAssignment());
evalAssignmentTarget.setDirectlyAssigned(ctx.assignmentPath.size() == 1);
evalAssignmentTarget.setAssignmentPath(ctx.assignmentPath.clone());
EvaluatedAssignmentTargetImpl evalAssignmentTarget = new EvaluatedAssignmentTargetImpl(
targetType.asPrismObject(), segment.isMatchingOrder(),
ctx.assignmentPath.clone(), segment.getAssignment(), isValid);
ctx.evalAssignment.addRole(evalAssignmentTarget, mode);

if (mode != PlusMinusZero.MINUS && segment.isProcessMembership()) {
if ((isNonNegative(mode)) && segment.isProcessMembership()) {
PrismReferenceValue refVal = new PrismReferenceValue();
refVal.setObject(targetType.asPrismObject());
refVal.setTargetType(ObjectTypes.getObjectType(targetType.getClass()).getTypeQName());
Expand Down Expand Up @@ -769,9 +767,9 @@ private void evaluateSegmentTarget(AssignmentPathSegmentImpl segment,
evaluateFromSegment(subAssignmentPathSegment, mode, ctx);
}

if (evaluationOrder.getSummaryOrder() == 1 && targetType instanceof AbstractRoleType) {
if (evaluationOrder.getSummaryOrder() == 1 && targetType instanceof AbstractRoleType && isNonNegative(mode)) {

for(AuthorizationType authorizationType: ((AbstractRoleType)targetType).getAuthorization()) {
for (AuthorizationType authorizationType: ((AbstractRoleType)targetType).getAuthorization()) {
Authorization authorization = createAuthorization(authorizationType, targetType.toString());
ctx.evalAssignment.addAuthorization(authorization);
}
Expand All @@ -788,6 +786,12 @@ private void evaluateSegmentTarget(AssignmentPathSegmentImpl segment,
LOGGER.trace("Evaluating segment target DONE for {}", segment);
}

private boolean isNonNegative(PlusMinusZero mode) {
// mode == null is also considered negative, because it is a combination of PLUS and MINUS;
// so the net result is that for both old and new state there exists an unsatisfied condition on the path.
return mode == PlusMinusZero.ZERO || mode == PlusMinusZero.PLUS;
}

private void checkRelationWithTarget(AssignmentPathSegmentImpl segment, FocusType targetType, QName relation)
throws SchemaException {
if (targetType instanceof AbstractRoleType) {
Expand Down
6 changes: 6 additions & 0 deletions ...l-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/AssignmentPathSegmentImpl.java
View file
Expand Up @@ -413,6 +413,12 @@ public String toString() {
if (assignmentType.getConstruction() != null) {
sb.append("Constr '").append(assignmentType.getConstruction().getDescription()).append("' ");
}
if (assignmentType.getFocusMappings() != null) {
sb.append("FMappings (").append(assignmentType.getFocusMappings().getMapping().size()).append(") ");
}
if (assignmentType.getPolicyRule() != null) {
sb.append("Rule '").append(assignmentType.getPolicyRule().getName()).append("' ");
}
}
ObjectReferenceType targetRef = assignmentType != null ? assignmentType.getTargetRef() : null;
if (target != null || targetRef != null) {
Expand Down
65 changes: 29 additions & 36 deletions ...pl/src/main/java/com/evolveum/midpoint/model/impl/lens/EvaluatedAssignmentTargetImpl.java
View file
@@ -1,4 +1,4 @@
/**
/*
* Copyright (c) 2015-2016 Evolveum
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -15,47 +15,46 @@
*/
package com.evolveum.midpoint.model.impl.lens;

import java.util.ArrayList;
import java.util.Collection;

import com.evolveum.midpoint.model.api.context.EvaluatedAssignmentTarget;
import com.evolveum.midpoint.prism.PrismObject;
import com.evolveum.midpoint.util.DebugUtil;
import com.evolveum.midpoint.xml.ns._public.common.common_3.AbstractRoleType;
import com.evolveum.midpoint.xml.ns._public.common.common_3.AssignmentType;
import com.evolveum.midpoint.xml.ns._public.common.common_3.ExclusionPolicyConstraintType;
import com.evolveum.midpoint.xml.ns._public.common.common_3.FocusType;
import com.evolveum.midpoint.xml.ns._public.common.common_3.PolicyConstraintsType;
import com.evolveum.midpoint.xml.ns._public.common.common_3.*;

import java.util.ArrayList;
import java.util.Collection;

/**
* @author semancik
*
*/
public class EvaluatedAssignmentTargetImpl implements EvaluatedAssignmentTarget {

PrismObject<? extends FocusType> target;
private boolean directlyAssigned;
private boolean evaluateConstructions;
private AssignmentPathImpl assignmentPath; // TODO reconsider (maybe we should store only some lightweight information here)
private AssignmentType assignment;
final PrismObject<? extends FocusType> target;
private final boolean evaluateConstructions;
private final AssignmentPathImpl assignmentPath; // TODO reconsider (maybe we should store only some lightweight information here)
private final AssignmentType assignment;
private Collection<ExclusionPolicyConstraintType> exclusions = null;
private final boolean isValid;

EvaluatedAssignmentTargetImpl(
PrismObject<? extends FocusType> target, boolean evaluateConstructions,
AssignmentPathImpl assignmentPath, AssignmentType assignment,
boolean isValid) {
this.target = target;
this.evaluateConstructions = evaluateConstructions;
this.assignmentPath = assignmentPath;
this.assignment = assignment;
this.isValid = isValid;
}

@Override
public PrismObject<? extends FocusType> getTarget() {
return target;
}

public void setTarget(PrismObject<? extends FocusType> target) {
this.target = target;
}

@Override
public boolean isDirectlyAssigned() {
return directlyAssigned;
}

public void setDirectlyAssigned(boolean directlyAssigned) {
this.directlyAssigned = directlyAssigned;
return assignmentPath.size() == 1;
}

@Override
Expand All @@ -68,32 +67,25 @@ public boolean isEvaluateConstructions() {
return evaluateConstructions;
}

public void setEvaluateConstructions(boolean evaluateConstructions) {
this.evaluateConstructions = evaluateConstructions;
}

@Override
public AssignmentType getAssignment() {
return assignment;
}

public void setAssignment(AssignmentType assignment) {
this.assignment = assignment;
}

@Override
public AssignmentPathImpl getAssignmentPath() {
return assignmentPath;
}

public void setAssignmentPath(AssignmentPathImpl assignmentPath) {
this.assignmentPath = assignmentPath;
}

public String getOid() {
return target.getOid();
}

@Override
public boolean isValid() {
return isValid;
}

public Collection<ExclusionPolicyConstraintType> getExclusions() {
if (exclusions == null) {
exclusions = new ArrayList<>();
Expand Down Expand Up @@ -132,8 +124,9 @@ public String debugDump(int indent) {
DebugUtil.debugDumpWithLabel(sb, "Target", target, indent + 1);
sb.append("\n");
DebugUtil.debugDumpWithLabel(sb, "Assignment", String.valueOf(assignment), indent + 1);
sb.append("\n");
DebugUtil.debugDumpWithLabel(sb, "isValid", isValid, indent + 1);
return sb.toString();
}


}

0 comments on commit 87328ee

Please sign in to comment.