Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-16168

Moodle CAS crashes if no LDAP is set up

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.2, 2.0, 2.1, 2.2
    • Fix Version/s: 1.9.13, 2.0.4, 2.1.2
    • Component/s: Authentication
    • Labels:
      None
    • Environment:
      This was discovered using Moodle on an Ubuntu server, though I doubt the issue is related to the environment.

      Description

      After configuring CAS on a fresh Moodle install and updating the mdl_user table with:

      update mdl_user set auth='cas';

      we get successfully redirected on login to the CAS server and back to Moodle as we would expect.

      However, it then returns an error about not being able to connect to the LDAP server. We aren't using LDAP, and we didn't configure Moodle to use LDAP, so that would be a bug.

      The patch that we made to work around the bug was to comment out the calls to update_user_record and create_user_record that are found in lib/moodlelib.php. It would be better if Moodle recognized that we were not using LDAP and that it wouldn't try to update or create users in that case.

        Gliffy Diagrams

        1. cas_minus_ldap.patch
          0.9 kB
          Matthew Turney
        2. cas_minus_ldap-2.0.patch
          2 kB
          Ivan Dackiewicz

          Issue Links

            Activity

            Hide
            johanr Johan Reinalda added a comment -

            We are having the exact same issue trying to get CAS setup.
            We are providing all user, course and enrollments via IMS to Moodle.

            We would like to use CAS, but moodle needs an ldap config (fails without it). Witjhout editing source, we cannot accomplish this.

            Johan

            Thunderbird School of Global Management
            www.thunderbird.edu

            Show
            johanr Johan Reinalda added a comment - We are having the exact same issue trying to get CAS setup. We are providing all user, course and enrollments via IMS to Moodle. We would like to use CAS, but moodle needs an ldap config (fails without it). Witjhout editing source, we cannot accomplish this. Johan Thunderbird School of Global Management www.thunderbird.edu
            Hide
            pho3nixf1re Matthew Turney added a comment -

            I solved this by adding an if statement that returns false if there is no LDAP host url. Attaching patch file from my GIT repo.

            Show
            pho3nixf1re Matthew Turney added a comment - I solved this by adding an if statement that returns false if there is no LDAP host url. Attaching patch file from my GIT repo.
            Hide
            pho3nixf1re Matthew Turney added a comment -

            Here is the patch file that I generated from my hack to make CAS work without LDAP while still allowing LDAP to work if its configured.

            Show
            pho3nixf1re Matthew Turney added a comment - Here is the patch file that I generated from my hack to make CAS work without LDAP while still allowing LDAP to work if its configured.
            Hide
            mblake Michael Blake added a comment -

            Petr, can you have a look at this one - an MP is reporting it's affecting one of their clients. I'm not sure the patch that Matthew has attached addresses the entire issue. Thanks.

            Show
            mblake Michael Blake added a comment - Petr, can you have a look at this one - an MP is reporting it's affecting one of their clients. I'm not sure the patch that Matthew has attached addresses the entire issue. Thanks.
            Hide
            skodak Petr Skoda added a comment -

            I am no LDAP or CAS expert, reassigning to our guru.

            Show
            skodak Petr Skoda added a comment - I am no LDAP or CAS expert, reassigning to our guru.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Has anyone else tested this patch?

            Show
            dougiamas Martin Dougiamas added a comment - Has anyone else tested this patch?
            Hide
            iarenaza Iñaki Arenaza added a comment -

            After talking about this with Martin and Petr, I think the patch from Matthew is the best solution for 1.9 without changing the current semantics/behaviour of the CAS plugin.

            The only (minor) drawback is that if you actually want to use LDAP to pull your user details in and forget to specify the LDAP host, you won't get any diagnostics back.

            This also assumes that you'll be pulling your user details from somewhere else (CSV file, manual editions, etc.). Otherwise the user will need to fill in the details herself.

            By the way, there's a side effect to this patch: even if CAS doesn't use the data mapping fields for user details when no LDAP host is configured, the locking of the user detail fields is respected. If this is not desired behaviour, more patching will be required.

            Finally, all of this is 1.9.x only. Moodle 2.0 is a different story and we'll need to refactor a bit of code.

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - After talking about this with Martin and Petr, I think the patch from Matthew is the best solution for 1.9 without changing the current semantics/behaviour of the CAS plugin. The only (minor) drawback is that if you actually want to use LDAP to pull your user details in and forget to specify the LDAP host, you won't get any diagnostics back. This also assumes that you'll be pulling your user details from somewhere else (CSV file, manual editions, etc.). Otherwise the user will need to fill in the details herself. By the way, there's a side effect to this patch: even if CAS doesn't use the data mapping fields for user details when no LDAP host is configured, the locking of the user detail fields is respected. If this is not desired behaviour, more patching will be required. Finally, all of this is 1.9.x only. Moodle 2.0 is a different story and we'll need to refactor a bit of code. Saludos. Iñaki.
            Hide
            bushido Mark Nielsen added a comment -

            Would any of the other CAS auth hooks cause errors when LDAP is not being used (EG: user_exists, sync_roles, etc)?

            Show
            bushido Mark Nielsen added a comment - Would any of the other CAS auth hooks cause errors when LDAP is not being used (EG: user_exists, sync_roles, etc)?
            Hide
            iarenaza Iñaki Arenaza added a comment -

            As far as I can tell, the only place that could cause errors right now is sync_roles() (note: I'm always talking about 1.9)

            user_exist() is only called from login/signup_form.php if the auth plugin is configured as the self registration plugin, which is not possible for CAS.

            On the other hand, sync_roles() is unconditionally called from authenticate_user_login() (in lib/moodlelib.php), which in turn calls iscreator().
            There, ff we haven't configured a LDAP server but have configured any of the two course creators settings (attribute creator, group creator), then we try to connect to a LDAP server and abviously fail.

            I'd say this is a rather strange situation, but I'm going to play safe and exit early from iscreator() if there's no LDAP server configured.

            Unless I've missed something in the code analysis I've done, the rest of the LDAP-related code paths shouldn't be reached unless you run cas_ldap_sync_users.php. But you shouldn't be running that unless you have a LDAP server to sync with, should you? Anyway, just to play safe again, I'm going to exit early from sync_users() if there's no LDAP server configured.

            Mark, thanks a lot for prompting me to have a deeper look at it

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - As far as I can tell, the only place that could cause errors right now is sync_roles() (note: I'm always talking about 1.9) user_exist() is only called from login/signup_form.php if the auth plugin is configured as the self registration plugin, which is not possible for CAS. On the other hand, sync_roles() is unconditionally called from authenticate_user_login() (in lib/moodlelib.php), which in turn calls iscreator(). There, ff we haven't configured a LDAP server but have configured any of the two course creators settings (attribute creator, group creator), then we try to connect to a LDAP server and abviously fail. I'd say this is a rather strange situation, but I'm going to play safe and exit early from iscreator() if there's no LDAP server configured. Unless I've missed something in the code analysis I've done, the rest of the LDAP-related code paths shouldn't be reached unless you run cas_ldap_sync_users.php. But you shouldn't be running that unless you have a LDAP server to sync with, should you? Anyway, just to play safe again, I'm going to exit early from sync_users() if there's no LDAP server configured. Mark, thanks a lot for prompting me to have a deeper look at it Saludos. Iñaki.
            Hide
            allegre Guillaume Allegre added a comment -

            Please if you integrate this patch, also have a look at #22277, patch which I submitted 4 months ago.
            It includes the current patch, and adds a parameter to let the admin choose if the cas user can always autosubscribe or must have a local account.

            Show
            allegre Guillaume Allegre added a comment - Please if you integrate this patch, also have a look at #22277, patch which I submitted 4 months ago. It includes the current patch, and adds a parameter to let the admin choose if the cas user can always autosubscribe or must have a local account.
            Hide
            bushido Mark Nielsen added a comment -

            Is there a hold up on this ticket?

            Show
            bushido Mark Nielsen added a comment - Is there a hold up on this ticket?
            Hide
            allegre Guillaume Allegre added a comment -

            Sorry, I don't understand.
            I tried to bring useful information, but if you think my comment is irrelevant,
            I apologize.

            Show
            allegre Guillaume Allegre added a comment - Sorry, I don't understand. I tried to bring useful information, but if you think my comment is irrelevant, I apologize.
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Sorry for the delay Mark. I've been side-tracked by a couple of work-related things.

            I've just committed the fixes I mentionned last week, so I'm closing this for 1.9.

            The fix for 2.0 is still pending, so I'll reopen the bug and mark it as 2.0 only (the tracker doesn't let me mark the bug as fixed for one version and pending for another one, and I don't want to open a new ticket to keep history in just one place).

            Thanks again to Matthew Turney for the original patch and Mark Nielsen for the feedback

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Sorry for the delay Mark. I've been side-tracked by a couple of work-related things. I've just committed the fixes I mentionned last week, so I'm closing this for 1.9. The fix for 2.0 is still pending, so I'll reopen the bug and mark it as 2.0 only (the tracker doesn't let me mark the bug as fixed for one version and pending for another one, and I don't want to open a new ticket to keep history in just one place). Thanks again to Matthew Turney for the original patch and Mark Nielsen for the feedback Saludos. Iñaki.
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Oops! I forgot to comment on Guillame's request.

            I've been told that 1.9 is in bug-fix only mode and that we shouldn't be adding new features. So I wonder if you wouldn't mind if we re-target your feature request (MDL-22277) for 2.0.

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Oops! I forgot to comment on Guillame's request. I've been told that 1.9 is in bug-fix only mode and that we shouldn't be adding new features. So I wonder if you wouldn't mind if we re-target your feature request ( MDL-22277 ) for 2.0. Saludos. Iñaki.
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Reopening for 2.0.

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Reopening for 2.0. Saludos. Iñaki.
            Hide
            allegre Guillaume Allegre added a comment -

            Answer to Iñaki Arenaza :
            I wouldn't mind if you re-target MDL-22277 ; although I think it's quite close to this bugfix.

            Show
            allegre Guillaume Allegre added a comment - Answer to Iñaki Arenaza : I wouldn't mind if you re-target MDL-22277 ; although I think it's quite close to this bugfix.
            Hide
            czerynon Ivan Dackiewicz added a comment -

            I solved this problem in 2.0 using the same idea of Matthew while working in the "Red Social Educativa Argentina" project.

            Show
            czerynon Ivan Dackiewicz added a comment - I solved this problem in 2.0 using the same idea of Matthew while working in the "Red Social Educativa Argentina" project.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks all - This has been integrated now.

            Cheers
            San

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks all - This has been integrated now. Cheers San
            Hide
            dougiamas Martin Dougiamas added a comment -

            I'm going to pass this because:

            • it's just a little too hard for us to set up a whole CAS environment to test this
            • it looks like several others have tested it already
            • I trust Inaki
            Show
            dougiamas Martin Dougiamas added a comment - I'm going to pass this because: it's just a little too hard for us to set up a whole CAS environment to test this it looks like several others have tested it already I trust Inaki
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Martin,

            I can provide a virtual machine with CAS installed and configured (it's the one I use myself to develop and test the patches), in case you (Moodle HQ) are interested for future testing.

            It uses only free software, so copying and distributing it is not a problem

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Martin, I can provide a virtual machine with CAS installed and configured (it's the one I use myself to develop and test the patches), in case you (Moodle HQ) are interested for future testing. It uses only free software, so copying and distributing it is not a problem Saludos. Iñaki.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Before closing this...

            is there any reason for this being only 20_STABLE and not master at all?

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Before closing this... is there any reason for this being only 20_STABLE and not master at all? Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            And now this is part of Moodle 2.1beta, yay! Closing.

            NOTE: Plz confirm if this must be ported to master, creating new linked issue or whatever, TIA!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And now this is part of Moodle 2.1beta, yay! Closing. NOTE: Plz confirm if this must be ported to master, creating new linked issue or whatever, TIA!
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Sorry, my fault.

            Yes, this should be ported to master (I've been out of touch with Moodle develpment for a few months and haven't kept up to date).

            What's the preferred way to deal with it? Opening a new issue and linking here? Reopening this one?

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Sorry, my fault. Yes, this should be ported to master (I've been out of touch with Moodle develpment for a few months and haven't kept up to date). What's the preferred way to deal with it? Opening a new issue and linking here? Reopening this one? Saludos. Iñaki.
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Reopening this for 2.1 and HEAD (I pushed the fixes for 2.0.x only).

            Integration request will follow in a couple of minutes.

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Reopening this for 2.1 and HEAD (I pushed the fixes for 2.0.x only). Integration request will follow in a couple of minutes. Saludos. Iñaki.
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Changes are exactly the same as those that are already integrated in 2.0.4

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Changes are exactly the same as those that are already integrated in 2.0.4 Saludos. Iñaki.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            iarenaza Iñaki Arenaza added a comment -

            PILL branches rebased

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - PILL branches rebased Saludos. Iñaki.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (we use to create new issues, but it's not really critical, just FYI). Integrating this now (21 and master)...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (we use to create new issues, but it's not really critical, just FYI). Integrating this now (21 and master)...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (accumulating all versions where this has been finally applied)

            Due to the difficulties getting this tested, it would be great to get confirmation from somebody running 2.0/2.1 with the fix above applied.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (accumulating all versions where this has been finally applied) Due to the difficulties getting this tested, it would be great to get confirmation from somebody running 2.0/2.1 with the fix above applied. TIA and ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            As said above, any feedback will make this pass the testing phase easily. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! As said above, any feedback will make this pass the testing phase easily. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing as far as the new patch is 100% the same than the applied originally for 20_STABLE (and was passed).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing as far as the new patch is 100% the same than the applied originally for 20_STABLE (and was passed). Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay!

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay! Closing, ciao

              People

              • Votes:
                20 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  1/Aug/11