search

openwebwork / webwork2

Course management front end for WeBWorK
http://webwork.maa.org/wiki/Main_Page
Other
141 stars 164 forks source link

Move the ConfigValues array out of defaults.config. #2372

Closed drgrice1 closed 3 months ago

drgrice1 commented 3 months ago

This array is now defined in lib/WeBWorK/ConfigValues.pm and is returned by the getConfigValues method. The $LTIConfigValues hash is also defined in that file and added to the array when getConfigValues is called if LTI authentication is enabled for the course. This means that these are no longer configurable. No config file can modify them. Furthermore, the array is not even ever added to the course environment. Instead it is just used by the course configuration module which is the only place it was ever used.

This means that if a course's course.conf file includes authen_LTI.conf, it does not also need to include the LTIConfigValues.config file. That file in fact no longer exists.

Also remove all access to the paramcache outside of the WeBWorK::Controller module. That is an internal implementation detail that should never be accessed directly outside of that file.

drgrice1 commented 3 months ago

I forgot to mention another thing that this fixes. Currently the WeBWorK::ContentGenerator::Instructor::Config module does not check permissions in the pre_header_initialize method. This means that anyone that can log into the course could construct a request and submit it to the instructor_config route and change a course`s settings (or course title). If that request has the correct parameters, then it would result in those settings being saved. So if students are allowed to log in to the course (they usually are) or guest users can log in, then those users can change any course settings they want to if they can figure out what the needed parameters are. Note that the template does check permissions, so the html page won't render, but that is all after the settings are already saved.

This pull request adds the permission check to stop that.

Alex-Jordan commented 3 months ago

Locally in production, I added $LTIVersion to that array of LTI config options, so faculty can opt in (for now) to an LTI 1.3 setup. Is that good (or bad) to add here now that this won't be configurable?

More broadly, this has LMS_name, LMS_url, external_auth, LTIGradeMode, LMSManageUserData, and debug_lti_parameters.

That leaves out debug_lti_grade_passback, LTIVersion, LTIAccountCreationCutoff, LTIGradeOnSubmit, LTICheckPrior, LTIMassUpdateInterval, preferred_source_of_username, fallback_source_of_username, strip_domain_from_email, lowercase_username, preferred_source_of_student_id, BasicConsumerSecret. And maybe more.

Some of those are probably clearly things that only a system admin should have control over. But some of them could be helpful to let instructors set when the WW server serves multiple institutions with multiple LMSs. If this collection won't be configurable, we should consider adding a few of these. Maybe we start by eliminating the ones that should never be changed by an instructor. What do you think?

drgrice1 commented 3 months ago

I think that it would be fine to add them to the $LTIConfigValues hash. Note that technically, the other variables you listed were not configurable. The conf/LTIConfigValues.config file is a distribution file that is not meant to be modified. So you were making local modifications to the webwork2 source. You can always still do that. It is just in a different file.

Alex-Jordan commented 3 months ago

OK, I'll work up a list of what I think would be helpful and open a PR after this one. @pstaabp already approved and I just tested with no issues, so merging this now.

drgrice1 commented 3 months ago

Sounds good.