Moodle
  1. Moodle
  2. MDL-31762

Validation when creating users through webservices needs to be reviewed and fixed

    Details

      Description

      While reviewing a change Apu was making (MDL-25027) I noticed a couple of thigns that the webservice function that creates users was missing:

      1. Username must be lower case (reference: https://github.com/samhemelryk/moodle/blob/master/user/editadvanced_form.php#L172)
      2. Username should be using PARAM_USERNAME or at least cleaning with it and checking for differences (reference as above)
      3. user_create_user should be checking password policy etc (I think there may be an issue for this)

      Cheers
      Sam

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Aparup Banerjee added a comment -
            • linking issue that handling password policy
            • also linking issue that will hopefully centralise validation calls.
            Show
            Aparup Banerjee added a comment - linking issue that handling password policy also linking issue that will hopefully centralise validation calls.
            Hide
            Jason Ilicic added a comment -

            Has there been any further progress on this issue? A client has reported this in which I'd like to get hold of a fix if possible.

            Show
            Jason Ilicic added a comment - Has there been any further progress on this issue? A client has reported this in which I'd like to get hold of a fix if possible.
            Hide
            Sam Hemelryk added a comment -

            Hi Jason,
            Given that you've had a client actually make a report about this I am increasing the priority on the issue.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Jason, Given that you've had a client actually make a report about this I am increasing the priority on the issue. Cheers Sam
            Hide
            Jason Ilicic added a comment -

            Thanks Sam, looking forward to a fix!

            Show
            Jason Ilicic added a comment - Thanks Sam, looking forward to a fix!
            Hide
            Jérôme Mouneyrac added a comment -

            Sending for peer-review. Note that the comment about password police check is already in the current code.

            Show
            Jérôme Mouneyrac added a comment - Sending for peer-review. Note that the comment about password police check is already in the current code.
            Hide
            Rossiani Wijaya added a comment -

            This looks good Jerome.

            Show
            Rossiani Wijaya added a comment - This looks good Jerome.
            Hide
            Jérôme Mouneyrac added a comment -

            Thanks Rosie, sending it to integration.

            Show
            Jérôme Mouneyrac added a comment - Thanks Rosie, sending it to integration.
            Hide
            Jérôme Mouneyrac added a comment -

            I added 2.1 branch to see if there were any conflict, it works. I let you cherry-pick on 2.2 Thanks. Rebasing master.

            Show
            Jérôme Mouneyrac added a comment - I added 2.1 branch to see if there were any conflict, it works. I let you cherry-pick on 2.2 Thanks. Rebasing master.
            Hide
            Jérôme Mouneyrac added a comment -

            rebased.

            Show
            Jérôme Mouneyrac added a comment - rebased.
            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
            Sam Hemelryk added a comment -

            Thanks Jerome, this has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks Jerome, this has been integrated now
            Hide
            Rajesh Taneja added a comment -

            I am getting error while creating user with upper-case letter (which is fine). Just wondering if this error can be improved to define the problem. Shouldn't this be "Username policy is defined in Moodle security config. Must be lowercase." as mentioned in patch?

            I got following error while creating Testusername5 user by core_user_create_users, same error appeared with core_user_update_users

            <?xml version="1.0" encoding="UTF-8" ?>
            <EXCEPTION class="invalid_parameter_exception">
            <MESSAGE>Invalid parameter value detected</MESSAGE>
            <DEBUGINFO>users =&gt; Invalid parameter value detected: username =&gt; Invalid parameter value detected: Invalid external api parameter: the value is &quot;Testusername5&quot;, the server was expecting &quot;username&quot; type</DEBUGINFO>
            </EXCEPTION>
            

            Failing this for now, because of error message. Please feel free to pass this, if you think returned error is correct.

            Show
            Rajesh Taneja added a comment - I am getting error while creating user with upper-case letter (which is fine). Just wondering if this error can be improved to define the problem. Shouldn't this be "Username policy is defined in Moodle security config. Must be lowercase." as mentioned in patch? I got following error while creating Testusername5 user by core_user_create_users, same error appeared with core_user_update_users <?xml version="1.0" encoding="UTF-8" ?> <EXCEPTION class="invalid_parameter_exception"> <MESSAGE>Invalid parameter value detected</MESSAGE> <DEBUGINFO>users =&gt; Invalid parameter value detected: username =&gt; Invalid parameter value detected: Invalid external api parameter: the value is &quot;Testusername5&quot;, the server was expecting &quot;username&quot; type</DEBUGINFO> </EXCEPTION> Failing this for now, because of error message. Please feel free to pass this, if you think returned error is correct.
            Hide
            Jérôme Mouneyrac added a comment -

            Hi Raj,
            thanks for testing. It's the normal failling error by the valid_parameters() function. The lib functions are not the one throwing the error message, it's valid_parameters(). It's why it's confusing. You can pass it.

            PS: if you want to display an error message specific to the username then some similar checks to the lib functions need to be done just before valid_parameters(). But we rarely do that. I would suggest if you want it to be implemented, create an issue and if there are multiple votes we will add a specific error message. However we avoid to do that otherwise we would end up revalidate the entire description, it's what valid_parameters() does automatically.

            Cheers,
            Jerome

            Show
            Jérôme Mouneyrac added a comment - Hi Raj, thanks for testing. It's the normal failling error by the valid_parameters() function. The lib functions are not the one throwing the error message, it's valid_parameters(). It's why it's confusing. You can pass it. PS: if you want to display an error message specific to the username then some similar checks to the lib functions need to be done just before valid_parameters(). But we rarely do that. I would suggest if you want it to be implemented, create an issue and if there are multiple votes we will add a specific error message. However we avoid to do that otherwise we would end up revalidate the entire description, it's what valid_parameters() does automatically. Cheers, Jerome
            Hide
            Rajesh Taneja added a comment -

            Thanks Jerome,

            Will request Michael to re-open this for testing. Will check if someone has reported this earlier, if not then probably it's not important

            Show
            Rajesh Taneja added a comment - Thanks Jerome, Will request Michael to re-open this for testing. Will check if someone has reported this earlier, if not then probably it's not important
            Hide
            Rajesh Taneja added a comment -

            Thanks for explaining that, Jerome
            Error is visible and user is not created for username having uppercase letter/s

            Show
            Rajesh Taneja added a comment - Thanks for explaining that, Jerome Error is visible and user is not created for username having uppercase letter/s
            Hide
            Eloy Lafuente (stronk7) added a comment -

            UPDATE tracker_issues
               SET status = 'Closed',
                  comment = 'Thanks!'
            WHEN participants = 'Did a gorgeous work'
            

            This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            Show
            Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: