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.ISSUE:
When the code in hook init to log a user out is triggered, it is possible for a value to be undefined entirely and throw this notice:
Notice: Undefined variable: user_allowed_default_login in simplesamlphp_auth_init() (line 183 of ...
CAUSE:
Variable was not properly intialized, or the check for the value was not handled as isseet()..
Patch to fllow...
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | init-logout-notice-2717473-5.patch | 1.77 KB | a-fro |
| #4 | init-logout-notice-2717473-4.patch | 1.78 KB | a-fro |
| #2 | init-logout-notice-2717473-1-2.patch | 553 bytes | jnicola |
| #2 | init-logout-notice-2717473-1-1.patch | 442 bytes | jnicola |
Comments
Comment #2
jnicola CreditAttribution: jnicola commentedAttaching two patches that both accomplish removing the unnecessary notice.
init-logout-notice-2717473-1-1.patch works by initializing the variable in advance.
init-logout-notice-2717473-1-2.patch works by using an isset() on the value.
Both accomplish the goal. Pick your flavor and run with it. I prefer initialization personally. The concept of !isset() can be a bit vague when reviewing others code.
Comment #3
jnicola CreditAttribution: jnicola commentedComment #4
a-fro CreditAttribution: a-fro commentedThanks @jnicola. I ran into this issue as well, but also a related bug. If a user leaves the settings for both the roles list and the user account field empty, then the module should assume that the local account can log in.
I've updated the patch in a way that seems to improve the intention of the settings. As it stands, upgrading our existing site, which didn't specify any roles that should be allowed to log in (since its a new setting), meant that NO local accounts could log in given the current logic. This patch fixes that issue.
Comment #5
a-fro CreditAttribution: a-fro commentedSorry, silly syntax error in that last patch. Here's another shot.
Comment #6
sherakama CreditAttribution: sherakama as a volunteer commentedPatch in #5 solved the local login issue for me. Thanks!
RTBC
Comment #7
sherakama CreditAttribution: sherakama as a volunteer commentedComment #8
jnicola CreditAttribution: jnicola commentedEh... I don't think when it comes to something relating to security like this that the assumption should be MORE functionality. That's a dangerous presumption, despite how well intentioned as it may be.
Comment #9
a-fro CreditAttribution: a-fro commented@jnicola, the problem is, without the patch, I'm unable to upgrade simplesamlphp_auth. My local accounts are no longer able to log in because I haven't updated this setting. Furthermore, creating new roles in the system should not require that I come to simplesamlphp_auth and add that role's ability to log in. The system should assume that an admin who hasn't configured the role based settings isn't using this feature.
Comment #10
jnicola CreditAttribution: jnicola commentedI would still disagree with automatic enabling everything. Convenience should never trump security, at least not for the Drupal community at large.
A different and better technique for dealing with locking oneself out is to edit the simplesamlphp_auth_activate value in the Variable table to a value of FALSE (IE: 0). This will disable the SSO part of the module and let you login as normal. If you need a good visual GUI for doing this with your DB check out Sequel Pro.
Comment #11
a-fro CreditAttribution: a-fro commentedPerhaps I've confused the issue by adding my patch to yours, which is a separate issue. We happened to upgrade to php7 and upgrade simplesamlphp_auth at the same time. Given that my patch touched the same code as yours, I thought it the best place to add it.
To try to be clear, the $allowed_default_login_roles functionality that was added introduces a bug, because any site using an older version of simplesamlphp_auth loses the ability for Drupal accounts to log in. In our case, we need to have both SSO and Drupal accounts, because we have both internal and external users, the latter who can't use SSO.
Furthermore, given that the code was already checking to see if $allowed_default_login_users was being used, it is important to assume that an admin who hasn't added any roles to $user_allowed_default_login_roles wants all roles to be able to log in. That's not a security issue, it keeps backwards compatibility with users like us.
I'd argue that, perhaps, simplesamlphp_auth_allowdefaultlogin should default to FALSE, but again, I think that would break backwards compatibility since not all admins would have set that variable.
Comment #12
sherakama CreditAttribution: sherakama as a volunteer commentedI second a-fro,
Even on a clean install it was not working the way I expected it to. With local login enabled and no options selected from the roles or users I was not able to log in with a local account. My expectation is that with local login enabled and no option set in the roles or users options that I should be able to log in with ALL of the local accounts. This is much preferred as it will prevent issues and maintenance later as roles or users are added.
I think the patch in #5 does a good job in checking the available configuration and does not add a security hole.
Comment #13
jnicola CreditAttribution: jnicola commentedI could definitely see this as as separate issue.
Comment #14
denix CreditAttribution: denix as a volunteer commented#5 works fine
Comment #15
dmundra#5 works great. I would say if this not fixed then maybe only the variable warning should be fixed and the description for the field "Which users should be allowed to login with local accounts?" should not say "If left blank, all local accounts can login."