We need to take a harder look at all areas where we return responses, and make sure we return proper cache metadata or make sure the responses are never cached.

Here's a list of all the places that I think need another look.

  1. LogoutController (/caslogout): This logs a user out of Drupal and then redirects them to CAS server logout URL to log them out there as well. This controller is only relevant for logged in users. Anonymous users that hit it should receive a 403 or perhaps be redirected to the homepage (and that's OK to cache). For authenticated users, the redirect response to the CAS server for logout can be cached, but needs to vary by user session (there's a cache context for that). Prolly best to just not cache this at all by setting no_cache option in the route config.
  2. ServiceController (/casservice): Lots of dynamic stuff in here, and lots of different redirect responses that can be returned depending. I think it's best that we turn off caching completely for this controller by setting no_cache in route config.
  3. ProxyCallbackController (/casproxycallback): This controller stores the PGT mapping and returns a simple 200 "OK" response. It should not be cached. Even if we did cache it, it would never result in a cache-hit, because the CAS server would always hit this url with a new set of query string params. Disable by setting no_cache in route config.
  4. CasSubscriber: This handles the implementation of the gateway and forced auth features. Since it operates on all page requests, there's no specific page we can simply disable caching on (like we can do for the above controllers). When we return redirect responses to CAS server for "forced login", we can indeed cache that response, and vary it on CAS settings. For gateway redirects, it's a little more complicated. We may be able to cache some of the responses in that case, and vary then by CAS settings and some other user info.

Comments

bkosborne created an issue. See original summary.

bkosborne’s picture

Status: Active » Needs review
StatusFileSize
new4.19 KB

Here's a patch that I think resolves the issues. I added the no_cache config params for each of the three routes I listed above.

For the CAS Subscriber which implements the forced login and gateway features, I returned a non-cacheable redirect response (by creating my own redirect response that extends from the abstract SecuredRedirectResponse). That appeared to be the only way to create a non-cacheable external redirect.

I tried to get a cacheable response working for the forced login in the subscriber, since it should technically be possible (unlike gateway which will never work), but there's still a large mess of things we would need to vary the response with. The biggest concern there was the user agent string because of checks we do for crawlers. So I just return a non-cacheable redirect there as well.

Status: Needs review » Needs work

The last submitted patch, 2: cache_fixes.patch, failed testing.

The last submitted patch, 2: cache_fixes.patch, failed testing.

The last submitted patch, 2: cache_fixes.patch, failed testing.

The last submitted patch, 2: cache_fixes.patch, failed testing.

The last submitted patch, 2: cache_fixes.patch, failed testing.

bkosborne’s picture

Status: Needs work » Needs review
StatusFileSize
new0 bytes

Patch was missing new class.

bkosborne’s picture

StatusFileSize
new5.33 KB

ugh

The last submitted patch, 8: cache_fixes_2.patch, failed testing.

The last submitted patch, 8: cache_fixes_2.patch, failed testing.

  • bkosborne committed b7934c0 on 8.x-1.x
    Issue #2649772 by bkosborne: Resolve caching and redirect loop bugs
    
bkosborne’s picture

Status: Needs review » Fixed

Committed

Status: Fixed » Closed (fixed)

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