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
    • Rank:
      39788

      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.

        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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda added a comment -

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

        Show
        Petr Škoda 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: