give better information to certin users who are denied entry #2252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drgrice1
approved these changes
Nov 14, 2023
There was a problem hiding this comment.
I think this is okay. However, note that this does not apply to any authentication module that overrides the get_credentials or check_user methods. Both of the LTI authentication modules do this. Furthermore, both of those modules have similar code for this. They should probably be modified in the same way.
Although, it seems that there is a missing step in the authentication modules in general. Authentication should not be checking permissions before verifying credentials to begin with (so the change in order here is correct). It would be better if the permission checks would be in an additional method that comes after the verification. Then the other modules could just call that even if they override the verification method. This is easy to implement. Just add another step in the do_verify method, and the authentication procedure becomes
- get_credentials
- check_user
- verify_user
- check_user_permissions
|
By the way, there is something odd here. I checked out this branch from GitHub and got |
pstaabp
approved these changes
Nov 20, 2023
|
I changed the login permission level to professor and got the new login message. |
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Feb 15, 2024
This was broken by openwebwork#2252. The issue is that `login_proctor` users with `Proctor` status do not have the `allow_course_access` behavior. Thus the check on line 509 of `lib/WeBWorK/Authen.pm` fails. So this skips that check if the `login_type` starts with "proctor". This will only be true if the `WeBWorK::Authen::Proctor` module is the current authentication module and the `proctor_user` parameter is set. Note that the proctor authen module already checks all of the necessary permissions, so this check is not needed. When the check on line 509 of `lib/WeBWorK/Authen.pm` was in the `check_user` method of `WeBWorK::Authen` this check was not even done by the `WeBWorK::Authen::Proctor` module because that module overrides the `check_user` method. To test this create a proctored test that is set with a "Proctor Authorization Type" of "Both Start and Grade", and create a `login_proctor` user with the `Proctor` status. Proctor authentication should succeed if the `login_proctor` user's credentials are entered on the proctor login page. For the develop branch this not only fails, but the message "This user is not allowed to log in to this course" is shown. While that message is true, it is not applicable here. That is the whole point of a `login_proctor`. Note that set level proctors (hidden users created if you set the "Password" in the "Proctoring Parameters" for a test) automatically have "Proctor" status. So set level proctoring is completely broken.
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Feb 15, 2024
This was broken by openwebwork#2252. The issue is that `login_proctor` users with `Proctor` status do not have the `allow_course_access` behavior. Thus the check on line 509 of `lib/WeBWorK/Authen.pm` fails. So this skips that check if the `login_type` starts with "proctor". This will only be true if the `WeBWorK::Authen::Proctor` module is the current authentication module and the `proctor_user` parameter is set. Note that the proctor authen module already checks all of the necessary permissions, so this check is not needed. When the check on line 509 of `lib/WeBWorK/Authen.pm` was in the `check_user` method of `WeBWorK::Authen` this check was not even done by the `WeBWorK::Authen::Proctor` module because that module overrides the `check_user` method. To test this create a proctored test that is set with a "Proctor Authorization Type" of "Both Start and Grade", and create a `login_proctor` user with the `Proctor` status. Proctor authentication should succeed if the `login_proctor` user's credentials are entered on the proctor login page. For the develop branch this not only fails, but the message "This user is not allowed to log in to this course" is shown. While that message is true, it is not applicable here. That is the whole point of a `login_proctor`. Note that set level proctors (hidden users created if you set the "Password" in the "Proctoring Parameters" for a test) automatically have "Proctor" status. So set level proctoring is completely broken. Also move session creation and setting of `$self->{initial_login}` to 1 until after the checks now on lines 509-521 of `lib/WeBWorK/Authen.pm`. Those things should not be done if `verify_normal_user` is returning 0.
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Feb 21, 2024
This was broken by openwebwork#2252. The issue is that `login_proctor` users with `Proctor` status do not have the `allow_course_access` behavior. Thus the check on line 509 of `lib/WeBWorK/Authen.pm` fails. So this skips that check if the `login_type` starts with "proctor". This will only be true if the `WeBWorK::Authen::Proctor` module is the current authentication module and the `proctor_user` parameter is set. Note that the proctor authen module already checks all of the necessary permissions, so this check is not needed. When the check on line 509 of `lib/WeBWorK/Authen.pm` was in the `check_user` method of `WeBWorK::Authen` this check was not even done by the `WeBWorK::Authen::Proctor` module because that module overrides the `check_user` method. To test this create a proctored test that is set with a "Proctor Authorization Type" of "Both Start and Grade", and create a `login_proctor` user with the `Proctor` status. Proctor authentication should succeed if the `login_proctor` user's credentials are entered on the proctor login page. For the develop branch this not only fails, but the message "This user is not allowed to log in to this course" is shown. While that message is true, it is not applicable here. That is the whole point of a `login_proctor`. Note that set level proctors (hidden users created if you set the "Password" in the "Proctoring Parameters" for a test) automatically have "Proctor" status. So set level proctoring is completely broken. Also move session creation and setting of `$self->{initial_login}` to 1 until after the checks now on lines 509-521 of `lib/WeBWorK/Authen.pm`. Those things should not be done if `verify_normal_user` is returning 0.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if you are a user with a role of
DroppedorProctor, when you try to log in, you get the messageInvalid user ID or password.This is particularly confusing/misleading for a student who was dropped who tries to get back in to WeBWorK using the username/password that worked for them in the past.You also get the same message if the
loginpermission level has been set high. I sometimes do this if I want to temporarily deny all students access to a course. SeeingInvalid user ID or password.is again misleading.This PR changes things so that if one of the above situations happens, if that person gets past the point of entering a valid username/password combination, they are told more specifically
This user is not allowed to log in to this course. It lets them know they have the right credentials, but for some reason are denied access right now.I don't believe this leaks information, since they have to have the correct username/password to get this message.