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

Malformed short names in custom fields cause multiple inconsistencies

    XMLWordPrintable

    Details

    • Affected Branches:
      MOODLE_29_STABLE, MOODLE_30_STABLE, MOODLE_31_STABLE

      Description

      As defined in the custom fields edit form, the field short name is case-sensitive and it is validated and cleaned as PARAM_ALPHANUM using the expression: preg_replace('/[^A-Za-z0-9]/i'
      So, are valid short names any of:
      foo
      Foo
      1foo

      This has several side effects:

      1) short names are saved in the user_info_field table verbatim, but are often compared in a case-insensitive way, e.g. grade_helper::get_user_profile_fields() fragment

              $customprofilefields = array_map('trim', explode(',', $CFG->grade_export_customprofilefields));
      		...
      			foreach ($customfields as $field) {
                      // Make sure we can display this custom field
                      if (!in_array($field->shortname, $customprofilefields)) {
      

      works properly only when char-cases matchs.

      2) as result, these produces inconsistent behaviours, as reported e.g. MDL-44982, MDL-38129, MDL-41145, etc... Moreover, changing a short name from upper- to lowercase, cause attribute mapping in db, LDAP and SSO authentication goes messy

      3) a minor annoyance: in many point, short names are used as object property names, e.g.: auth_plugin_base::get_custom_user_profile_fields()

      	$this->customfields[] = 'profile_field_'.$proffield->shortname;
      

      Even if in this case it works properly, it can lead to some difficulty if someone have to access a specific custom field directly (e.g. a plugin or https://moodle.org/mod/forum/discuss.php?d=91370) because PHP variable/property names cannot start with a number, thus
      $proffield->1foo
      is invalid.

      Proposed solution: add a new param type PARAM_FIELDNAME with this behaviour:

              case PARAM_FIELDNAME:
                  // Remove everything not `a-zA-Z0-9`.
                  $param =  preg_replace('/[^A-Za-z0-9]/i', '', $param);
      			return strtolower(preg_replace('/^[0-9]+/', '', $param));
      

      and use it in profile_define_base::define_form_common()
      Add a similar param type PARAM_FIELDNAME_LIST:

              case PARAM_FIELDNAME_LIST:
                  $params = array();
      			foreach (explode(',', $param) as $fieldname) {
      				$fieldname = preg_replace('/[^A-Za-z0-9]/i', '', $fieldname);
      				if ($fieldname) {
      					$params[] = strtolower($fieldname);
      				}
      			}
      			return implode(',', params);
      

      and use it everywhere field name list occours, such as in grades.php

      Impact: no impact on new defined custom fields, possibly configuration breaks when an existing custom field is edited

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              Unassigned
              Reporter:
              aulaweb AulaWeb Università di Genova
              Participants:
              Component watchers:
              Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias, Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias, Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Sara Arjona (@sarjona), Víctor Déniz Falcón
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated: