Skip to content

Commit

Permalink
Add an authentication session.
Browse files Browse the repository at this point in the history 
This requires another change to the `key` table.  The `set_id` column
has been removed, and in its place is a `session` key.  This column is a
JSON column and holds all session values.

Note there are really two sessions that can be used.  A cookie session
(for this the Mojolicious session is used) or a database session. Which
is used is determined by the `manage_session_via` course environment
setting.

The database session works by saving a hash in the stash key named
'webwork2.database_session'.  This hash is saved to the database in the
`after_dispatch` hook.  This means that regardless of which session is
used, session values can be set anytime after the session is created in
authentication and before the `after_dispatch` hook is called. That is
pretty much at anytime in a content generator module. Do not directly
use the `Mojolicious::Controller::session` method (which will only set
cookie session values) except in the authentication modules.  Instead
call the `WeBWorK::Authen::session` method which takes care of setting
the values for the correct session type (database or cookie).

One value the session now holds is what the `set_id` column stored
before.

In addition proctor authentication is completely reworked.  Instead of
the hack to use a separate proctor key, session values are used.  There
are some additional security measures implemented to make it harder for
a student to hijack the session and gain access to a test after a
proctoring session without having submitted the test as observed
in #2243 and #2244.  First, the proctor username and password (and the
submit button) must come from a POST request.  Parameters from a GET
request are ignored for these things.  It is a little harder to
construct a POST request than a GET request. Second, the data saved in
the session is specific to the set and version.  So it is not possible
for a student to open a new version of a test anymore, only to regain
access to the version that was being worked. Third, the proctor_user is
no longer saved in a hidden field in the GatewayQuiz form.  If proctor
authorization has been granted, then the authorization is saved in the
session, and so the proctor_user is not needed.

Some of the derived authentication modules will need some changes to
work with this.  Of course the LTIAdvantage, LTIAdvanced, and Proctor
modules have already been changed and tested.  Looking at the LDAP and
CAS code, it seems that they should still work with this, but I can't
test those.  Most likely Cosign, Moodle, and Shibboleth will need
changes.  I don't know if anyone still uses the Moodle module. They
really shouldn't, and should use LTI instead.  I think that module
should be deleted.
  • Loading branch information
@drgrice1
drgrice1 committed Feb 23, 2024
1 parent 7d9eafe commit 785cc28
Show file tree
Hide file tree
Showing 13 changed files with 337 additions and 324 deletions.
1 change: 1 addition & 0 deletions lib/Mojolicious/WeBWorK.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 . ']',
Expand Down
6 changes: 5 additions & 1 deletion lib/WeBWorK.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 {
Expand Down
188 changes: 139 additions & 49 deletions lib/WeBWorK/Authen.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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:

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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<session_management_via> is "session_cookie" then the Mojolicous cookie session
is used. If C<session_management_via> 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<session> sub-hash can be set with this method. The
C<user_id>, C<key>, and C<timestamp> should be set directly in the
C<webwork2.database_session> 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<after_dispatch> 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
Expand All @@ -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);
Expand All @@ -696,16 +806,16 @@ 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;
}

# 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};
Expand All @@ -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;
Expand All @@ -730,23 +839,18 @@ 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;

return if $c->{rpc};

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}
);
Expand All @@ -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 {
Expand Down
41 changes: 2 additions & 39 deletions lib/WeBWorK/Authen/LTIAdvanced.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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}) {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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}|");
Expand Down Expand Up @@ -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}) . "|";
Expand Down
Loading

0 comments on commit 785cc28

Please sign in to comment.