Compatibility with Persistent Login

Maybe related to #168210: Trouble with LDAP assigned roles ?

Maybe CAS should implement hook_user('login') ?

The obvious problem here is that users logging in via CAS are never presented with a login form that Drupal has any control over. Thus, I think that if the CAS module is installed, PL should place a "Remember me" option in users' account settings. Then, where PL implements hook_user('login'), it should check if this option is set.

Come to think of it, this option should probably be present regardless of whether or not CAS is installed. This would allow PL to work with other login methods such as OpenID as well.

When PL performs autologin, it uses user_external_login(), an API implemented by user module that invokes hook_user('login') to acknowledge other modules the user logged in, and regenerates the Drupal user session. This last step causes any data stored in $_SESSION to be lost.

So if someone is using PL and CAS, OpenID or similar, then we have here several modules using user_external_login(). That means modules need to implement hook_user('login') to capture the login event when other modules use user_external_login() and react accordingly. Here PL module uses the Drupal API correctly. It is the job of other modules capture the login event, even if it comes from other sources as far as these sources use the Drupal API correctly.

PL implements hook_user('login'), so it will capture the event when any other module logs the user in. However, it will not issue the PL cookie if $edit['persistent_login'] is not set. If there's another module that provides its own login form, then it should set $edit['persistent_login'] for PL to know a new PL cookie needs to be generated. There's nothing PL module can do to guess if a login event means PL should create its cookie if there's another module that implements its own login form/page bypassing Drupal's login interface.

I've been looking at the code of CAS, and it doesn't implement hook_user('login'), so it cannot capture the login event when PL performs autologin when the PL cookie is present and valid. Also, since it provides its own login page, and it doesn't set $edit['persistent_login'], then PL won't create its cookie.

Transfering the issue to CAS module queue since there's little PL can do here to know if the PL cookie needs to be created when the Drupal login forms are bypassed. Also, CAS should implement hook_user('login') to capture user login events performed by other modules, such as the autologin PL performs when the PL cookie exists and it is valid.

Not sure what we can do here either, or what you mean by setting $edit['persistent_login'], and under what conditions I would do so For modules like CAS and openid, the athentication pages are provided by a completely different site, probably in a different domain.

So I'm unsure what you're asking for here. It would seem that persistent login ought to fire in the init hook if it were trying to log someone in when a cookie is present, as CAS does.

FYI: 6.x version of CAS DOES fire the login hook, but it doesn't "Implement" a login hook because it is not doing anything at session login time.

I agree with danep here. If you had the persistent login cookie created form the account settings form, it seems like the only sensible way. The CAS module NEVER presents a login page to the user, but rather redirects to external non-drupal sites for authentication. We have no control over the user login page, since its assumed to be at a completely different site in a a completely different domain.

There are 2 problems here.

1) If CAS does not implement hook_user('login'), then it cannot capture logins performed by other modules.

2) Since CAS completely bypasses the login form, then it cannot take into account requirements from other modules that need to alter those forms.

So, either CAS is not compatible with PL, or maybe CAS could be implemented in a different way, see openid core module, so that it can be integrated easily with other modules that hook into the Drupal login system.

I think the problem is that even if CAS were to implement hook_user('login'), there would be no way for it to set the PL cookie in the first place because it never presents a login form to the user. With CAS, a user wishing to log in to a Drupal site clicks a "dumb" link that takes them to the authentication site (which is completely independent of the Drupal site). They do the actual logging in there, and the authentication site returns them to the Drupal site along with a ticket that literally contains only the user's username, which the CAS module uses to log them in. So you can see that there is no way for the CAS module to present users with any extra login options such as "Remember me".

Now I think the solution is that either CAS or PL can put the "Remember me" option in users' account settings instead of on a login form. If CAS did that, then at the point where it logs the user in could it pass the option via hook_user('login') to PL? I'm not sure this is ideal because we are effectively tricking PL - we are pulling the "Remember me" preference from the database and passing it off as a form selection. Also, any other alternative login module wishing to interact with PL would have to implement this functionality separately. This is why I think it makes more sense to solve this problem from within PL.

