Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

drgrice1
Copy link
Member

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 drgrice1 force-pushed the config-values-not-in-defaults branch 5 times, most recently from 79ecd4b to 6a7c5b5 Compare March 27, 2024 15:59
@drgrice1
Copy link
Member Author

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.

Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

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 drgrice1 force-pushed the config-values-not-in-defaults branch from 6a7c5b5 to 9284a33 Compare April 3, 2024 21:44
@Alex-Jordan
Copy link
Contributor

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
Copy link
Member Author

drgrice1 commented Apr 4, 2024

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 Alex-Jordan merged commit 35c1b87 into openwebwork:develop Apr 4, 2024
2 checks passed
@Alex-Jordan
Copy link
Contributor

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
Copy link
Member Author

drgrice1 commented Apr 4, 2024

Sounds good.

@drgrice1 drgrice1 deleted the config-values-not-in-defaults branch April 4, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants