SEC-2201: Add ability to implement a custom Sid

[spring-projects-issues]

stanislav bashkirtsev (Migrated from SEC-2201) said:

From time to time people need to implement a custom Sid. Searches on the internet show that (see links below).
In most cases these people needed to support Groups of Sids. My case was similar. I agree that in most situations people don't need this, but additional flexibility for complicated situations would be great.
The pull request is waiting there for a year, but no one even commented there.

The change is very simple:

• Adding a method Sid#getSidId() instead of casting Sids all the time to PrincipalSid or GrantedAuthority
• Adding a SidFactory which can be replaced by anyone who's using the framework

This pull request doesn't actually change anything, the logic stays the same, but flexibility is added.
Here are examples of people asking for this flexibility:

• http://forum.springsource.org/showthread.php?108967-Spring-Security-3-ACL-and-groups
• http://forum.springsource.org/showthread.php?70412-Groups-and-ACL-s-correct
• http://forum.springsource.org/showthread.php?59446-Can-a-group-name-be-used-as-a-SID-in-an-ACE
• http://stackoverflow.com/questions/6940321/spring-acl-questions
• http://stackoverflow.com/questions/1920959/group-and-acl-on-spring-security

[spring-projects-issues]

stanislav bashkirtsev said:

I could even help with writing docs covering this functionality.

[spring-projects-issues]

Rob Winch said:

First, my sincere apologies about the delays in responding to this. I have some feedback on the PR submitted.

• There are non-passive changes to core interfaces. While, this is going to be a major release, these non-passive changes will not be made lightly. We should try to figure out a way to make this passively. There are likely other ways this could be implemented passively, and we should strive for that since many people build off of the interfaces.
• Based upon SidFactory#create(String,boolean) accepting a boolean for the principal or not, the current implementation seems to assume a principal or not principal approach. This seems limited to me. How would we support principal, groups, and roles for example? Perhaps you can provide an example of how you would intend this to be extended?
• As you mentioned we will need documentation

[spring-projects-issues]

stanislav bashkirtsev said:

There are non-passive changes to core interfaces
Could you explain what you mean by active/passive change? For me passive means that it won't affect anyone/anything and active - that it will. This particular change doesn't affect anything, even though it adds a notion of SidFactory, the code uses default implementation which is exactly the same as it was before.

How would we support principal, groups, and roles for example? Perhaps you can provide an example of how you would intend this to be extended?Well, what we did in our project is we implemented Sid interface with UserSid and GroupSid along with our own SidFactory. And here is how the data look for us:

