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

core_user_get_users_by_id - ERRORCODE: invalidresponse

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.6, 2.3.3, 2.4
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Web Services
    • Labels:
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      phpunit moodlelib_testcase lib/tests/moodlelib_test.php
      If no issue caused by moodlelib_testcase::test_clean_param_timezone, then it works fine.

      Show
      phpunit moodlelib_testcase lib/tests/moodlelib_test.php If no issue caused by moodlelib_testcase::test_clean_param_timezone, then it works fine.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36773-master

      Description

      When making web service calls to core_user_get_users_by_id, we're noticing that some users with timezones of numeric values (such as 10.0) are returning errors. We upgraded from Moodle 2.0.x and those were working fine before using the old moodle_user_get_users_by_id call. The difference seems to be the PARAM checking type, where before it was ALPHANUMEXT, now it is TIMEZONE.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              nmares Nathan Mares added a comment - - edited

              get_list_of_timezones() which is used to populate the timezone list for the user profile edit page appears to return invalid timezones which would be causing invalid timezones to be saved in user records.

              The comments around PARAM_TIMZONE say:

              PARAM_TIMEZONE - expected timezone. Timezone can be int +(0-13) or float +(0.5-12.5) or string seperated by '/' and can have '-' &/ '_' (eg. America/North_Dakota/New_Salem, America/Port-au-Prince)

              The bit I should highlight is the "int +-(0-13)"

              Currently get_list_of_timezones() returns some values that are invalid according to the regex for PARAM_TIMEZONE in clean_param.

              I knocked up a quick bit of code to test that the values returned are valid:

              <?php
               
              define('MOODLE_INTERNAL', 1);                                           
                                                                                            
              require_once('config.php');                                                
                                                           
              $timezones = get_list_of_timezones();    
                                                                                 
              foreach ($timezones as $timezone=>$label) {                             
                if (clean_param($timezone, PARAM_TIMEZONE) != $timezone) {           
                  echo "Invalid timezone $timezone<br>";                       
                }                                                              
              }

              I've included output below showing which ones didn't pass though the regex test:

              Invalid timezone -13.0
              Invalid timezone -12.0
              Invalid timezone -11.0
              Invalid timezone -10.0
              Invalid timezone 10.0
              Invalid timezone 11.0
              Invalid timezone 12.0
              Invalid timezone 13.0

              The regex test is for PARAM_TIMEZONE is this:

              /^(([+-]?(0?[0-9](\.[5|0])?|1[0-3]|1[0-2]\.5))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

              I see two possible solutions:

              1) Change get_list_of_timezones() to return valid timezones only (without trailing 0's) and optionally fix up broken data in people's mdl_user table; or
              2) Change that regex to allow the timezones returned by get_list_of_timezones(), e.g. /^(([+-]?(0?[0-9](\.[5|0])?|1[0-3]|1[0-2]\.[5|0]))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

              Show
              nmares Nathan Mares added a comment - - edited get_list_of_timezones() which is used to populate the timezone list for the user profile edit page appears to return invalid timezones which would be causing invalid timezones to be saved in user records. The comments around PARAM_TIMZONE say: PARAM_TIMEZONE - expected timezone. Timezone can be int + (0-13) or float + (0.5-12.5) or string seperated by '/' and can have '-' &/ '_' (eg. America/North_Dakota/New_Salem, America/Port-au-Prince) The bit I should highlight is the "int +-(0-13)" Currently get_list_of_timezones() returns some values that are invalid according to the regex for PARAM_TIMEZONE in clean_param. I knocked up a quick bit of code to test that the values returned are valid: <?php   define('MOODLE_INTERNAL', 1); require_once('config.php'); $timezones = get_list_of_timezones(); foreach ($timezones as $timezone=>$label) { if (clean_param($timezone, PARAM_TIMEZONE) != $timezone) { echo "Invalid timezone $timezone<br>"; } } I've included output below showing which ones didn't pass though the regex test: Invalid timezone -13.0 Invalid timezone -12.0 Invalid timezone -11.0 Invalid timezone -10.0 Invalid timezone 10.0 Invalid timezone 11.0 Invalid timezone 12.0 Invalid timezone 13.0 The regex test is for PARAM_TIMEZONE is this: /^(( [+-] ?(0? [0-9] (\. [5|0] )?|1 [0-3] |1 [0-2] \.5))|(99)|[ [:alnum:] ] (\/?[ [:alpha:] _-]) )$/ I see two possible solutions: 1) Change get_list_of_timezones() to return valid timezones only (without trailing 0's) and optionally fix up broken data in people's mdl_user table; or 2) Change that regex to allow the timezones returned by get_list_of_timezones(), e.g. /^(( [+-] ?(0? [0-9] (\. [5|0] )?|1 [0-3] |1 [0-2] \. [5|0] ))|(99)|[ [:alnum:] ] (\/?[ [:alpha:] _-]) )$/
              Hide
              nmares Nathan Mares added a comment -

              Actually that regex in option 2 was bad and would have failed for timezone "13.0".

              I've attached a patch which has a better (working) one and updated the unit tests in lib/tests/moodlelib_tests.php

              Show
              nmares Nathan Mares added a comment - Actually that regex in option 2 was bad and would have failed for timezone "13.0". I've attached a patch which has a better (working) one and updated the unit tests in lib/tests/moodlelib_tests.php
              Hide
              jerome Jérôme Mouneyrac added a comment - - edited

              Thanks Nathan, I think too that the regex is missing .0 check. Thanks for the fix, and for the unit test update
              Attach a git commit next time, it make it easy to keep record of the commit author.

              Show
              jerome Jérôme Mouneyrac added a comment - - edited Thanks Nathan, I think too that the regex is missing .0 check. Thanks for the fix, and for the unit test update Attach a git commit next time, it make it easy to keep record of the commit author.
              Hide
              nmares Nathan Mares added a comment -

              Hi Jerome - sorry, thought I had attached a commit. Will check next time.

              Just looking at you're diff now. Looks like you used my first regex. Its bad because it doesn't validate "-13.0" or "13.0" as timezones which are values returned from get_list_of_timezones().

              The one below (from the attached patch) is better because it allows an optional ".0" after any + or - integer-only value but doesn't permit values of "13.5" or "-13.5" which shouldn't pass.

              /^(([+-]?(0?[0-9](\.[5|0])?|1[0-3](\.0)?|1[0-2]\.5))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

              Show
              nmares Nathan Mares added a comment - Hi Jerome - sorry, thought I had attached a commit. Will check next time. Just looking at you're diff now. Looks like you used my first regex. Its bad because it doesn't validate "-13.0" or "13.0" as timezones which are values returned from get_list_of_timezones(). The one below (from the attached patch) is better because it allows an optional ".0" after any + or - integer-only value but doesn't permit values of "13.5" or "-13.5" which shouldn't pass. /^(( [+-] ?(0? [0-9] (\. [5|0] )?|1 [0-3] (\.0)?|1 [0-2] \.5))|(99)|[ [:alnum:] ] (\/?[ [:alpha:] _-]) )$/
              Hide
              jerome Jérôme Mouneyrac added a comment - - edited

              Ah thanks Nathan. I saw that and got influenced by phpdoc in the way, and forget the main reason I thought supporting all .0 is good. I think we should support all YX.0 in order to be consistent as we support X.0 (in case peerreviewer/integrator wonder why).
              Changing the regex.

              Show
              jerome Jérôme Mouneyrac added a comment - - edited Ah thanks Nathan. I saw that and got influenced by phpdoc in the way, and forget the main reason I thought supporting all .0 is good. I think we should support all YX.0 in order to be consistent as we support X.0 (in case peerreviewer/integrator wonder why). Changing the regex.
              Hide
              jerome Jérôme Mouneyrac added a comment -

              Sending to peer-review.

              Show
              jerome Jérôme Mouneyrac added a comment - Sending to peer-review.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Jerome,
              Patch looks perfect. Pushing for integration review.
              [y] Syntax
              [y] Output
              [y] Whitespace
              [-] Language
              [-] Databases
              [y] Testing
              [-] Security
              [-] Documentation
              [y] Git
              [y] Sanity check

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Jerome, Patch looks perfect. Pushing for integration review. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Jerome, integrated to 22, 23, 24 and master.

              Show
              poltawski Dan Poltawski added a comment - Thanks Jerome, integrated to 22, 23, 24 and master.
              Hide
              poltawski Dan Poltawski added a comment -

              Working fine

              Show
              poltawski Dan Poltawski added a comment - Working fine
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

              Closing, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/Jan/13