Ok.. first of all I have to say, I'm not entirely sure which version's bug this is.
When I tried version 2.x I used the following configuration:
- Allow authentication with local Drupal accounts
- Didn't select any roles on "Which ROLES should be allowed to login with local accounts?"
- Didn't write anything in "Which users should be allowed to login with local accounts?"
I was logged in as user 1, configuration saved & continued using the module.. Though that was how it is supposed to be used.
But, then I tried version 3.x using exactly the same options.
When I pressed "save", I got instantly logged out.
I tried to login again (using drush uli's one-time login link), and got instantly kicked out again.
After some testing (& after reading the code), I realized that I should have added "administrator" role or user 1 on roles/users allowed to logged in locally. Maybe this felt really strange to me because version 2.x (which I tried first) had a different behavior, but in any case I don't think it is suppose to log out the administrator when clicking save.
Strange thing is that code looks almost identical in both versions (in simplesamlphp_auth_init in 2.x and in simplesaml_auth_moderate_local_login at 3.x)
Firstly, in my opinion, I think that at least user 1 should be excluded or module needs something else to be done here.. maybe like checking only during login. For example, I would like to use one-time login links from drush in all users (for testing), but do not allow them to authenticate without simplesaml.
Comment | File | Size | Author |
---|---|---|---|
#8 | simplesamlphp_auth-local_account_automatically_loggedout-2478499-8.patch | 1.8 KB | pbuyle |
#7 | 2478499-8.x-3.x-7.patch | 655 bytes | svendecabooter |
#6 | 2478499-7.x-3.x-6.patch | 1.01 KB | svendecabooter |
#2 | simplesamlphp_auth-automatic-logout-2478499-2.patch | 1.65 KB | malik.kotob |
Comments
Comment #1
snufkin CreditAttribution: snufkin commentedIt does sound that we've lost some functionality when compared to 2.x, the 3.x behaviour should be the same as in 2.x in this regard. I've added the issue to the 3.x-RC blockers.
Comment #2
malik.kotob CreditAttribution: malik.kotob at Acquia commentedthere's definitely a bug in the hook_init implementation, however, if you don't require that the SP (service provider) initiate the log in automatically, you can benefit from just removing the hook_init altogether in the .module file.
attached is a patch for 3.x which removes the hook.
Comment #3
snufkin CreditAttribution: snufkin commentedThe SP detecting the users session was a feature we had in 2.x, so if we remove this now we would lose functionality from the previous stable release. Also in an issue I can't find right now people fought to bring the hook_init back, so I think there is demand for the features this enables and we shouldnt remove it again.
Comment #4
dstol@snufkin, we realize that's not something you're going to commit. I think, at minimum, there should be a variable_get that can toggle the hook_init and bail out early from it. The other option would just be to module_implements_alter the hook_init out.
Comment #5
svendecabooterWould it be reasonable to set the default value for the local roles settings in SimpleSAMLphp to the "Administrator" role setting in Drupal core?
This Drupal "Administrator" role setting can be set at Administration » Configuration » People » Account settings.
Though it is not required that this variable points to a role, in the Standard install profile for Drupal 7 an Administrator role gets created and assigned to user 1.
I'm assuming a large majority of Drupal sites has this variable set.
If not set, no default role would be selected in the SimpleSAMLphp configuration, still making the originally reported issue a problem, but one that is less likely to occur.
We could also update the installation instructions to warn for that.
Comment #6
svendecabooterAttached a patch that introduces the behaviour described above.
Comment #7
svendecabooterAnd a version for 8.x-3.x as well.
Comment #8
pbuyle CreditAttribution: pbuyle at Floe design + technologies commentedThe logic to denies non-SAML-authenticated access to the site is already correctly implemented in
simplesaml_auth_moderate_local_login()
, is there any reason not to re-use it insimplesamlphp_auth_init()
(as done in the attached patch)?Note: Previous patches will only ensure a role is granted local account login (once the settings form is saved). The descriptions on the form are not clear whether or not a role must be provided for local accounts login. However,
simplesaml_auth_moderate_local_login()
support not requiring any role or ID.