wyona / yanel

http://www.yanel.org
Apache License 2.0
10 stars 5 forks source link

Small bug in YanelServlet.setIdentity() #4

Open baszero opened 12 years ago

baszero commented 12 years ago

Today I discovered this little bug.

Requirement:

Current Solution via Yanel API:

YanelServlet.setIdentity(new Identity(user, user.getEmail()), environment.getRequest().getSession(), realm);

Bug:

Example of not working code:

YanelServlet.setIdentity(id1); // we refresh the identity with new groups
Identity id2 = getEnvironment().getIdentity();

id2 does not contain the latest changes to the identity!

Proposal:

If I find some time, I'll send a pull request and make a reference to this issue here.

Workaround currently used in my realm:

YanelServlet.setIdentity(id1);
getEnvironment().setIdentity(id1);
michaelwechner commented 12 years ago

IIRC the reason for this is because of performance of access control. During login the groups of a user are determined and then added to the session such that checking each request re authorization with policy manager will be very fast.

The problem now is that let's say the groups of a user are changed, but this particular user has still a session, then the session does not learn about this change.

baszero commented 12 years ago

What you say is absolutely true, as of my description above, I don't want to authorize the user over and over again, that would certainly impact performance which I do not want.

But if Yanel provides a method to "update" an identity, Yanel should at least update all references to the identity instance so that this is consistent. As setIdentity() is only called under very special conditions, I would think it's ok to put more logic into that method (or provide another method which is more expensive).

michaelwechner commented 12 years ago

Updating the session is a problem, because session A can for example change the identity, whereas session B should updated accordingly, but sesssion A has no way to access session A (except maybe via the session listener).

Maybe the easiest workaround would be to use "last-modified* of identity, which means session B could check the last modified and if changed, then session B could reload the identity.

baszero commented 12 years ago

Hi Michael, please check my description of the requirement: I only mean updating the identity within the same session.

Example: you log in, you do something in the system as a normal user and then you get a new group assigned (e.g. you click on an invitation from an administrator in order to become an "editor", just as an example).

So I don't want to refresh any other session. If you just use Yanel.setIdentity() as described in my issue, there is a bug if you use the environment object in the same request.

baszero commented 12 years ago

I updated the section "Bug" and added "Workaround" section at the end of the issue description.