shiro-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tauren Mills <tau...@tauren.com>
Subject Re: Roles and permissions
Date Fri, 17 Jul 2009 19:24:46 GMT
Hi Daniel,

Thanks for all of your thoughts. I appreciate you taking the time to
respond.  My answers are also inline.

>> if ( currentUser.isPermitted( "project:schedule:world_domination" ) ) {
>>    log.info("You are permitted to 'schedule' the 'project' with name
>> (id) 'world_domination'.  ");
>> } else {
>>    log.info("Sorry, you aren't allowed to 'schedule' the
>> 'world_domination' 'project'!");
>> }
>
> The code looks fine in general. There are 2 things I notice:
> 1) isPermitted is a method of the Subject interface.
> Your variable naming implies that you combine your User class with
> Shiro's interface. This may or may not be a good idea. Depends on your
> use case, I guess. The point at issue is that you may be better of not
> to couple your application/business logic to the security framework.

I'm not actually using that code, I just borrowed it as an example
from the getting started page on the shiro site to make my application
easier to describe.  My User object (called Member) isn't combined
with Shiro's interface. My application has a MemberService with this
method:

public Member getCurrentMember() {
	log.debug("Getting current member");
	if (SecurityUtils.getSubject() == null) {
		return null;
	}
	final Long currentMemberId = (Long) SecurityUtils.getSubject().getPrincipal();
	if( currentMemberId != null ) {
		return getMember(currentMemberId);  // performs dao.load(id)
	} else {
		return null;
	}
}

> 2) I personally dislike the if / else style that this snippet would
> presumably grow into. I'd recommend to check with negative logic and
> quit early. This saves a level of indentation and more important one
> layer of cognitive context to manage:
>
> void scheduleProject(String projectId) {
>    Subject subject = SecurityUtils.getSubject();
>    if (!suject.isPermitted("project:schedule:world_domination"))
>        throw new NoPermissionException();
>    // TODO: do the work
> }

I agree that your implementation is more elegant than the sample from
the Shiro getting started page.  Again, I do this differently than the
example I provided. For instance, I have a panel in my app that
displays a list of actions the user can perform if the user is a
"scheduler". So I do something like this:
		
SchedulerActionPanel scheduler = new SchedulerActionPanel("scheduler") {
	@Override
	public boolean isVisible() {
		return SecurityUtils.getSubject().hasRole("scheduler");
	}
};

> re 1) In general, your code would only check for permissions, not
> roles. No calls like subject.hasRole("admin"), only calls to
> subject.isPermitted("project:createNew") etc. Plain functionality and
> permissions match 1:1, while functionality and roles match n:1. So,
> with your application coded against roles, a) every time you add
> functionality you change the meaning of the role. But what's even
> worse is that b) if security needs change (managers shall no longer be
> able to xyz), you'll need to modify your code (design-time change). If
> your application is coded against permissions, the permission check is
> still the same, only the mapping who or which role effectively has
> this permission will need to change (run-time change).

Thanks for this great advice! I think I understand it much better now.
The bottom line is my application should only check for permissions,
not roles. So my isVisible() code above could be changed to something
like:
	return SecurityUtils.getSubject().hasPermission("project:viewScheduleActions");

The user could belong to a Role called "scheduler" that includes the
permission "project:viewScheduleActions".  This is because ALL users
with the "scheduler" permission should be able to view that panel.

>> I can see using Roles for things like:
>>  -- display a Schedule Projects button if I'm a member of the Scheduler role.
>>  -- note that I should see this button if I have either Scheduler or
>> Admin role for any project
>
> As stated above: Don't. Use a permission like
> "project:schedule:the_project_id" and assign this permission to both
> roles (dynamically) in the privilege management system.

Yes, totally makes sense!

> I see. The problem here is, that some of your application logic
> implies access privileges.
> I'd store e.g. the project manager in the application model, and take
> care for some work flow to always change the application model's data
> with the security realms' data in a transactional way.

I'm not totally clear on what you mean.

> All of this is very much dependant on how your application and data
> model will develop.
> You could e.g. just have a n:m mapping of users and projects named
> "involvedInProjects".

I see your point here, but I'm not sure how well it will work in my
case. I've gone into more details below.

>> Should I be using a Permission entity instead of a string? Will Shiro
>> support this?  This way it's easier to build hibernate queries and
>> mappings.  Take for instance:
>
> Actually, Shiro is built around Permission instances. But ever since
> the mighty WildcardPermission got introduced, it prooved so flexible,
> that it became the default and it's very rare that you need anything
> else.

After sending off my message yesterday I discovered the
org.apache.shiro.authz.permission.DomainPermission class. This looks
like exactly what I would need to do it the way I was suggesting to
make it easier to build hibernate queries that return the proper
results that I need.

> My first shot would be to have a single "is involved in" relation
> mapping n users to m projects. You can then query for "all projects,
> the current user is involved in".
> Then iterate over this collection and check if the current user has
> the permission to do something about it.

I understand what you are saying. I would create a simpler hibernate
query that returns all of the projects the user is involved with. Then
I leave it up to my application logic to check each result and decide
if it should be displayed or not using the Shiro api.

The problem I see with this approach is that I use a DataProvider to
paginate data sets. If I ask the DataProvider to return results 1 -
20, it will return the first 20 rows. If my application logic then
checks each row, and hides it, I would be displaying a UI that says
"Results 1 - 20 out of 84", but I might only display 15 rows. Even if
I move the permission checking logic into the DataProvider, it won't
be easy to do pagination correctly. The DataProvider creates hql (that
hibernate turns into sql count(*), limit, etc.) to find the number of
results and limit the results returned.

> To sum up my advices: Separate your application logic and model from
> the security management.
> Ask yourself: Does the application really need to know who has which
> roles/permissions? Isn't it enough, if it can rely that certain
> actions will be prohibited if the issuer of the action is not
> privileged to do so?

The interaction between the application logic/hibernate queries and
shiro is really what I'm mainly confused about. The applications I've
built in the past did not require anything more than role based
security. I was able to encode into my hibernate queries all of the
logic to grab the exact datasets from the database that I wanted to
display.

Thanks to your help, I think I'm starting to better understand. And
yes, I do want my application to not need to know who has what
roles/permissions, but just if the action is permitted or not. But I'm
still not totally convinced that I shouldn't use something similar to
DomainPermission. For instance, I could do the following to make my
hibernate queries work:

I would extend the DomainPermission class (as suggested in it's
JavaDoc) to my own Permission pojo that contains domain, actions,
targets. And my User would have a Set<Permission>. This way I could
create hibernate queries that return the proper results. But those
permissions could be easily used to create DomainPermission objects
that could be added to Shiro's AuthorizationInfo as well.

Of course, since Permission properties can contain a list of multiple
values for each property, my queries would have to use like("%value%")
where clauses.

However, I guess if my application did this, I would have all of the
information about which projects a user is involved in stored in the
User's permissions.  I wouldn't really need a User.involvedInProjects
set.  This doesn't seem quite right to me, so I think I'm still
heading down the wrong path and this design idea is flawed.  Any
thoughts?

Also, please consider this code taken from my Realm(extends
AuthorizingRealm) class:

protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection
principals) {
	Long memberId = (Long) principals.fromRealm(getName()).iterator().next();
	Member member = memberService.getMember(memberId);
	if (member != null) {
    		SimpleAuthorizationInfo info = new SimpleAuthorizationInfo();
    		for (Role role : member.getRoles()) {
    			info.addRole(role.getName());
    			info.addStringPermissions(role.getPermissions());
    		}
    		return info;
    	} else {
    		return null;
    	}
}

If I added a User.permissions property, I could add something like this to it:
  	info.addObjectPermissions(member.getPermissions());

Information about a user's Roles and permissions is then available in
a Shiro AuthorizationInfo. This means that my application should be
querying Shiro's api to get permissions.  My app doesn't need to know
about a user's roles/perms since it can just ask Shiro if the user is
privileged to do something.

So even though my application domain model contains the role and
permission information, you are suggesting that I do not program the
application logic to directly query User.getRoles or
User.getPermissions.  This makes sense now!

However, I'm still confused on how to display paginated lists of data
from hibernate. What I suggested above (having hibernate queries
include User.permissions like("%value%") clauses) is making my
hibernate queries do just that. It is not using shiro for security,
but bypassing it and querying the domain model directly.

Any further thoughts would be appreciated!

Tauren

Mime
View raw message