If CAS were to implement hook_user('login'), then it would be great, as it could deal with programatic logins. That's not just a PL issue.

Adding an option to the user profile to automatically check "Remember Me" is not a good solution as it cannot be changed when it is really needed, which is at login time.

Well, the login form is somehow an essential part of the user login system. Please, see these issues when the login system was re-worked to support this kind of situations:

- #153372: openid & user_external_login() snafu
- #148419: Refactor distributed auth out of user.module

Also, look at how openid module in core is implemented.

In openid you still specify your username on a site sepcfic login form, but in some modules like webserver_auth and cas the username and password are provided by non-drupal technology and so the login form is not needed, that is why we use the init_hook strategy rather than the hooK_user strategy, In these modules you don't have to say you are a specific person. The module allows just getting authed atomatically without going to any site form if you're logged into the single sign-on server already when you first visit the drupal site. In such cases there simply is no UI to add a checkbox to and there certainly is no form to alter.

We could force the user to go through some specific login form that would only have the checkbox for pl, I suppose, but doing so would cripple much of the deep linking auth functionality that makes cas so powerful, and would be a low priority feature for me to implement. I suppose patches are welcome.

I don't think that any such mandatory interstitial PL login form should be implemented, if that's what you are describing. Part of the beauty (and purpose) of CAS is that authentication is as unobtrusive as possible, and even transparent (for instance if you are already authenticated before visiting the site.) Forcing users to click through a page with nothing but a "Remember me" checkbox on it doesn't make sense.

@danep: The "remember me" option needs to be requested at login time, so that's the moment when the user can decide if it is safe to check the option or not, depending on personal circumstances at that particular moment.

@metzlerd: I haven't looked at how CAS works, so I don't know it is possible to perform whatever CAS needs to do behind the scenes while still providing the Drupal login form to the user when authentication is required. If that's not possible, then I don't know what PL can do here to capture the current user need to use the "Remember me" feature or not.

PL implements hook_user('login') which is the place it needs to know if the user wishes to be remembered or not. PL uses the login form to request this information, if CAS bypasses the form, then there should be an alternative way to request that option to the user. Is there any way PL can request CAS to ask this option to the user? If so, then I would be happy to implement that. But I would not like to implement this option in the user profile as it may lead to unsafe situations.

I completely understand your reluctance to do this. I'm unaware of any way to get extra items prompted for on the cas login page itself. #11 is the only way I can think of to do it, but it sounds like danep wouldn't really find that appealing. If no one has any other ideas, should we mark this to won't fix?

Have we ruled out the possibility of making this a user account preference on the CAS side of things? At the point where CAS logs the user in could it pass this preference via hook_user('login') to PL as if it were coming from a login form? Looking at the code I think such a solution would work but I can't be sure.

There are significant security concerns here. You'd basically be leaving yourself logged into every internet cafe you visited your website with. I'm reticent to build such a feature myself, especially with the PL mainainer indicating it's a security risk.

However, what about having a page provided that is NOT the user profile page that a user could visit once to set this feature? Still trying to figure out whether this is something that CAS should provide or PL. Ideally the page would just let you set something to tell you you wanted to keep the cookies. (maybe set as another cookie preference) , and then redirect you to a page that required our protected login so that the user_login hook would fire, creating the PL cookie? Does that sound possible?

Ah, I believe I understand what you are saying about the security risk now- instead of maintaining your login preference on a per-computer basis, you would be maintaining it on a per-account basis, which means that you may unintentionally stay logged in on a public computer. Sorry I didn't realize this until now. You could tie the preference to both the computer and the account, but this is getting somewhat complicated. I will have to think about it more, and maybe see how other sites using CAS avoid this problem

How about this: users already get a "Logged in via CAS as xx" message after logging in. What if we gave them the option of being remembered at that point? So make the message "Logged in via CAS as xx; click here to stay logged in on this computer". Then hook into PL at that point. Could that work?

