Moodle
  1. Moodle
  2. MDL-16168

Moodle CAS crashes if no LDAP is set up

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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.
    • Rank:
      18940

      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.

      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
          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
          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
          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
          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
          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
          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
          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
          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
          Petr Škoda added a comment -

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

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

          Has anyone else tested this patch?

          Show
          Martin Dougiamas added a comment - Has anyone else tested this patch?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Mark Nielsen added a comment -

          Is there a hold up on this ticket?

          Show
          Mark Nielsen added a comment - Is there a hold up on this ticket?
          Hide
          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
          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
          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
          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
          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
          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
          Iñaki Arenaza added a comment -

          Reopening for 2.0.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Reopening for 2.0. Saludos. Iñaki.
          Hide
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Thanks all - This has been integrated now.

          Cheers
          San

          Show
          Sam Hemelryk added a comment - Thanks all - This has been integrated now. Cheers San
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Iñaki Arenaza added a comment -

          PILL branches rebased

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - PILL branches rebased Saludos. Iñaki.
          Hide
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

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

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

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

          Closing, ciao

          Show
          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: