Prevent incorrect 2-step login order causing session fixation (merge …

…commit)

Merge branch 'bugfix/session-fixation-bug-breaks-login' into 'main'
* fix oidc unit test regressions

* prevent incorrect 2-step login order causing session fixation

Closes #1176
See merge request https://gitlab.ci.csc.fi/sds-dev/sd-connect/swift-browser-ui/-/merge_requests/233

Approved-by: Monika Radaviciute <[email protected]>
Co-authored-by: Sampsa Penna <[email protected]>
Merged by Joonatan Mäkinen <[email protected]>

swift_browser_ui/ui/login.py

@@ -110,6 +110,8 @@ async def oidc_end(request: aiohttp.web.Request) -> aiohttp.web.Response:
             origin=str(setd["set_origin_address"]),
         )
 
+    session.changed()
+
     return response
 
 
@@ -126,12 +128,13 @@ async def handle_login(
 
     if setd["oidc_enabled"]:
         session = await aiohttp_session.get_session(request)
-        if "oidc" in session:
+        if session.new or "oidc" not in session:
+            session.invalidate()
+            response.headers["Location"] = "/"
+        else:
             response = aiohttp.web.FileResponse(
                 str(setd["static_directory"]) + "/login2step.html"
             )
-        else:
-            response.headers["Location"] = "/"
 
     else:
         response.headers["Location"] = "/login/front"
@@ -148,7 +151,8 @@ async def sso_query_begin(
 
     if request and setd["oidc_enabled"]:
         session = await aiohttp_session.get_session(request)
-        if "oidc" not in session:
+        if session.new or "oidc" not in session:
+            session.invalidate()
             return aiohttp.web.Response(status=302, headers={"Location": "/"})
     if not setd["has_trust"]:
         response = aiohttp.web.FileResponse(str(setd["static_directory"]) + "/login.html")
@@ -173,7 +177,8 @@ async def sso_query_begin_oidc(
 
     if request and setd["oidc_enabled"]:
         session = await aiohttp_session.get_session(request)
-        if "oidc" not in session:
+        if session.new or "oidc" not in session:
+            session.invalidate()
             return aiohttp.web.Response(status=302, headers={"Location": "/"})
     if not setd["has_trust"]:
         response = aiohttp.web.FileResponse(str(setd["static_directory"]) + "/login.html")
@@ -322,6 +327,10 @@ async def login_with_token(
         else await aiohttp_session.new_session(request)
     )
 
+    if setd["oidc_enabled"] and (session.new or "oidc" not in session):
+        session.invalidate()
+        return aiohttp.web.Response(status=302, headers={"Location": "/"})
+
     session["at"] = time.time()
 
     session["referer"] = request.url.host
@@ -360,8 +369,10 @@ async def login_with_token(
             },
         ) as resp:
             if resp.status == 401:
+                session.invalidate()
                 raise aiohttp.web.HTTPUnauthorized(reason="Token is not valid")
             if resp.status == 403:
+                session.invalidate()
                 raise aiohttp.web.HTTPForbidden(reason="No access to service with token.")
             ret = await resp.json()
 
@@ -404,6 +415,7 @@ async def login_with_token(
     # in practice this might happen if there are sd connect projects that
     # don't have Allas enabled
     if not session["projects"]:
+        session.invalidate()
         request.app["Log"].debug("possible sdConnectProjects and Allas projects mismatch")
         raise aiohttp.web.HTTPForbidden(
             reason="There are no projects available for this user."

swift_browser_ui/ui/middlewares.py

@@ -15,7 +15,9 @@
 ]
 
 
-def return_error_response(error_code: int) -> web.Response:
+def return_error_response(
+    error_code: int, reason: str = "Unknown reason."
+) -> web.Response:
     """Return the correct error page with correct status code."""
     # Read the error response page fully before dumping body since
     # aiohttp.web.FileResponse is not an option, for info see
@@ -28,11 +30,13 @@ def return_error_response(error_code: int) -> web.Response:
         headers={
             "Cache-Control": "no-cache, no-store, must-revalidate",
             "Content-Type": "text/html",
+            "X-Error-Reason": reason,
             "Pragma": "no-cache",
             "Expires": "0",
         },
         body=error_body,
     )
+
     if error_code == 401:
         resp.headers["WWW-Authenticate"] = 'Bearer realm="/", charset="UTF-8"'
 
@@ -144,7 +148,7 @@ async def error_middleware(request: web.Request, handler: AiohttpHandler) -> web
         return response
     except web.HTTPException as ex:
         if ex.status in to_process:
-            return return_error_response(ex.status)
+            return return_error_response(ex.status, ex.reason)
         if ex.status in container_errors:
             raise ex
         if ex.status > 405 and ex.status < 500:

tests/common/mockups.py

@@ -54,14 +54,17 @@ def setUp(self):
             self.aiohttp_session_new_session_mock,
         )
         self.aiohttp_session_get_session_oidc_mock = unittest.mock.AsyncMock()
-        self.aiohttp_session_get_session_oidc_mock.return_value = {
-            **self.session_return,
-            "oidc": {
-                "userinfo": {},
-                "state": "",
-                "access_token": "",
-            },
+        self.oidc_session_return = aiohttp_session.Session(
+            "test-identity-2",
+            new=False,
+            data=dict(self.session_return),
+        )
+        self.oidc_session_return["oidc"] = {
+            "userinfo": {},
+            "state": "",
+            "access_token": "",
         }
+        self.aiohttp_session_get_session_oidc_mock.return_value = self.oidc_session_return
         self.p_get_sess_oidc = unittest.mock.patch(
             "swift_browser_ui.ui.api.aiohttp_session.get_session",
             self.aiohttp_session_get_session_oidc_mock,

tests/ui_unit/test_login.py

@@ -27,6 +27,10 @@ def setUp(self):
             "swift_browser_ui.ui.login.aiohttp_session.get_session",
             self.aiohttp_session_get_session_mock,
         )
+        self.p_get_sess_oidc = unittest.mock.patch(
+            "swift_browser_ui.ui.login.aiohttp_session.get_session",
+            self.aiohttp_session_get_session_oidc_mock,
+        )
 
     async def test_oidc_start(self):
         """Test oidc initial request."""
@@ -364,7 +368,7 @@ async def test_login_with_token(self):
 
         self.setd_mock["oidc_enabled"] = True
         self.setd_mock["sdconnect_enabled"] = False
-        with patch1, patch2, self.p_get_sess:
+        with patch1, patch2, self.p_get_sess_oidc:
             resp = await swift_browser_ui.ui.login.login_with_token(
                 self.mock_request,
                 token,