Skip to content

Prevent circular ACL parent relationships #19557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yashdotdev13 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prevent circular ACL parent relationships #19557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,11 @@ public void setOwner(Sid newOwner) {
@Override
public void setParent(@Nullable Acl newParent) {
this.aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL);
Assert.isTrue(newParent == null || !newParent.equals(this), "Cannot be the parent of yourself");
Acl parent = newParent;
while (parent != null) {
Assert.isTrue(parent != this, "Cannot create a circular parent relationship");
parent = parent.getParentAcl();
}
this.parentAcl = newParent;
}

Expand Down
27 changes: 27 additions & 0 deletions acl/src/test/java/org/springframework/security/acls/domain/AclImplTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,33 @@ public void gettersAndSettersAreConsistent() {
assertThat(new PrincipalSid("ben")).isEqualTo(acl.getOwner());
}

@Test
public void setParentRejectsAncestor() {
MutableAcl parentAcl = new AclImpl(new ObjectIdentityImpl(TARGET_CLASS, 101), 101,
this.authzStrategy, this.pgs, null, null, true, new PrincipalSid("joe"));
MutableAcl childAcl = new AclImpl(new ObjectIdentityImpl(TARGET_CLASS, 102), 102,
this.authzStrategy, this.pgs, null, null, true, new PrincipalSid("joe"));

childAcl.setParent(parentAcl);

assertThatIllegalArgumentException().isThrownBy(() -> parentAcl.setParent(childAcl));
}

@Test
public void setParentRejectsIndirectAncestor() {
MutableAcl grandParentAcl = new AclImpl(new ObjectIdentityImpl(TARGET_CLASS, 101), 101,
this.authzStrategy, this.pgs, null, null, true, new PrincipalSid("joe"));
MutableAcl parentAcl = new AclImpl(new ObjectIdentityImpl(TARGET_CLASS, 102), 102,
this.authzStrategy, this.pgs, null, null, true, new PrincipalSid("joe"));
MutableAcl childAcl = new AclImpl(new ObjectIdentityImpl(TARGET_CLASS, 103), 103,
this.authzStrategy, this.pgs, null, null, true, new PrincipalSid("joe"));

parentAcl.setParent(grandParentAcl);
childAcl.setParent(parentAcl);

assertThatIllegalArgumentException().isThrownBy(() -> grandParentAcl.setParent(childAcl));
}

@Test
public void isSidLoadedBehavesAsExpected() {
List<Sid> loadedSids = Arrays.asList(new PrincipalSid("ben"), new GrantedAuthoritySid("ROLE_IGNORED"));
Expand Down