Skip to content
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.

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

SpringSecurityUserProvider should populate SimpleFeatureUser with granted authorities #67

Closed
parxier opened this issue Jan 15, 2014 · 10 comments

Comments

@parxier
Copy link

@parxier parxier commented Jan 15, 2014

I'm trying to implement Spring Security based ActivationStrategy that would activate feature based on authorities granted to the user. I can read authorities from authentication security context and make a decision based on that, but then passed in FeatureUser parameter would be of no use to me.

In keeping everything abstracted nicely and since SpringSecurityUserProvider already is reading and iterating through granted authorities I thought it'd be nice if SpringSecurityUserProvider could populate SimpleFeatureUser returned with collection of granted authorities via setAttribute() call. Then ActivationStrategy can simply read granted authorities from FeatureUser parameter instead of reading from security context again. This way implemented ActivationStrategy wouldn't even be tied to Spring Security granted authorities and can be used with other security frameworks as far as list of granted authorities can be read from FeatureUser attribute.

@parxier
Copy link
Author

@parxier parxier commented Jan 15, 2014

I'll try to provide pull request if I have time.

@chkal
Copy link
Member

@chkal chkal commented Jan 15, 2014

Hey @parxier,

that's a fantastic idea and totally makes sense to me. I'm looking forward to your pull request. Let me know if you need any help or additional information.

Christian

parxier pushed a commit to parxier/togglz that referenced this issue Jan 16, 2014
- make SpringSecurityUserProvider to populate "authorities" user attribute with a set of granted authorities;
- add UserAuthorityActivationStrategy activation strategy that is configurable with a comma separated
  list of authorities and activates feature if user has at least one of those authorities;
@parxier
Copy link
Author

@parxier parxier commented Jan 16, 2014

I haven't worked with github before. It doesn't seem to allow me to create a pull request from a single commit. I pushed few other things into my forked repo which I don't want to include in the pull request.

You can have a look at my commit if you want until I figure out how to create a proper pull request. :-)

SHA: 8302b71

@parxier
Copy link
Author

@parxier parxier commented Jan 16, 2014

Also I'm not sure if spring-security module is the right place for UserAuthorityActivationStrategy since it has no direct dependency on Spring Security. Maybe togglz-core is the better place for it?

@chkal
Copy link
Member

@chkal chkal commented Jan 18, 2014

Hey @parxier!

Thanks a lot. I'll have a look at your commit ASAP. I'm very busy currently, so it may take some days. Sorry.

Christian

chkal added a commit that referenced this issue Jan 20, 2014
- make SpringSecurityUserProvider to populate "authorities" user attribute with a set of granted authorities;
- add UserAuthorityActivationStrategy activation strategy that is configurable with a comma separated
  list of authorities and activates feature if user has at least one of those authorities;
@chkal
Copy link
Member

@chkal chkal commented Jan 20, 2014

Hey @parxier,

AFAIK GitHub doesn't allow pull requests from single commits. If you already committed the change together with other stuff you would have to create a new branch, cherry-pick the commit and then send the pull request based on this branch. But don't worry, I already did this for you.

I merged your pull request. It looks awesome. And it is great that you included tests. :)

Maybe you are right and UserAuthorityActivationStrategy should go into the core module. Enabling features for users with certain permissions is a very common use case. And I think it will be very easy to populate the user attribute in the UserProvider implementations for other frameworks.

However, to be honest, in this case I would like to rename the strategy. I think the term "authority" isn't used so much in other security frameworks and is not very common (outside of Spring Security) for the concept of a "role". We could think about renaming strategy to something like UserRoleActivationStrategy.

@parxier
Copy link
Author

@parxier parxier commented Jan 20, 2014

I'm glad you liked it. Regarding tests I have worked in "95%+ test coverage required to push" dev environments for too long to dodge writing tests. It just feels doing something really really bad otherwise. :-)

Feel free to rename strategy whatever you like, "role" instead of "authority" is absolutely fine by me. It will require changing some of the constants and attribute names in effected classes though. Give me a shout if you need help with that.

@chkal
Copy link
Member

@chkal chkal commented Jan 20, 2014

Awesome. :)

I think I will find some time this evening for this refactoring.

Thanks again for your contribution. :)

@chkal chkal closed this Jan 20, 2014
@chkal
Copy link
Member

@chkal chkal commented Jan 21, 2014

I just pushed out the refactoring we talked about. So the latest 2.1.0-SNAPSHOT will contain the new structure as soon as the CI server has finished deploying the snapshots.

It would be great if you could give the snapshots a try so we can make sure everything works as expected.

Thanks again for your help with this. :)

@parxier
Copy link
Author

@parxier parxier commented Jan 21, 2014

No problem. I'll give it a try.

On 21 January 2014 17:58, Christian Kaltepoth notifications@github.comwrote:

I just pushed out the refactoring we talked about. So the latest
2.1.0-SNAPSHOT will contain the new structure as soon as the CI server has
finished deploying the snapshots.

It would be great if you could give the snapshots a try so we can make
sure everything works as expected.

Thanks again for your help with this. :)


Reply to this email directly or view it on GitHubhttps://github.com//issues/67#issuecomment-32825518
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.