Forum OpenACS Development: Login timeout problem in 5.0

Collapse
Posted by Lars Pind on
We have a problem with expiring logins.

In the current default install, about 24 hours after your last login, you will start receiving pages that look like you're logged in fine, but don't show you the things you have permission to see. There's nothing to tell you that you must refresh your login, nor is there a link to do so.

The workaround that I normally use is to click "Your Workspace", which will prompt a refresh, but take me to my workspace, then hit the back button twice, then refresh.

Obviously, this is unacceptable, and an unfortunate consequence of the new login scheme.

Here is the motivation for the current solution again:

- Logins that never expire is a bad thing, because if you just *once* accidentally leave a cookie on someone else's computer, you're doomed, the only way to fix it is to create a new account for yourself! (And I've seen too many posts of the type "oops, that wasn't Don posting, that was me, he just left his cookies on my box")

- I always liked very much that both Amazon and Yahoo shows you non-sensitive yet personalized information when you show up, but before you retype your password. Only if you click through a link from Your Account, or proceed to checkout, do you get prompted for a password. Also, when you do get to type in your password, they already filled in the username/email part, as they know that already.

So that's what I implemented for OpenACS, but with some unintended side-effects.

If we want to take advantage of this, long-term, we need to make the distinction between non-sensitive personalized information and sensitive personalized information in the system. That's tedious work that we can't get done for the current release.

An example of what needs to happen is this: When displaying the link to the site-wide admin pages, for example, it doesn't matter if your login is expired. But when showing the site-wide admin page, we do not accept an expired login. Hence, the check on the page that displays the link should check with [ad_conn untrusted_user_id], but the check on the site-wide admin page itself should use [ad_conn user_id], and require a non-null user_id.

So in the interim, here are a couple of other possible solutions:

1) Set to never expire logins by default. I don't think this is a good solution, because, as mentioned above, non-expiring logins are bad for your security.

2) Change the default page header to say when your login is expired and offer a link to refresh it. This is annoying, because it defeats the purpose of making you feel welcome immediately, and it's still confusing, because some things may not show up, and you have to recognize that fact, then realize that it's because your login expired, then refresh it. Poor usability.

3) Have a parameter, defaulting to on, that, whenever your login is expired, we automatically bump you to the login page to refresh. This would be safe, but prevents us from showing non-sensitive personalized information even for applications that know how to handle it.

4) Whenever you do a permission check with no -user_id flag (i.e., checking permissions for the current user), if there is a user with an expired login, we redirect to the login page to refresh the login.

5) Same as above, but we only redirect to the login page if the permissions check for "The Public" (user_id 0) returns 'false' and the same permission check with the expired login would've returned 'true', meaning you're seeing different results from what you would have.

4 and 5 above requires that we never pass in a -user_id of the current user when we want to check permissions for the current user. Alternatively, we could short-cut it and say that if you pass in a user_id equal to the current user, we do the same.

I think #1 and #2 above are ruled out. #3 offers a quick and safe solution. #4 offers an equally quick and safe solution with more of the benefits. #5 lets us postpone refreshing the login out a tiny bit longer, which is good, but not necessary until we've had time to think more about the use-cases.

I do think this is a seriously annoying bug right now, and
that we should get one of the solutions into the current release.

Comments?

/Lars

Collapse
Posted by Carl Coryell-Martin on
I think that an amazon style implementation is a big mistake by default.

In the case of amazon, there are very clear demarcations between public and personal content and I don't think their programmers will have much difficulty figuring that out.

I think our users will think they are logged in if their name shows up in the welcome box (I know I do) and will be rightfully confused (as you were to discover that they are not actually logged in).

Second as a programmer I don't want to have think about if I know who the user is and seperately if they are validated or not.

The story for 3 is easiest to understand and works well.  We can pre-populate the login form with their email to make things easier for them.

4(with modification) would work fine for me as well as long as we covered all of the necessary checks (request processor etc)

Collapse
Posted by Peter Marklund on
I vote for solution 3 as it's the only one that seems totally safe to me.