As an alternative, what if we provide a "Remember me" checkbox beneath the CAS login link. This sets a session variable when checked. Then when the user is returned from the CAS site, we read this session variable. If it is set, we then hook into PL.

I like this....

Patches welcome.

Would need to be implemented in the login block to be secure, but yes. Seems doable.

I don't know how easy #18 would be, you'd need to implement it using AJAX, which seems like overkill. I think #17 (providing a link to stay logged in after the user is already logged in) is the best approach. Is there a way to hook into PL and set a remember-me cookie after a user is already logged in?

I think all you need to do is set $edit['persistent_login'] = 1 and then pass $edit to hook_user('login'). Something like this:

Though, it seems to me that CAS module would have to use user_external_login() rather than module_invoke_all('user', 'login', ...);, as it might simplify the D6 version of CAS.

Thanks for the tip! Here is a patch that implements that code using a link that appears after the user has logged in (as described in #17). It also adds an administrative option to allow or disallow this capability. I have tested it fairly rigorously on my site and it seems to work and be secure.

Glad it helped. :)

I do not use CAS myself, but looking at the patch here's a couple of suggestions:

- Wrap $form['account']['cas_allow_rememberme'] in a check for module_exists('persistent_login'). ie. don't show if PL is not enabled.

- Use l() rather than hardcoding links.

- Security issue: Make sure a URL with ?rememberme=1 works only when triggered from a known location by the user, so it probably needs to include a unique token or something, or maybe using a form instead of drupal_set_message(). This is to prevent someone else from creating PL sessions of other users (using a XSS hole somewhere else, providing a link and convince a user to follow it, when using a hacked/public computer, ...).

Looks like you're doing this hook invoke when you hit CAS inside the if $user->uid test. Which would execute every page load after the user is logged in. This should happen where the other hook_login stuff is invoked, Look for the invocation of hook login at line 289. You may have to persist a cookie or something to get through the redirects.

Also just to be sure, you should roll this patch against head, first.

This is only my second or third patch for Drupal, so I'm still learning and I definitely appreciate everyone's constructive feedback.

@markus: I'm afraid I'm not clear on what the security issue is. Could you walk through in more detail how one might exploit it?

@metzlerd: What if I simply moved the module_invoke_all('user', 'login', $edit, $user); inside of the conditional above it that checks to see if the "remember me" parameter was just passed? Although the conditional will still be run on every page load, it will fail because it checks for the "remember me" parameter.

@markus: I'm afraid I'm not clear on what the security issue is. Could you walk through in more detail how one might exploit it?

Sure.

1) Let's say your patch is applied. So I can append ?rememberme=1 to a URL and create a persistent login.
2) Now, let's suppose I log on from a hacked/public computer. Here you don't want to use ?rememberme=1, but someone else could. And that would allow someone else to take over your session when you leave the computer because someone else created a persistent login for you.

That could happen on a public computer, but it could also happen if someone sends you a link to the site with rememberme=1 hidden in URL arguments. So that you don't have real control on when a persistent login is created.

Security comes into layers, and this is one that should be closed, IMHO, to prevent unexpected situations if other layers are bypassed.

Another example: See how delete operations in Drupal use a confirmation form. That's not just by design, it's to prevent from CSRF attacks. So here, with PL, it's the same thing. Creation of the PL needs to be protected against CRSF, and than means a form is necessary. A simply GET request is open to CRSF attack.

For further information:

http://en.wikipedia.org/wiki/Cross-site_request_forgery
http://en.wikipedia.org/wiki/Cross-site_scripting

Yep, my recommendation would be to capture this information in the cas user login block. Change it to present a form that you could put on a a page. with a submit button and a check box. That code is redundant since we have the login hook anyway.

Dave

No, we really don't want to fire this on every page load. Remember you're invoking module hooks, and there may be other modules that take advantage of this hook and do different things (redirect users for first login, etc), we only want the login hook to fire if we're in the act of logging in.

