easy entry to a course from admin course - #3103
Conversation
c318c9b to
25a31e2
Compare
|
This is now retargeted to |
914c17f to
a7d9a03
Compare
|
Develop is now up to date. So you can rebase onto it now. |
Co-authored-by: Claude <noreply@anthropic.com>
25a31e2 to
734f08d
Compare
|
Rebased and pushed. |
drgrice1
left a comment
There was a problem hiding this comment.
These are just code suggestions. I am still working on analyzing the effect of this on other authentication modules.
I have created a pull request to this branch with these suggested code changes.
|
|
||
| use WeBWorK::CourseEnvironment; | ||
| use WeBWorK::DB; | ||
| use WeBWorK::Debug; |
There was a problem hiding this comment.
This needs to be
| use WeBWorK::Debug; | |
| use WeBWorK::Debug qw(debug); |
and is the reason for the conflict.
| my $sessions = $c->app->sessions; | ||
| my $adminCookieName = 'WeBWorKCourseSession.' . $ce->{admin_course_id}; | ||
| my $cookieMethod = $sessions->encrypted ? 'encrypted_cookie' : 'signed_cookie'; | ||
| my $rawValue = $c->$cookieMethod($adminCookieName); |
There was a problem hiding this comment.
This should be
| my $sessions = $c->app->sessions; | |
| my $adminCookieName = 'WeBWorKCourseSession.' . $ce->{admin_course_id}; | |
| my $cookieMethod = $sessions->encrypted ? 'encrypted_cookie' : 'signed_cookie'; | |
| my $rawValue = $c->$cookieMethod($adminCookieName); | |
| my $sessions = $c->app->sessions; | |
| my $rawValue = $c->signed_cookie('WeBWorKCourseSession.' . $ce->{admin_course_id}); |
Webwork uses signed_cookies (the default). So there is not need to check if the session cookie is encrypted or not. It isn't. Also the encrypted cookie feature was not added to Mojolicious until version 9.39, and we currently allow version 9.34 or newer (except a few bad versions) of Mojolicious. Even if we do move to requiring newer versions of Mojolicious and use encrypted cookies, this check would not be necessary. This would then use the encrypted_cookie method instead. We know which one we are using, so no need to check.
| my $adminSession = eval { $sessions->deserialize->(b64_decode($rawValue)) }; | ||
| return 0 unless $adminSession; |
There was a problem hiding this comment.
There is no need for the eval here. Mojolicious does not use an eval in their code for this, so I see no reason that we should. In fact, might as well use essentially their code here and make this
| my $adminSession = eval { $sessions->deserialize->(b64_decode($rawValue)) }; | |
| return 0 unless $adminSession; | |
| return 0 unless my $adminSession = $sessions->deserialize->(b64_decode($rawValue)); |
| return 0 unless $adminUserID && $adminKey; | ||
|
|
||
| # Confirm the admin session is still valid | ||
| my $AdminKey = $db_admin->getKey($adminUserID); |
There was a problem hiding this comment.
Do not use Pascal case for variable names. Use camel case. I have been working on removing that sort of thing in the webwork2 code. I recommend using the variable $adminKeyRecord here, since $adminKey is already used above, and that makes the difference clear here. The actual key in the database is the key column, and this is the database record that contains that column.
|
|
||
| return 0 unless _admin_course_has_create_delete_permission($ce_admin, $db_admin, $adminUserID); | ||
|
|
||
| my $AdminPassword = $db_admin->getPassword($adminUserID); |
There was a problem hiding this comment.
This also should not be Pascal case. I recommend using $adminPasswordRecord for consistency with the $adminKeyRecord variable above.
| return 0 unless defined $activity_role && exists $ce_admin->{userRoles}{$activity_role}; | ||
| my $role_permlevel = $ce_admin->{userRoles}{$activity_role}; | ||
|
|
||
| my $PermissionLevel = $db_admin->getPermissionLevel($user); |
There was a problem hiding this comment.
I realize this variable was copied from Authz.pm, but this also should not be Pascal case. The Authz.pm file needs a lot of clean up. It is a mess.
| my $self = shift; | ||
| my $c = $self->{c}; | ||
|
|
||
| my $user_id = $self->{user_id}; |
There was a problem hiding this comment.
There is no need for a local $user_id variable. Just use $self->{user_id} in the two places the $user_id variable is used in this method.
| && $coursePassword->password eq $self->{admin_cross_course_password}) | ||
| { | ||
| $self->{log_error} = 'admin cross-course login: no matching password for this user in this course'; | ||
| $self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE); |
There was a problem hiding this comment.
Delete this line. The only case in which this method is called is if the credential_source is 'admin_cross_course', and in that case $self->{error} is also ignored (see line 206). So there is no point in setting it.
| my $expiration = $adminSession->{expiration} // $sessions->default_expiration; | ||
| my $expires = delete $adminSession->{expires}; | ||
| return 0 if !$expires && $expiration || defined $expires && $expires <= time; |
There was a problem hiding this comment.
I forgot to mention this. I included it in the pull request.
Most of this code seems to be copied from the load method of the Mojolicious::Sessions package. They delete the expires key from the session, but this shouldn't. This could be cleaned up and changed to
| my $expiration = $adminSession->{expiration} // $sessions->default_expiration; | |
| my $expires = delete $adminSession->{expires}; | |
| return 0 if !$expires && $expiration || defined $expires && $expires <= time; | |
| return 0 | |
| if !$adminSession->{expires} && ($adminSession->{expiration} // $sessions->default_expiration) | |
| || defined $adminSession->{expires} && $adminSession->{expires} <= time; |
|
So as far as the other authentication modules go, here is my assessment.
So probably for now, you could just add comments in the documentation stating that this does not work with Saml2 and Shibbolith. Perhaps later, if there is demand, this could be extended to work for those. |
This is marked draft. Even though I'm targeting WeBWorK-2.21 right now, that is only so that the diff is clearly visible in GitHub. Later this will be re-targeted to develop, following the 2.21 release.
This (optional feature, off by default) makes it so that if you are using cookies for session management (not keys) and if you have a valid active session in the admin course, then that will smoothly grant you access into any other course. Some conditions are needed, of course:
create_and_delete_courses).The main feature here (from my perspective) is that you can click links in the admin course and just be granted a session in the course you clicked on. This even works if that other course only allows users to enter through an LMS. You can also just click any link to any course, like say one in a student help email, and gain a session cookie. And you won't need to type a password.
All of this still requires 2FA for the course you are entering, assuming 2FA is enabled for that course, for a user of your level. That's actually something I would prefer not to have to do if I'm already authenticated in the admin course. But that could be changed later if this PR is not too objectionable.
Technical note: just because your user in the admin course and user in some other course have the same password, they would still have different password hashes if passwords were set independently. This really only works if the user in the other course were added to that other course as an admin user at the time the other course was initialized.