Moodle
  1. Moodle
  2. MDL-31762

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

    Details

    • Rank:
      38362

      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

        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: