Skip to content

Remove the filtering of large courses on the course admin "Manage OTP Secrets" page and fix security vulnerability.#2718

Merged
Alex-Jordan merged 2 commits into
openwebwork:WeBWorK-2.20from
drgrice1:manage-otp-remove-large-course-restrictions
May 20, 2025
Merged

Remove the filtering of large courses on the course admin "Manage OTP Secrets" page and fix security vulnerability.#2718
Alex-Jordan merged 2 commits into
openwebwork:WeBWorK-2.20from
drgrice1:manage-otp-remove-large-course-restrictions

Conversation

@drgrice1
Copy link
Copy Markdown
Member

This page loads quickly regardless of if these courses are filtered out. Furthermore the menus are generally responsive even when these large courses are included. The only case where something is a bit slow is in the case that on the "Copy Single Secret" tab the selected "Source Course ID" is one of these large courses. In that case if you click on the "Source User ID" it takes some time for the dropdown menu to appear and even then it has to be an astronomically large course for that to be slow (more than 20,000 users). However, even if the large courses are included as long as a large course is not selected the menus are quick.

It is interesting that the multiple select elements on the "Copy Multiple Secrets" tab are still fast even with astronomically large courses. It is just the single select elements on the "Copy Single Secret" tab that experience a noticeable slowdown.

In any case, the point is that there is no reason to filter these courses. Doing so just adds unnecessary steps for the user.

@somiaj: This is probably not the direction you though this would go!

@drgrice1
Copy link
Copy Markdown
Member Author

Note that I haven't tested copying a large number of OTP secrets. Looking at the code I can see that that is going to need work. I expect that this will be exceedingly slow in fact. I see that there not only is heavy database access in for loops, but there is even construction of database objects inside user loops. Unfortunately, getting a large number of users with OTP secrets to work with and test things is a problem in itself.

@drgrice1
Copy link
Copy Markdown
Member Author

Actually, it wasn't so hard to set up a basic test case. I just copied one OTP secrect to all the users in the database. With initial testing it seems that things aren't that bad. There may be some improvement that could be done, but things are not drastically slow. I realized that @somiaj implemented caching of constructed database objects in the loop (I missed that in my quick assessment before), so that part doesn't actually slow things down.

I am not sure that it is worth it to do much here since things are not that bad.

@drgrice1
Copy link
Copy Markdown
Member Author

I just noticed that there is a security vulnerability on this page. The page should prevent the current user in the admin course from modifying there own OTP secret in the admin course in any way. They should not be able to reset their own OTP secret and should not be able to overwrite their OTP secret with one for any user in another course by copying from that course. It is probably okay to allow them to copy their OTP secret to another course I suppose. Although that is certainly questionable.

@drgrice1 drgrice1 force-pushed the manage-otp-remove-large-course-restrictions branch 3 times, most recently from 7028aa4 to 2a5146a Compare April 24, 2025 15:22
@drgrice1
Copy link
Copy Markdown
Member Author

This now also prevents a user in the admin course from modifying their own OTP secret. So this is now a security vulnerability pull request as well.

@drgrice1 drgrice1 changed the title Remove the filtering of large courses on the course admin "Manage OTP Secrets" page. Remove the filtering of large courses on the course admin "Manage OTP Secrets" page and fix security vulnerability. Apr 24, 2025
@drgrice1 drgrice1 force-pushed the manage-otp-remove-large-course-restrictions branch 3 times, most recently from 84e1e17 to 0f579db Compare April 27, 2025 16:24
@drgrice1 drgrice1 changed the base branch from develop to WeBWorK-2.20 April 29, 2025 12:21
@drgrice1 drgrice1 force-pushed the manage-otp-remove-large-course-restrictions branch from 0f579db to fc0217f Compare April 29, 2025 20:15
Copy link
Copy Markdown
Contributor

@somiaj somiaj left a comment

Choose a reason for hiding this comment

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

Confirmed I cannot overwrite my admin OTP secret, thanks for catching that.

@drgrice1 drgrice1 force-pushed the manage-otp-remove-large-course-restrictions branch 2 times, most recently from 13bd049 to 28f1b5f Compare May 13, 2025 14:40
drgrice1 added 2 commits May 20, 2025 13:46
… Secrets" page.

This page loads quickly regardless of if these courses are filtered out.
Furthermore the menus are generally responsive even when these large
courses are included.  The only case where something is a bit slow is in
the case that on the "Copy Single Secret" tab the selected "Source
Course ID" is one of these large courses.  In that case if you click on
the "Source User ID" it takes some time for the dropdown menu to appear
and even then it has to be an astronomically large course for that to be
slow (more than 20,000 users).  However, even if the large courses are
included as long as a large course is not selected the menus are quick.

It is interesting that the multiple select elements on the "Copy
Multiple Secrets" tab are still fast even with astronomically large
courses.  It is just the single select elements on the "Copy Single
Secret" tab that experience a noticeable slowdown.

In any case, the point is that there is no reason to filter these
courses.  Doing so just adds unnecessary steps for the user.
@drgrice1 drgrice1 force-pushed the manage-otp-remove-large-course-restrictions branch from 28f1b5f to d81c523 Compare May 20, 2025 18:47
@Alex-Jordan Alex-Jordan merged commit b937168 into openwebwork:WeBWorK-2.20 May 20, 2025
2 checks passed
@drgrice1 drgrice1 deleted the manage-otp-remove-large-course-restrictions branch May 20, 2025 20: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