Use core required_param for toggle parameters.

[doctorlard]

No description provided.

[gjb2048]

Thanks for the patch Jonathan,

Please could you explain the rational for the refactoring. Also removing 'PARAM_TOPCOLL' will break this line: https://github.com/gjb2048/moodle-format_topcoll/blob/MOODLE_28/format.php#L91 which is there to ensure that only the 'PARAM_TOPCOLL' format is allowed to change the data.

Also important historical information has been lost by the removal of this line: catalyst@54c55c8#diff-f1abe56d2faf84b984f11269df229fd3L223

Therefore given the omission, please provide details of the testing you have done to validate the quality of the patch.

Thanks,

Gareth

[doctorlard]

Hi there - hopefully I can explain this commit here.

1. required_topcoll_param() has a second parameter, $type, which is only ever PARAM_TOPCOLL, and since it is only used for the topic toggle values in this plugin, is redundant; it is passed to clean_topcoll_param() which has a switch statement on $type with only one case.
2. There is only one call to required_topcoll_param() in settopcollpref.php and we can enforce the type within the function (since we can't enforce it in core required_param() without hacking lib/moodlelib.php)
3. Rather than copy the core code and use $_POST and $_GET we can rely on core required_param() to do that for us, using PARAM_RAW (which handles UTF8 for us but otherwise performs no filtering). This is the main purpose of the commit, which delegates security risks around HTTP parameters to the core code. It also makes PARAM_TOPCOLL redundant since a) PARAM_RAW is sufficient and b) we are performing our own filtering anyway.

You're right though! :-) I missed the usage in format.php, and for that matter, the tests. Since $type was not being passed through settopcollpref.php anymore, it was still working when I was testing, despite the undefined constant (PHP Notice) and the blank string that would have been in $USER->ajax_updatable_user_prefs.

I guess for the purposes of keeping user_preference_allow_ajax_update() happy we could keep PARAM_TOPCOLL since we're now enforcing the type and filtering through required_topcoll_param().

Also, I need to update the commit message and add "This work was made possible through funding from Te Rito Maioha Early Childhood New Zealand"

[gjb2048]

Thank you for the explanation Jonathan,

I need to carefully examine what you have said in depth before pulling.

The original intent of the belt and braces approach in 'settopcollpref.php' was to prevent an inadvertent security back door to the underlying API provided by the core equivalent of 'settopcollpref.php'. And therefore if a hacker called 'settopcollpref.php' instead to gain access. So just need to be sure that the changes do not allow this.

[gjb2048]

Having said all of this, all of this substantial effort is because the documenting comment in the core API was wrong and I believed it! See the grief and agro I went through with: https://tracker.moodle.org/browse/MDL-46754 - I was pulling my hair out!

[doctorlard]

Yes I saw that :-) Upon further thought I've amended the PARAM_TOPCOLL usage in format.php to use PARAM_RAW, since that's what is checked with required_param, within required_topcoll_param. I also amended the commit message.

[gjb2048]

Thanks Jonathan.

[gjb2048]

FYI, master branch now includes new code to issue a 406 when the data is invalid: e386424