do not allow LTI access to admin course

[Alex-Jordan]

If the authentication module is an LTI authentication module and the course is the admin course, this moves on to the next authentication module.

This is to kill any potential for an LMS user to spoof their LTI data and enter the admin course as one of its admin users.

[taniwallach]

I think this should be configurable. In general, I do agree that the worry is valid, but some institutions may trust LTI if they use MFA and other methods, while password based access to WeBWorK does not have MFA or any protection against dictionary attacks.

[Alex-Jordan]

@taniwallach I made it configurable. Does the comment I left starting at line 722 sound right to you?

[taniwallach]

Suggestions made about the comment + 1 small error noted.
I have not tested the code yet.

[Alex-Jordan]

@taniwallach Thanks for seeing that mistake and the suggestion. I fixed the mistake and lightly edited the suggestion.

[somiaj]

@Alex-Jordan did you see my review? Anyways, I don't think defaults.conf is the correct place for that comment, and you should add that to files that are actually edited by users. You should probably also remove the commented out lines for Moodle and LDAP as well, since I don't think users should be editing defaults.conf.

[Alex-Jordan]

@somiaj I do not see any previous review/comments from you in this PR before the most recent one. And only Tani shows up as a reviewer when I'm looking at this in GitHub.

you should add that to files that are actually edited by users.

This is not quite as simple as it seems at first. Of course, it is right that users should not edit defaults.config. But then where should this be? Which subset of the following?

• localOverrides.conf
• authen_LTI.conf
• authen_ldap.conf
• authen_CAS.conf

Some authentication modules have their own config files, and some do not (Shibboleth [not working, but someone recently posted they will work on it], Moodle). Sometimes an individual course.conf sets $authen{user_module}. Sometimes indirectly by including an authentication config file, and sometimes directly. Sometimes $authen{user_module} is set site-wide using localOverrides.conf. It's all potentially confusing if things are overrided in multiple places.

So which is better: having the explanation in one place (defaults.config) or duplicated in each of those authentication config files? I landed on "one place" and let the admin work out where to make the override. I suspect similar thinking is why those two commented out options are there in defaults.config. Note that defaults.config has many things explained in comments where there is no corresponding override lines in other config files. So it's not a new precedent to explain something there but expect people to make the override somewhere else.

I've lost previous iterations of this, but at some point prior to Tani's copy for the explanation, I think I had an explicit suggestion that these things should be overrided in an authentication module config file. What do you think about adding that explicit note for the admin advising them to figure out which makes sense for overriding in their use case: localOverrides.conf, various authentication module config files, or individual course.conf files?

[somiaj]

Why did you remove ldap here and have have moodle twice?

[somiaj]

Though it does seem weird to have commented out options here, I didn't think users should be editing defaults.conf, so having editable comments here seems strange.

[somiaj]

You should add this to authen_ldap.conf.dist, authen_LTI.conf.dist, and authen_CAS.conf.dist like $authen{user_module}, as that is where users will actually edit this. That would be a good place to have a comment and maybe commented out options for the appropriate module to enable it. I don't think users should be editing defaults.conf, so the comment I think is a little out of place here and should maybe just state what this does.

[somiaj]

@Alex-Jordan that is odd, I wonder why you cannot see my reviews. Anyways Tani caught the same thing I did, and you summarized my other one. I have attached some screenshots of what I see in this PR, so I'm confused as to why you (or maybe others) don't see them.

Okay, an error on my end, for some reason the review was just 'pending' and I had to click a finalize button, new to me, since I saw the review in the PR, I assumed others did too...my mistake.

Anyways, for new users I don't think looking in defaults.conf is a common thing to do. The instructions state making copies of the various .dist files and editing those, so options that only appear in defaults.config might get missed.

My original suggestion was just to follow what is currently being done with $authen{user_module}, and add this new option to the various auth files. For LDAP I suggest enabling it by default, but not for LTI. I could see users being confused if they wanted to use LDAP and for some reason it won't work for the admin course since the only info about a different setting is hidden in defaults.conf file.

Although it may not be new to have options hiding in defaults.conf with descriptions on them, to me I think the descriptions in defaults.conf should be more to developers, and the description for users and and options to edit should be in the .dist files that system admins are meant to edit when setting up their server.

[Alex-Jordan]

I saw your review and comments came through this afternoon.

I updated it to put commented out options for all this in localOverrides. And I put uncommented options in each of the authentication module config files that match what those files set for user_module.

More feedback is welcome.

[somiaj]

Do you want to comment out the LTI methods here, to make default behavior LTI cannot be used to access the admin course (like your original intent was). I think this as reasonable since LTI can also create student users (they can't do much in the admin course), but that could be another reason it should be disabled by default?

[Alex-Jordan]

Now that it's configurable, it feels OK to let the default behavior go unchanged from what it has been up to now. I don't really have an opinion though. In my own servers, I'll make sure that LTI can't be used to access the admin course either way.

[somiaj]

@Alex-Jordan I figured out what I did wrong, and submitted my old review just so you could see the comments and I wouldn't have to copy them again. I figured out what I was doing incorrectly, there is both a 'enter single comment' and 'start a review', the second needs to be finalized. I have only left single comments in the past so didn't realize that 'start a review' as an option until now.

[somiaj]

In localOverrides.conf you added the commented out authentication options before the includes that include the auth_LTI.conf and auth_LDAP.conf, which have the same options not commented out. So if a user removes the comments to configure the auth methods there, they will just be overridden by what is in the last auth_Foo.conf file included. I think you should move these to after the includes, so they can be used to override anything the configuration files may be doing when a user removes the comments.

[Alex-Jordan]

if a user removes the comments to configure the auth methods there, they will just be overridden by what is in the last auth_Foo.conf file included.

My perspective is that is exactly what should happen. The authentication module config files are "loaded last" conceptually, even if they come in the middle of localOverrides.conf. In some installations, the authentication module config files won't even be loaded until somewhere in the course.conf file.

In the case of LDAP, it is odd to have a commented out line that suggests using it (in localOverrides.conf at line 464) before loading the LDAP config file (at line 512). But what is at line 464 is just what was previously in defaults.config, which also came before the loading of the LDAP config file.

[drgrice1]

This looks good to me at this point.

[Alex-Jordan]

This is rebased now, I think just awaiting @taniwallach's review. I did lose track if I got to everything @somiaj mentioned, and please alert me if there is more to adjust.

[somiaj]

I recall you addressed everything I mentioned, but I am going to mention it again just in case. Note, I'm okay with things the way they are, I'm just sharing my observations on hypotheticals.

I would still remove LTI access by default (maybe comment it out) in the admin course. This seems like a reasonable default to me, vs just allowing admins to remove it. Sure it changes current behavior, but I think for the better.

I still think the order in which the config options are read should be addressed a bit more. Or maybe add comments about this, to avoid possible confusing. Currently if someone wanted to override the settings in localOverrides.conf and uncomments/edits the options there, they will be overridden in authen_LTI.conf, authen_LDAP.conf, etc, without first modifying those files. For my setup, I would have to copy/paste the settings in localOverrides.conf and move them (so I can use both LTI and LDAP) to below the include lines.

I did understand your argument that in some cases that the config files may not be read until course.conf, so probably not an easy answer for this.

[Alex-Jordan]

I think you are right that it would be good to change the default. I wonder if there is anyone it would even effect. Who is currently intentionally accessing their admin course using LTI? So I will make that change unless someone else stops me.

As we saw with a recent forum thread, the situation with the config files for authentication is a mess. Some solution may be needed that is beyond the scope here.

[Alex-Jordan]

@drgrice1 I believe I addressed the recent things you mentioned here.

[drgrice1]

It looks like you have addressed the issues that have been put forth.

@taniwallach, could you look at this, and if you are satisfied give it an approval to remove the block on merging?

[Alex-Jordan]

@drgrice1 This has a conflict now following the merge of #2333. Since I don't follow everything that you are changing in #2333, would you mind looking this over and suggesting how to address the conflict?

And then @taniwallach, following addressing the conflict, can you look at this and lift your block on merging?

[drgrice1]

Yeah, I will look at it.

[drgrice1]

The easiest way to fix the conflict here is to merge origin/develop into this branch. When you do that the following merge conflict will occur:

<<<<<<< HEAD
	my $authen_ref = ref($c->authen);
	if ($c->ce->{courseName} eq 'admin' && !(grep(/^$authen_ref$/, @{ $c->ce->{authen}{admin_module} }))) {
		$self->write_log_entry("Cannot authenticate into admin course using $authen_ref.");
		$c->stash(
			authen_error => $c->maketext(
				'There was an error during the login process.  Please speak to your instructor or system administrator.'
			)
		);
		return ($self->call_next_authen_method());
	}

	my $result    = $self->do_verify;
	my $error     = $self->{error};
	my $log_error = $self->{log_error};
=======
	return $self->call_next_authen_method if !$self->request_has_data_for_this_verification_module;
>>>>>>> origin/develop

Just delete the version control conflict markers and move return $self->call_next_authen_method if !$self->request_has_data_for_this_verification_module; before the changes you added here. Then delete the lines my $result = ..., my $error = ..., and my $log_error = ....

Although, I also see some code issues that need to be fixed. I will comment on those separately.

[drgrice1]

Here are the things that I saw before.

[Alex-Jordan]

OK, I made all those updates. @taniwallach this is ready for you to review.

[somiaj]

Looked over things again and looks good. I tested that by default I get the error There was an error during the login process. Please speak to your instructor or system administrator. when trying to login via LTI by default (so it is correctly not letting me log in via LTI), and if I add WeBWorK::Authen::LTIAdvanced to $authen{admin_module} then it tries to login via LTI (though I get an error since I didn't have LTI setup correctly to login, but see via the debug parameters it attempted to).

[drgrice1]

Since this now has two approvals, I will override @taniwallach disapproval, and merge it. I believe that his issues have been addressed, and that he just has not had the time to look at this.