Moodle

NTLM SSO conflicts with autologinguest setting.

Details

  • Type: Sub-task Sub-task
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.9
  • Fix Version/s: DEV backlog
  • Component/s: Authentication
  • Labels:
  • Affected Branches:
    MOODLE_19_STABLE

Description

With HTTPS (probably not pertinent) and NTLM SSO enabled, the autologinguest does not log in visitors to course URLs using the guest account. This is because the call to auth plugins' loginpage_hook functions in login/index.php happens before the check for a guest login.

Moodle forum discussion:
http://moodle.org/mod/forum/discuss.php?d=121761

Please can someone have a look at the following suggested patch for sanity or glaring security ramifications before I commit it
http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=133e18d281e89449f5544092132681563a5c9fdf

Cheers, J

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda (skodak) added a comment -

I do not like the patch at all, we can not skip the hooks this way, I guess you can add the guest exception into your plugin

Show
Petr Škoda (skodak) added a comment - I do not like the patch at all, we can not skip the hooks this way, I guess you can add the guest exception into your plugin
Hide
Permalink
Iñaki Arenaza added a comment -

Petr,

I don't think the patch is that bad. After all, if something is requesting guest login it doesn't make much sense to run through all the auth hooks as we are not interested in authenticating the user at all. Am I missing something?

Saludos. Iñaki.

Show
Iñaki Arenaza added a comment - Petr, I don't think the patch is that bad. After all, if something is requesting guest login it doesn't make much sense to run through all the auth hooks as we are not interested in authenticating the user at all. Am I missing something? Saludos. Iñaki.
Hide
Permalink
Martin Dougiamas added a comment -

I agree, I think the patch makes sense...

What examples of auth plugin functionality would this kill?

Show
Martin Dougiamas added a comment - I agree, I think the patch makes sense... What examples of auth plugin functionality would this kill?
Hide
Permalink
Petr Škoda (skodak) added a comment -

The point is that the hook allows auth plugin to do "anything". This patch prevents execution of hooks in some conditions is not backwards compatible and also makes the hooks less flexible.
Moving the guest autologin into hooks is no extra work, so where is the problem?

Show
Petr Škoda (skodak) added a comment - The point is that the hook allows auth plugin to do "anything". This patch prevents execution of hooks in some conditions is not backwards compatible and also makes the hooks less flexible. Moving the guest autologin into hooks is no extra work, so where is the problem?
Hide
Permalink
Jonathan Harker added a comment -

I see your point, I guess it's just that I don't like copying and pasting the same code into several different places (i.e. each hook will need to do the same check).

Show
Jonathan Harker added a comment - I see your point, I guess it's just that I don't like copying and pasting the same code into several different places (i.e. each hook will need to do the same check).
Hide
Permalink
Petr Škoda (skodak) added a comment -

the check can be placed into one function so it would be a one line change in each hook

Show
Petr Škoda (skodak) added a comment - the check can be placed into one function so it would be a one line change in each hook
Hide
Permalink
Iñaki Arenaza added a comment -

Having mulled over it on the way home, I think it's actually a bug in the NTLM SSO code. If the code determines that the user can't be authenticated (for whatever reasons), it should end up redirecting back to the login page (as it does) keeping the auto login guest 'flag' the login page set at the beginning. And it currently doesn't.

I think something along the lines of the attached patch (for 1.9) could be a proper fix.

Saludos. Iñaki.

Show
Iñaki Arenaza added a comment - Having mulled over it on the way home, I think it's actually a bug in the NTLM SSO code. If the code determines that the user can't be authenticated (for whatever reasons), it should end up redirecting back to the login page (as it does) keeping the auto login guest 'flag' the login page set at the beginning. And it currently doesn't. I think something along the lines of the attached patch (for 1.9) could be a proper fix. Saludos. Iñaki.
Hide
Permalink
Mike Norton added a comment -

Iñaki, that will take care of problem #1 that I described in my forum post that Jonathan linked to above, but it would still leave me with problem #2. To allow people to access guest courses through my SSO reverse-proxy, they need to be able to get to the course without the NTLM even being attempted. Attempting NTLM will trap my users at the proxy server's login form.

Now maybe this is totally out-to-lunch, but... what if the "auto guest login" was actually an auth plugin of its own? Then as a site admin I could have the option of setting its order higher than the LDAP, but other admins who still want to attempt NTLM prior to auto-guest could put it lower.

Show
Mike Norton added a comment - Iñaki, that will take care of problem #1 that I described in my forum post that Jonathan linked to above, but it would still leave me with problem #2. To allow people to access guest courses through my SSO reverse-proxy, they need to be able to get to the course without the NTLM even being attempted. Attempting NTLM will trap my users at the proxy server's login form. Now maybe this is totally out-to-lunch, but... what if the "auto guest login" was actually an auth plugin of its own? Then as a site admin I could have the option of setting its order higher than the LDAP, but other admins who still want to attempt NTLM prior to auto-guest could put it lower.
Hide
Permalink
Jonathan Harker added a comment -

I like the idea of autologinguest being a separate auth plugin. That could also take care of a potential (separate issue) conflict with loginhttps as well.

Show
Jonathan Harker added a comment - I like the idea of autologinguest being a separate auth plugin. That could also take care of a potential (separate issue) conflict with loginhttps as well.

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.