Thanks again; I definitely understand the significance of the security hole now. I changed the login block from a static link to a form, which includes a "Remember me" checkbox if PL is enabled and the admin option allows it. This works well on my site, and I think it addresses all of the issues mentioned. I'm sure it's not perfect though, so I appreciate further feedback.

Oh, I have not tried this patch against HEAD- I haven't had time to set it up in my dev environment yet.

Forgot to attach the patch...

I tested the patch against HEAD and it seems to work fine.

This patch did not apply successfuly. Please make sure you've got current code from CVS. Thanks,

$ patch cas.module cas-384756-30.patch
patching file cas.module
Hunk #1 FAILED at 203.
Hunk #2 FAILED at 393.
Hunk #3 succeeded at 718 with fuzz 2 (offset 100 lines).
Hunk #4 FAILED at 738.
Hunk #5 succeeded at 747 (offset 99 lines).
3 out of 5 hunks FAILED -- saving rejects to file cas.module.rej

Weird, part of it might have been a case change issue. Anyway, here is a re-rolled one.

Not sure why but I continue to get errors trying to apply this patch -- Different one this time. Although I've applied other submitted patches succesfully, I suppose it could be a problem with my environment....? Could you try and download a fresh copy of cas.module from CAS Head and see if this patch applies for you?

$ patch cas-384756-34.patch cas.module
patch unexpectedly ends in middle of line
patch: **** Only garbage was found in the patch input.

I'm not sure, I am generating the patch (unified format) using WinMerge and applying it to HEAD using TortoiseSVN without any trouble. Here is the patch in "normal" rather than "unified" style (according to WinMerge), perhaps you can try that?

If that doesn't work, and you are on Linux (which appears to be the case), my guess is that this has something to do with newline characters.

Ok sorry for the mess, but I was actually messing up applying 34. cas-384756-34.patch is in the correct format, and it finally applied cleanly, but it has some basic syntax errors which lead me to believe that this head patched version has never been run. Started with a sytax error at 488 (where a comma was missing) and then a missing { in the if persistent login check arountd 484. These were such basic errors that it made me wonder whether you generated -34.patch from the correct code.

Could you correct these errors, perform some testing and resubmit?

Please do use the unified diff format. That confusion was all mine. Guess I needed some coffee when I first tried to apply the diff?

That's very strange. I did test patch 30 against HEAD - it applied cleanly for me, and I tested the code on my development server. To generate 34, I just diffed this patched version against the original HEAD (I figured you were just having trouble with line number references or something). You are correct that I didn't test 34, because I didn't see any reason why it would be functionally different from 30. Yet, clearly, it was :)

So, having said all of that- give this one a shot. I rolled, applied, and tested it against HEAD.

It looks like this works ok, but with the latest beta of persistent login I got an error when using the check_box -- (too many values passed to setcookie) from the persistent_login module. If you'll confirm that this is not a problem with cas side of the code, I'll be happy to commit it.

That is odd. I am running PL 6.x-1.4-beta6 and have not seen any such errors with either the patched 6.x-1.0 or HEAD versions of CAS. It sounds very much like a problem with PL and not CAS.

@metzlerd #40: That's probably that you're not using PHP 5.2. It was a little mistake I did when packing beta6 and not testing under PHP4. Please, try with the latest dev snapshot of PL and sorry for the headache.

Yep, works against the dev snapshot of the persistent login module. FYI: I'm running php 5.1.6. Committed to head.

Thanks to both of you for being so easy to work with.

danep What do you think about moving the checkbox above the button? Would that be more consistent from a ui perspective?

Dave

Ah, good to know it works. Please, drop me a line when you create a new stable release of CAS, as I could add your project to the list of those compatible with PL on the project page (currently, for modules dealing with user login et al, I'm only aware of Login Toboggan, and now this one). :)

I'm glad I could help, thanks for your assistance and patience as well. As for the checkbox, you are probably right- moving it above the submit button would make it more consistent with the rest of the Drupal UI.

Automatically closed -- issue fixed for 2 weeks with no activity.