Use phpCAS::setCacheTimesForAuthRecheck() instead of a custom cookie.

The intent of the code is that it is making sure that the check for login only happens once per session. It's trying to cut down on the redirect. forced login check aside, Can you verify whether cas is functional on site B? It sounds like it might not be. You may need to enable cas debugging to verify as to why.

Is it possible that your two domains are sharing cookies?

I think what we probably should do is unset the cookie when a user successfully logs in.

Also, should we be setting the $path on the cookies, similar to user_cookie_save()?

@dpalmer: Did you have a chance to test the patch in #4?

Deleting the cookie would not allow an administrator to log out of Drupal without also logging out of CAS.

Subscribe.

I'm also having troubles with the gateway mode. Nothing ever clears the cas_login_checked cookie once it's been set (short of a browser restart, which clears browser session cookies), so if you've been autologged once during your browser session, you'll never be autologged again whatever you do (e. g. log out / re-login from CAS).

So when should we delete the cookie? When the user successfully logs in? Upon logout?

The gateway feature does seem to be quite problematic.

Is there a better way to architect this?

Subscribe
Same problem here. It was working prior to the 6.x-3 line. I just tried it again on our servers using the 6.x-2 line and that works.

sher1: can you be a bit more explicit about which problem you are having?

For work on fixing this, it'd be best to describe exactly what you do to trigger the error and what the expected behavior would be.

One quick solution, by the way, is change your link to http://siteB/cas?destination=
.

OK, so I am very sorry. I thought I had posted more info on this on Monday and haven't been back till now. Here are how things work/have worked for us.

If we log in to CAS, whether via a link on another web page or directly on the CAS login page, and then we go to one of the modules using the 6.x-2 code base, it will automatically log us in on that server, as expected. If we go to a page that is using the 6.x-3 code base, it will not login automatically. If we click the login link, it will take us to the CAS page which automatically redirects us back since we already are authenticated with CAS.

When the gateway functionality works, going to a page that has CAS authentication on it will automatically check the CAS server for a valid CAS ticket and log you in based on that valid ticket OR if there is not a valid ticket, it will do nothing more.

sher1: Can you try the following sequence:

1. Quit and restart your browser (to clear any session cookies you have).
2. Go directly to the CAS server and log in.
3. Go to the 6.x-3.x site.

Are you automatically logged in?

There are tests written which should ensure this type of setup is functional.

----

Now, if I understand you correctly, what is broken is:
1. Quit and restart your browser.
2. Go directly to CAS server and log in.
3. Go to 6.x-2.x site ("Site A"). You are automatically logged in.
4. Go to 6.x-3.x site ("Site B"). You are not automatically logged in.

----

Or have I misunderstood you?

First part ---
Did those things (also used Firefox Developer tools to delete all cookies) and closed browser.
Opened browser and logged in directly on the CAS server. Used same window to type in Site B. Does not gateway

Second part --
You have described it exactly correct.

Are these two sites on the same server by any chance? Is it possilbe that the cookie from the first site is being passed to the second site? Hence the problem?

They are on different servers. Different cookie domain too

Can you download SimpleTest and try the provided test routines?

I am having trouble running the tests. I just remembered there is one significant difference between the sites: the one that isn't logging in automatically is running pressflow 6.20 and not vanilla drupal like the other sites. Will that make a difference? Can you send me a link on how to run the tests since I am not getting it working?
Thanks

You know, making sure your user has permission to run tests is kind of critical and can make someone feel really dumb when they don't look there. Results are attached

Hmm, the tests might require SimpleTest 6.x-2.x-dev.

Installed the dev line and am attaching the results

Well these error messages are a lot more enlightening. Simpletest needs phpCAS, and is set up to download and extract it on demand if it doesn't find it in a predictable location -- in this case sites/default/files/simpletest/cas/CAS-1.2.1/CAS.php. Go ahead and download and extract phpCAS in that directory and see what happens.

I admit this could certainly be improved, however the test routines were written so that they would work on qa.drupal.org without much care to running them locally.

Alternatively you can try the attached patch which changes the test routines to try to locate phpCAS using the Libraries API module.

I really appreciate all of your help in debugging this!

No, thank you for all the great work you are doing and for all the help. I have patch the test and run them again. Should I just be running the gateway part? I got a bunch of errors back when I ran all the CAS tests. Is there a good way to output this so a file to send?

One of the repeated errors is that it can't find the CAS library. We installed it with pear and the autofind for the CAS module config finds it just fine. It is in /usr/share/pear/CAS.php

Here is a spreadsheet of the db table

Meaning the results of the simpletest run

sher1: Okay, new patch (like in #23). Make sure to reset to the stock cas.test file before applying the patch.

This one sets the CAS library directory to /usr/share/pear. (Which is what I assume you have it set to in the CAS configuration page). If it should be a different path instead, I think you'll see how to change it.

Ran the cas gateway test again and it still failed with results I don't understand. I just noticed that this is still on the dev line. Should I update to the 6.x-3 stable and then run it again?

Yeah, upgrade to 6.x-3.0, however there haven't been many (or any) changes in a few weeks.

The error messages are coming from some bugs in phpCAS, namely:
https://issues.jasig.org/browse/PHPCAS-103 and https://issues.jasig.org/browse/PHPCAS-105

What version of phpCAS are you using? If you are using 1.2.1 or 1.1.3, you can get patched versions of CAS/client.php at
http://drupal.org/files/issues/CAS-1.1.3-CAS-client.txt or http://drupal.org/files/issues/CAS-1.2.1-CAS-client_0.txt

Both of these should be fixed in 1.2.2 when that comes out at the end of the month.

OK, we have upgraded to the casphp 1.2.2 client and are still unable to get this working. Here is the most recent simpletest result. I just ran it for the gateway feature.

What version of SimpleTest are you using? I'd suggest the 6.x-2.x-dev version.

I changed to the simpletest 6.x-2.x-dev version. I am attaching a spreadsheet of the response.

In the errors it says it is using
CAS.php found in sites/default/files/simpletest/cas/CAS-1.2.1.
instead of the one I upgraded to with pear. Is that something I can fix in the test?

I changed the test file to grab the 1.2.2 but the results look the same except for the change of version on those lines.

I need to mention too that when I do a gateway test with the example I get a redirect error.
I am including the phpcas error log and the gateway.php file I used for testing.

You can see the error, with or without logging in, by going to
https://home-dev.byu.edu/home/test/gateway.php

Thanks for the gateway.php script and your error log file.

Copying the gateway.php file to a local installation and running it works absolutely fine for me. Which is quite odd. Here are the headers returned by your server and my local installation:

$ curl -I http://home-dev.byu.edu/home/test/gateway.php
HTTP/1.1 302 Found
Date: Thu, 09 Jun 2011 01:15:45 GMT
Server: Apache/2.2.17 (EL)
X-Powered-By: PHP/5.2.16
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Pragma: no-cache
Location: https://cas.byu.edu/cas/login?service=http%3A%2F%2Fhome-dev.byu.edu%2Fhome%2Ftest%2Fgateway.php&gateway=true
Content-Type: text/html; charset=UTF-8
Set-Cookie: PHPSESSID=eekslgirdldnjlq2q241vih2u7; path=/

$ curl -I http://localhost:8888/gateway.php
HTTP/1.1 302 Found
Date: Thu, 09 Jun 2011 01:15:33 GMT
Server: Apache/2.0.63 (Unix) PHP/5.3.2 DAV/2
X-Powered-By: PHP/5.3.2
Set-Cookie: PHPSESSID=45e21a7139a4447ad393d795c266a22a; path=/
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Pragma: no-cache
Location: https://cas.byu.edu/cas/login?service=http%3A%2F%2Flocalhost%3A8888%2Fgateway.php&gateway=true
Content-Type: text/html

The only difference of note seems to be the location of the Set-Cookie header. This might just be a red herring though.

Edit: The cookie still seems to be getting set, so that shouldn't be the issue.

I've been looking at your logs and find something interesting. When we visit this page we get an immediate redirect loop BEFORE the user has been authenticated, but your log file shows successful authentication of a ticket. So how can this be? Are you sure that you're not authenticated with CAS when you do this request for the first time? Perhaps you were logged into your cas server already? If so is there a chance that you could try again without being logged into the cas server first?

Also could you try setting your cas uri to 'cas' instead of '/cas'? This has worked to solve some pretty bizzare cas problems in the past, although I've never gotten a handle as to why.

I changed the uri to cas in the gateway test and that didn't make any difference. I have also tested with making sure that I was logged out (hitting the CAS logout page before testing the gateway page). Right now I am not sure what is going on. I am going to do a little more testing and see if I can get more details.

OK, so I have the gateway test script working fine on it now. My problem was related to bad permissions on the directory where the sessions were supposed to be created. Now we are back to where we were with the simpletest results. What should I be doing now to work this one out?

I suspect you'll find that everything works now. Give it a shot.

Sorry, just getting back to this. It is still not working the way it should be. If I login, via the CAS login page directly or redirected from another site, it still does not gateway. I don't have anyone complaining anymore about this so maybe we just need to table it for now. We are going to be migrating at least one site to drupal 7 in October so maybe we just see how that one goes. Thoughts?

I am having the same issue at my institution. I have applied the patch from comment #4 and it does not make a difference.

We used to have a our own homegrown single-sign-on implementation and thus had a Drupal module to go with it which facilitate the single-sign-on functionality. We included a module option to "always verify user with server". I believe adding this as an option would be great so those who don't mind hitting the cas server on every page view can do so.

This option would therefore ignore $_COOKIE['cas_login_checked'], which I am doing in my hacked cas module that I have running one of my sites.

Is this an option that users want? I'm figuring out how to work with the new git repository, so I will provide a patch with this new option and post it up soon.

Attached is my patch stated in comment #44. This is my first patch that I have submitted, so please forgive if I left anything out.

Patch includes:
- adding a cas_check_always variable to the admin page.
- modified _cas_allow_check_for_login() to ignore the cas_login_checked cookie.
- modified cas_uninstall() to delete the new variable.

I'm a bit unsure as to why this is related to the original issue here. You're adding a feature to an a bug support request. I'm still unclear as to why the feature is necessary, but this seems to be a separate issue. The original issue is "Cas Auto login not working between two sites". The existing gateway functionality is "supposed" to work. Could you perhaps open this as a separate issue with some clarity as to why the "always verify" is necessary?

Dave

I agree that this seems like a feature request (and not a bug report).

Nonetheless, I'd prefer to see us call phpCAS::checkAuthentication() on every attempt and use the built in limiting functionality of phpCAS:

    /**
     * Set the times authentication will be cached before really accessing the CAS server in gateway mode:
     * - -1: check only once, and then never again (until you pree login)
     * - 0: always check
     * - n: check every "n" time
     *
     * @param $n an integer.
     */
    public static function setCacheTimesForAuthRecheck($n) {

I applied patch from comment #45 and it worked ok until I activated option "Verify on every page view" for second site with same patch applied. After that whenever I tried to login site redirects back to Drupal login page and not to CAS login page. I modified it a little. Attached find patches for D6 and D7.

If possible can you update the relevant CAS Gateway test to encompass the new desired behavior? (See the cas.test file).

CAS gateway and Always verify functionalities do same thing and I changed my patches by removing Always verify and by changing gateway functionality to work as expected.

Initially I removed only cookie check from function _cas_allow_check_for_login() but after, whenever I tried to login to CAS from Drupal login page it was redirecting me back to Drupal login page. I changed function _cas_force_login() to check form_id from $_POST and if request is made from login form it forces redirect to CAS.

Also I tested module without having the gateway enabled and it seems to work ok.

Previous patches are not ok, attached find revised patches

THis will not quite work, because the desire is of _cas_force_login is to require a cas login before a user can access a page. In many cases whole sites are set up this way, and don't really allow anonymous access. The behavior should be that when I say in the redirect settings, require login for specific pages that a user gets redirected to the cas site for any of those pages, regardless as to whether the user clicks the login form.

At the risk of being off-topic, I think for 7.x-2.x we should fundamentally reconsider our assumptions about the CAS login process. In particular, we may want to insist that CAS logins are *only* initiated when visiting the 'cas' path.

• _cas_force_login() could be achieved by redirecting to 'cas?destination=
  '
• _cas_check_login() could be done by redirecting to 'cas?destination=
  &gateway=true'

Since 6.x-3.0 is out, we cannot really change the architecture... :(

In #47, I was thinking about something like the attached patch. Don't try this out on a production server. In particular, there is an upgrade routine which removes the existing cas_check_first variable preventing an easy rollback to before the patch.

The attached patch is *completely untested!*

I think this approach has a lot of merits. It may help eliminate the possibility of loops which is a really good thing.

Why does using the gateway feature prevent logging a user out when logging out of cas? My experience of this is to the contrary, so I'm curious as to why this warning language is in the patch?

Hmm..... A thought just occurred to me. If you're suggesting that we wouldn't test for single sign-out because we're not operating in hook_boot, then that would be a really bad thing. Single sign-out needs to work at all costs.

bfroehle, thanks for the patch. Found 2 problems, on line 81 there is a typo setCacheTiemsForAuthRecheck(), probably you meant setCacheTimesForAuthRecheck().

After I open the site I get error phpCAS::setCacheTimesForAuthRecheck(): type mismatched for parameter $header (should be `string') which seems to be caused by bad implementation of setCacheTimesForAuthRecheck().

After corrections patch seems to work.

The attached patch should fix both of the identified errors in #60. To get around the second I just cast the variable to a string.

Tested 3 new options added for Check with the CAS server to see if the user is already logged in and here is what I found:

Never
CAS log in works (Using link "Log in using CAS" and then press "Log in" button)
Auto log in is not working - user is not logged in when node/1 is accessed

Once, but not again until login
CAS login works
Auto log in is not working

Once, but not again until login
CAS login is not working
Auto log in is working

Expected:
Never
CAS log in works
Auto log in is not working

Once, but not again until login
CAS login works
Auto log in is working

Once, but not again until login
CAS log in is working
Auto log in is working

Andrei: Thanks for the review! I think with a bit of effort we should be able to hammer out these last few bugs.

Also we'll need to bump the minimum phpCAS version requirements to account for using this new functionality.

Found one bug -- the error before "phpCAS::setCacheTimesForAuthRecheck(): type mismatched for parameter $header (should be `string')" is actually a bug in phpCAS --- the function actually wants an integer (and not a string).

So I've changed that one line and am uploading a new patch for testing.

p-andrei: Do you think you can give the patch in #65 a test? The testbot seems to like it. :)

Dave, Andrei: Any thoughts on this patch?

This is a nice feature addition --- you can check to see if the user is already logged in never, once, and always --- opposed to just never and once as before. In addition I think the settings UI is easier to understand as a dropdown instead of a checkbox.

From an architecture standpoint the change moves from relying on our own cookie to a session variable that phpCAS manages.

One final note:

+++ b/cas.installundefined
@@ -337,3 +338,19 @@ function cas_update_6304() {
+ * Switch from cas_gateway to cas_check_frequency.

This should be "Switch from cas_check_first" to "cas_check_frequency".

I like the technical direction of this patch. I still wonder about the following language:

Enabling this may prevent logging out of Drupal without also logging out of CAS.

I don't understand how/why that is true? What about enabling gateway login could have any possible affect on what happens at logout? Is there something in this patch that I'm missing in reading it? This seems more related to the age old "logout" vs. "cas logout" debate.

I'll try and give this patch further testing tomorrow.

Let's say you always check to see if you are logged in with the CAS server. Then when you logout of Drupal using "/logout" your Drupal session is destroyed but your global CAS login remains. So the next time you visit a page, the Drupal checks with the CAS server to see if you are logged in, and ... well it is as if you never logged out of Drupal in the first place.

I see, and because the "Once" setting is detected with a session variable, it gets destroyed with the drupal session, right? Do you think we should force a user logout hook implementation of cas logout if this option is set? It might be less confusing.

I'm not sure there is a robust way to force CAS logout on '/logout', except perhaps to use hook_menu_alter() to override the 'page callback'. I think it'll get confusing if we sometimes to CAS logout and sometimes not --- for now I think we'll just have to recommend that they enable (and use) the CAS Logout menu items instead of the regular logout menu items.

That makes sense... Might add that recommendation to the description text then.

Patch tests out well. I've been thinking about cleaning up language and instructions, so we can defer that to another issue and work on it in the 7.x branch. For me it also "breaks drupal logout" as expected.

Isn't that desireable? I mean, if the user is going to logout this should be done from the CAS server (via "CAS Logout"). At least, for my purposes that's how I hope the CAS modules would work. (I haven't yet figured out how to get it to work this way yet.)

There seems to be a support request hidden in your comment? Have you enabled the "CAS Logout" menu item as suggested earlier in this thread?

There are cases where I want to force a session close on a drupal site without forcing a logout from "every CAS" authenticated site. It really depends on how tightly you're trying to integrate your cas enabled sites.

I have added language domain for my website as in the following -:

Default language Chinese -> test.example.com
Language English -> testen.example.com

The website uses CAS Authentication with phpCAS 2.0.

I am having the same problem as discussed on this issue. e.g., when user logs in on test.example.com and then changes the language, they are redirected to testen.example.com but as not logged in. They need to log in again on the testen.example.com site.

Main domain => test.example.com
Sub domain => testen.example.com

Does CAS work with language domains?

I do not think this is related to cas. The real issue here is more likely that your session doesn't carry over to the new site, but rather you are starting a new one. The cookie domains are likely different. So unless you are requiring login or have cas configured to check for a pre-exisiting login, you're going to not be logged into the second one.

This is not the same issue as is being reported here, so if you'd like to file a support request please create another issue.

That's very perceptive. I think I do have a support issue. It has to do with how the CAS "gateway" feature is implemented. Here is a related link (that I posted): StackOverflow: Drupal 6 CAS "client" won't login automatically I also solved my issue, but I'm wondering if the Gateway feature needs more form options on how to control the behavior. (I'd be willing to help code if needed.)

Ok, this might be important. For anonymous users on cached pages hook_init() doesn't get called. Instead, hook_boot() should be used for cached pages. This means that the code in cas_init() maybe should be moved to cas_boot().

barobba: The hook_boot() issue is known, see #1280474: "Check with the CAS server to see if the user is already logged in?" option does not work with caching.

Hi,
I think that the hook_boot could fix bug for some of you because like in #18 i got the bug by using pressflow and not the standard drupal.
And with pressflow when you have the "normal" cache settings the gateway mode was broken, see http://drupal.org/node/1280474.

David

The patch in #65 no longer applies.

Re-rolled version of the patch in #65.

Again, with style.

Forward port to 7.x-1.x.

I think these two patches are ready to go, but I'll wait a few days before committing.

I applied patch #86 to cas 6.x-3.2 on drupal 6. But then the admin user (uid = 1) was no longer able to login (through /drupal_login, bypassing CAS SSO) to run update.php. After reversing the patch, the admin user is now back in.

So I think there might be a bug in the code.

Thanks for the report. I'll definitely investigate when I have time.

Im having an infinite redirect loop after I added this patch.
Any idea what could be causing this, I'm breaking my head on this.

"Check with the CAS server to see if the user is already logged in?" is set to always.

Attached log.

If anyone is having the same issue, I fixed this by disabling "Frontpage Redirect Handler" on the Global Redirect configuration page (D7).

Appling the #87 patch I'm not more able to login in normal mode, only by cas. If I try to login with standard Drupal login form, the system reload the login page without login the user!

I found a temporaly solution.

In the CAS settings is necessary to insert in the Excluded Pages field (under Redirection fieldset) the path user.

After that, I'm again able to login as normal drupal user, only from the user page of course. In my case it's enough because the login block has only the CAS login exposed.

Worked for me. Hope it can help you too.

The reason you are having issues with this is whenever the LoginCheck is called, it appends a ?destination= to the current page. IE. If you visit, "site.com/node/10", you end up on "site.com/node/10?destination=node%2f10". This destination breaks form submissions, it also breaks webforms. I have not found where exactly this is occurring, but it appears to be somewhere in the phpCAS::checkAuthentication(); call, which is odd as that is a library and not drupal functions. I also found a workaround, which was checking for webform node types in the init hook.

Anyone else have any ideas on where to look for this, or if the ?destination is even necessary?

#87: 869926-87-setCacheTimesForAuthRecheck-7.patch queued for re-testing.

I hate to bother but is this patch ready for the dev branch soon?

It would be nice to see this patch available since we require it on our instance of Drupal.
Thanks

@steverweber: Perhaps you could test the patch (on a dev site) and report back if it works, solves your need, and if you experienced any bugs? A few positive reports would certainly help move things along.

Aright, I'll report findings in a weeks time.

broken when using this patch with

Drupal, logout don't seem to logout the user... I need to use the service url directly https://cas.uwaterloo.ca/cas/logout

Drupal, login requires me to wait around 1 minute or retry many times before clicking login will open the CAS login screen.

What does seem to work is if i'm already logged into CAS this will automatically login Drupal.

So yes this patch could use some work before going to dev branch.

Hmm, looks like there is a bug then. Steve, thanks for the report.

seems that issue #1280474: "Check with the CAS server to see if the user is already logged in?" option does not work with caching is connected and should be resolved first.

I have experienced this as well with the patch in #87.

The login button behavior is very strange, as it doesn't redirect me to the CAS login page at all. However, if I exclude the paths user and user/*, it works fine. We have excluded those paths in the past to make it simpler for uid 1 to log in to the site, particularly on sites with no anonymous view (as otherwise all the pages would redirect to CAS and uid 1 could never log in). It doesn't seem like a ludicrous idea to me to always exclude the check for user* paths (or to always exclude them when the 'always' method is on).

The logout behavior is exactly what I would expect, however. Local Drupal logout doesn't kill your CAS session, so the next time the check for a CAS session is made, it will find one and start a new Drupal session. This is exactly how CAS works on sites with no anonymous view at all: if you attempt a Drupal logout, the Drupal session ends, it lands you on a page that redirects you to CAS to login (because there is no anonymous view), you have a valid CAS session, so you get logged back in "automatically". You must use the logout method that the CAS module provides (/caslogout) so that you kill your Drupal session and get redirected out to kill your CAS session. That is the only way to properly log out of an SSO'd session.

The patch in #87 seemed dated against the 2013-Sep-30 dev release. Recreated accordingly.

Corresponding change for Drupal 6, to make the patch in #86 apply to the 2013-Sep-30 dev release.

EDIT:

Upon further review, the problem I had posted about in this comment is unrelated to this issue and the patches. I will post a new issue when about it shortly.

I believe there is a bug with the 'Always check' functionality provided by this patch. When an anonymous user attempts to submit any form (any POST request really) it gets routed through the CAS gateway check, and the data that was POSTed gets lost when it gets back to Drupal.

The best example is the default search.

In 'always check' mode, an anonymous user cannot use http://example.com/search . Anything they search for will be lost in the gateway process and the page will reload at the blank search bar again.

Note: This is not the issue that I reference in my previous comment. That issue is #2175203: Gateway mode mangles query parameters for truly anonymous users.

Here is a new version of the patch in 105 that includes a bulit-in check to only check CAS on GET requests. This approach makes the most sense to me for fixing the problems with anonymous form submissions.

I was correct in 107 above. #2175203: Gateway mode mangles query parameters for truly anonymous users really was a problem with this patch, and not with the default behavior.

The problem, as outlined in that issue:

The following scenario illustrates the bug. In this scenario, the anonymous user has no cookies related to the site (think and test with an 'incognito' browsing session). The server is configured to use gateway mode.

Anonymous user (no CAS or Drupal session) goes directly to a URL with the format:
https://example.com/search?query=foo .

CAS module checks CAS server, no session, they are returned to
https://example.com/search?destination=search%3Fquery%3D=foo
which means the query isn't processed by the page.

I had several patches in that issue; however, all of them ended up breaking the login block behavior.

The patch I'm attaching addresses the issues I raised with my prior comments. It passes all tests locally, so I expect to see 2 fails and the same exception footprint as the last patches in #2196469: Add option to force redirect back to https and #2149843: Use the proper fixed-service-url for proxy-authenticated requests..

As expected, those two fails and all the exceptions, which in my mind at this point is pretty close to a pass (it could actually fail those tests if whatever is causing them to fail currently wasn't there). I hope to figure out why those tests fail on the testbot and not locally soon so the testbot won't reject all the patches.

I don't think anyone considered what would happen with page caching here. Anytime a session variable is saved for a user, no cached page will be served to the user.

Since we dropped the cookie method and instead use phpCAS::setCacheTimesForAuthRecheck(), anyone who enabled the gateway feature would make page caching useless on their site.

Gateway mode doesn't work with page caching in its current manifestation either, since hook_init() doesn't run on cached pages. The discussion in #1280474: "Check with the CAS server to see if the user is already logged in?" option does not work with caching talks about the problem. This patch does nothing that changes that behavior, other than add the option to Always check rather than only checking once, making the problem more visible.

I have to read more on that issue, but looks like there's work to fix that caching issue by implementing hook_boot for the gateway feature. Assuming that got in, then we'd once again run into issues if this patch got in as well. I think we should strive to get the gateway feature working with caching enabled.

The solution presented in the patch for that issue skirts the issue by disabling page caching if gateway mode is meant to be triggered by that request. Part of the functionality introduced by the patch in this issue is the ability to gateway on every request made by an anonymous user, which, when integrated with the patch from that issue, would result in no page caching at all for anonymous users, the same state of affairs we have now.

I suspect that the actual hook_boot vs. hook_init issue won't be solved until Drupal 8 (when the REQUEST EventListener replaces them) because of the massive infrastructural changes that would be required to fix it in Drupal 7.

If you'd like to give that a try though, I'd be glad to offer review for such a patch. However, that has nothing to do with this issue, as it does nothing to worsen the behavior of gateway mode with respect to page caching.

If you'd like to give that a try though, I'd be glad to offer review for such a patch. However, that has nothing to do with this issue, as it does nothing to worsen the behavior of gateway mode with respect to page caching.

Yes ok, I agree it doesn't worsen the existing behavior. I looked at the hook_boot patch in the other issue and have some concerns about that as well. Let me see if I can spike on this over the week. Maybe I can come up with something more elegant that.

Also my earlier concern over this approaches use of session variables doesn't matter anyway. It seems that CAS's checkAuthentication() will always set a session variable ($_SESSION['phpCAS']['unauth_count']). So as long as we make a check for this at all, caching is thrown out the window.

Alright. Man this was a tough one to review - there are so many moving pieces and redirects involved here. This issue as a whole is all over the place. There's a mix of unrelated issues people have reported and a feature crept in as well. Hopefully we can close this out ASAP.

Attached is another iteration on the patch that simplifies things quite a bit.

This code below was removed. It's overly complicated and not needed. We instead just need to specify the current_path() as the service URL and append any query string params that are present.

+  $destination = drupal_get_destination();
+  $destination = $destination['destination'];
+  $destination = cas_login_destination($destination);
+  $params = array();
+  if ($service != 'cas') {
+    if ($pos = strpos($destination, '?')) {
+      $query = substr($destination, $pos + 1);
+      $array = preg_split('/[&=]/', $query);
+      for ($i = 0; $i < count($array); $i = $i + 2) {
+        $params[$array[$i]] = urldecode(urldecode(($array[$i + 1])));
+      }
+    }
+  }
+  else {
+    $params = array('destination' => $destination);
+  }

The code for the cas_login_page was also completely removed save for a "fallback" redirect to the homepage. All logic related to this page callback is handled in hook_init.

I also added comments throughout to help others understand what's going on.

The testbot is confused; however, you do have three legitimate test failures, all related to redirecting to the wrong place.

The basic idea behind all that crazy code in my patch that you stripped out is this:

You need to resolve the current destination parameter, if any (this is what your patch is failing on in the tests). This requires a call to drupal_get_destination. Once you do that, you have a mess of encoded parameters on your hands if the destination included additional parameters. The crazy regex/for-loop/urldecode handles all that so you end up with some usable parameters.

As for removing the function and jamming it into the code itself, I don't really see a reason to do that. If your patch needs to modify the function (and/or its signature), that's fine (indeed, my patch did so). However, I don't think we should be reintroducing complexity into the main hook_init; it makes the code harder to read.

If you have an available environment to do so, please consider running the simpletests locally. The testbot fails on the current 7.x-1.x branch for some reason I'm unable to reproduce locally, but all the tests pass for me locally with just a 'Standard' profile Drupal install and cas module.

Yeah I assumed a bunch of tests would fail. I'll work on it.

You need to resolve the current destination parameter, if any (this is what your patch is failing on in the tests). This requires a call to drupal_get_destination. Once you do that, you have a mess of encoded parameters on your hands if the destination included additional parameters. The crazy regex/for-loop/urldecode handles all that so you end up with some usable parameters.

Are you talking about when there's an existing destination parameter in the URL when the gateway is activated? So if I a user is on /myform?destination=/blahblah, we would want to return them to that URL exactly after they've been logged in (or not) from the gateway, and not actually perform the redirect to /blahblah? Trying to understand this use case better.

As for removing the function and jamming it into the code itself, I don't really see a reason to do that. If your patch needs to modify the function (and/or its signature), that's fine (indeed, my patch did so). However, I don't think we should be reintroducing complexity into the main hook_init; it makes the code harder to read.

I think what I didn't like about this function is that it was a page callback for /cas and it was also being called from within hook_init as well. It was just quite confusing. The /cas page callback is only ever called for forced authentication, while the logic within it is useful anytime a user logs in successfully. I agree that the code within hook_init (actually cas_login_check) is getting out of control. I'll just abstract that logic into a separate function once more.

Also, I'm on IRC all day during the week in #drupal and #drupal-support. May be faster to chat about some of this stuff if you're available as well.

I don't understand this test:

    // Initial login with redirection goes to specified destination.
    $node = $this->drupalCreateNode();
    variable_set('cas_first_login_destination', "node/$node->nid");
    $cas_name = $this->randomName();
    $account = $this->casLogin($cas_name);
    $this->assertUrl("node/$node->nid", array('query' => array('destination' => '')));
    $this->drupalLogout();

Why would we assert the user landed on a page with a destination parameter that is empty? Why would there be a destination parameter there at all?

@bkosborne: FYI, some of the tests may be overly specific, testing for empty parameters which are optional at best. In this case it might just be best to fix the test at the same time.

Yeah, we had a chat about that on IRC where we decided the test should be improved at the same time (albeit with many false starts).

On a related note, ##drupal-cas (yes the double has is deliberate) on freenode.

123: 869926-123-setCacheTimesForAuthRecheck.patch queued for re-testing.

OK this one should pass. Fixed two tests that we discussed. A third that was failing was related to home homepage redirects are handled. I took out some of the homepage stuff I tried to fix in here since it can be done in a sep issue, while I already filed.

This should pass.

Now that this passes, I'll review this one locally. We have some funky scenarios here to test with (that's how I ended up with my version in the first place).

Would appreciate other review; this is a pretty high priority for us right now.

This could actually probably use a few more tests, since the patch added the "always check" feature.

Wrote a couple more tests. I noticed that even with gateway is set to "check once", it prevents users from logging out locally. This is annoying but makes sense considering how gateway works. Updating some of the help text to reflect that.

No longer applies with latest commits. Rerolled, and fixed a couple of comments.

Last patch contained a typo in the gateway mode help text. I also broke up the text into multiple t() calls to avoid html in the string passed to t(). There are other places in the code where this happens, but as we're touching this particular segment in this code, might as well clean it up now.

hah I was looking for that typo before and couldn't find it!

Steps to test the patch in #138 courtesy of @yalet:

1. Log into site A
2. Visit site B (be auto-signed in)
3. Log out of site B
4. Log out of CAS
5. Log out of site A
6. Log into site A
7. Visit site B

"At this point, you should be auto-logged in, however, the bug (if reproduced) would mean that you were not."

7.x-1.x-dev

At step 7, I was not logged in automatically. I did not receive an 'Access denied' error, but was just taken to the home page as an anonymous user.

7.x-1.x-dev + #138

At step 7, I was logged in automatically.

Committed.

yay! good work all.

Any chance of this for D6?

dunx, not likely. We're focusing most of our dev time on D8 at the moment.