SEC-479: New Acl Package enhancements/issues

[spring-projects-issues]

David Geary(Migrated from SEC-479) said:

After some work with the new acls package I would like to raise the following issues / feedback.

1) AFTER INVOCATION COLLECTION FILTERING
The collection filtering functionality could be more reusable, ie instead of AbstractACLProvider being a super class for AclEntryAfterInvocationCollectionFilteringProvider and AclEntryAfterInvocationProvider these two classes (which themselves contain no ACL code) should allow the logic in the hasPermission(authentication, domainObject) method to be injected as the implementation of an interface.

The AbstractACLProvider would then become the supplied implementation of this interface, but it would be possible to supply other ways of authorising object access instead of using ACLs, for example we are looking at a ‘rule’ based mechanism that would match authorities in the current authentication object with ‘attributes’ of the object providing a way to filter the object list without ACLs eg a user can only see green cars (this may be a generalisation of the label voter in SEC-354)

This would also allow (via customisation) the hasPermission method to delegate to more than one mechanism for authorisation if a combination is needed (this would perform better than chaining providers as the collection would only need to be navigated once) and it would allow (again via customisation) the ability to use inner objects in the domain object by modifying the domain object before passing it to the authorisation mechanism (similar to the setInternalMethod mechanism in the voters and would enable a custom implementation of the functionality required in SEC-16)

I can provide my modified code for this if that’s useful – without this I find you have to reimplement the entire collection filtering functionally to modify it slightly.

2) OBJECTIDENTITY ISSUES

a) Enforcing use of the Class type in getJavaType() on the ObjectIdentity interface seems overly restrictive, it would be more flexible if this returned a string (and the default implementation used the class name) – its converted to a string for storage in the acl tables anyway.

For example we have a brief version of an object and a full version – they have different classes but are the same type and represent the same instance. Additionally we have a generic flexible data object that has its own internal type that would make more sense to use as the acl type. The old acl package allowed a pluggable strategy to return a string for the id/type combination which would have allowed overriding the use of the java class.

Instead of Class getJavaType() this could just be Serializable getType()

b) The java interface allows a string id (Serializable getIdentifier()), but the jdbc implementation insists it is a long – maybe the acl table should allow string ids?

c) The current ObjectIdentity instance needs both the id and type. There needs to be some way to handle the common situation where you only have the objects id. Eg securing service/dao methods such as

deleteDomainObject(long id)
retrieveDomainObject(long id)
or various update style operations which may not supply the full object (on the service layer).

Currently the new acl code relies on the full object to extract the id. It’s possible to use the long id as the domain object and a custom object identity retrieval strategy that has the object type set on it as a way around this but I think this needs more support in the framework eg being able to supply the object type / class on the voter

For the read methods this can be handled using the AclEntryAfterInvocationProvider as at that point you have the full object, but then what is the point of using a voter with read permission – ie does it ever make sense to use a voter for reads? Even if the answer is to use AclEntryAfterInvocationProvider the delete and update issues still need addressed.

The contacts example gets round this (to support both the aspect and the business logic of accessing the acl in the implementaion to delete it) by having the service method

public void delete(Contact contact)

but this seems artificial – this would surely normally be:

public void delete(long contactID)

and a method such as

public void changeEmail(long contactID, String newEmaiAddress)

would present the same problem.

3) ACL IMPLEMENTATION

a) In the ACLImpl.isGranted() method the permission check is done via an exact match on the masks ie

if ((ace.getPermission().getMask() == permission[i].getMask()) && …..

surely this should just check the required permission is set in the ace ie something like

(ace.getPermission().getMask() & permission[i].getMask()) == permission[i].getMask())

this is what the old acl pacakge did and seems the correct approach ie i want one ACE to specifiy the combination of permissions for a sid , and this should grant a permission if that is set in the ACE mask.

Currently you have to define a separate ACE for each base permission which seems to defeat the purpose of using a bit mask.

b) Again in the ACLImpl.isGranted() the logic is stated as being:

‘If the ACE specifies to grant access, the method will return
true. If the ACE specifies to deny access, the loop will stop and the next permission iteration will be performed’

This implies that any granting ACE will allow the operation, I would have thought the denys would take precedence? Ie you use denys as a specific block, so if there are any denys the action is not allowed. Eg a user is a memebr of a group that allows an access to an object, but they personally have explicitly been denied it. This seems a more natural approach – is a a good reason for doing it the other way round?

4) OTHER ISSUES

a) The SQL does not work on oracle (both in BasicLookupStrategy and JdbcMutableAclService)
b) The current implementation requires an owner to be specified although this is stated to be optional

[spring-projects-issues]

David Geary said:

Some further information on potential changes to the ACLImpl (3 above):

See forum post http://forum.springframework.org/showthread.php?p=139749#post139749 (post 4)

Basically the logic we need in the acl is to apply precedence to denies over grants and to principalsids over grantedauthoritysids.
If the aces are ordered appropriately (user denies, user grants, group denies, group grants) this can save some time as once a deny is found no further checking of the ace list needs to be done, however all sids need to be checked.

At the least it needs to be easier to override the supplied acl implementation – make the ace list accessible from subclases?

[spring-projects-issues]

Ben Alex said:

Should be against component SecurityACL.

[spring-projects-issues]

Taylor Mathewson said:

Looks like the bitmasking stuff has been split off into SEC-1140

Adding this for reference purposes for those who get here via google like I did.

[spring-projects-issues]

Luke Taylor said:

I'm closing this issue as being superseded by the linked issues, which can be addressed individually. If there is anything specific that you feel is missing, then feel free to open other issues (on for each item, rather than as a single entry).