openwebwork / webwork2

Course management front end for WeBWorK
http://webwork.maa.org/wiki/Main_Page
Other
145 stars 165 forks source link

Add an authentication session. #2334

Closed drgrice1 closed 7 months ago

drgrice1 commented 7 months ago

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.

Alex-Jordan commented 7 months ago

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 commented 7 months ago

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).

Alex-Jordan commented 7 months ago

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 commented 7 months ago

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.

drgrice1 commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

drgrice1 commented 6 months ago

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