Forum OpenACS Development: Remember Me Feature Broken!

Collapse
Posted by Dave Bauer on
http://cvs.openacs.org/browse/OpenACS/openacs-4/packages/acs-tcl/tcl/security-procs.tcl?r2=1.61&r1=1.60

This change makes the ad_session_id cookie a browser session cookie killing the ability to have a persistent login even if the feature is enabled and the user selects that option.

I am not sure if there is another reason for this change that I didn't understand but the remember me feature is broken in OpenACS 5.6.

Collapse
Posted by Carl Robert Blesius on
Gustaf,

Those changes were added by you.

Can you comment on this?

We want to fix this feature without breaking what looks to be a security fix.

Thanks,
Carl

Collapse
Posted by Gustaf Neumann on
The referenced change had two security related effects: One was against session hijacking, which was possible before (revealed by an external security audit, we did last year). The second one is about cookie deletion upon browser close (we had this issue that in public classrooms, people close their browser, and the next person opening the browser is still logged in as the previous user). It looks like "remember me" (whatever this means in detail) and logout-on-close cannot be realized at the same time. Victor is currently looking into the details, a fix will be out soon.
Collapse
Posted by Victor Guerra on
In order to avoid discarding the cookie ad_session_id when browser closes, one would need to know weather the user checked, in the login form, the option "remember me". But at the moment of setting the cookie this info is not available.

Therefore we thought about adding to the values list of ad_user_login ( or ad_user_login_secure in case of being under secure connections ) the value of the "remember me" check box with the purpose of querying it when setting the ad_session_id. When user don't want to be remembered then the ad_sesssion_id will be discarded otherwise expiration date is set to whatever value is configured on the SessionTimeout parameter.

In our case we don't allow persistent logins which means that this change will still continue to discard the ad_session_id cookie, so we are on the safe side.
Would this change make sense to you all? I already did this modifications in my local instance and works well.

Collapse
Posted by Dave Bauer on
The behavior of remembering the user's identity, if not their login, was added a long time ago.

This allows users whose session has expired, but are browsing public resources, to not be required to login again. They are asked for their password to perform an action that requires privileges the public does not have. This is seperate from persistent login. The login session can expire without requiring the user to type their password again if they are viewing public pages (ie: openacs.org behaves this way.)

So I think any change needs to still support this beheavior, at least as a parameter.

I haven't reviewed Victor's proposal to see if it supports this case yet.

Collapse
Posted by Dave Bauer on
Ok I reviewed the patch proposed by victor and it mostly works.

It does not support the "user is identified by an expired login cookie, allow access to public resources and show "Hi User" type message. (ie: amazon.com model).

To do this we could add a parameter AllowUntrustedPublicAccess or something similar that would check the user_id cookie for validity but not expiration.

This would allow a valid,expired cookie to be used with a matching IP address for identity but not authorization.

We could make the ad_user_logout conditional on checking the cookie without expiration, and checking the new parameter. This would allow site owners to decide which behavior makes sense.

For a private site, logging out on expiration (or brower close) makes the most sense. For a ecomerce site, for example, requiring login only to checkout or otherwise POST data to the server makes sense.

Collapse
Posted by Dave Bauer on
Hi, any response to my suggestion to add a new parameter to fix the previous behavior for semi-public sites?
Collapse
Posted by Victor Guerra on
Yes.. at the moment ( even with my patch applied ) users are being logged out when retrieval of the session cookie fails ( for either of the reasons: session cookie does not exist, validation of signature of session cookie fails or session cookie expiration).

So I think that adding this parameter for controlling weather or not to force the logout makes sense. Then, using a combination of the parameters AllowPersistentLoginP and this new parameter ( AllowUntrustedPublicAccess perhaps ) would be enough to cover all possible scenarios.

We could then proceed this way.

Collapse
Posted by Torben Brosten on
Dave,

I'd like to see the optional behavior return by adding an AllowUntrustedPublicAccess parameter. The lack of this feature (when lacking) has been one of the primary negative feedback from users of OpenACS sites I administer --ie having to make a registered user login to see a publicly available page. Maybe rename parameter NoLoginReq4User2cPublicContent.. anyway It's the parameter description that needs to make this clear. :^)

cheers,

Torben

Collapse
Posted by Carl Robert Blesius on
I'd like to second the description improvements.

I feel like the only way to really know what all of these do is by reading the code. Not ideal.

Could you include a review and update of the descriptions as part of this work Dave?

I'm happy to review and test.

Collapse
Posted by Dave Bauer on
Yeah overall we should hide the parameter names and have a "Human Readable short parameter name" and "Human readable long description" and the camel case parameter names should be internal only.

Yet another thing to do ...

Collapse
Posted by Jim Lynch on
Torbin, just set permissions. done.