Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The simplesamlphp_auth_init() function runs on every page load and sets a session cookie, which causes Varnish cache misses.
To get around this, we have applied the following patch to the init function right at the top:
// Bail on pages that aren't concerned with user login.
if (arg(0) !== 'saml_login' && arg(0) !== 'user' && arg(0) !== 'my-account') {
return;
}
I'm not very familiar with the module as I took on this work from another developer and was subsequently pulled off it again, but perhaps you could consider something like this in a future release?
I'm aware that my patch above will prevent auto-login-on-every-page behavior, but for high performance sites this is probably necessary.
Comments
Comment #1
joshk CreditAttribution: joshk commented+1 for this. Having it start a session on every pageload makes reverse-proxy caching next to impossible. Would love to see this as a configurable option.
Comment #2
andy.smith CreditAttribution: andy.smith commentedWe were experiencing this same issue. I have applied the patch and it seems to have solved the issue.
Comment #3
Mark Theunissen CreditAttribution: Mark Theunissen commentedIt also seems that the cookie is not destroyed when a user logs out. This should probably be fixed too.
Comment #4
andy.smith CreditAttribution: andy.smith commentedIt appears like the cookie is not destroyed until after the user has logged out AND closed the browser - if they just log out they will not be served cached pages.
Comment #5
antgiant CreditAttribution: antgiant commentedThere are several problems with this approach.
I have attached a patch that takes a slightly different approach. I only load the init function when it is needed. Calling it from the login, logout and block display functions. So warning if you turn on the block it will break caching for any page it shows up on.
Finally, there are two patches. One for the current D6 version of the module and one for the Current D7 version of the module. Hopefully this helps.
Comment #6
rgristroph CreditAttribution: rgristroph commentedI also had this issue, and I used the original suggestion of a patch in the init hook to fix it.
Comment #7
MrHaroldA CreditAttribution: MrHaroldA commentedCan anybody explain me why all this activity needs to be done *every* page view in hook_init()? Isn't it sufficient to run this code only on the /saml_login page callback?
Ah, that's why! ;)
Comment #8
MrHaroldA CreditAttribution: MrHaroldA commentedHere's a patch that disables the login code with a separate setting in the admin form: "Try to validate the SAML session on each page view."
It changes the _simplesamlphp_auth_isEnabled() behavior by checking that variable and the current path.
Comment #9
antgiant CreditAttribution: antgiant commentedI'm sorry but I'm pretty sure you are misunderstanding both my patch and how the module works.
First, with my patch hook_init() is not called on every page load (unless you turn on the block provided by this module). You can see this with both my comment above the init function and by the _(underscore) in front of _simplesamlphp_auth_init(). That underscore both makes the method private and prevents hook_init() from calling it.
Second, your patch is effectively the same as disabling the block provided by this module as that is where the auto login functionality lives. Thus it is also true that your patch breaks the block functionality unless your new checkbox is checked.
Finally, as I commented hard coding URLs is a bad idea. First, your single hard coded URL breaks the ability to detect the standard login page and take action when the module is configured to block it. Second, your patch breaks all custom login URLs. Third, as with the original patch your patch breaks the logout link unless your new checkbox is checked.
In short, please don't use your patch just disable the block and you will get the functionality or your patch without the nasty side effects. (Assuming my patch is installed.)
Comment #10
antgiant CreditAttribution: antgiant commentedHiding new patch as I believe it makes things worse.
Comment #11
MrHaroldA CreditAttribution: MrHaroldA commentedYeah, you're right ... I thought I could fix it in a single callback, instead of everywhere, but I missed the block (don't use it) and the logout page.
I do believe your _simplesamlphp_auth_init() should be called after the _simplesamlphp_auth_isEnabled() check.
But that's just a minor issue, which this module is filled with ;)
Comment #12
chustedde CreditAttribution: chustedde commentedThe previous Drupal 7 patch works for existing users, but will throw a database exception if a new user tries to log in via SAML. This patch is the same as the one by antgiant, minus one call to _simplesamlphp_auth_init() that was causing a duplicate call to user_save(), causing the db error.
Comment #13
MrHaroldA CreditAttribution: MrHaroldA commentedI wrote a new SimpleSAMLphp implementation that I'd like to sandbox in a couple of weeks. It's only around 150 lines long and supports ErrorURL, cleans up cookies, uses the Drupal API everywhere, Varnish-tested, and lots of other fixes ...
We had to get our DigiD login ready for an audit, and it would require more than a couple of patches to bend this module to our needs.
My module is quite heavily geared towards DigiD, I'll have a look if I can make it more generic and publish it as a leaner version of the simplesamlphp_auth module.
Comment #14
geekwisdom CreditAttribution: geekwisdom commented@MrHaroldA - I have considered making a simplified saml SP module for Drupal using sporto https://code.google.com/p/sporto/ as the SAML SP implementation. I've also considered making it an installation choice in this module (i.e., Basic single IdP environment or Advanced and multiple IdP environments). I'd be interested to see your simplified module.
Comment #15
MrHaroldA CreditAttribution: MrHaroldA commented@geekwisdom / @all: the code will be available as part of a distribribution for Dutch government sites, but if you can't wait that long: please use my contact form to email me, and I'll mail you the DigiD-feature in advance ;)
Comment #16
dstolNeither the patch in #5 or the patch in #12 work. Both produce fatals at various points in the login flow.
Comment #17
grasmash CreditAttribution: grasmash commentedI've created a 7.x-3.x branch of this module with significant refactoring. It includes patches from the following issues:
#2190487: User_login form should have a federated link
#2213283: When simplesamlphp_auth_activate is 0 watchdog messages flood the syslog/dblog
#2221679: Destruction of session causes fatal error
#2020009: Session cookie being set, breaking through Varnish cache.
#1824194: hook_user_insert issues when user adding a new user is logged in via sso
#2205901: Allow SimpleSAML authentication on a per-user basis
https://github.com/grasmash/simplesamlphp_auth/tree/7.x-3.x
Given that so much work has been done (and it will not cleanly merge with 7.x-2.x) I propose that a new 7.x-3.x-dev branch be added to the module.
Comment #18
nicksanta CreditAttribution: nicksanta commentedI have attached a reroll of patch #12.
That patch is missing 2 calls to _simplesamlphp_auth_init(); one in hook_user_insert(), the other in simplesamlphp_auth_is_authenticated(). This results in the following error:
I have also added a gate to _simplesamlphp_auth_init() to ensure it is not accidentally executed multiple times per page load.
Edit: @grasmash - this patch applies cleanly to your 3.x branch on github.
Comment #20
snufkin CreditAttribution: snufkin commentedLets focus on implementing this against 3.x, it was set up based on Matts fork. Could you reroll the patch?
Comment #21
dealancer CreditAttribution: dealancer commentedAlso here is related issue https://www.drupal.org/node/2377727 - session cookie still exists after user logs out.
Comment #24
mstrelan CreditAttribution: mstrelan commentedHere is a straight reroll of #18 against 7.x-3.x-dev. I've not tested it and no idea if it's the right approach.
Comment #25
mvcWe are using #18 with 7.x-2.0-alpha2 in prod and it works as advertised for us.
Comment #26
baikho