Also, I fail to understand why 2 is unacceptable. I guess this is a question of terminology, but to me, if user_id is 0, then you are not logged in, and I think it's wrong and confusing that the Logout link appears in the header. Instead I would like to see the login link there.

Collapse
Posted by Lars Pind on
So how about this:

For 5.0, we just change the site-master template to show you as not logged in when your login is expired. It's just a simple change in the site-master template.

Then on HEAD for 5.1, we can experiment with something more clever, and if we decide that it's infeasible, we can easily revert to the 5.0 solution.

/Lars

Collapse
Posted by Don Baccus on
On the implementation side, I think the untrusted_user_id and user_id distinctions - and when to use them - is inherently error-prone and I predict that we'll see various packages, particularly those done in contrib and custom packages done for clients by various people, get it wrong.  This will lead to inconsistent behavior.  Security should be OK for old packages though that continue to check for user_id in all cases.  If we don't use the two ids consistently I don't personally see much value in the new functionality.

So burying the login checking code in the permission check Tcl proc seems like a good idea.  #4 or $5 seems straightforward but chasing down all the permissions checks will take time and of course we need to document how to use the new scheme.

I shouldn't be forced to log in to see a page the public can see, though.  That just seems wrong.  In a sense it penalizes me for having logged in and walked away without logging out.  So I guess #5 makes sense.  This has the potential for slowing down the request processor which does a read check but then again the RP check would lead to consistent behavior.

Since we don't have any applications that know how to display personalized but non-sensitive information at the moment, I think #3 is best for 5.0 and any future functionality in this area ought to be TIP'd before we implement it.

Collapse
Posted by Don Baccus on
I didn't see Peter's or Lars's posts before posting myself but it sounds like we're pretty much in agreement for 5.0?

And that we'll do further experimentation on HEAD (with a final 5.1 decision made via TIP, I assume)?

Collapse
Posted by Dave Bauer on
It's almost 4 years later and this is still not really resolved :)

Here is what I learned today.

permission::permission_p will call auth::require_login if you have an expired session and the privielge that is being checked is not granted to the public.

This can cause unexpected requests to login to view a public page (say a weblog) if your session has expired.

This happens when the code contains something like this

set admin_p [permission::require_permission \
    -object_id [ad_conn package_id] \
    -party_id [ad_conn user_id] \
    -privilege admin

If the public does not have admin on the requested object BUT the untrusted user (user_id of expired session) does have admin the user will be redirected to login.

It doesn't make sense to require to user to login just to view the link. This is what Lars mentioned in the original post.

It appears permission_p calls auth::require_login because permisison::require_permission calls permission_p.

In any case. if you are calling permisison_p frm within a user tcl script it doesn't make sense to require login just to view a link etc. The script should call 1) auth::require_login and/or 2) permission::require_permission to require a certain permission to view the page.

One solution might be
adding a switch to permission::permission_p -require_login that is used by permission::require_permission that forces the login, but uses ot permission::permission_p to just return the boolean to present a admin link would not force a login.

Any comments?

Collapse
Posted by Malte Sussdorff on
Yes, I reported that problem some time ago and you already mentioned the problem back then and I totally agree it needs fixing. We can either have a user cookie and expire that one as well, or we just fix permission_p (or whatever you would like to fix in what variety).
Collapse
Posted by Don Baccus on
Seems reasonable as long as the RP uses require_permission (or the switch) to enforce admin-only access to admin pages.

(I'm too tired after flying home from madrid to look right now)

Collapse
Posted by Dave Bauer on
If anyone reads this, remind me to discuss this at the next OCT meeting.

Thanks!

Collapse
Posted by Dave Bauer on
Don, I am reviewing all the calls to permission_p in acs-tcl procs to make sure it works as expected. The RP does use require_permission.
Collapse
Posted by Dave Bauer on
There is a simpler solution!!

If you call permission_p to create an admin button etc.
And you don't want to require login
use the -nologin switch to permission_p.
Its been there for awhile.

This seems much safer.

One could propose making nologin the default, but like noquote, I feel better if permission procs are paranoid.