Spring Security ACL can't be owned by a GrantedAuthoritySid

Describe the bug ACL can't be owned by a GrantedAuthoritySid

To Reproduce As user with a role named TEST, after successfully changing an ACL ownership in this way:

acl.setOwner(new GrantedAuthoritySid("ROLE_TEST"));
aclService.updateAcl(acl);

I can't perform further modifications to the ACL (eg. changing back the ownership to me).

see: https://github.com/spring-projects/spring-security/blob/master/acl/src/main/java/org/springframework/security/acls/domain/AclAuthorizationStrategyImpl.java#L92

Expected behavior Any user with ROLE_TEST granted authority should be able to change the ACL.

Comment From: jgrandja

@bberto I'm trying to understand the issue but it's not clear at the moment. Can you put together a test that demonstrates the issue and another test that you would expect to pass.

Comment From: bberto

@jgrandja I created a pull request including the test and a proposed fix

Comment From: jgrandja

@bberto Thank you for providing the PR to demonstrate your use case.

The sample code you provided:

acl.setOwner(new GrantedAuthoritySid("ROLE_TEST"))

is not correct from a logical entity relationship viewpoint.

An Acl is an entity and so is the Acl.getOwner(). A Principal is an entity, which owns an Acl. However, a GrantedAuthority is an attribute of a Principal and NOT an entity representation. Therefore, an Acl cannot be owned by a GrantedAuthoritySid since it's an attribute representation and NOT an entity - only an entity can own an Acl.

Hope this makes sense?

Based on this, I'm going to close this issue and associated PR.

Comment From: bberto

Your explanation is reasonable. But, reading documentation, seems that Acl is not owned by a Principal but by a Sid:

Sid: The ACL module needs to refer to principals and GrantedAuthority[] s. A level of indirection is provided by the Sid interface, which is an abbreviation of "security identity". Common classes include PrincipalSid (to represent the principal inside an Authentication object) and GrantedAuthoritySid

ACL_SID allows us to uniquely identify any principal or authority in the system ("SID" stands for "security identity") [...] ACL_OBJECT_IDENTITY stores information for each unique domain object instance in the system. Columns include [...] a foreign key to the ACL_SID table to represent the owner of the domain object instance

I think that, if only a Principal can own an Acl, this should be enforced in the API. At the moment I can perform the provided code: acl.setOwner(new GrantedAuthoritySid("ROLE_TEST")); but this bring to an "invalid" ownership and after that I can't perform further changes.

My PR allow to correctly manage Acl after this assignment. Otherwise I think that setOwner method (and documentation) should be reviewed.

Comment From: jgrandja

@bberto You have a valid point. I spoke to @rwinch about this and we're going to go ahead with the PR. I'll re-open and re-review.