search

openwebwork / webwork2

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

allow customization of admin course ID #2295

Closed Alex-Jordan closed 8 months ago

Alex-Jordan commented 9 months ago

This allows a site to use something other than admin for the admin course ID. The purpose is to remove the ability for bad actors to know the location of the admin course.

To test, after loading this branch, you need to immediately edit site.conf to get the new line that initializes $admin_course_id. Then restart webwork2.

One thing the PR does is allow the "admin" course (or whatever its name is changed to) to be the source course to copy from when creating a new course. I'm not sure if this is necessary and maybe we can remove that change. But in any case, next make a new course (you can call it "my-admin" for testing). Its templates folder can be copied from the "admin" course. (And if #2290 is merged, then more could be copied as well.)

Then in site.conf, change $admin_course_id to be the ID of the new course, and restart webwork2. Now if you go to "admin", it should look like a regular course. And if you go to the new course, it should function as the admin course. In practice you would then also clean up by copying archive files from the old "admin" course to the new course (since they live parallel to templates/ they did not copy over when creating the new course). And I think you would also archive and close the original "admin" course at that time.

Alex-Jordan commented 9 months ago

Several PRs are open at this time which make changes to (or about) the admin course: #2288, #2290, #2291, #2292, #2293. At least some of them probably assume a hard coded 'admin' here and there. It might be best to review and merge those, then adjust this PR to catch the additional places where 'admin' is hard coded.

pstaabp commented 9 months ago

Not seeing this working. I changed the name of the admin_course_id to ww_admin. Going to /webwork2/ww_admin brings up the course not found error. Does the course directory need to be moved to the new location?

Also, will the database tables need to be updated?

Alex-Jordan commented 9 months ago

Not seeing this working.

Just checking, but did you:

This does not take the existing admin course and rename it. It allows you to declare some other course ID to serve as the admin course. So if you don't use "admin", you actually must create the new course first. I don't think there's anything special about the new course. Just create it like you would create a new instructional course. Later, you might want to transfer the archives folder from the old admin course into the new ww_admin course.

Also, will the database tables need to be updated?

I don't think so. But I think you were thinking of this as the admin course getting altered instead of what I just described above.

pstaabp commented 9 months ago

I see now that I will need to create the new admin course. That's how the database tables and directory will work.

pstaabp commented 9 months ago

I want to point out that after making a new course that will become the admin course, the admin user needs to have admin privileges in order for the admin course to function--which is good that those are checked.

drgrice1 commented 9 months ago

One thing that is a bit odd with this is that this $admin_course_id can be set in a course's course.conf file. For example, it could be set to be the course id of the course itself. This fortunately turns out to not be a security vulnerability, but does do odd things to the site navigation. Try it out!

I think that site.conf really needs to not be in the course environment at all. Instead these settings should something separate. Probably these settings should be in the webwork2.mojlicious.yml file and be application configuration settings instead. Something to think about for future work.

Alex-Jordan commented 9 months ago

Some scripts probably assume the admin course name is admin. Notably, the upgrade admin tables script. Can they be adjusted to make use of variables from site.conf? Or perhaps they already do...

drgrice1 commented 9 months ago

Good catch. That should be added to this pull request.

The upgrade_admin_db.pl script already uses a course environment. So it should be an easy thing to adapt it to use the $ce->{admin_course_id}. You will need to create a minimal course environment before the one that is currently used though, because the one it uses is specific to the admin course using the hard coded course name admin for the admin course.

The dump_past_answer script skips the hard coded admin course. Don't worry about that script though. That script is rather broken right now. I have a branch with that script fixed, and will put in a pull request with that soon. So I will take care of that in that pull request.

The update-OPL-statistics.pl script also skips the hard coded admin course. That one should be easy to fix since it already has a minimal course environment to work with. Just change line 14 to use the $ce variable rather than the hard coded 'admin'.

I believe those are the only scripts that do anything with the admin course directly.

Alex-Jordan commented 9 months ago

I updated the scripts but did not test them. My school and its vendors have had power outages all week from a severe ice storm and tonight I can't get to the test server I use. I just edited these locally without testing on a real database. I think these changes work, but someone should take a look.