Auto login / isPassive support improvements

[sammarshallou]

Note: This pull request currently includes 2 commits but the first commit is just my other pull request about making Behat testing easier #419.

This feature is a new configuration option that allows users to configure auth_saml2 to attempt passive login automatically, without the user having to go to a login page.

This is useful for servers that allow users to access without being logged in (e.g. public access), but where some users might be already logged on to the institutional identity provider.

There are two options: try at the start of a session, or try every time a cookie changes. The first option is simple but means it won't detect login if you go to the server before logging in. The second option can be useful if you are able to configure your identity provider to set a cookie, within the domain used by the Moodle system, when the user logs in.

In both cases, it will not do anything when you are already logged in (this is an autologin feature, not autologout).

[brendanheywood]

thanks heaps @sammarshallou

I've only had a very superficial look:

1. At some point ages ago there was a real embedded idp for behat testing, I'm honestly not sure what its status is:

https://github.com/catalyst/moodle-auth_saml2/tree/master/tests/idp

If your patch does completely replace this can you please remove the old stuff please?

2. On the second commit, isPassive was kinda half supported already so it would be good to double check if this is needed and if there is a gap align the two as needed (this is probably a missing documentation problem)

auth.php:535:        $passive = $this->config->duallogin == saml2_settings::OPTION_DUAL_LOGIN_PASSIVE;
auth.php-536-        $passive = (bool)optional_param('passive', $passive, PARAM_BOOL);
auth.php-537-        $params = ['isPassive' => $passive];
auth.php-538-        if ($passive) {
auth.php-539-            $params['ErrorURL'] = "{$CFG->wwwroot}/login/index.php?saml=0";
auth.php-540-        }

[sammarshallou]

Thanks for taking a look Brendan, I appreciate it, I know everyone's busy!

Re first point - I'll figure out the existing embedded IdP for Behat in more detail and do a full replacement.

Re second point, about passive support - unless I've missed something, this doesn't duplicate the existing support. The difference is, the existing passive support only works on the login screen. For most Moodle installs (including some of ours) where login is usually required e.g. to access a course, this is fine as you will immediately get to a screen where you're prompted to login, so then it will do the passive login right away. The issue for us is that we have a couple of sites that are largely open to the public, so you can go to many pages without logging in, which means it doesn't do the passive login - and e.g. staff who are already logged in to the IdP don't get the various edit links, because they're still just guest on moodle.

[sammarshallou]

Thank you for the review! All good comments, I'll work on this and respond to each one soon.

[sammarshallou]

I've now updated based on all the review comments (I think). Feel free to say if there are more things need changing or you disagree with anything, I'm happy to change it again.

Since I did a force push, it's not obvious from history what I changed in response to the review, so I'm attaching here a patch file of the differences.

autologinchanges.txt

[dmitriim]

Hi @sammarshallou ! Thank you for you changes.
Sorry but I maybe miss something, but I'm still not 100% clear why we apply autologin on pages that do not require login at all and ignore others? I would think that this would be helpful for IDPs (e.g. like OKTA) that do not support autologin.

[sammarshallou]

Yes, this feature is intended to only run on pages that don't require login (either because they literally don't call require_login, or because they allow automatic guest login).

The reason for this is that if the page isn't one of these, i.e. if it DOES require login (& not allow guest autologin), then the user will be redirected to the login screen. And auth_saml2 already has an automatic login feature once you get to the login screen (set 'duallogin' to 'Passive mode' if you also allow other logins, or 'No' if you only allow login from the one SAML IdP). So I didn't want to overlap with the existing feature which already works well.

In terms of whether it works on different IdPs (I don't know about OKTA sorry), this new feature requires passive mode support from the IdP, the same as the existing automatic login from login screen, so I don't think there should be any difference in terms of support.

[sammarshallou]

I've rebased this (all the CI tests fail for master and this branch due to the other issue I pushed, but I did run them in a test branch including that fix and it passes still). Any chance of getting it in?

[dmitriim]

Sorry @sammarshallou. It somehow disappeared from my radar. Unfortunately, I won't be able to finish up this review. From the first glance the patch looks good and does what it supposed to do. However, I think it can be somehow used for for IDPs that do not support passive mode (like OKTA). But that could be done in a separate request when someone will need it.

I'll ping @brendanheywood or maybe @danmarsden to check if they have some time.

[sammarshallou]

Thanks @dmitriim, I understand not having time!

By the way if it would help, my project manager said we might have a (small) budget available that could pay for Catalyst time to finish code reviewing this & get it in - I'm not sure if that is in any way feasible, maybe all the relevant people are busy for the near future anyway. But if it would help, let me know and I'll ask our project manager to get in touch.

[danmarsden]

Thanks for the ping @dmitriim - I've had a look through the code and I couldn't see anything that I thought had to be changed - the fact that this is turned off by default, doesn't seem to affect anyone that has it turned off, comes with some good behat tests, would make me happy to +1 this.

@dmitriim if your reccommendation is to merge this in, I'm happy to +1 and hit the button :-)

[dmitriim]

@danmarsden yeah. Go ahead. I'm wondering why CI is not running?

[danmarsden]

it's something weird - I can see it's passing on Sam's own repo - I'm guessing it will run (and pass) when I merge it.

[danmarsden]

@sammarshallou had another PR that fixed that so I've merged it in and that seems to have fixed it.

[sammarshallou]

Thank you both. :)