Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an authentication session. #2334

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

drgrice1
Copy link
Member

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.

The session key is no longer saved in a hidden field in forms when session_management_via is "cookie_session". This means that the session key is not visible anywhere in the DOM or in URL parameters when using cookies for session management, and JavaScript has no access to it. This is something that malicious javascript can exploit. Of course this also means that webwork2's own javascript can't access it either. So for this to work, the rpc endpoints used by webwork2 need to have cookies enabled so that the endpoints used by the library browser and such still work. However, the html2xml endpoint still has cookies always disabled. For the render_rpc endpoint, the disableCookies parameter can be set to disable cookies.

Note that an authentication module can disable cookies by setting the disable_cookies stash value. This is how the html2xml endpoint disables cookies. The Cosign and Shibboleth authentication modules have been adapted to use this as well, instead of overriding the WeBWorK::Authen cookie methods. In fact overriding those methods won't disable cookies anymore entirely. The Proctor authen module overrides those methods for a different reason. It does not disable cookies, but does not set the user_id, key, and timestamp authentication values in the cookie.

Note that the "theme" hidden field has also been removed. That isn't even an authentication parameter, and doesn't work anyway.

This builds on #2333, and is part 2 of 3 of the authentication system revamp.

@drgrice1 drgrice1 force-pushed the authentication-session branch 2 times, most recently from 1fc5c48 to 885811d Compare February 23, 2024 22:26
@drgrice1 drgrice1 force-pushed the authentication-session branch 2 times, most recently from 0775adf to c40b9e7 Compare February 23, 2024 22:56
@drgrice1 drgrice1 force-pushed the authentication-session branch 4 times, most recently from ed7461d to b97e361 Compare February 29, 2024 21:52
drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Mar 2, 2024
This is a stopgap measure since this is already fixed in openwebwork#2334, but this
was missed in openwebwork#2333.  openwebwork#2334 completely rewrites this module.
drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Mar 2, 2024
This is a stopgap measure since this is already fixed in openwebwork#2334, but this
was missed in openwebwork#2333.  openwebwork#2334 completely rewrites this module.
@drgrice1 drgrice1 force-pushed the authentication-session branch from b97e361 to 710e7d3 Compare March 2, 2024 00:30
@Alex-Jordan
Copy link
Contributor

I have read the description of this and I mostly understand it, and it sounds good. I skimmed the code changes, but ttbh I was just looking for typos and not trying to follow the code.

I am reluctant to actually test this because I'd need to alter the key tables for all courses, and then alter them back later. But am I overthinking it? Is it actually pretty risk-free to do drop the set_id column?

@drgrice1
Copy link
Member Author

drgrice1 commented Mar 5, 2024

You can test this without dropping the set_id column. Its existence won't cause an issue. Although dropping it and then adding it back when you switch back to develop also won't cause issues. Also, you only need to upgrade the courses that you test this with. You don't need to change the key table for other courses.

That is all assuming you aren't testing this on a production server (hopefully not).

@drgrice1 drgrice1 force-pushed the authentication-session branch from 710e7d3 to 3ba2fc7 Compare March 5, 2024 21:12
@Alex-Jordan
Copy link
Contributor

If I had $session_management_via = "session_cookie" and I log into a course, and then change to $session_management_via = "key", I expected that I would lose my session and need to sign in again. But this did not happen and I was able to continue navigating in the course. I just want to check if that is expected behavior.

@drgrice1
Copy link
Member Author

drgrice1 commented Mar 5, 2024

Yeah, that is expected behavior. That is the same as with the develop branch currently as well, and I believe it works that way with 2.18 and before as well.

Note that session_management_via = 'key' does not disable cookies. In fact the "Remember Me" check box that is shown on the login page when using "key" management of the session means to use cookies to preserve the session.

What is happening when you change the setting from "session_cookie" to "key" is that the cookie is still there. So it thinks it is supposed to "Remember Me" and gets the session key from the cookie. Thus continuing the session.

Note that the other way around will also (somewhat) work. If you set session_management_via = 'key', sign in, and change to session_management_via = 'session_cookie', then the session will also continue because the key was in forms and URL parameters. However, if you enter a URL into the address bar without the session key URL parameter immediately after changing from "session_cookie" to "key", then you will need to sign in again.

Note that the session will not really be entirely preserved with this pull request. Only the "user_id" and "key" columns in the database are actually shared between the two sessions. When using a "session_cookie" the rest of the session is in the cookie, and when using the "key" management approach the session is saved in the database. So there are things that would get messed up if this change is made on a production server. So I would not recommend making this type of change on a production server, particularly once courses are in progress.

Copy link
Contributor

@Alex-Jordan Alex-Jordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked out the branch and tried logging into courses under both session management options, and I had no issues. Is there anything I should specifically test?

@drgrice1
Copy link
Member Author

drgrice1 commented Mar 5, 2024

If you have a way to test LTI authentication for a student with the $permissionLevels{navigation_allowed} set to login_proctor or above, that would be good. With this pull request, that uses the session to store the set_id instead of the removed set_id column in the database.

Also test proctor authentication. That uses the session as well. There is no proctor key in the database anymore. The proctor authentication module is completely rewritten.

drgrice1 added 2 commits March 6, 2024 07:28
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 openwebwork#2243 and openwebwork#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.
…ment_via is "cookie_session".

This means that the session key is not visible anywhere in the DOM or in
URL parameters when using cookies for session management, and JavaScript
has no access to it.  This is something that malicious javascript can
exploit.  Of course this also means that webwork2's own javascript can't
access it either.  So for this to work, the rpc endpoints used by
webwork2 need to have cookies enabled so that the endpoints used by the
library browser and such still work.  However, the html2xml endpoint
still has cookies always disabled.  For the render_rpc endpoint, the
disableCookies parameter can be set to disable cookies.

