Enforce user permissions on Classlist Editor page #1888
Conversation
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
f9964fa to
159e292
Compare
|
A quick note on the Also, what exactly do you mean by the class was added to match other themes? The system.html.ep file is used by all of the provided themes. The actual file is in the math4 directory, and the other themes just link to that file. I think that the login status looks better without the |
I was using the default math4 theme on develop, and my login-status was aligned left before this change. I'm guessing that I probably missed refreshing with npm install... (confirmed)
I was referring to math4-green/yellow/red, which each contain a system.html.ep file with
The vertical spacing is now resolved after updating my css with npm install. Login is slightly higher than center, and I'm okay with that -- my layout was unpleasant simply for lack of running the correct version of the css files. |
|
Strange, I see now that all the files are linked as you say. I'm not sure what confusion I'm running on here... I can revert this change, it's unrelated anyhow. |
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
159e292 to
6705ba6
Compare
There was a problem hiding this comment.
This generally looks good. The inefficiency of the delete_handler needs a little attention though.
Also, there is another way that an admin user can be modified in the class list editor that this does not prevent. That is by importing and replacing a user. If you export users to a file, then edit an admin user in the file (change the password or permission level in particular), and then import the file replacing users, then the admin user is modified. I was able to demote the admin user to a student, and change the password to something invalid.
templates/ContentGenerator/Instructor/UserList/user_list.html.ep
Outdated
Show resolved
Hide resolved
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
6705ba6 to
4975960
Compare
|
Okay, the delete method has been made more efficient, the translation strings should be preserved, and I tidied up the errant space in the conditional. 👍 |
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
4975960 to
93495ff
Compare
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
93495ff to
3a63317
Compare
prevent password changes align login-status right efficiency preserve translated text don't get cocky, kid block import of users with elevated permissions
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
3a63317 to
78703d4
Compare
|
This should be all cleaned up now. Users may not use the import action to create or modify accounts with permissions higher than their own. |
|
Minor issue: UserList.html.ep displays the wrong user count. I may need to rethink the approach to skipping privileged rows or else fixing this is going to be super hack-y. |
|
On initial look, I don't see an issue with the user count. It seems like you can't select an admin in any table in the classlist editor. Do we want to allow for export, filter or sort. That doesn't seem like a security problem, although this is quite minor. |
drdrew42
force-pushed
the
feature/classlist-permissions
branch
10 times, most recently
from
aa074ba to
41f3ccc
Compare
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
41f3ccc to
87aa629
Compare
|
Found and fixed a couple more oversights. I think it's possible to ditch the use of allUserIDs... is it worth it? |
the database for each user, get them all in a single query. From the develop branch to the feature/classlist-permissions branch the timing for the load of the classlist editor page for a course with more than 18,000 users increased from 240 milliseconds to 1.69 seconds. That is a considerable slow down. The main reason for this is what is changed here. This brings the page load time back down to 549 milliseconds. Of course those timings are from one page load, but similar timings occur on repetition. This is still twice as long as long of a load time as before. Part of this is because previously when loading a large class (more than 200 users) no permissions were loaded at first since only the permissions for visible users were loaded. Although, timing shows that with the change suggested here the user and permission queries are about the same as before. Timing further shows that the real slow down is the sorting. Previously only visible users were sorted. Now all users are sorted regardless of which are shown.
|
@drdrew42: I put in a pull request to this branch with one suggested change. Instead of retrieving the permissions from the database one at a time, get them all in one query. This gives a considerable speed boost in page loading time for large classes. |
|
Good catch on the visible input on a click on the header column. |
|
So it should be worth it to sort only the visible users? I can put in that change along with the improved permissions fetch... |
|
That would certainly be worth it. Sorting is always a computationally intensive process. So if it could be avoided if not needed, then that would be worth it. |
…gestion Speed up permission retrieval from the database considerably.
|
Yeah, now timing is back down to what it was when no users are shown for a large class! Good work. Only the visible users really need to be sorted anyway. |
|
Yeah, there's no sense in refactoring just to make things worse! |
|
I removed the usage of the visible_user_string when I switched a click on a header into a post request. I missed those in pre_header_initialize though. Another good catch. |
|
I'm getting the following error on changing a password of a user (I was logged in as a professor, the user is a student): |
The problem is line 565. @drdrew42 forgot to convert the array reference returned by |
templates/ContentGenerator/Instructor/UserList/user_list.html.ep
Outdated
Show resolved
Hide resolved
cleanup
drdrew42
force-pushed
the
feature/classlist-permissions
branch
from
02b0895 to
1f9c84d
Compare
|
Okay, updates are in! |
…slist Editor. Since the database is not queried to retrieve updated information after actions are performed the data for saved users needs to be updated in the cached lists so that the updated information is displayed when the templates are loaded later. This fixes openwebwork#1933. Also fix sorting that was also broken in openwebwork#1888. The fallback methods are needed in order for sorting to work correctly in case the first three do not sort thing properly. Currently, if you load the page with users that do not have the information that the first three methods sort on, then the users end up in a different order each time you load the page. For example, if using the default sort and users do not have first or last names.
…slist Editor. Since the database is not queried to retrieve updated information after actions are performed the data for saved users needs to be updated in the cached lists so that the updated information is displayed when the templates are loaded later. This fixes openwebwork#1933. Also fix sorting that was also broken in openwebwork#1888. The fallback methods are needed in order for sorting to work correctly in case the first three do not sort thing properly. Currently, if you load the page with users that do not have the information that the first three methods sort on, then the users end up in a different order each time you load the page. For example, if using the default sort and users do not have first or last names.
Description
Perhaps better described as a bugfix than a feature, it is time to prevent the modification/deletion of administrative users by users with non-admin privileges.
In order to accomplish this, several safeguards have been put into place.
Bonus
Somehow thetext-endcss class was omitted from the login-status column of the math4 theme, it's been added to match the other themes. Do we want to addmy-autoto this column as well? This change would vertically center the login-status content.