Moodle
  1. Moodle
  2. MDL-32787

Server-side code does not check whether the required custom profile filed is filled or not

    Details

    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Add a custom (text input) user profile field (settings -> Site administration -> Users -> User profile fields).
      3. Set "Is this field required?" to yes
      4. Go to edit profile (settings -> My profile settings -> Edit profile)
      5. Make sure "custom text input" field is required field and you can't save form with empty value.
      Show
      Log in as admin Add a custom (text input) user profile field (settings -> Site administration -> Users -> User profile fields). Set "Is this field required?" to yes Go to edit profile (settings -> My profile settings -> Edit profile) Make sure "custom text input" field is required field and you can't save form with empty value.
    • Workaround:
      Hide

      Add these lines to function profile_field_base::edit_validate_field before return $errors to perform check

      if ($this->is_required() && strlen($usernew->{$this->inputname}) == 0) {
        $errors[$this->inputname] = get_string('fieldrequired', 'error', $this->field->name);
      }

      Remove check for moodle/user:update capability in function profile_field_base::edit_field_set_required to show admin user requirement of the field

      // if ($this->is_required() and !has_capability('moodle/user:update', get_context_instance(CONTEXT_SYSTEM))) {
      if ($this->is_required()) {
        $mform->addRule($this->inputname, get_string('required'), 'required', null, 'client');
      }

      Show
      Add these lines to function profile_field_base::edit_validate_field before return $errors to perform check if ($this->is_required() && strlen($usernew->{$this->inputname}) == 0) { $errors[$this->inputname] = get_string('fieldrequired', 'error', $this->field->name); } Remove check for moodle/user:update capability in function profile_field_base::edit_field_set_required to show admin user requirement of the field // if ($this->is_required() and !has_capability('moodle/user:update', get_context_instance(CONTEXT_SYSTEM))) { if ($this->is_required()) { $mform->addRule($this->inputname, get_string('required'), 'required', null, 'client'); }
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-32787

      Description

      There is only check for uniqueness of data in function profile_field_base::edit_validate_field

      Also required custom profile field is marked as required only for users, that has no moodle/user:update capability in system context. For example these fields are not required for admin.

      If the required field is empty the program must point the user at the mistake.

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          I'm not sure why the capability check allows administrators to not have to add a value for the field if it is normally required. I might ask the developer who added this capability check why this was the case.

          If we can consistently ensure that the field is required, then we should not have to manually check that the field is given; the mform validation should check for this automatically.

          Show
          Michael de Raadt added a comment - I'm not sure why the capability check allows administrators to not have to add a value for the field if it is normally required. I might ask the developer who added this capability check why this was the case. If we can consistently ensure that the field is required, then we should not have to manually check that the field is given; the mform validation should check for this automatically.
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for the patch Andrey.
          Code looks good. Permission check was added as part of MDL-8096.

          One thing you might want to consider.

          1. As rule is being added by edit_field_set_required, we don't need another check in edit_validate_field. So it might be nice to remove it.

          FYI:
          Have added Petr, to make sure I am not missing anything.

          Show
          Rajesh Taneja added a comment - - edited Thanks for the patch Andrey. Code looks good. Permission check was added as part of MDL-8096 . One thing you might want to consider. As rule is being added by edit_field_set_required, we don't need another check in edit_validate_field. So it might be nice to remove it. FYI: Have added Petr, to make sure I am not missing anything.
          Hide
          Rajesh Taneja added a comment -

          I have updated your branch and pushing it for review.

          Show
          Rajesh Taneja added a comment - I have updated your branch and pushing it for review.
          Hide
          Andrey F. Mindubaev added a comment -

          we don't need another check in edit_validate_field

          You are right! Required field cannot have empty value, even without that check.

          Show
          Andrey F. Mindubaev added a comment - we don't need another check in edit_validate_field You are right! Required field cannot have empty value, even without that check.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrey,

          Will wait for Petr's response, to make sure there was no specific reason for adding capability check.

          Show
          Rajesh Taneja added a comment - Thanks Andrey, Will wait for Petr's response, to make sure there was no specific reason for adding capability check.
          Hide
          Petr Skoda added a comment -

          The has capability must be there, because when you create user as admin you do not know what should be there in many cases, you really want the users to fill in the blanks! Looking at the code it should be probably (!has_capability() and $otheruser), not sure though how to get the information that we are going to edit other user...

          Show
          Petr Skoda added a comment - The has capability must be there, because when you create user as admin you do not know what should be there in many cases, you really want the users to fill in the blanks! Looking at the code it should be probably (!has_capability() and $otheruser), not sure though how to get the information that we are going to edit other user...
          Hide
          Andrey F. Mindubaev added a comment -

          Petr, if we let admin user and users with Manager role leave custom fields empty, we should let them leave fields like Username, City/town and Country empty too. But we won't do this because required field is obligatory and it cannot be blank!

          I think, than the whole application and the event handlers user_created and user_updated in particular might expect non-empty values username, city, country and all custom required fields in $user object.

          I suggest that we should configure the behaviour of the application via get_config("admins_and_managers_can_leave_fields_blank") or something.

          Show
          Andrey F. Mindubaev added a comment - Petr, if we let admin user and users with Manager role leave custom fields empty, we should let them leave fields like Username , City/town and Country empty too. But we won't do this because required field is obligatory and it cannot be blank! I think, than the whole application and the event handlers user_created and user_updated in particular might expect non-empty values username , city , country and all custom required fields in $user object. I suggest that we should configure the behaviour of the application via get_config("admins_and_managers_can_leave_fields_blank") or something.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the input Petr,

          Just wondering, if we need to check update capability. Shouldn't we just check if it's otheruser then don't add rule.

          I have updated the branch. It will be very helpful, if you can please provide some more inputs.

          Show
          Rajesh Taneja added a comment - Thanks for the input Petr, Just wondering, if we need to check update capability. Shouldn't we just check if it's otheruser then don't add rule. I have updated the branch. It will be very helpful, if you can please provide some more inputs.
          Hide
          Petr Skoda added a comment -

          1/ did you try this in 2.3dev, it looks like you would get tons of E_STRICT problems
          2/ I do not understand "if (is_array($this->_customdata)", you control everything in these user edit forms, why not make sure the expected customdata is always passed there

          Show
          Petr Skoda added a comment - 1/ did you try this in 2.3dev, it looks like you would get tons of E_STRICT problems 2/ I do not understand "if (is_array($this->_customdata)", you control everything in these user edit forms, why not make sure the expected customdata is always passed there
          Hide
          Rajesh Taneja added a comment -

          Thanks again for the feedback Petr.

          1. I didn't get any E_STRICT problem. I created this patch on 2.3 dev
          2. I used "If(is_array" because editor options are set in same way (in same function) https://github.com/rajeshtaneja/moodle/blob/35e4c0f969dcd46224e4e68d39818a35f0a47f1d/user/editadvanced_form.php#L15

          FYI:
          IMO it should not be back-ported, as there is an API change.

          Show
          Rajesh Taneja added a comment - Thanks again for the feedback Petr. I didn't get any E_STRICT problem. I created this patch on 2.3 dev I used "If(is_array" because editor options are set in same way (in same function) https://github.com/rajeshtaneja/moodle/blob/35e4c0f969dcd46224e4e68d39818a35f0a47f1d/user/editadvanced_form.php#L15 FYI: IMO it should not be back-ported, as there is an API change.
          Hide
          Frédéric Massart added a comment -

          As discussed with you Raj, it looks like this patch is missing parts. Pushing back in development.

          Show
          Frédéric Massart added a comment - As discussed with you Raj, it looks like this patch is missing parts. Pushing back in development.
          Hide
          Rajesh Taneja added a comment -

          Sorry Fred,

          forgot to update edit.php and edit_form.php. Updated now.

          Show
          Rajesh Taneja added a comment - Sorry Fred, forgot to update edit.php and edit_form.php. Updated now.
          Hide
          Frédéric Massart added a comment -

          Great! As we discussed it IRL, feel now free to push it for integration!

          Show
          Frédéric Massart added a comment - Great! As we discussed it IRL, feel now free to push it for integration!
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          Pushing for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Fred, Pushing for integration review.
          Hide
          Petr Skoda added a comment -

          I disagree with the removal of has_capability() which forces admin/manager to include ALL required fields for other users, because this may be viewed as a regression by some admins.

          Show
          Petr Skoda added a comment - I disagree with the removal of has_capability() which forces admin/manager to include ALL required fields for other users, because this may be viewed as a regression by some admins.
          Hide
          Petr Skoda added a comment -

          OH, now I see it was replaced by "not current user" condition, that makes sense, sorry!

          Show
          Petr Skoda added a comment - OH, now I see it was replaced by "not current user" condition, that makes sense, sorry!
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr

          Show
          Rajesh Taneja added a comment - Thanks Petr
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now
          Hide
          Ankit Agarwal added a comment -

          This works as expected.
          Cheers!

          Show
          Ankit Agarwal added a comment - This works as expected. Cheers!
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: