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

Upload users option only allow the following authentication options: Manual account, or No login

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Administration
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Enable LDAP Authentication and Shibboleth by visiting Site administration > Plugins > Authentication > Manage authentication.
      2. Upload test CSV user file (users-upload-test-auth.csv) via the page Site administration > Users > Accounts > Upload users > Choose file and upload.
      3. In the Upload users preview page scroll down to the Default values section and select the drop-down menu in 'Choose an authentication method' and ensure you see 'LDAP server' in the following options but not 'Shibboleth'.
      4. Upload the file and ensure it works as expected.

      For master only

      1. Check that there is a nice explanation in auth/upgrade.txt documenting the new function.
      Show
      Enable LDAP Authentication and Shibboleth by visiting Site administration > Plugins > Authentication > Manage authentication. Upload test CSV user file (users-upload-test-auth.csv) via the page Site administration > Users > Accounts > Upload users > Choose file and upload. In the Upload users preview page scroll down to the Default values section and select the drop-down menu in 'Choose an authentication method' and ensure you see 'LDAP server' in the following options but not 'Shibboleth'. Upload the file and ensure it works as expected. For master only Check that there is a nice explanation in auth/upgrade.txt documenting the new function.
    • Workaround:
      Hide

      Specify the authentication plugin in the CSV file by adding an 'auth' column.

      Show
      Specify the authentication plugin in the CSV file by adding an 'auth' column.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-27953_master
    • Sprint:
      BACKEND Sprint 3
    • Story Points (Obsolete):
      8
    • Sprint:
      BACKEND Sprint 3

      Description

      When uploading a CSV file of users in 2.0.3 the "Choose an authentication" drop-down only gives the following options:

      Manual account
      No login

      In 1.9.x I get the following authentication options:

      Manual account
      No login
      LDAP server
      Email-based self-registration

        Gliffy Diagrams

        1. users-upload-test-auth.csv
          0.2 kB
          Mark Nelson
        2. USERUPLOAD-AUTH.csv
          0.3 kB
          John Paul Posada
        3. USERUPLOAD-NO-AUTH.csv
          0.3 kB
          John Paul Posada

          Issue Links

            Activity

            Hide
            aborrow Anthony Borrow added a comment -

            John Paul - Thanks for filing this issue. I would expect LDAP to be listed if it is enabled. Would you mind updating the testing instructions so that it is more step-by-step for the developer. Perhaps you could provide a sample csv file that could be used. In creating a new user, currently all of the authentication plugins are listed. I'm not sure if that is really helpful if they are not enabled. Might it be better if it only showed the available plugins. I am wanting to ensure that the behavior is consistent whether we are creating a new user from the UI, uploading new users via CSV, etc. Thanks for helping to think through exactly what we desire to happen in these scenarios. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - John Paul - Thanks for filing this issue. I would expect LDAP to be listed if it is enabled. Would you mind updating the testing instructions so that it is more step-by-step for the developer. Perhaps you could provide a sample csv file that could be used. In creating a new user, currently all of the authentication plugins are listed. I'm not sure if that is really helpful if they are not enabled. Might it be better if it only showed the available plugins. I am wanting to ensure that the behavior is consistent whether we are creating a new user from the UI, uploading new users via CSV, etc. Thanks for helping to think through exactly what we desire to happen in these scenarios. Peace - Anthony
            Hide
            johnpaultrack John Paul Posada added a comment -

            Hi Anthony,

            I agree that the behaviour should be consistant. I've updated the instructions as requested also.

            Cheers,
            John Paul

            Show
            johnpaultrack John Paul Posada added a comment - Hi Anthony, I agree that the behaviour should be consistant. I've updated the instructions as requested also. Cheers, John Paul
            Hide
            aborrow Anthony Borrow added a comment -

            Thanks for the updated instructions - those are quite helpful. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Thanks for the updated instructions - those are quite helpful. Peace - Anthony
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this and providing additional details.

            I've put it on our backlog and we'll try to get to it as soon as we can.

            In the meantime adding if you have a chance to work on a code solution, sharing that will help us and other users.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this and providing additional details. I've put it on our backlog and we'll try to get to it as soon as we can. In the meantime adding if you have a chance to work on a code solution, sharing that will help us and other users.
            Hide
            snydert Tamara Snyder added a comment -

            Hi - can you please confirm if this workaround works in 2.1? I don't have any dropdown to select authentication mode, I am using auth column in my upload users csv file with value set to cas which is what we are using. This worked fine in 2.0 but is not working with 2.1. Did test using same exact file to confirm and what succeeds in 2.0 gives error message in 2.1: "auth plugin not supported here." Need to upload several hundred users before classes start in few days. Any help greatly appreciated.

            Thanks,
            Tammy

            Show
            snydert Tamara Snyder added a comment - Hi - can you please confirm if this workaround works in 2.1? I don't have any dropdown to select authentication mode, I am using auth column in my upload users csv file with value set to cas which is what we are using. This worked fine in 2.0 but is not working with 2.1. Did test using same exact file to confirm and what succeeds in 2.0 gives error message in 2.1: "auth plugin not supported here." Need to upload several hundred users before classes start in few days. Any help greatly appreciated. Thanks, Tammy
            Hide
            snydert Tamara Snyder added a comment -

            Apologies - the workaround is working in 2.1. I did not check after receiving the error that user was properly created. They are in fact created despite the error. We just did not get the error in 2.0.

            Show
            snydert Tamara Snyder added a comment - Apologies - the workaround is working in 2.1. I did not check after receiving the error that user was properly created. They are in fact created despite the error. We just did not get the error in 2.0.
            Hide
            antonvaltaz Daniel Walters added a comment -

            The workaround does not work for me in 2.0.3+ (I am updating existing users, not creating new ones).

            I get the error:

            manual
            manual-->ldap
            Auth plugin not supported here

            But when I go to check on individual existing users, they are still listed as using manual authentication.

            Show
            antonvaltaz Daniel Walters added a comment - The workaround does not work for me in 2.0.3+ (I am updating existing users, not creating new ones). I get the error: manual manual-->ldap Auth plugin not supported here But when I go to check on individual existing users, they are still listed as using manual authentication.
            Hide
            hagelk Kris Hagel added a comment -

            This is still broken in Moodle 2.2.3+ (Build: 20120615). Also, the work around suggested does not work.

            Show
            hagelk Kris Hagel added a comment - This is still broken in Moodle 2.2.3+ (Build: 20120615). Also, the work around suggested does not work.
            Hide
            markn Mark Nelson added a comment -

            There is a function 'uu_supported_auths' that returns the authentication options you can choose when uploading users, however this has been hard coded to use 'manual', 'nologin', 'none' and 'email'. The PHPDocs for the function specify "If ppl want to use some other auth type they have to include it in the CSV file next on each line.". I need to discuss whether this should be changed, and if so, how are we going to implement it. Maybe by default the authentication method can not be selected unless it specifies that it can ..

            Show
            markn Mark Nelson added a comment - There is a function 'uu_supported_auths' that returns the authentication options you can choose when uploading users, however this has been hard coded to use 'manual', 'nologin', 'none' and 'email'. The PHPDocs for the function specify "If ppl want to use some other auth type they have to include it in the CSV file next on each line.". I need to discuss whether this should be changed, and if so, how are we going to implement it. Maybe by default the authentication method can not be selected unless it specifies that it can ..
            Hide
            markn Mark Nelson added a comment -

            I am changing this from a bug to an improvement and lowering the priority. The code is working as expected, although it can be improved on.

            Show
            markn Mark Nelson added a comment - I am changing this from a bug to an improvement and lowering the priority. The code is working as expected, although it can be improved on.
            Hide
            markn Mark Nelson added a comment -

            Ok, so 1.9 any plugin that was enabled was added to the drop down list, there was no restriction. In 2.0 there was an array created that had a hard coded list of authentication plugins that can be used, as stated above. The master diff I created introduces a new function that authentication plugins can override if they wish to be added to the list, the 2.3 and 2.4 fix is simply to add ldap to the array of allowed plugins.

            Show
            markn Mark Nelson added a comment - Ok, so 1.9 any plugin that was enabled was added to the drop down list, there was no restriction. In 2.0 there was an array created that had a hard coded list of authentication plugins that can be used, as stated above. The master diff I created introduces a new function that authentication plugins can override if they wish to be added to the list, the 2.3 and 2.4 fix is simply to add ldap to the array of allowed plugins.
            Hide
            abgreeve Adrian Greeve added a comment -

            [Y] Syntax
            [Y] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [*] Testing
            [-] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Hello Mark,

            I like the solution that you have given here. I can't spot anything wrong with it.

            Just one small thing. Perhaps you could change the name of the file that you are supposed to download and use to USERUPLOAD-NO-AUTH.csv. The other file has the 'auth' field filled out and so the option that you mention in the testing instructions does not get displayed.

            Thanks.

            Show
            abgreeve Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [*] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hello Mark, I like the solution that you have given here. I can't spot anything wrong with it. Just one small thing. Perhaps you could change the name of the file that you are supposed to download and use to USERUPLOAD-NO-AUTH.csv. The other file has the 'auth' field filled out and so the option that you mention in the testing instructions does not get displayed. Thanks.
            Hide
            markn Mark Nelson added a comment -

            As discussed, Adrian was looking at the workaround instructions, not my testing instructions. The file to use is users-upload-test-auth.csv as specified. Submitting to integration.

            Show
            markn Mark Nelson added a comment - As discussed, Adrian was looking at the workaround instructions, not my testing instructions. The file to use is users-upload-test-auth.csv as specified. Submitting to integration.
            Hide
            damyon Damyon Wiese 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.

            Thanks!

            Show
            damyon Damyon Wiese 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. Thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Mark,

            Looks fine, but we've not had Petr involved in this issue? Please involve him as component maintainer. Add him as a watcher at the least.

            Show
            poltawski Dan Poltawski added a comment - Hi Mark, Looks fine, but we've not had Petr involved in this issue? Please involve him as component maintainer. Add him as a watcher at the least.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            salvetore Michael de Raadt added a comment -

            Carrying into the next sprint so it can be completed.

            Show
            salvetore Michael de Raadt added a comment - Carrying into the next sprint so it can be completed.
            Hide
            markn Mark Nelson added a comment -

            Hi Petr, as the maintainer of the enrolment plugins, are you able to view these changes and let me know if they are acceptable or not? Thanks!

            Show
            markn Mark Nelson added a comment - Hi Petr, as the maintainer of the enrolment plugins, are you able to view these changes and let me know if they are acceptable or not? Thanks!
            Hide
            markn Mark Nelson added a comment -

            Hi Petr, I will be on holiday after today until April, so if you do view this and it seems fine can you push to integrations? Thanks.

            Show
            markn Mark Nelson added a comment - Hi Petr, I will be on holiday after today until April, so if you do view this and it seems fine can you push to integrations? Thanks.
            Hide
            skodak Petr Skoda added a comment -

            Hello,

            1/ please use get_auth_plugin() instead of manually creating instance
            2/ I like the new methods.
            3/ I do not understand why should admins upload ldap users, instead just execute ldap sync and done. It would be different if there was option to prevent adding of new accounts in ldap sync, but it is not there. my -2

            Show
            skodak Petr Skoda added a comment - Hello, 1/ please use get_auth_plugin() instead of manually creating instance 2/ I like the new methods. 3/ I do not understand why should admins upload ldap users, instead just execute ldap sync and done. It would be different if there was option to prevent adding of new accounts in ldap sync, but it is not there. my -2
            Hide
            markn Mark Nelson added a comment -

            Thanks Petr,

            1) Thanks, wasn't aware of that function. I agree, using this is better. Changed.
            3) I am not too sure myself as I have not had much experiencing using LDAP and Moodle. Maybe someone in the community can explain why ldap sync can not be used?

            Show
            markn Mark Nelson added a comment - Thanks Petr, 1) Thanks, wasn't aware of that function. I agree, using this is better. Changed. 3) I am not too sure myself as I have not had much experiencing using LDAP and Moodle. Maybe someone in the community can explain why ldap sync can not be used?
            Hide
            moodle.com moodle.com added a comment -

            Mark is away on holidays and will be back in a few weeks.

            I've carried this over to the next sprint so it can be completed when he returns.

            Show
            moodle.com moodle.com added a comment - Mark is away on holidays and will be back in a few weeks. I've carried this over to the next sprint so it can be completed when he returns.
            Hide
            markn Mark Nelson added a comment -

            The code is fine as I have addressed Petr's small point about using get_auth_plugin. Now it is simply a question as to why we should do this. As Petr stated in his third point, "why should admins upload ldap users, instead just execute ldap sync and done." I do not have enough knowledge on using LDAP and Moodle so cannot answer this. We are waiting on someone in the community to answer Petr's third point to provide a reason why this should be integrated.

            Show
            markn Mark Nelson added a comment - The code is fine as I have addressed Petr's small point about using get_auth_plugin. Now it is simply a question as to why we should do this. As Petr stated in his third point, "why should admins upload ldap users, instead just execute ldap sync and done." I do not have enough knowledge on using LDAP and Moodle so cannot answer this. We are waiting on someone in the community to answer Petr's third point to provide a reason why this should be integrated.
            Hide
            hagenjam James Hagen added a comment -

            I've been tracking this issue since discovering that when I uploaded details of users (i.e. existing LDAP-authenticated users) via CSV file in order to populate an existing cohort (see "Uploading users to a cohort" at http://docs.moodle.org/22/en/Cohorts), the users' records changed by default from LDAP authenticated to manual authenticated. This is unavoidable (we're using 2.4) because LDAP is not in the array of allowed plugins, as noted above. To answer the question at least in part, the ability to upload (ldap) users via csv file to populate cohorts definitely has benefits and "just executing an ldap sync" is not an alternative - for us.

            Show
            hagenjam James Hagen added a comment - I've been tracking this issue since discovering that when I uploaded details of users (i.e. existing LDAP-authenticated users) via CSV file in order to populate an existing cohort (see "Uploading users to a cohort" at http://docs.moodle.org/22/en/Cohorts ), the users' records changed by default from LDAP authenticated to manual authenticated. This is unavoidable (we're using 2.4) because LDAP is not in the array of allowed plugins, as noted above. To answer the question at least in part, the ability to upload (ldap) users via csv file to populate cohorts definitely has benefits and "just executing an ldap sync" is not an alternative - for us.
            Hide
            markn Mark Nelson added a comment -

            Thanks for your input James. Petr, what is your opinion on James's situation? I am going to take this off the current sprint and set it to backlog until a solution is finalised.

            Show
            markn Mark Nelson added a comment - Thanks for your input James. Petr, what is your opinion on James's situation? I am going to take this off the current sprint and set it to backlog until a solution is finalised.
            Hide
            markn Mark Nelson added a comment -

            Hey Petr Skoda, I have been going through my old issues and found this. Can you please review James's last comment abt why this would be useful and let me know if you think this should be integrated (obviously after a rebase since it was done so long ago).

            Show
            markn Mark Nelson added a comment - Hey Petr Skoda , I have been going through my old issues and found this. Can you please review James's last comment abt why this would be useful and let me know if you think this should be integrated (obviously after a rebase since it was done so long ago).
            Hide
            markn Mark Nelson added a comment -

            I spoke to Petr today and he thinks that the concept is fine. I will be pushing to integration shortly once I have rebased and considered a new function name other than "can_be_manually_assigned", as we use the term "assign" when talking about roles so it may be confusing. This will go into 2.4, 2.5 and master.

            Show
            markn Mark Nelson added a comment - I spoke to Petr today and he thinks that the concept is fine. I will be pushing to integration shortly once I have rebased and considered a new function name other than "can_be_manually_assigned", as we use the term "assign" when talking about roles so it may be confusing. This will go into 2.4, 2.5 and master.
            Hide
            markn Mark Nelson added a comment -

            I changed the function name to can_be_manually_set. Submitting to integration.

            Show
            markn Mark Nelson added a comment - I changed the function name to can_be_manually_set. Submitting to integration.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Mark this has been integrated now.

            Great to see the discussion on the requirement for these changes, that was the first thing I thought myself reading thought the description.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Mark this has been integrated now. Great to see the discussion on the requirement for these changes, that was the first thing I thought myself reading thought the description.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            tested on master, 2.5 and 2.4, all works.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited tested on master, 2.5 and 2.4, all works.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better!

            "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar."
            ~ Professor Farnsworth

            Show
            samhemelryk Sam Hemelryk added a comment - Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better! "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar." ~ Professor Farnsworth
            Hide
            cttxg Teresa Gibbison added a comment - - edited

            I found this issue while reviewing our core code hacks. We use an institution specific authentication method so have to manually add it to the uu_supported_auths $whitelist array. I understand the thought of hardcoding these as a "guarantee" they will work but it occurred to me that as an Administrator using an authentication method of our choice which obviously works properly (or we wouldn't be able to use it!) I should be able to configure whitelist (rather than just hacking the code).
            What are other's thoughts on allowing admins to configure their uu_supported_auths?

            --edit: sorry, looking at the master diff (rather than the 2.5 diff) this looks like it's going to be configurable - thanks

            Show
            cttxg Teresa Gibbison added a comment - - edited I found this issue while reviewing our core code hacks. We use an institution specific authentication method so have to manually add it to the uu_supported_auths $whitelist array. I understand the thought of hardcoding these as a "guarantee" they will work but it occurred to me that as an Administrator using an authentication method of our choice which obviously works properly (or we wouldn't be able to use it!) I should be able to configure whitelist (rather than just hacking the code). What are other's thoughts on allowing admins to configure their uu_supported_auths? --edit: sorry, looking at the master diff (rather than the 2.5 diff) this looks like it's going to be configurable - thanks
            Hide
            markn Mark Nelson added a comment -

            Hi Teresa,

            We don't tend to introduce new functions into existing APIs in stable branches, which is why the diffs differ dramatically (alliteration ftw).

            Regards,

            Mark

            Show
            markn Mark Nelson added a comment - Hi Teresa, We don't tend to introduce new functions into existing APIs in stable branches, which is why the diffs differ dramatically (alliteration ftw). Regards, Mark
            Hide
            timhunt Tim Hunt added a comment -

            Good news! the feature I need has already been implemented.

            Bad news it is 2.6 only.

            So, the fact the upgrade.txt file was committed to 2.5.2 is just confusing, since can_be_manually_set does not actually exist there.

            It is also a bit confusing that the can_be_manually_set API does not seem to affect the standard create users form, just the bulk upload one.

            Show
            timhunt Tim Hunt added a comment - Good news! the feature I need has already been implemented. Bad news it is 2.6 only. So, the fact the upgrade.txt file was committed to 2.5.2 is just confusing, since can_be_manually_set does not actually exist there. It is also a bit confusing that the can_be_manually_set API does not seem to affect the standard create users form, just the bulk upload one.
            Hide
            markn Mark Nelson added a comment -

            Wow Tim, you are right. The documentation should not have been pushed into 2.4 or 2.5! I will create a tracker issue to remove these. In the testing instructions it does say 'for master only' so I assume it was mistakenly integrated into those branches.

            Show
            markn Mark Nelson added a comment - Wow Tim, you are right. The documentation should not have been pushed into 2.4 or 2.5! I will create a tracker issue to remove these. In the testing instructions it does say 'for master only' so I assume it was mistakenly integrated into those branches.
            Hide
            markn Mark Nelson added a comment -

            The commit I created describing the change in the documentation has been altered for 2.5 and 2.4 to display those version numbers. I only ever created one for master. Seems like there was some confusion when it was integrated where it as assumed the new API was applied to all branches.

            Show
            markn Mark Nelson added a comment - The commit I created describing the change in the documentation has been altered for 2.5 and 2.4 to display those version numbers. I only ever created one for master. Seems like there was some confusion when it was integrated where it as assumed the new API was applied to all branches.
            Hide
            markn Mark Nelson added a comment -

            I have fixed this in MDL-42421 - thanks for spotting this Tim.

            Show
            markn Mark Nelson added a comment - I have fixed this in MDL-42421 - thanks for spotting this Tim.

              People

              • Votes:
                5 Vote for this issue
                Watchers:
                16 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13

                  Agile