Skip to content

Commit

Permalink
Merge pull request #797 from dsmf/fix/649-should-not-accept-subset-of…
Browse files Browse the repository at this point in the history 
…-and-constraints

Fix/649 should not accept subset of and constraints
  • Loading branch information
@ds-jhartmann
ds-jhartmann authored Jul 12, 2024
2 parents 1be0037 + 51429f5 commit 996b3ac
Show file tree
Hide file tree
Showing 5 changed files with 363 additions and 268 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ _**For better traceability add the corresponding GitHub issue number in each cha
### Fixed

- Access and Usage Policy Validation flow correction. #757
- IRS Policy Validation does not accept subset of AND constraint any longer. #649

### Changed

Expand Down
13 changes: 12 additions & 1 deletion ...nt/src/main/java/org/eclipse/tractusx/irs/edc/client/policy/ConstraintCheckerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,26 +54,37 @@ public boolean hasAllConstraint(final Policy acceptedPolicy, final List<Constrai

private boolean isValidOnList(final Constraint constraint, final List<Constraints> acceptedConstraintsList) {
return acceptedConstraintsList.stream()
.anyMatch(acceptedConstraints -> isSameAs(constraint, acceptedConstraints));
.allMatch(acceptedConstraints -> isSameAs(constraint, acceptedConstraints));
}

private boolean isSameAs(final Constraint constraint, final Constraints acceptedConstraints) {

if (constraint instanceof AtomicConstraint atomicConstraint) {
return acceptedConstraints.getOr().stream().anyMatch(p -> isSameAs(atomicConstraint, p))
|| acceptedConstraints.getAnd().stream().anyMatch(p -> isSameAs(atomicConstraint, p));
}

if (constraint instanceof AndConstraint andConstraint) {

// AND means the number of constraints must be the same
if (acceptedConstraints.getAnd() != null
&& acceptedConstraints.getAnd().size() != andConstraint.getConstraints().size()) {
return false;
}

return andConstraint.getConstraints()
.stream()
.allMatch(constr -> isInList(constr, Optional.ofNullable(acceptedConstraints.getAnd())
.orElse(Collections.emptyList())));
}

if (constraint instanceof OrConstraint orConstraint) {
return orConstraint.getConstraints()
.stream()
.anyMatch(constr -> isInList(constr, Optional.ofNullable(acceptedConstraints.getOr())
.orElse(Collections.emptyList())));
}

return false;
}

Expand Down
105 changes: 68 additions & 37 deletions ...rc/test/java/org/eclipse/tractusx/irs/edc/client/policy/ConstraintCheckerServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,17 @@
import org.eclipse.edc.policy.model.AndConstraint;
import org.eclipse.edc.policy.model.AtomicConstraint;
import org.eclipse.edc.policy.model.OrConstraint;
import org.eclipse.tractusx.irs.edc.client.testutil.CamelCaseToSpacesDisplayNameGenerator;
import org.eclipse.tractusx.irs.edc.client.testutil.TestConstants;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.DisplayNameGeneration;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

@DisplayNameGeneration(CamelCaseToSpacesDisplayNameGenerator.class)
class ConstraintCheckerServiceTest {

ConstraintCheckerService cut = new ConstraintCheckerService();
private final ConstraintCheckerService cut = new ConstraintCheckerService();

@Test
void shouldAcceptSimpleAtomicConstraint() {
Expand All @@ -47,7 +52,7 @@ void shouldAcceptSimpleAtomicConstraint() {
final AtomicConstraint simpleAtomicConstraint = createAtomicConstraint(TestConstants.PURPOSE,
TestConstants.ID_3_1_TRACE);

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(simpleAtomicConstraint));
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(simpleAtomicConstraint));

assertThat(result).isTrue();
}
Expand All @@ -60,7 +65,7 @@ void shouldNotAcceptWrongLeftOperandAtomicConstraint() {
final AtomicConstraint simpleAtomicConstraint = createAtomicConstraint(unknownLeftExpression,
TestConstants.ID_3_1_TRACE);

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(simpleAtomicConstraint));
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(simpleAtomicConstraint));

assertThat(result).isFalse();
}
Expand All @@ -73,7 +78,7 @@ void shouldNotAcceptWrongRightOperandAtomicConstraint() {
final AtomicConstraint simpleAtomicConstraint = createAtomicConstraint(TestConstants.PURPOSE,
unknownRightExpression);

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(simpleAtomicConstraint));
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(simpleAtomicConstraint));

assertThat(result).isFalse();
}
Expand All @@ -91,26 +96,49 @@ void shouldAcceptAndConstraint() {
new Operand(TestConstants.MEMBERSHIP, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));

assertThat(result).isTrue();
}

