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

Add more configurable LTI variables to config page. #2447

Merged
merged 5 commits into from
Aug 6, 2024

Conversation

somiaj
Copy link
Contributor

@somiaj somiaj commented Jun 25, 2024

Adds LTIGradeOnSubmit, LTIMassUpdateInterval, LTI{v1p1}{BasicConsumerSecret} to the list of variables that can be configured by a course instructor.

@Alex-Jordan
Copy link
Contributor

Over in #2372, I posted that I was going to open a PR like this one, but I never did.

However I don't think these particular items should be available to an instructor by default. The settings for LTIGradeOnSubmit and LTIMassUpdateInterval help the server admin manage traffic/resources.

On an institutional server, does a course need LTI{v1p1}{BasicConsumerSecret} set differently from other courses? I'm only familiar with our setup at PCC, where there is a sitewide external learning tool in D2L for WeBWorK that uses the same secret for all courses.

When I look over the list of possible things to add in this area, nothing stands out as something that an instructor (As opposed to a site admin) should have control over. (And I'm not even sure about some of the options we have now.)

Thinking about a multi-institutional server, there are some things that would be nice to do differently for different institutions. However at least in the case of Runestone, each institution already has a config file where these things can be set for all courses from that institution. So I'm not sure even there.

One thing at PCC that is nice this summer, is that some instructors are continuing with LTI 1.1 and some are opting in to use LTI 1.3. So it is nice to let them choose themselves which one to use in the course config page. But even that will not matter in the Fall, when we fully move to 1.3.

@drgrice1
Copy link
Member

I think that having these available for those that might want to allow instructors to set them directly is not an entirely bad thing. However, I think that comments should be added to the documentation about these noting that a system administrator might be advised to NOT make these available to instructors.

Although, I also think it is inconsistent to add $LTI{v1p1}{BasicConsumerSecret}, but not add the equivalent $LTI{v1p3} parameters.

@somiaj
Copy link
Contributor Author

somiaj commented Jun 28, 2024

Here I have been letting my instructors configure this. The reason I added the mass update interval is I like to tell my instructors to set that to -1 once their class is over to stop all updates for that course. I added the update on submit, because I have an instructor who was using course (not homework) grade mode, and only wanted mass updates, not constant updates for their course.

As for the secrete, in Canvas, LTI 1.1 can be fully configured by the instructor, so I have my instructors create a unique secrete per class to use, this can help keep old/bad links from working from a different course and I think is a bit better than using a single site wide secret. None of these are there by default, the admin still has to turn them on, but I can add comments to be careful when enabling them.

As for LTI 1.3, I thought one main difference between LTI 1.1 and 1.3 (at least in Canvas), is 1.1 could be configured by an instructo while 1.3 has to be configured an admin of the LMS. Since I wasn't sure how 1.3 works and which of the secrets to add, I didn't include them in the list. Which of the LTI 1.3 secret variables could an instructor configure without needing to work with the admin of the LMS?

@drgrice1
Copy link
Member

Canvas (and I beleive D2L) do not allow instructors to create LTI 1.3 external tools. However, Moodle does. So those using Moodle may want to allow their users to configure LTI 1.3.

The variables that are the equivalent of the LTI 1.1 $LTI{v1p1}{BasicConsumerSecret} are $LTI{v1p3}{PlatformID}, $LTI{v1p3}{ClientID}, $LTI{v1p3}{DeploymentID}, $LTI{v1p3}{PublicKeysetURL}, $LTI{v1p3}{AccessTokenURL}, $LTI{v1p3}{AccessTokenAUD}, and $LTI{v1p3}{AuthReqURL}. Yeah, there are a lot more for LTI 1.3.

I am not really advocating that any of these (including $LTI{v1p1}{BasicConsumerSecret}) be made available in the LTI configuration tab though.

@dlglin
Copy link
Member

dlglin commented Jun 28, 2024

Canvas (and I beleive D2L) do not allow instructors to create LTI 1.3 external tools.

In D2L my understanding is that determining which roles can add new LTI tools can be configured by the system administrator. The instances I'm familiar with don't allow instructors to do it, but there may be some institutions that do (though it's probably very uncommon).

@dlglin
Copy link
Member

dlglin commented Jul 2, 2024

Adding LTI{v1p1}{BasicConsumerSecret} here will show the server-wide value of that variable to the professor, which is a significant security vulnerability. If this is going to be made available to professors then the default value will need to be obfuscated.

In fact, should the override value (the value entered by the professor) be obfuscated as well?

@somiaj somiaj force-pushed the add-lti-config-items branch from 94f57fb to 20e3d89 Compare July 29, 2024 20:52
somiaj added 2 commits August 1, 2024 11:21
Adds LTIGradeOnSubmit, LTIMassUpdateInterval, LTI{v1p1}{BasicConsumerSecret}
to the list of variables that can be configured by a course instructor.
@somiaj somiaj force-pushed the add-lti-config-items branch from 20e3d89 to 2706ded Compare August 1, 2024 19:45
@somiaj
Copy link
Contributor Author

somiaj commented Aug 1, 2024

Some updates. I made it so the LTI configurations are all secret, in that the SECRET_STRING = '*****' will be displayed instead of the default value. If a user has changed the value from the default, their value will be displayed in the entry field. This way the user cannot access system or default secrets. Currently this only works for text entries. If a user enters in the SECRET_STRING, the option will be reverted to its default value.

There is one small issue here, if a user guesses the default value, they can enter it in, and they will see their entry change to the SECRET_STRING. This would allow for a brute force attack on figuring out what the values are set to (I don't see an easy way to fix this, though unsure if it is something to worry about either).

If you want to just test the secret strings without setting up all the LTI stuff, you can edit lib/WeBWork/ConfigValues.pm and add secret => 1 to a text filed, I did most my testing by adding this to courseFiles{course_info} variable.

I also made it so only admin users can modify the LTI secrets by default to also limit users access to those configuration options if an admin chooses to turn them on for their users.

somiaj added 2 commits August 1, 2024 13:56
Used to keep the system (or default) LTI keys hidden from users
when configuring them via the instructor course configuration page.
This is done by replacing any output with a SECRET_STRING, set
to '*****', that is displayed to the user. Currently this option
only works for 'text' configuration objects.
@somiaj somiaj force-pushed the add-lti-config-items branch from 2706ded to 5dbd939 Compare August 1, 2024 19:57
Copy link
Member

@drgrice1 drgrice1 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 now.

@Alex-Jordan Alex-Jordan merged commit bf11f98 into openwebwork:WeBWorK-2.19 Aug 6, 2024
2 checks passed
@somiaj somiaj deleted the add-lti-config-items branch August 17, 2024 15:15
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.

4 participants