+----+-----------+--------------------+
| id | principal | sid                |
+----+-----------+--------------------+
|  6 |         1 | user:2             |
|  7 |         1 | user:3             |
|  1 |         1 | user:anonymousUser |
|  2 |         0 | usergroup:1        |
|  3 |         0 | usergroup:2        |
+----+-----------+--------------------+```
That's just our example of how this can be implemented. Another option was to alter the table *acl_sid* which looked to be too large, so we decided to go with the simplest approach which worked perfectly for us.

As you mentioned we will need documentationOkay, but first you should decide whether you'll accept the change ;)

[spring-projects-issues]

Rob Winch said:

Thank you for your fast response.

Could you explain what you mean by active/passive change?

I mean that Sid is a core interface to Spring Security. Due to the changes made in this PR, if custom implementations of Sid exist, then the code would no longer compile (the custom implementations would not have the newly added methods on it).

I think we might be able to reach a middle ground that remains passive and allows you to achieve your custom requirements.

• AclAuthorizationStrategyImpl would introduce a new method protected Sid createCurrentUser(Authentication). You would then be able to override this method and delegate to your custom SidFactory
• BasicLookupStrategy would be updated
  ** Remove the final clause from the class
  ** All existing public methods would be marked as final
  ** Add a new method protected Sid createSid(boolean isPrincipal, String sid). You would then be able to override this method and delegate to your custom SidFactory
• JdbcMutableAclService would have a new protected method final Long createOrRetrieveSidPrimaryKey(String sidName, boolean sidIsPrincipal, boolean allowCreate). You would then be able to override this method and delegate to your custom SidFactory

The suggested changes minimize the changes for Spring Security and still allow you to meet your custom requirements.

• Spring Security would not need DefaultSidFactory or SidFactory, but you can keep these in your custom code base
• The non-passive changes to Sid, GrantedAuthoritySid, and PrincipalSid would not be necessary. Your custom codebase can extend Sid (i.e. SmartSid) and the custom SidFactory can return custom SmartSid implementations.
• SidRetrievalStrategyImpl is extremely simple, so you can create your own implementation of this that delegates to your custom DefaultSidFactory

Please let me know if this will let you accomplish your goals.

Note that at the moment these changes are not high on my priority list, but if you are able to send an updated PR with these changes I would be happy to merge them. I promise much faster responses in the future (again sorry about that). I think assuming there is sufficient Javadoc we would not need updates to the reference for these changes either.

[spring-projects-issues]

stanislav bashkirtsev said:

I mean that Sid is a core interface to Spring Security. Due to the changes made in this PR, if custom implementations of Sid exist, then the code would no longer compile (the custom implementations would not have the newly added methods on it).Aha, got it. Well, I think that's not a concern here because there is no way you can use your custom Sid with Spring ACL because Principal & GrantedAuthority are hardcoded inside of the framework. Even if you implement a Sid interface, you won't be able to make SS work with it. That's why such PR was created in the first place. The only option you have is to extend existing Sids and pass these objects at least to some methods, but then you won't have any compilation troubles - the newly added method is implemented by existing Sids already.

Please let me know if this will let you accomplish your goals.I think it will. But using current PR you have much cleaner code. Just imagine that you're creating these classes from the scratch - isn't there any reason why you won't create an interface instead of these if's? The code will be more complicated both in core SS and implementations. Moreover you'll might have to add Sids to other classes, do you really want to use if-else there as well?

If we don't reach an agreement on this, I'll try to create a PR with the option you choose. It's still better than using a patched version of SS :)

[spring-projects-issues]

Rob Winch said:

Well, I think that's not a concern here because there is no way you can use your custom Sid with Spring ACL because Principal & GrantedAuthority are hardcoded inside of the framework. Even if you implement a Sid interface, you won't be able to make SS work with it.

If there is an interface that is public people will use it. You may not have been able to get it to work with the provided jdbc acl support. However, that doesn't mean that others haven't been able to get something to work. Additionally, people are likely to implement their on acl support. One example comes to mind is the UserSid example you provided which implements Sid. A very quick search of github shows quite a few examples of others implementing the Sid interface. Of course my search is not perfect and only shows the publicly available implementations of Sid on github. I'm quite confident that after over 6 years of the Sid interface existing, if we could look in other public / private corporate repositories we would find many other examples.

But using current PR you have much cleaner code.

The current PR is non passive, so it is really a no go. I am all about ensuring we enable these use cases, but this is not something I see provided within the framework itself.

If we don't reach an agreement on this, I'll try to create a PR with the option you choose.

This sounds good. I will keep an eye out for the PR. You might mention it on the JIRA as my github notifications can get out of control and I might miss it.

PS: One thing to note that is lacking in our current ACL support is that you must do the checks after the results come back from the database. This causes all sorts of problems when using paging for large data sets. I am currently interested in investing access control support with Spring Data JPA (i.e. think automatically updates the query based upon access control). Eventually we would extend support for other Spring Data implementations. For more information, see SEC-2409

[spring-projects-issues]

Rob Winch said:

Moved to 4.0.x Backlog until PR is available

[spring-projects-issues]

stanislav bashkirtsev said:

We at last have sent you a new version of the change as you asked: PR #115. I'm closing the original one as it's obsolete.

[spring-projects-issues]

Rob Winch said:

Thank you for the updates. I will take a look at this next week.

[spring-projects-issues]

Mikhail Stryzhonok said:

We have updated pull request. Unit tests and simple example were added.

[spring-projects-issues]

Rob Winch said:

Thanks for the PR. This is merged into master now f20219d