@Test
void shouldNotAcceptAndConstraintWithOneLessElement() {
final AndConstraint andConstraint = createAndConstraint(
List.of(createAtomicConstraint(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY,
TestConstants.STATUS_ACTIVE),
createAtomicConstraint(TestConstants.MEMBERSHIP, TestConstants.STATUS_ACTIVE),
createAtomicConstraint(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));
@Nested
@DisplayName("Tests asserting that for AND all must match in both ways (no subset match allowed)")
class SubsetAndTests {
@Test
void shouldNotAcceptAndConstraintWithOneMoreElement() {

final Policy acceptedPolicy = createPolicyWithAndConstraint(
List.of(new Operand(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));
final AndConstraint andConstraint = createAndConstraint(
List.of(createAtomicConstraint(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY,
TestConstants.STATUS_ACTIVE),
createAtomicConstraint(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));
final Policy acceptedPolicy = createPolicyWithAndConstraint(
List.of(new Operand(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.MEMBERSHIP, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

assertThat(result).isFalse();
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));

assertThat(result).isFalse();
}

@Test
void shouldNotAcceptAndConstraintWithOneLessElement() {

final AndConstraint andConstraint = createAndConstraint(
List.of(createAtomicConstraint(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY,
TestConstants.STATUS_ACTIVE),
createAtomicConstraint(TestConstants.MEMBERSHIP, TestConstants.STATUS_ACTIVE),
createAtomicConstraint(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

final Policy acceptedPolicy = createPolicyWithAndConstraint(
List.of(new Operand(TestConstants.FRAMEWORK_AGREEMENT_TRACEABILITY, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));

assertThat(result).isFalse();
}
}

@Test
Expand All @@ -127,7 +155,7 @@ void shouldRejectAndConstraintWhenOneIsDifferent() {
new Operand(unknownLeftExpression, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(andConstraint));

assertThat(result).isFalse();
}
Expand All @@ -144,7 +172,7 @@ void shouldAcceptOrConstraint() {
new Operand(TestConstants.MEMBERSHIP, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(orConstraint));
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(orConstraint));

assertThat(result).isTrue();
}
Expand All @@ -161,7 +189,7 @@ void shouldRejectOrConstraintIfAnyMatch() {
List.of(new Operand(unknownLeftExpression, TestConstants.STATUS_ACTIVE),
new Operand(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));

boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(orConstraint));
final boolean result = cut.hasAllConstraint(acceptedPolicy, List.of(orConstraint));

assertThat(result).isFalse();
}
Expand All @@ -180,12 +208,14 @@ void shouldBeNullsafeOnNoAnd() {
.toList();

final Policy acceptedOrPolicy = createPolicyWithConstraint(new Constraints(null, constraints));
final boolean resultOr = cut.hasAllConstraint(acceptedOrPolicy, List.of(orConstraint, andConstraint));
assertThat(resultOr).isFalse();
final boolean result = cut.hasAllConstraint(acceptedOrPolicy, List.of(orConstraint, andConstraint));

assertThat(result).isFalse();
}

@Test
void shouldBeNullsafeOnNoOr() {

final OrConstraint orConstraint = createOrConstraint(
List.of(createAtomicConstraint(TestConstants.PURPOSE, TestConstants.ID_3_1_TRACE)));
final AndConstraint andConstraint = createAndConstraint(
Expand All @@ -196,33 +226,34 @@ void shouldBeNullsafeOnNoOr() {
.map(operand -> new Constraint(operand.left(),
new Operator(OperatorType.EQ), operand.right()))
.toList();

final Policy acceptedAndPolicy = createPolicyWithConstraint(new Constraints(constraints, null));
final boolean resultAnd = cut.hasAllConstraint(acceptedAndPolicy, List.of(orConstraint, andConstraint));
assertThat(resultAnd).isFalse();

final boolean result = cut.hasAllConstraint(acceptedAndPolicy, List.of(orConstraint, andConstraint));

assertThat(result).isFalse();
}

private Policy createPolicyWithAndConstraint(List<Operand> operands) {
List<Constraint> and = operands.stream()
.map(operand -> new Constraint(operand.left, new Operator(OperatorType.EQ),
operand.right))
.toList();
Constraints constraints = new Constraints(and, new ArrayList<>());
final List<Constraint> and = operands.stream()
.map(operand -> new Constraint(operand.left, new Operator(OperatorType.EQ),
operand.right))
.toList();
final Constraints constraints = new Constraints(and, new ArrayList<>());
return createPolicyWithConstraint(constraints);
}

private Policy createPolicyWithOrConstraint(List<Operand> operands) {
List<Constraint> or = operands.stream()
.map(operand -> new Constraint(operand.left, new Operator(OperatorType.EQ),
operand.right))
.toList();
Constraints constraints = new Constraints(new ArrayList<>(), or);
final List<Constraint> or = operands.stream()
.map(operand -> new Constraint(operand.left, new Operator(OperatorType.EQ),
operand.right))
.toList();
final Constraints constraints = new Constraints(new ArrayList<>(), or);
return createPolicyWithConstraint(constraints);
}

private Policy createPolicyWithConstraint(Constraints constraints) {
Permission permission = new Permission(PolicyType.ACCESS, constraints);
List<Permission> permissions = List.of(permission);
final Permission permission = new Permission(PolicyType.ACCESS, constraints);
final List<Permission> permissions = List.of(permission);
final String policyId = "policyId";
return org.eclipse.tractusx.irs.edc.client.policy.Policy.builder()
.policyId(policyId)
Expand Down
Loading

0 comments on commit 996b3ac

Please sign in to comment.