From 22fe4a988a31c78f01ddc62bd1d9cb9c5074046b Mon Sep 17 00:00:00 2001 From: moson-mo Date: Fri, 26 May 2023 11:21:16 +0200 Subject: [PATCH] fix: make AURSID a session cookie if "remember me" is not checked This should match more closely the expectation of a user. A session cookie should vanish on browser close and you thus they need to authenticate again. There is no need to bump the expiration of AURSID either, so we can remove that part. Signed-off-by: moson-mo --- aurweb/cookies.py | 33 --------------------------------- aurweb/routers/auth.py | 7 ++++++- aurweb/templates.py | 13 ++----------- doc/web-auth.md | 18 ++++++------------ test/test_cookies.py | 30 ++++++++++++++++++------------ 5 files changed, 32 insertions(+), 69 deletions(-) diff --git a/aurweb/cookies.py b/aurweb/cookies.py index cb4396d7..022cff1e 100644 --- a/aurweb/cookies.py +++ b/aurweb/cookies.py @@ -1,6 +1,3 @@ -from fastapi import Request -from fastapi.responses import Response - from aurweb import config @@ -33,33 +30,3 @@ def timeout(extended: bool) -> int: if bool(extended): timeout = config.getint("options", "persistent_cookie_timeout") return timeout - - -def update_response_cookies( - request: Request, - response: Response, - aursid: str = None, -) -> Response: - """Update session cookies. This method is particularly useful - when updating a cookie which was already set. - - The AURSID cookie's expiration is based on the AURREMEMBER cookie, - which is retrieved from `request`. - - :param request: FastAPI request - :param response: FastAPI response - :param aursid: Optional AURSID cookie value - :returns: Updated response - """ - secure = config.getboolean("options", "disable_http_login") - if aursid: - remember_me = request.cookies.get("AURREMEMBER") == "True" - response.set_cookie( - "AURSID", - aursid, - secure=secure, - httponly=secure, - max_age=timeout(remember_me), - samesite=samesite(), - ) - return response diff --git a/aurweb/routers/auth.py b/aurweb/routers/auth.py index 98a655e3..46dee3a4 100644 --- a/aurweb/routers/auth.py +++ b/aurweb/routers/auth.py @@ -69,7 +69,12 @@ async def login_post( if user.Suspended: return await login_template(request, next, errors=["Account Suspended"]) - cookie_timeout = cookies.timeout(remember_me) + # If "remember me" was not ticked, we set a session cookie for AURSID, + # otherwise we make it a persistent cookie + cookie_timeout = None + if remember_me: + cookie_timeout = aurweb.config.getint("options", "persistent_cookie_timeout") + perma_timeout = aurweb.config.getint("options", "permanent_cookie_timeout") sid = _retry_login(request, user, passwd, cookie_timeout) if not sid: diff --git a/aurweb/templates.py b/aurweb/templates.py index 89316d6d..d20cbe85 100644 --- a/aurweb/templates.py +++ b/aurweb/templates.py @@ -10,7 +10,7 @@ from fastapi import Request from fastapi.responses import HTMLResponse import aurweb.config -from aurweb import cookies, l10n, time +from aurweb import l10n, time # Prepare jinja2 objects. _loader = jinja2.FileSystemLoader( @@ -145,13 +145,4 @@ def render_template( ): """Render a template as an HTMLResponse.""" rendered = render_raw_template(request, path, context) - response = HTMLResponse(rendered, status_code=int(status_code)) - - sid = None - if request.user.is_authenticated(): - sid = request.cookies.get("AURSID") - - # Re-emit SID via update_response_cookies with an updated expiration. - # This extends the life of a user session based on the AURREMEMBER - # cookie, which is always set to the "Remember Me" state on login. - return cookies.update_response_cookies(request, response, aursid=sid) + return HTMLResponse(rendered, status_code=int(status_code)) diff --git a/doc/web-auth.md b/doc/web-auth.md index dbb4403d..c8604fed 100644 --- a/doc/web-auth.md +++ b/doc/web-auth.md @@ -22,17 +22,11 @@ in the following ways: ### Max-Age The value used for the `AURSID` Max-Age attribute is decided based -off of the "Remember Me" checkbox on the login page. Both paths -use their own independent configuration for the number of seconds -that each type of session should stay alive. - -- "Remember Me" unchecked while logging in - - `options.login_timeout` is used -- "Remember Me" checked while logging in - - `options.persistent_cookie_timeout` is used - -Both `options.login_timeout` and `options.persistent_cookie_timeout` -indicate the number of seconds the session should live. +off of the "Remember Me" checkbox on the login page. If it was not +checked, we don't set Max-Age and it becomes a session cookie. +Otherwise we make it a persistent cookie and for the expiry date +we use `options.persistent_cookie_timeout`. +It indicates the number of seconds the session should live. ### Notes @@ -89,7 +83,7 @@ The following list of steps describes exactly how this verification works: 1. Was the `AURSID` cookie delivered? 1. No, the algorithm ends, you are considered unauthenticated 2. Yes, move on to 2 -2. Was the `AURREMEMBER` cookie delivered with a value of 1? +2. Was the `AURREMEMBER` cookie delivered with a value of `True`? 1. No, set the expected session timeout **T** to `options.login_timeout` 2. Yes, set the expected session timeout **T** to `options.persistent_cookie_timeout` diff --git a/test/test_cookies.py b/test/test_cookies.py index a0ace68f..dd4143cb 100644 --- a/test/test_cookies.py +++ b/test/test_cookies.py @@ -1,4 +1,5 @@ from datetime import datetime +from http import HTTPStatus import pytest from fastapi.testclient import TestClient @@ -56,7 +57,6 @@ def test_cookies_login(client: TestClient, user: User): resp = request.post("/login", data=data) local_time = int(datetime.now().timestamp()) - expected_timeout = local_time + config.getint("options", "login_timeout") expected_permanent = local_time + config.getint( "options", "permanent_cookie_timeout" ) @@ -64,22 +64,15 @@ def test_cookies_login(client: TestClient, user: User): # Check if we got permanent cookies with expected expiry date. # Allow 1 second difference to account for timing issues # between the request and current time. + # AURSID should be a session cookie (no expiry date) assert "AURSID", "AURREMEMBER" in resp.cookies for cookie in resp.cookies.jar: if cookie.name == "AURSID": - assert abs(cookie.expires - expected_timeout) < 2 + assert cookie.expires is None if cookie.name == "AURREMEMBER": assert abs(cookie.expires - expected_permanent) < 2 - - # Make some random http call. - # We should get an (updated) AURSID cookie with each request. - sid = resp.cookies.get("AURSID") - with client as request: - request.cookies = {"AURSID": sid} - resp = request.get("/") - - assert "AURSID" in resp.cookies + assert cookie.value == "False" # Log out with client as request: @@ -102,14 +95,27 @@ def test_cookies_login(client: TestClient, user: User): # Check if we got a permanent cookie with expected expiry date. # Allow 1 second difference to account for timing issues # between the request and current time. + # AURSID should be a persistent cookie expected_persistent = local_time + config.getint( "options", "persistent_cookie_timeout" ) - assert "AURSID" in resp.cookies + assert "AURSID", "AURREMEMBER" in resp.cookies for cookie in resp.cookies.jar: if cookie.name in "AURSID": assert abs(cookie.expires - expected_persistent) < 2 + if cookie.name == "AURREMEMBER": + assert abs(cookie.expires - expected_permanent) < 2 + assert cookie.value == "True" + + # log in again even though we already have a session + with client as request: + resp = request.post("/login", data=data) + + # we are logged in already and should have been redirected + assert resp.status_code == int(HTTPStatus.SEE_OTHER) + assert resp.headers.get("location") == "/" + def test_cookie_language(client: TestClient, user: User): # Unauthenticated reqeuests should set AURLANG cookie