Moodle
  1. Moodle
  2. MDL-28998

Custom user profile field entry allows non-unique value when it should not

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      1. Create a custom user profile field type and implement field validation with the edit_validate_field() function.
      2. Add a custom user profile field of custom field type created in step 1, set the field to display on user signup.
      3. Register a new user via email-based self-registration and enter an invalid value for the custom field.
      4. We expect the field to be validated and a warning message to show, but the field is not validated and the registration succeeds.

      Show
      1. Create a custom user profile field type and implement field validation with the edit_validate_field() function. 2. Add a custom user profile field of custom field type created in step 1, set the field to display on user signup. 3. Register a new user via email-based self-registration and enter an invalid value for the custom field. 4. We expect the field to be validated and a warning message to show, but the field is not validated and the registration succeeds.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-28998-master
    • Rank:
      18546

      Description

      A new custom user profile field type (in the /user/profile/field directory) has been created. The edit_field_add() function has been used to display the field and the edit_validate_field() function has been used to validate the field input upon form submission. The problem is that the field is only validated when updating the field value on the user's profile, but not on user signup (email-based self-registration).

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for raising this.

        I tested this and I was able to enter a value that was not unique when a unique value was expected (which was bad), but that was the only sort of validation that I could think to test. What kind of validation are you expecting?

        Show
        Michael de Raadt added a comment - Thanks for raising this. I tested this and I was able to enter a value that was not unique when a unique value was expected (which was bad), but that was the only sort of validation that I could think to test. What kind of validation are you expecting?
        Hide
        John Grobler added a comment -

        In our case we also validate for uniqueness, which we cannot get to work (non-unique entries are accepted and stored).

        Show
        John Grobler added a comment - In our case we also validate for uniqueness, which we cannot get to work (non-unique entries are accepted and stored).
        Hide
        Michael de Raadt added a comment -

        OK. I've updated the summary of this issue and bumped the priority.

        Obviously the code to validate uniqueness of the field exists, but it is not being applied correctly.

        In the meantime feel free to help us work on this issue.

        Show
        Michael de Raadt added a comment - OK. I've updated the summary of this issue and bumped the priority. Obviously the code to validate uniqueness of the field exists, but it is not being applied correctly. In the meantime feel free to help us work on this issue.
        Hide
        Ankit Agarwal added a comment -

        Hi,
        The signup form was missing the call to function that is responsible for validating customisable profile fields. Besides that the signup validation function handels submitted data as an array where is its handeled as an object at other places (profile edit), so I had to convert and use the submitted data. It might be a good idea to make this consistent in another improvment MDL.
        submitting for review.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi, The signup form was missing the call to function that is responsible for validating customisable profile fields. Besides that the signup validation function handels submitted data as an array where is its handeled as an object at other places (profile edit), so I had to convert and use the submitted data. It might be a good idea to make this consistent in another improvment MDL. submitting for review. Thanks
        Hide
        Dan Poltawski added a comment -

        Sorry for the delay in peer review..

        Looks good to me. I would have just cast the object in the function but that is not important - please submit for integration!

        Show
        Dan Poltawski added a comment - Sorry for the delay in peer review.. Looks good to me. I would have just cast the object in the function but that is not important - please submit for integration!
        Hide
        Ankit Agarwal added a comment -

        Thanks Dan for the review.
        Submitting for integration
        Thanks

        Show
        Ankit Agarwal added a comment - Thanks Dan for the review. Submitting for integration Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some hours ago...

        the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Thanks Ankit - this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Ankit - this has been integrated now
        Hide
        Petr Škoda added a comment -

        Did you test it with debug mode enabled? The user->id does not exist yet during signup validation, but the code tries to use it.

        Show
        Petr Škoda added a comment - Did you test it with debug mode enabled? The user->id does not exist yet during signup validation, but the code tries to use it.
        Hide
        Ankit Agarwal added a comment -

        Hi Petr,
        Thanks for pointing that out, not sure how it passed my testing. Anyways had a chat with Dan about this and I have set user->id to 0 since thats the default value used in profile_base_class as well.

        Submitting again.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Petr, Thanks for pointing that out, not sure how it passed my testing. Anyways had a chat with Dan about this and I have set user->id to 0 since thats the default value used in profile_base_class as well. Submitting again. Thanks
        Hide
        Sam Hemelryk added a comment -

        Thanks for spotting that Petr and thanks Ankit for getting a fix up for it.
        I was two minds about whether it should be using 0 or -1 however when looking through code I see that it won't actually make any difference. (Was looking at -1 as that is what other uses of profile_validation are using).

        Up for testing again

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks for spotting that Petr and thanks Ankit for getting a fix up for it. I was two minds about whether it should be using 0 or -1 however when looking through code I see that it won't actually make any difference. (Was looking at -1 as that is what other uses of profile_validation are using). Up for testing again Cheers Sam
        Hide
        Rajesh Taneja added a comment -

        Thanks Ankit, Works great

        Few things noticed, while testing this:

        1. profile_field_base::edit_validate_field has one argument but when called from profile_validation it pass two arguments.
        2. profile_field_base::edit_validate_field for loop seems redundant, same logic can be used in sql to get unique values which current not entered by this user.

        Let me know if you want me to open issues for same.

        Show
        Rajesh Taneja added a comment - Thanks Ankit, Works great Few things noticed, while testing this: profile_field_base::edit_validate_field has one argument but when called from profile_validation it pass two arguments. profile_field_base::edit_validate_field for loop seems redundant, same logic can be used in sql to get unique values which current not entered by this user. Let me know if you want me to open issues for same.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        FCT (fixed, closing, thanks). Ciao

        "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
        ~ Benjamin Disraeli

        Show
        Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

          People

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

            Dates

            • Created:
              Updated:
              Resolved: