openwebwork / webwork2

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

Add more configurable LTI variables to config page. #2447

Open somiaj opened 1 week ago

somiaj commented 1 week ago

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

Alex-Jordan commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 3 days ago

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?