diff --git a/lib/Mojolicious/WeBWorK.pm b/lib/Mojolicious/WeBWorK.pm index 6546073950..cdb3865b44 100644 --- a/lib/Mojolicious/WeBWorK.pm +++ b/lib/Mojolicious/WeBWorK.pm @@ -172,6 +172,7 @@ sub startup ($app) { $SIG{__WARN__} = $c->stash->{orig_sig_warn} if defined $c->stash->{orig_sig_warn}; if ($c->isa('WeBWorK::ContentGenerator') && $c->ce) { + $c->authen->store_session if $c->authen; writeTimingLogEntry( $c->ce, '[' . $c->url_for . ']', diff --git a/lib/WeBWorK.pm b/lib/WeBWorK.pm index d0756cf6fd..2c6c42ada2 100644 --- a/lib/WeBWorK.pm +++ b/lib/WeBWorK.pm @@ -240,7 +240,7 @@ async sub dispatch ($c) { # we need to also check on the proctor. Note that in the gateway quiz # module this is double checked to be sure that someone isn't taking a # proctored quiz but calling the unproctored ContentGenerator. - if ($c->current_route =~ /^proctored_gateway_quiz|proctored_gateway_proctor_login$/) { + if ($c->current_route =~ /^(proctored_gateway_quiz|proctored_gateway_proctor_login)$/) { my $proctor_authen_module = WeBWorK::Authen::class($ce, 'proctor_module'); runtime_use $proctor_authen_module; my $authenProctor = $proctor_authen_module->new($c); @@ -251,6 +251,10 @@ async sub dispatch ($c) { await WeBWorK::ContentGenerator::LoginProctor->new($c)->go; return 0; } + } else { + # If any other page is opened, then revoke proctor authorization if it has been granted. + # Otherwise the student will be able to re-enter the test without again obtaining proctor authorization. + delete $c->authen->session->{proctor_authorization_granted}; } return 1; } else { diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm index 299bba9e6c..403785b66e 100644 --- a/lib/WeBWorK/Authen.pm +++ b/lib/WeBWorK/Authen.pm @@ -329,7 +329,7 @@ sub get_credentials { } if (defined $cookieUser) { - $self->{user_id} = $cookieUser; + $self->{user_id} = trim($cookieUser); $self->{session_key} = $cookieKey; $self->{cookie_timestamp} = $cookieTimeStamp; $self->{login_type} = "normal"; @@ -470,7 +470,7 @@ sub maybe_send_cookie { return if $c->{rpc}; - my ($cookie_user, $cookie_key, $cookie_timestamp, $setID) = $self->fetchCookie; + my ($cookie_user, $cookie_key, $cookie_timestamp) = $self->fetchCookie; # Send a cookie if any of these conditions are met: @@ -501,7 +501,7 @@ sub maybe_send_cookie { ); if ($used_cookie || $unused_valid_cookie || $user_requests_cookie || $session_management_via_cookies) { - $self->sendCookie($self->{user_id}, $self->{session_key}, $setID); + $self->sendCookie($self->{user_id}, $self->{session_key}); } else { $self->killCookie; } @@ -613,35 +613,140 @@ sub unexpired_session_exists { return defined $Key && time <= $Key->timestamp + $self->{c}->ce->{sessionKeyTimeout}; } -# Clobbers any existing session for this $userID. -# A random key is generated, and that key is returned. -# When this is called in Proctor.pm, the actual user id is passed in via $trueUserID. -# The $userID is modified in that case and will not work in the hasPermissions call. +# Uses an existing session and session key if a key was found previously with a valid timestamp. Otherwise a random key +# is generated, and a new session and session key created. The key from the session is returned in any case. sub create_session { - my ($self, $userID, $trueUserID) = @_; + my ($self, $userID) = @_; my $c = $self->{c}; my $ce = $c->ce; my $db = $c->db; + my $newKey; + + if (!$c->stash->{'webwork2.database_session'} || !$c->stash->{'webwork2.database_session'}{user_id}) { + my @chars = @{ $ce->{sessionKeyChars} }; + srand; + $newKey = join('', @chars[ map rand(@chars), 1 .. $ce->{sessionKeyLength} ]); + $c->stash->{'webwork2.database_session'} = + { user_id => $userID, key => $newKey, timestamp => time, session => {} }; + } else { + $newKey = $c->stash->{'webwork2.database_session'}{key}; + } + + # If navigation is restricted, then set the set_id in the session. + $self->session(set_id => $c->stash->{setID}) + if $c->stash->{setID} && !$c->authz->hasPermissions($userID, 'navigation_allowed'); + + return $newKey; +} + +=head2 session + +This method can be used to get or set values in the session. Note that if +C is "session_cookie" then the Mojolicous cookie session +is used. If C is "key", then only the session in the +database is used. Note that database session is really a hash stored in +C<< $c->stash->{'webwork2.database_session} >> that has the following structure: + + { user_id => $userID, key => $key, timestamp => $timestamp, session => {} } + +Only keys in the C sub-hash can be set with this method. The +C, C, and C should be set directly in the +C hash. + +A single value from the session can be obtained as follows. + + $authen->session('key1'); - my @chars = @{ $ce->{sessionKeyChars} }; +Values can be set as in the following examples. - srand; - my $newKey = join('', @chars[ map rand(@chars), 1 .. $ce->{sessionKeyLength} ]); + $authen->session(key1 => 'value 1', key2 => 'value 2'); + $authen->session({ key1 => 'value 1', key2 => 'value 2' }); - my $setID = !$c->authz->hasPermissions($trueUserID // $userID, 'navigation_allowed') ? $c->stash('setID') : ''; +The entire session can be obtained as a hash reference as follows. - my $Key = $db->newKey(user_id => $userID, key => $newKey, timestamp => time, set_id => $setID); + my $session = $authen->session; - # DBFIXME this should be a REPLACE - eval { $db->deleteKey($userID) }; - eval { $db->addKey($Key) }; - if ($@) { - warn "Difficulty adding key for userID $userID: $@"; - eval { $db->putKey($Key) }; - warn "Couldn't put key for userid $userID either: $@" if $@; +=cut + +sub session { + my ($self, @params) = @_; + my $c = $self->{c}; + + # If session_management_via is not "session_cookie" (so should be "key"), then use the database session. + if ($c->ce->{session_management_via} ne 'session_cookie') { + $c->stash->{'webwork2.database_session'} //= { user_id => $self->{user_id}, session => {} }; + my $session = $c->stash->{'webwork2.database_session'}{session}; + + # Note that the return values are the same as those returned by the + # Mojolicious::Controller::session method in the following cases. + + # Return the session hash. + return $session unless @params; + + # Get session values. + return $session->{ $params[0] } unless @params > 1 || ref $params[0]; + + # Set session values. + my $values = ref $params[0] ? $params[0] : {@params}; + @$session{ keys %$values } = values %$values; + + return $c; } - return $newKey; + # If session_management_via is "session_cookie", then use the Mojolicious cookie session. + return $c->session(@params); +} + +=head2 store_session + +Store the database session. This is called after the current request has been +dispatched (in the C hook). This allows database session values +to be set or modified at any point before that is done. + +=cut + +sub store_session { + my $self = shift; + my $db = $self->{c}->db; + + if (my $session = $self->{c}->stash->{'webwork2.database_session'}) { + if ($WeBWorK::Debug::Enabled) { + debug('Saving database session. The database session contains'); + debug("\t$_ => $session->{$_}") for grep { $_ ne 'session' } keys %$session; + debug("\tsession => {"); + debug("\t\t$_ => $session->{session}{$_}") for keys %{ $session->{session} }; + debug("\t}"); + } + + my $key = $db->newKey($session); + # DBFIXME: This should be a REPLACE (but SQL::Abstract does not have REPLACE -- SQL::Abstract::mysql does!). + eval { $db->deleteKey($session->{user_id}) }; + eval { $db->addKey($key) }; + if ($@) { + warn "Difficulty adding key for userID $session->{user_id}: $@"; + eval { $db->putKey($key) }; + warn "Couldn't put key for userid $session->{user_id} either: $@" if $@; + } + } elsif ($self->{user_id}) { + debug('Deleting database session.'); + eval { $db->deleteKey($self->{user_id}) }; + } + + if ($WeBWorK::Debug::Enabled) { + # The cookie will actually be sent by the next line of the Mojolcious::Controller::rendered method after the + # after_dispatch hook in which this method is called. + my $cookieSession = $self->{c}->session; + if (keys %$cookieSession) { + if ($cookieSession->{expires} && $cookieSession->{expires} == 1) { + debug('The cookie session is expired.'); + } else { + debug('The cookie session contains'); + debug("\t$_ => $cookieSession->{$_}") for keys %$cookieSession; + } + } + } + + return; } =head2 check_session @@ -667,11 +772,16 @@ sub check_session { my $keyMatches = defined $possibleKey && $possibleKey eq $Key->key; - my $timestampValid = time <= $Key->timestamp + $ce->{sessionKeyTimeout}; + my $currentTime = time; + + my $timestampValid = + $ce->{session_management_via} eq 'session_cookie' && defined $self->{cookie_timestamp} + ? $currentTime <= $self->{cookie_timestamp} + $ce->{sessionKeyTimeout} + : $currentTime <= $Key->timestamp + $ce->{sessionKeyTimeout}; if ($keyMatches && $timestampValid && $updateTimestamp) { - $Key->timestamp(time); - $db->putKey($Key); + $Key->timestamp($currentTime); + $self->{c}->stash->{'webwork2.database_session'} = { $Key->toHash } if $keyMatches && $timestampValid; } return (1, $keyMatches, $timestampValid); @@ -696,8 +806,8 @@ sub killSession { } $self->forget_verification; - $self->killCookie if $ce->{session_management_via} eq 'session_cookie'; - $c->{db}->deleteKey($self->{user_id}) if defined $self->{user_id}; + $self->killCookie; + delete $c->stash->{'webwork2.database_session'}; return; } @@ -705,7 +815,7 @@ sub killSession { # Cookie management # Note that this does not really "fetch" the session cookie. It just gets -# the user_id, key, timestamp, and set_id from the cookie. +# the user_id, key, and timestamp from the session cookie. sub fetchCookie { my $self = shift; my $c = $self->{c}; @@ -716,11 +826,10 @@ sub fetchCookie { my $userID = $c->session->{user_id}; my $key = $c->session->{key}; my $timestamp = $c->session->{timestamp}; - my $setID = $c->session->{set_id}; if ($userID && $key) { debug(qq{fetchCookie: Returning userID="$userID", key="$key", timestamp="}, $timestamp, '"'); - return ($userID, $key, $timestamp, $setID); + return ($userID, $key, $timestamp); } else { debug('fetchCookie: Session cookie does not contain valid information. Returning nothing.'); return; @@ -730,7 +839,7 @@ sub fetchCookie { # Note that this does not actually "send" the cookie. It merely sets the default session values in the cookie. # The session cookie is actually sent by Mojolicious when the response is rendered. sub sendCookie { - my ($self, $userID, $key, $setID) = @_; + my ($self, $userID, $key) = @_; my $c = $self->{c}; my $ce = $c->ce; @@ -738,15 +847,10 @@ sub sendCookie { my $courseID = $c->stash('courseID'); - # This sets the setID in the cookie on initial login. - $setID = $c->stash('setID') - if !$setID && $c->authen->was_verified && !$c->authz->hasPermissions($userID, 'navigation_allowed'); - $c->session( user_id => $userID, key => $key, timestamp => time, - $setID ? (set_id => $setID) : (), # Set how long the browser should retain the cookie. expiration => $ce->{CookieLifeTime} eq 'session' ? 0 : $ce->{CookieLifeTime} ); @@ -760,20 +864,6 @@ sub killCookie { return; } -# This method is only used for a user that does not have the navigation_allowed permission, -# and is used to restrict that user to a specific set that the user is authenticated with. -sub get_session_set_id { - my $self = shift; - my $setID; - - if ($self->{c}->ce->{session_management_via} eq 'key') { - my $Key = $self->{c}->db->getKey($self->{c}->param('user')); - return $Key->set_id; - } else { - return $self->{c}->session->{set_id}; - } -} - # Utilities sub write_log_entry { diff --git a/lib/WeBWorK/Authen/LTIAdvanced.pm b/lib/WeBWorK/Authen/LTIAdvanced.pm index e06280b3be..3f7a1f91ab 100644 --- a/lib/WeBWorK/Authen/LTIAdvanced.pm +++ b/lib/WeBWorK/Authen/LTIAdvanced.pm @@ -92,10 +92,9 @@ sub request_has_data_for_this_verification_module { my $self = shift; my $c = $self->{c}; - # See comment in get_credentials() if ($c->{rpc}) { - debug("LTIAdvanced returning 1 because it is an rpc call"); - return 1; + debug("LTIAdvanced returning 0 because it is an rpc call"); + return 0; } # This module insists that the course is configured for LTI 1.3. @@ -125,19 +124,6 @@ sub get_credentials { debug("LTIAdvanced::get_credentials has been called\n"); - # This next part is necessary because some parts of webwork (e.g., - # WebworkWebservice.pm) need to replace the get_credentials() routine, - # but only replace the one in the parent class (out of caution, - # presumably). Therefore, we end up here even when authenticating - # for WebworkWebservice.pm. This would cause authentication failures - # when authenticating javascript web service requests (e.g., the - # Library Browser). - # Similar changes are needed in check_user() and verify_normal_user(). - if ($c->{rpc}) { - debug("falling back to superclass get_credentials (rpc call)"); - return $self->SUPER::get_credentials(@_); - } - ## Printing parameters to main page can help people set things up ## so we dont use the debug channel here if ($ce->{debug_lti_parameters}) { @@ -307,12 +293,6 @@ sub check_user { debug("LTIAdvanced::check_user has been called for user_id = |$user_id|"); - # See comment in get_credentials() - if ($c->{rpc}) { - #debug("falling back to superclass check_user (rpc call)"); - return $self->SUPER::check_user(@_); - } - if (!defined($user_id) || (defined $user_id && $user_id eq "")) { $self->{log_error} .= "no user id specified"; $self->{error} = $c->maketext( @@ -381,17 +361,6 @@ sub verify_normal_user { debug("LTIAdvanced::verify_normal_user called for user |$user_id|"); - # See comment in get_credentials() - if ($c->{rpc}) { - #debug("falling back to superclass verify_normal_user (rpc call)"); - return $self->SUPER::verify_normal_user(@_); - } - - # Call check_session in order to destroy any existing session cookies and Key table sessions - my ($sessionExists, $keyMatches, $timestampValid) = $self->check_session($user_id, $session_key, 0); - - debug("sessionExists='", $sessionExists, "' keyMatches='", $keyMatches, "' timestampValid='", $timestampValid, "'"); - my $auth_result = $self->authenticate; debug("auth_result=|${auth_result}|"); @@ -420,12 +389,6 @@ sub authenticate { my $self = shift; my ($c, $user) = map { $self->{$_}; } ('c', 'user_id'); - # See comment in get_credentials() - if ($c->{rpc}) { - #debug("falling back to superclass authenticate (rpc call)"); - return $self->SUPER::authenticate(@_); - } - debug("LTIAdvanced::authenticate called for user |$user|"); debug "ref(c) = |" . ref($c) . "|"; debug "ref of c->{paramcache} = |" . ref($c->{paramcache}) . "|"; diff --git a/lib/WeBWorK/Authen/LTIAdvantage.pm b/lib/WeBWorK/Authen/LTIAdvantage.pm index 49ad069138..04eaac4bd5 100644 --- a/lib/WeBWorK/Authen/LTIAdvantage.pm +++ b/lib/WeBWorK/Authen/LTIAdvantage.pm @@ -116,15 +116,13 @@ sub verify ($self) { # # To save this to the database a user_id value is needed that is unique for this request, satisfies the database # constrains on user_id's, and won't collide with webwork user_id's. So the login_hint (the LMS user id) and - # courseID are joined with ',set_id:' (hacking into the existing login proctor hack -- what an ugly hack to - # begin with). That means the LMS user id will also be needed to get the information back from the database, - # and so this is included in the state as well. + # courseID are joined with ',set_id:'. That means the LMS user id will also be needed to get the information + # back from the database, and so this is included in the state as well. # # To make matters worse, courseID's can contain hyphens, but user_id's can not. Fortunately courseID's can not # contain ampersats, while user_id's can. So the hyphens in the courseID are replaced with ampersats. # - # Finally, hack into the existing "nonce" key hack. The actual state and nonce are joined with a tab - # character and saved in the set_id field. Since the key value is "nonce", the database will not check + # Finally, hack into the existing "nonce" key hack. Since the key value is "nonce", the database will not check # to see that the user_id exists in the user table. my $key_id = join(',set_id:', $c->param('login_hint'), $c->stash->{courseID} =~ s/-/@/gr); @@ -137,7 +135,7 @@ sub verify ($self) { user_id => $key_id, key => 'nonce', timestamp => time, - set_id => join("\t", $c->stash->{LTIState}, $c->stash->{LTINonce}) + session => { lti_state => $c->stash->{LTIState}, lti_nonce => $c->stash->{LTINonce} } ); $c->db->addKey($key); @@ -161,7 +159,7 @@ sub get_credentials ($self) { # See the comments about the hacks involved here in the WeBWorK::Authen::LTIAdvantage::verify method above. my $key_id = join(',set_id:', $c->stash->{lti_lms_user_id}, $c->stash->{courseID} =~ s/-/@/gr); my $key = $c->db->getKey($key_id); - ($c->stash->{LTIState}, $c->stash->{LTINonce}) = split "\t", $key->set_id; + ($c->stash->{LTIState}, $c->stash->{LTINonce}) = ($key->session->{lti_state}, $key->session->{lti_nonce}); $c->db->deleteKey($key_id); $self->purge_old_state_keys; @@ -196,7 +194,6 @@ sub get_credentials ($self) { if (!defined $claims->{'https://purl.imsglobal.org/spec/lti/claim/deployment_id'} || $claims->{'https://purl.imsglobal.org/spec/lti/claim/deployment_id'} ne $ce->{LTI}{v1p3}{DeploymentID}) { - $c->log->info($claims->{'https://purl.imsglobal.org/spec/lti/claim/deployment_id'}); $self->{error} = $c->maketext( 'There was an error during the login process. Please speak to your instructor or system administrator.'); warn "Incorrect deployment id received in response.\n" if $ce->{debug_lti_parameters}; @@ -414,7 +411,7 @@ sub purge_old_state_keys ($self) { my $db = $c->db; my $time = time; - my @userIDs = $db->listKeys(); + my @userIDs = $db->listKeys; my @keys = $db->getKeys(@userIDs); my $modCourseID = $ce->{courseName} =~ s/-/@/gr; @@ -479,10 +476,6 @@ sub verify_normal_user ($self) { debug("LTIAdvantage::verify_normal_user called for user |$user_id|"); - # Call check_session in order to destroy any existing session cookies and key table sessions. - my ($sessionExists, $keyMatches, $timestampValid) = $self->check_session($user_id, $session_key, 0); - debug('sessionExists="', $sessionExists, '" keyMatches="', $keyMatches, '" timestampValid="', $timestampValid, '"'); - my $auth_result = $self->authenticate; debug("auth_result=|${auth_result}|"); diff --git a/lib/WeBWorK/Authen/Proctor.pm b/lib/WeBWorK/Authen/Proctor.pm index e9952e99aa..56ccc0df35 100644 --- a/lib/WeBWorK/Authen/Proctor.pm +++ b/lib/WeBWorK/Authen/Proctor.pm @@ -14,7 +14,7 @@ ################################################################################ package WeBWorK::Authen::Proctor; -use base qw/WeBWorK::Authen/; +use base 'WeBWorK::Authen'; =head1 NAME @@ -24,10 +24,15 @@ WeBWorK::Authen::Proctor - Authenticate gateway test proctors. use strict; use warnings; -use WeBWorK::Debug; + +use WeBWorK::Utils qw(x); use WeBWorK::DB::Utils qw(grok_vsetID); -use constant GENERIC_ERROR_MESSAGE => 'Invalid user ID or password.'; +use constant GENERIC_ERROR_MESSAGE => x('Invalid user ID or password.'); + +# Note that throughout this module only parameters in the request body_params are accepted (other than the +# effectiveUser). This means that only parameters for a POST request are allowed. GET request parameters are ignored. +# This is a security measure as it is more difficult to engineer a fake POST request than a GET request. sub verify { my $self = shift; @@ -36,7 +41,7 @@ sub verify { # At this point the usual authentication has already occurred and the user has been verified. If the # use_grade_auth_proctor option is set to 'No', then proctor authorization is not not needed. So return # 1 here to skip proctor authorization and proceed on to the GatewayQuiz module which will grade the test. - if ($c->param('submitAnswers')) { + if ($c->req->body_params->param('submitAnswers')) { my ($setName, $versionNum) = grok_vsetID($c->stash('setID')); my $userSet = $c->db->getMergedSetVersion($c->param('effectiveUser'), $setName, $versionNum); return 1 if $userSet && $userSet->use_grade_auth_proctor eq 'No'; @@ -45,162 +50,157 @@ sub verify { return $self->SUPER::verify(@_); } -# this is similar to the method in the base class, with these differences: +# This is similar to the method in the base class, with these differences: # 1. no guest logins # 2. no cookie -# 3. user_id/session_key/password come from params proctor_user/proctor_key/proctor_passwd +# 3. no session key +# 4. user_id/password come from POST request params proctor_user/proctor_passwd sub get_credentials { my ($self) = @_; - my $c = $self->{c}; - my $ce = $c->ce; - my $db = $c->db; + my $c = $self->{c}; my ($set_id, $version_id) = grok_vsetID($c->stash('setID')); - # at least the user ID is available in request parameters - if (defined $c->param('proctor_user')) { - my $student_user_id = $c->param('effectiveUser'); - $self->{user_id} = $c->param('proctor_user'); - if ($self->{user_id} eq $set_id) { - $self->{user_id} = "set_id:$set_id"; - } - $self->{session_key} = $c->param('proctor_key'); - $self->{password} = $c->param('proctor_passwd'); - $self->{login_type} = - $c->param('submitAnswers') ? "proctor_grading:$student_user_id" : "proctor_login:$student_user_id"; + if (defined $c->req->body_params->param('proctor_user')) { + $self->{user_id} = $c->req->body_params->param('proctor_user'); + $self->{user_id} = "set_id:$set_id" if $self->{user_id} eq $set_id; + $self->{password} = $c->req->body_params->param('proctor_passwd'); + $self->{login_type} = $c->req->body_params->param('submitAnswers') ? 'proctor_grading' : 'proctor_login'; $self->{credential_source} = 'params'; return 1; + } elsif ($c->authen->session('proctor_authorization_granted') && !$c->req->body_params->param('submitAnswers')) { + $self->{login_type} = 'proctor_login'; + $self->{credential_source} = 'session'; + return 1; } + + return 0; } -# duplicates method in superclass, adding additional check for permission -# to proctor quizzes +# If proctor authorization is granted a proctor user is not needed. So skip checking the user. sub check_user { - my $self = shift; - my $c = $self->{c}; - my $ce = $c->ce; - my $db = $c->db; - my $authz = $c->authz; - - my $submitAnswers = $c->param('submitAnswers'); - my $user_id = $self->{user_id}; - my $past_proctor_id = $c->param('past_proctor_user') || $user_id; - - # for set-level authentication we prepended "set_id:" - my $show_user_id = $user_id; - $show_user_id =~ s/^set_id://; - - if (defined $user_id and ($user_id eq '' || $show_user_id eq '')) { - $self->{log_error} = 'no user id specified'; - $self->{error} = 'You must specify a user ID.'; - return 0; - } + my $self = shift; + return 1 if $self->{credential_source} eq 'session'; + return $self->SUPER::check_user; +} - my $User = $db->getUser($user_id); +# This is similar to the method in the base class except that instead of creating a session, this just sets the +# "proctor_authorization_granted" value in the session. Note that the session used is the session of the original +# authentication module for this request. Furthermore it checks the proctor user permissions instead of the usual user +# login permissions. +sub verify_normal_user { + my $self = shift; + my $c = $self->{c}; - unless ($User) { - $self->{log_error} = 'user unknown'; - $self->{error} = GENERIC_ERROR_MESSAGE; - return 0; - } + # If the test is being submitted, then proctor credentials are always required. Note that if use_grade_auth_proctor + # is 'No', then the verify method will have returned 1, and this never happens. For an ongoing login session, only + # a key with versioned set information is accepted, and that version must match the requested set version. The set + # id will not have a version when opening a new version. For that new proctor credentials are required. + if ($self->{login_type} eq 'proctor_login' + && $c->stash('setID') =~ /,v\d+$/ + && $c->authen->session('proctor_authorization_granted') + && $c->authen->session('proctor_authorization_granted') eq $c->stash('setID')) + { + return 1; + } else { + my $auth_result = $self->authenticate; - # proctors may be tas, instructors, or proctors; if the last, they - # do not have the behavior course_access, so we don't bother to - # check that here. they must, however, be able to login, which - # it seems to me is an overlap between course permissions and - # course status behaviors. + if ($auth_result > 0) { + my $db = $c->db; + my $authz = $c->authz; - unless ($authz->hasPermissions($user_id, 'login')) { - $self->{log_error} = 'user not permitted to login'; - $self->{error} = GENERIC_ERROR_MESSAGE; - return 0; - } + my $user_id = $self->{user_id}; - if ($submitAnswers) { - unless ($authz->hasPermissions($user_id, 'proctor_quiz_grade')) { - # only set the error if this proctor is different - # than the past proctor, implying that we have - # tried to grade with a new proctor id - if ($past_proctor_id ne $user_id) { - $self->{log_error} = 'user not permitted to proctor quiz grading.'; - $self->{error} = - "User $show_user_id is not authorized to proctor test grade submissions in this course."; + # Prepended "set_id:" for set-level authentication. + my $show_user_id = $user_id; + $show_user_id =~ s/^set_id://; + + # A proctor user may have the Proctor status which does not have the course_access behavior. + # So don't check that here. However, the user must be able to login. + unless ($authz->hasPermissions($user_id, 'login')) { + $self->{log_error} = 'user not permitted to login'; + $self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE); + return 0; } - return 0; - } - } else { - # Need a UserSet to determine if it is configured to skip grade proctor - # authorization to grade the quiz. Require a grade proctor permission level - # to start a quiz that skips authorization to grade it. This ensures that - # a grade proctor level of authorization is always required. - my ($setName, $versionNum) = grok_vsetID($c->stash('setID')); - my $userSet = $db->getMergedSet($c->param('effectiveUser'), $setName); - unless ( - $authz->hasPermissions($user_id, 'proctor_quiz_grade') - || (($userSet->use_grade_auth_proctor eq 'Yes' || $userSet->restricted_login_proctor eq 'Yes') - && $authz->hasPermissions($user_id, 'proctor_quiz_login')) - ) - { - # Set the error based on if a single set password was required, a grade - # grade proctor was required to start, or a login proctor was required. - if ($userSet->restricted_login_proctor eq 'Yes') { - $self->{log_error} = 'invalid set password to start quiz.'; - $self->{error} = 'This quiz requires a set password to start, and the password was invalid.'; - } elsif ($userSet->use_grade_auth_proctor ne 'Yes') { - $self->{log_error} = - 'grade proctor required to login and user is not permitted to proctor quiz grading.'; - $self->{error} = "This quiz requires a grade proctor to start, and user $show_user_id is not " - . 'authorized to proctor test grade submissions in this course.'; + # As mentioned above the setID will not have the set version number if a new version is being opened. That + # will be added to the proctor session key by the GatewayQuiz module later. + if ($self->{login_type} eq 'proctor_grading') { + unless ($authz->hasPermissions($user_id, 'proctor_quiz_grade')) { + $self->{log_error} = 'user not permitted to proctor test grade submissions'; + $self->{error} = + $c->maketext('User [_1] is not authorized to proctor test grade submissions in this course.', + $show_user_id); + return 0; + } + $c->authen->session('proctor_authorization_granted' => $c->stash('setID')); } else { - $self->{log_error} = 'user not permitted to proctor quiz logins.'; - $self->{error} = "User $show_user_id is not authorized to proctor test logins in this course."; + # A UserSet is needed to determine if it is configured to skip grade proctor authorization. Require a + # grade_proctor permission level to start a quiz that skips authorization to grade it. This ensures that + # a grade proctor level of authorization is always required. + my ($setName, $versionNum) = grok_vsetID($c->stash('setID')); + my $userSet = $db->getMergedSet($c->param('effectiveUser'), $setName); + unless ( + $authz->hasPermissions($user_id, 'proctor_quiz_grade') + || (($userSet->use_grade_auth_proctor eq 'Yes' || $userSet->restricted_login_proctor eq 'Yes') + && $authz->hasPermissions($user_id, 'proctor_quiz_login')) + ) + { + # Set the error based on if a single set password was required, a grade + # proctor was required to start, or a login proctor was required. + if ($userSet->restricted_login_proctor eq 'Yes') { + $self->{log_error} = 'invalid set password to start quiz.'; + $self->{error} = + $c->maketext('This quiz requires a set password to start, and the password was invalid.'); + } elsif ($userSet->use_grade_auth_proctor ne 'Yes') { + $self->{log_error} = + 'grade proctor required to login and user is not permitted to proctor quiz grading.'; + $self->{error} = $c->maketext( + 'This quiz requires a grade proctor to start, and user [_1] is ' + . 'not authorized to proctor test grade submissions in this course.', + $show_user_id + ); + } else { + $self->{log_error} = 'user not permitted to proctor quiz logins.'; + $self->{error} = + $c->maketext("User [_1] is not authorized to proctor test logins in this course.", + $show_user_id); + } + return 0; + } + $c->authen->session('proctor_authorization_granted' => $c->stash('setID')); + } + return 1; + } else { + delete $c->authen->session->{'proctor_authorization_granted'}; + if ($auth_result == 0) { + $self->{log_error} = "authentication failed"; + $self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE); } return 0; } } } -# this is similar to the method in the base class, excpet that the parameters -# proctor_user, proctor_key, and proctor_passwd are used +# This is similar to the method in the base class, except that the parameters +# proctor_user and proctor_passwd are used and there is no session_key. sub set_params { my $self = shift; my $c = $self->{c}; $c->param('proctor_user', $self->{user_id}); - $c->param('proctor_key', $self->{session_key}); $c->param('proctor_passwd', ''); -} - -# rewrite the userID to include both the proctor's and the student's user ID -# and then call the default create_session method. -sub create_session { - my ($self, $userID, $newKey) = @_; - return $self->SUPER::create_session($self->proctor_key_id($userID), $newKey, $userID); + return; } -# rewrite the userID to include both the proctor's and the student's user ID -# and then call the default check_session method. -sub check_session { - my ($self, $userID, $possibleKey, $updateTimestamp) = @_; - - return $self->SUPER::check_session($self->proctor_key_id($userID), $possibleKey, $updateTimestamp); -} - -# proctor key ID rewriting helper -sub proctor_key_id { - my ($self, $userID, $newKey) = @_; - my $c = $self->{c}; - - my $proctor_key_id = $c->param('effectiveUser') . ',' . $userID; - $proctor_key_id .= ',g' if $self->{login_type} =~ /^proctor_grading/; - - return $proctor_key_id; -} +# Disable the session for proctors (instead use the session of the user authentication module). +sub create_session { } +sub check_session { } -# disable cookie functionality for proctors +# Disable cookie functionality for proctors. sub maybe_send_cookie { } +sub maybe_kill_cookie { } sub fetchCookie { } sub sendCookie { } sub killCookie { } diff --git a/lib/WeBWorK/ContentGenerator.pm b/lib/WeBWorK/ContentGenerator.pm index 6d5b3be00e..22254e641d 100644 --- a/lib/WeBWorK/ContentGenerator.pm +++ b/lib/WeBWorK/ContentGenerator.pm @@ -530,8 +530,8 @@ sub links ($c) { my $restricted_navigation = $authen->was_verified && !$authz->hasPermissions($userID, 'navigation_allowed'); # If navigation is restricted and the setID was not in the route stash, - # then get the setID this user is restricted to view from the authen cookie. - $setID = $authen->get_session_set_id if (!$setID && $restricted_navigation); + # then get the setID this user is restricted to view from the session. + $setID = $authen->session('set_id') if !$setID && $restricted_navigation; my $prettyProblemID = $problemID; @@ -1089,21 +1089,6 @@ sub hidden_authen_fields ($c, $id_prefix = undef) { return $c->hidden_fields('user', 'effectiveUser', 'key', 'theme'); } -=item hidden_proctor_authen_fields() - -Use hidden_fields to return hidden tags for request fields used in -proctor authentication. - -=cut - -sub hidden_proctor_authen_fields ($c) { - if ($c->param('proctor_user')) { - return $c->hidden_fields('proctor_user', 'proctor_key'); - } else { - return ''; - } -} - =item url_args(@fields) Return a hash containing values for each field mentioned in @fields, or all diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm index aa91832b46..6627f02c5f 100644 --- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm +++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm @@ -392,7 +392,7 @@ async sub pre_header_initialize ($c) { $tmplSet = $db->getMergedSet($effectiveUserID, $setID); # Now that is has been validated that this is a gateway test, save the assignment test for the processing of - # proctor keys for graded proctored tests. If a set was not obtained from the database, store a fake value here + # graded proctored tests. If a set was not obtained from the database, store a fake value here # to be able to continue. $c->{assignment_type} = $tmplSet->assignment_type || 'gateway'; @@ -500,7 +500,8 @@ async sub pre_header_initialize ($c) { my @setVersions = $db->getSetVersions(map { [ $effectiveUserID, $setID,, $_ ] } @setVersionIDs); for (@setVersions) { $totalNumVersions++; - $currentNumVersions++ if (!$timeInterval || $_->version_creation_time() > ($c->submitTime - $timeInterval)); + $currentNumVersions++ + if (!$timeInterval || $_->version_creation_time() > ($c->submitTime - $timeInterval)); } } @@ -644,14 +645,19 @@ async sub pre_header_initialize ($c) { $c->{invalidSet} = 'This set is closed. No new set versions may be taken.'; } - # If the set or problem is invalid, then delete any proctor keys if any and return. + # If the proctor session key does not have a set version id, then add it. This occurs when a student + # initialy enters a proctored test, since the version id is not determined until just above. + if ($c->authen->session('proctor_authorization_granted') + && $c->authen->session('proctor_authorization_granted') !~ /,v\d+$/) + { + if ($setVersionNumber) { $c->authen->session(proctor_authorization_granted => "$setID,v$setVersionNumber"); } + else { delete $c->authen->session->{proctor_authorization_granted}; } + } + + # If the set or problem is invalid, then delete any proctor session keys and return. if ($c->{invalidSet} || $c->{invalidProblem}) { if (defined $c->{assignment_type} && $c->{assignment_type} eq 'proctored_gateway') { - my $proctorID = $c->param('proctor_user'); - if ($proctorID) { - eval { $db->deleteKey("$effectiveUserID,$proctorID"); }; - eval { $db->deleteKey("$effectiveUserID,$proctorID,g"); }; - } + delete $c->authen->session->{proctor_authorization_granted}; } return; } @@ -878,32 +884,13 @@ async sub pre_header_initialize ($c) { # and answers can be recorded, then save the last answer for future reference. # Also save the persistent data to the database even when the last answer is not saved. - # First, deal with answers being submitted for a proctored exam. Delete the proctor keys that authorized the - # grading, so that it isn't possible to log in and take another proctored test without being reauthorized. - if ($c->{submitAnswers} && $c->{assignment_type} eq 'proctored_gateway') { - my $proctorID = $c->param('proctor_user'); - - # If there are no attempts left, delete all proctor keys for this user. - if ($set->attempts_per_version > 0 - && $set->attempts_per_version - 1 - $problem->num_correct - $problem->num_incorrect <= 0) - { - eval { $db->deleteAllProctorKeys($effectiveUserID); }; - } else { - # Otherwise, delete only the grading key. - eval { $db->deleteKey("$effectiveUserID,$proctorID,g"); }; - # In this case there may be a past login proctor key that can be kept so that another login to continue - # working the test is not needed. - if ($c->param('past_proctor_user') && $c->param('past_proctor_key')) { - $c->param('proctor_user', scalar $c->param('past_proctor_user')); - $c->param('proctor_key', scalar $c->param('past_proctor_key')); - } - } - # This is unsubtle, but we'd rather not have bogus keys sitting around. - if ($@) { - die "ERROR RESETTING PROCTOR GRADING KEY(S): $@\n"; - } - - } + # Deal with answers being submitted for a proctored exam. If there are no attempts left, then delete the + # proctor session key so that it isn't possible to start another proctored test without being reauthorized. + delete $c->authen->session->{proctor_authorization_granted} + if ($c->{submitAnswers} + && $c->{assignment_type} eq 'proctored_gateway' + && $set->attempts_per_version > 0 + && $set->attempts_per_version - 1 - $problem->num_correct - $problem->num_incorrect <= 0); my @pureProblems = $db->getAllProblemVersions($effectiveUserID, $setID, $versionID); for my $i (0 .. $#problems) { @@ -1204,7 +1191,8 @@ async sub pre_header_initialize ($c) { $c->{submitAnswers} && ( $will{recordAnswers} - || (!$set->version_last_attempt_time && $c->submitTime > $set->due_date + $ce->{gatewayGracePeriod}) + || (!$set->version_last_attempt_time + && $c->submitTime > $set->due_date + $ce->{gatewayGracePeriod}) ) ) || ( @@ -1228,7 +1216,8 @@ async sub pre_header_initialize ($c) { $c->{submitAnswers} && ( $will{recordAnswers} - || (!$set->version_last_attempt_time && $c->submitTime > $set->due_date + $ce->{gatewayGracePeriod}) + || (!$set->version_last_attempt_time + && $c->submitTime > $set->due_date + $ce->{gatewayGracePeriod}) ) ); @@ -1280,7 +1269,8 @@ async sub pre_header_initialize ($c) { my $pScore = 0; if (ref $pg) { # If a pg object is available, then use the pg recorded score and save it in the @probStatus array. - $pScore = compute_reduced_score($ce, $problems[$i], $set, $pg->{state}{recorded_score}, $c->submitTime); + $pScore = + compute_reduced_score($ce, $problems[$i], $set, $pg->{state}{recorded_score}, $c->submitTime); $probStatus[$i] = $pScore if $pScore > $probStatus[$i]; } else { # If a pg object is not available, then use the saved problem status. diff --git a/lib/WeBWorK/DB.pm b/lib/WeBWorK/DB.pm index 4d59f2504c..02fa05f33a 100644 --- a/lib/WeBWorK/DB.pm +++ b/lib/WeBWorK/DB.pm @@ -99,6 +99,7 @@ use Carp; use Data::Dumper; use Scalar::Util qw/blessed/; use HTML::Entities qw( encode_entities ); +use Mojo::JSON qw(encode_json decode_json); use WeBWorK::DB::Schema; use WeBWorK::DB::Utils qw/make_vsetID grok_vsetID grok_setID_from_vsetID_sql @@ -774,11 +775,18 @@ sub deletePermissionLevel { BEGIN { *Key = gen_schema_accessor("key"); - *newKey = gen_new("key"); *countKeysWhere = gen_count_where("key"); *existsKeyWhere = gen_exists_where("key"); *listKeysWhere = gen_list_where("key"); - *getKeysWhere = gen_get_records_where("key"); + # FIXME: getKeysWhere is never used, but if it is used the "session" in the returned keys is not JSON decoded. + *getKeysWhere = gen_get_records_where("key"); +} + +sub newKey { + my ($self, @values) = @_; + my $key = $self->{key}{record}->new(@values); + $key->session({}) unless ref($key->session) eq 'HASH'; + return $key; } sub countKeys { return scalar shift->listKeys(@_) } @@ -799,50 +807,41 @@ sub existsKey { sub getKey { my ($self, $userID) = shift->checkArgs(\@_, qw/user_id/); - return ($self->getKeys($userID))[0]; + my ($key) = $self->{key}->gets([$userID]); + $key->session(decode_json($key->session)) if $key; + return $key; } sub getKeys { my ($self, @userIDs) = shift->checkArgs(\@_, qw/user_id*/); - return $self->{key}->gets(map { [$_] } @userIDs); + my @keys = $self->{key}->gets(map { [$_] } @userIDs); + $_->session(decode_json($_->session)) for @keys; + return @keys; } sub addKey { - # PROCTORING - allow comma in keyfields my ($self, $Key) = shift->checkArgs(\@_, qw/VREC:key/); - # PROCTORING - check for both user and proctor - # we allow for two entries for proctor keys, one of the form - # userid,proctorid (which authorizes login), and the other - # of the form userid,proctorid,g (which authorizes grading) - # (having two of these means that a proctored test will require - # authorization for both login and grading). - if ($Key->user_id =~ /([^,]+)(?:,([^,]*))?(,g)?/) { - my ($userID, $proctorID) = ($1, $2); - croak "addKey: user $userID not found" - # unless $self->{user}->exists($userID); - unless $Key->key eq "nonce" or $self->{user}->exists($userID); - croak "addKey: proctor $proctorID not found" - # unless $self->{user}->exists($proctorID); - unless $Key->key eq "nonce" or $self->{user}->exists($proctorID); - } else { - croak "addKey: user ", $Key->user_id, " not found" - # unless $self->{user}->exists($Key->user_id); - unless $Key->key eq "nonce" or $self->{user}->exists($Key->user_id); - } + croak "addKey: user ", $Key->user_id, " not found" + unless $Key->key eq "nonce" || $self->{user}->exists($Key->user_id); + + my $keyCopy = $self->newKey($Key); + $keyCopy->session(encode_json($Key->session)) if ref($Key->session) eq 'HASH'; - eval { return $self->{key}->add($Key); }; + my $result = eval { $self->{key}->add($keyCopy) }; if (my $ex = caught WeBWorK::DB::Ex::RecordExists) { croak "addKey: key exists (perhaps you meant to use putKey?)"; } elsif ($@) { die $@; } + return $result; } sub putKey { - # PROCTORING - allow comma in keyfields my ($self, $Key) = shift->checkArgs(\@_, qw/VREC:key/); - my $rows = $self->{key}->put($Key); # DBI returns 0E0 for 0. + my $keyCopy = $self->newKey($Key); + $keyCopy->session(encode_json($Key->session)) if ref($Key->session) eq 'HASH'; + my $rows = $self->{key}->put($keyCopy); # DBI returns 0E0 for 0. if ($rows == 0) { croak "putKey: key not found (perhaps you meant to use addKey?)"; } else { @@ -855,13 +854,6 @@ sub deleteKey { return $self->{key}->delete($userID); } -sub deleteAllProctorKeys { - my ($self, $userID) = shift->checkArgs(\@_, qw/user_id/); - my $where = [ user_id_like => "$userID,%" ]; - - return $self->{key}->delete_where($where); -} - ################################################################################ # setting functions ################################################################################ @@ -2361,11 +2353,10 @@ sub check_user_id { # (valid characters are [-a-zA-Z0-9_.,@]) return 0; } } -# the (optional) second argument to checkKeyfields is to support versioned -# (gateway) sets, which may include commas in certain fields (in particular, -# set names (e.g., setDerivativeGateway,v1) and user names (e.g., -# username,proctorname) +# The (optional) second argument to checkKeyfields is to support versioned +# (gateway) sets, which may include commas in certain fields (in particular, +# set names (e.g., setDerivativeGateway,v1) sub checkKeyfields($;$) { my ($Record, $versioned) = @_; foreach my $keyfield ($Record->KEYFIELDS) { diff --git a/lib/WeBWorK/DB/Record/Key.pm b/lib/WeBWorK/DB/Record/Key.pm index 1f8315f4a6..e2c038739a 100644 --- a/lib/WeBWorK/DB/Record/Key.pm +++ b/lib/WeBWorK/DB/Record/Key.pm @@ -30,7 +30,7 @@ BEGIN { user_id => { type => "VARCHAR(100) NOT NULL", key => 1 }, key => { type => "TEXT" }, timestamp => { type => "BIGINT" }, - set_id => { type => "TINYTEXT" }, + session => { type => "TEXT NOT NULL DEFAULT '{}'" }, ); } diff --git a/lib/WeBWorK/Debug.pm b/lib/WeBWorK/Debug.pm index bf55fcd1fa..865de908c4 100644 --- a/lib/WeBWorK/Debug.pm +++ b/lib/WeBWorK/Debug.pm @@ -94,9 +94,10 @@ Write @messages to the debugging log. sub debug { my @message = @_; - @message = undefstr('###UNDEF###', @message); if ($Enabled) { + @message = undefstr('###UNDEF###', @message); + my ($package, $filename, $line, $subroutine) = caller(1); return if defined $AllowSubroutineOutput and not $subroutine =~ m/$AllowSubroutineOutput/; return if defined $DenySubroutineOutput and $subroutine =~ m/$DenySubroutineOutput/; diff --git a/templates/ContentGenerator/GatewayQuiz.html.ep b/templates/ContentGenerator/GatewayQuiz.html.ep index c74428452b..124e95b1cd 100644 --- a/templates/ContentGenerator/GatewayQuiz.html.ep +++ b/templates/ContentGenerator/GatewayQuiz.html.ep @@ -388,7 +388,6 @@ % <%= form_for $action, name => 'gwquiz', method => 'POST', class => 'problem-main-form', begin =%> <%= $c->hidden_authen_fields =%> - <%= $c->hidden_proctor_authen_fields =%> % % # Hacks to use a javascript link to trigger previews and jump to subsequent pages of a multipage test. <%= hidden_field pageChangeHack => '' =%> diff --git a/templates/ContentGenerator/LoginProctor.html.ep b/templates/ContentGenerator/LoginProctor.html.ep index bca4805257..02635f8309 100644 --- a/templates/ContentGenerator/LoginProctor.html.ep +++ b/templates/ContentGenerator/LoginProctor.html.ep @@ -42,16 +42,12 @@ %= form_for current_route, method => 'POST', begin % # Add the form data posted to the requested URI in hidden fields. % my @fields_to_print = - % grep { !/^(user|effectiveUser|passwd|key|force_password_authen|proctor_user|proctor_key|proctor_passwd)$/ } - % param; + % grep { !/^(user|effectiveUser|passwd|key|force_passwd_authen|proctor_user|proctor_passwd)$/ } param; % if (@fields_to_print) { <%= $c->hidden_fields(@fields_to_print) %> % } <%= $c->hidden_authen_fields =%> % - <%= hidden_field past_proctor_user => param('past_proctor_user') || param('proctor_user') =%> - <%= hidden_field past_proctor_key => param('past_proctor_key') || param('proctor_key') =%> - % % if (param('submitAnswers') % || ($userSet->restricted_login_proctor eq '' || $userSet->restricted_login_proctor eq 'No')) % {