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

timezone parameter in create_user web service function in /user/externallib.php throwing error

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1
    • Fix Version/s: 2.0.4
    • Component/s: Web Services
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      TESTCASE 1:
      Run testmoodlelib.php simpletest.

      Instructions to run testmoodlelib.php simpletest
      1. Navigate to unit test (Development -> unit test)
      2. enter "lib/simpletest/testmoodlelib.php" in textbox.
      3. click "Run test"

      TESTCASE 2:
      1. Enable Webservices
      2. Run unit test (Site adminstration -> Development -> unit tests) "webservice/simpletest/testwebservice.php".

      How to Enable webservices
      1. Enable web services (Site adminstration -> Advanced features)
      2. Enable protocols (Site adminstration -> Plugins -> Web Services -> Manage Protocols ). Enable Rest &/ XMLRPC
      3. Add External services ( Site adminstration -> Plugins -> Web Services -> External Services). Add function "moodle_user_create_users" to this external service
      4. Manage Tokens ( Site adminstration -> Plugins -> Web Services -> Manage Tokens). Add the "External Service" which we created in step 3 and copy the token value.
      5. Open webservice/simpletest/testwebservice.php and replace $this->testtoken value with above token value.
      6. set " $this->testxmlrpc" or "$this->testrest" to true
      7. set moodle_user_create_users to true.
      8. Now you are ready to run simple test

      Show
      TESTCASE 1: Run testmoodlelib.php simpletest. Instructions to run testmoodlelib.php simpletest 1. Navigate to unit test (Development -> unit test) 2. enter "lib/simpletest/testmoodlelib.php" in textbox. 3. click "Run test" TESTCASE 2: 1. Enable Webservices 2. Run unit test (Site adminstration -> Development -> unit tests) "webservice/simpletest/testwebservice.php". How to Enable webservices 1. Enable web services (Site adminstration -> Advanced features) 2. Enable protocols (Site adminstration -> Plugins -> Web Services -> Manage Protocols ). Enable Rest &/ XMLRPC 3. Add External services ( Site adminstration -> Plugins -> Web Services -> External Services). Add function "moodle_user_create_users" to this external service 4. Manage Tokens ( Site adminstration -> Plugins -> Web Services -> Manage Tokens). Add the "External Service" which we created in step 3 and copy the token value. 5. Open webservice/simpletest/testwebservice.php and replace $this->testtoken value with above token value. 6. set " $this->testxmlrpc" or "$this->testrest" to true 7. set moodle_user_create_users to true. 8. Now you are ready to run simple test
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull Master Branch:
      wip-mdl-27759-integration-master

      Description

      In create_user webservice function (/user/externallib.php), throw error for string timezones.
      This is happening because parameter check for timezone is PARAM_ALPHANUMEXT and it doesn't allow '/' (which string timezone format).

      Currently we don't have any PARAM defined for timzones, so probably we can define a new PARAM for timezone which should check for string to be either float or string with max one '/'

        Gliffy Diagrams

          Activity

          Hide
          salvetore Michael de Raadt added a comment -

          Could this also come in as a raw param and be processed by the script that uses it? That would depend on how often we need to parse a timezone parameter I suppose.

          Michael;

          Show
          salvetore Michael de Raadt added a comment - Could this also come in as a raw param and be processed by the script that uses it? That would depend on how often we need to parse a timezone parameter I suppose. Michael;
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Currently we are checking timezone input in webservice plugin only (We don't validate timezone in admin/edit profile).
          It can come as a PARAM_RAW but then it will not be validated anywhere and goes straight to database. I am not sure if that's acceptable.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Currently we are checking timezone input in webservice plugin only (We don't validate timezone in admin/edit profile). It can come as a PARAM_RAW but then it will not be validated anywhere and goes straight to database. I am not sure if that's acceptable.
          Hide
          andyjdavis Andrew Davis added a comment -

          Hi. In the comment just above the definition of PARAM_TIMEZONE just after "string seperated by '/' and can have '-' &/ '_'" add an example or two. Most people won't have any idea what a timezone string should look like so examples are always good.

          You should add a test_clean_param_timezone() function in lib/simpletest/testmoodlelib.php. Use the unit tests to define both what data is acceptable and what data is not acceptable.

          Show
          andyjdavis Andrew Davis added a comment - Hi. In the comment just above the definition of PARAM_TIMEZONE just after "string seperated by '/' and can have '-' &/ '_'" add an example or two. Most people won't have any idea what a timezone string should look like so examples are always good. You should add a test_clean_param_timezone() function in lib/simpletest/testmoodlelib.php. Use the unit tests to define both what data is acceptable and what data is not acceptable.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Added:
          1. example in comment for valid timezone.
          2. Testcase test_clean_param_timezone in lib/simpletest/testmoodlelib.php

          Show
          rajeshtaneja Rajesh Taneja added a comment - Added: 1. example in comment for valid timezone. 2. Testcase test_clean_param_timezone in lib/simpletest/testmoodlelib.php
          Hide
          andyjdavis Andrew Davis added a comment -

          Well done

          Show
          andyjdavis Andrew Davis added a comment - Well done
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Ho, I'm keeping this under review to get these fixed:

          1) right now it (master) does not merge clean. I guess other recent changes @ user/externallib.php are causing conflict.
          2) In unit tests it's better to avoid using the $message param in common asertXXX() methods. Only if some extra information needs to be revealed they should be used. Else it's better to keep standard output to happen.
          3) I think valid timezones like 'PST8PDT' are already supported by the regexp. I just would add one extra test for it.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Ho, I'm keeping this under review to get these fixed: 1) right now it (master) does not merge clean. I guess other recent changes @ user/externallib.php are causing conflict. 2) In unit tests it's better to avoid using the $message param in common asertXXX() methods. Only if some extra information needs to be revealed they should be used. Else it's better to keep standard output to happen. 3) I think valid timezones like 'PST8PDT' are already supported by the regexp. I just would add one extra test for it. Ciao
          Hide
          rajeshtaneja Rajesh Taneja added a comment - - edited

          Hello Eloy,

          1. I have created a new integration branch resolving conflict in user/externallib.php
          https://github.com/rajeshtaneja/moodle/blob/b4e29077d3c00f3b25ed90a75cf23c97a5823119/user/externallib.php#L606
          There was one conflict for PARAM_TIMEZONE.
          2. Removed the message param from assert
          3. Added testcase for PST8PDT.

          Show
          rajeshtaneja Rajesh Taneja added a comment - - edited Hello Eloy, 1. I have created a new integration branch resolving conflict in user/externallib.php https://github.com/rajeshtaneja/moodle/blob/b4e29077d3c00f3b25ed90a75cf23c97a5823119/user/externallib.php#L606 There was one conflict for PARAM_TIMEZONE. 2. Removed the message param from assert 3. Added testcase for PST8PDT.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Thanks!, re-reviewing it now...

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Thanks!, re-reviewing it now...
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I had to add some of the improvements in master back to 20_STABLE manually (asserTrue() calls, PST8PDT..) FYI, the proposed branch did not include them.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I had to add some of the improvements in master back to 20_STABLE manually (asserTrue() calls, PST8PDT..) FYI, the proposed branch did not include them.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Eloy
          Sorry, I forgot to update 20_STABLE with PST8PDT.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Eloy Sorry, I forgot to update 20_STABLE with PST8PDT.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks guys - passed.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks guys - passed.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Upstream, upstream, this is part of upstream, upstream... thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Upstream, upstream, this is part of upstream, upstream... thanks!

            People

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

              Dates

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