Note that an authentication module can disable cookies by setting the
disable_cookies stash value.  This is how the html2xml endpoint disables
cookies.  The Cosign and Shibboleth authentication modules have been
adapted to use this as well, instead of overriding the `WeBWorK::Authen`
cookie methods.  In fact overriding those methods won't disable cookies
anymore entirely.  The Proctor authen module overrides those methods for
a different reason.  It does not disable cookies, but does not set the
user_id, key, and timestamp authentication values in the cookie.

Note that the "theme" hidden field has also been removed.  That isn't
even an authentication parameter, and doesn't work anyway.
@drgrice1 drgrice1 force-pushed the authentication-session branch from 3ba2fc7 to 6ded4b5 Compare March 6, 2024 13:28
Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked both key and session_cookie modes of session handling.

Also, checked the proctoring--seems to work well and checked that both the set_id and the version is stored for proctor authorization.

@Alex-Jordan Alex-Jordan merged commit 4c8629a into openwebwork:develop Mar 7, 2024
2 checks passed
@drgrice1 drgrice1 deleted the authentication-session branch March 7, 2024 20:36
@drgrice1
Copy link
Member Author

There is a new limitation that this change introduces that I just discovered. That is the number of courses that you can be signed into at the same time for the same webwork server. This is caused by the size of the header which is increased considerably by this change, and the increase is due to cookie size. Previously the session cookie was roughly 100 bytes (and the total header size roughly 700 to 800 bytes). With this change the session cookie size is around 1500 bytes (and the total header size typically more than 2000 bytes). Mojolicious places a limit of 8192 bytes on the header size.

Note that if you are signed in to multiple courses for the same domain, then the session cookies for all courses are included in all requests. So the size of the cookies are cumulative for the total header size. This means that previously you could easily sign in to 60 or more courses for the same webwork server at the same time. Now 4 is pretty much the max, and you might not be able to get that if there are other session values in the cookie (for instance once #2337 is merged cookies will temporarily grow for a single request when flash values are added).

Generally, I don't think this is too much of a problem as you usually are only signed in to one or two courses at a time (and you can still easily sign in to 3 at a time, and maybe another). However, if you sign into one course, and close the tab or window without signing out, then the cookie is still there and will affect the limit when signing into other courses. So if you often use more than one webwork course from the same server, you will want to remember to sign out. If it is still a problem, the limit can be increased. For Mojolicious (and hypnotoad or morbo) this can be increased using the MOJO_MAX_LINE_SIZE environment variable. Note that if you proxy via apache2 or nginx, they also use an 8K limit so those limits would also need to be increased.

This is another downside of webwork2's structure with courses being separate instances. However, from the browser standpoint, they aren't since they all share the same domain.

Note that technically the webwork2 path limits what is included as well. So cookies that are dedicated to another path (like /my-other-site-on-same-server) will not contribute to this size limitation. Although other cookies that are only restricted to the root url (/) would.

By the way, once this limit is exceeded you will end up being kicked out of first course that tries to make a request such that the header exceeds the size limit. If serving directly you will see errors such as Use of uninitialized value $hostname in string ne at /opt/webwork/webwork2/lib/WeBWorK/Controller.pm line 76. and Use of uninitialized value $user_agent in concatenation (.) or string at /opt/webwork/webwork2/lib/WeBWorK/Authen.pm line 873. If you are proxying via apache, then the request won't even reach the webwork2 app, and it will just show an error about the header size being exceeded. Actually things are perhaps a bit worse in that case also. Since the request doesn't reach webwork2, you don't actually get signed out of the course. So all of the cookies persist, and you can't access any of the courses again until you delete some (or all) of the cookies.

@Alex-Jordan
Copy link
Contributor

Can you point me in the right direction to delete cookies I need to delete after hitting this issue?

This got me thinking about something like a computer in a student computer lab. How likely is it that this will become a problem in that environment?

@drgrice1
Copy link
Member Author

Do the students log into a university account in this computer lab? It is rather rare that computer labs on institution campuses don't require that students sign in to their institution account. If they are signed in to an account like that they don't use the same browser sessions. So it won't affect them. The cookies that one user has in their browser session won't show up in another user's browser session.

If these computers are truly public workstations that anyone can just walk in and use, and students are not signing out ... then we have a problem. That is a massive security vulnerability. However, then the session limitation would become a problem.

@drgrice1
Copy link
Member Author

To delete cookies for a specific site, I use the developer tools. In Firefox this is under the "Storage" tab in the developer tools. Select "Cookies" under that, and then the domain, and then all cookies are listed. You can right click on one to delete it. In Chrome cookies are under the "Application" tab. The interface is the same as for Firefox though.

@Alex-Jordan
Copy link
Contributor

What you say about computer labs makes sense, students do have to sign in to something like Windows365 (or something like that, I'm not sure what it actually is called.)

Now 4 is pretty much the max

Here is a rare but not impossible scenario. Am instructor manages separate WW courses for homework and quizzes (to help shut off access to homework during a proctored quiz). In a classroom, a student signs into the homework course for review before a quiz. Then into the quiz course because the class is starting the quiz. Then there is a login_proctor login, and then at the end the actual proctor (the instructor?) gives credentials for grading to commence. Do those last two sign-ins create cookies?

@drgrice1
Copy link
Member Author

Proctor authentication no longer uses cookies at all. So no, that is not a problem.

@drgrice1
Copy link
Member Author

Proctor authentication uses the user's session. So that is all in the first cookie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants