diff --git a/server/auth/auth_oauth.py b/server/auth/auth_oauth.py index b2724c57..efc79c30 100644 --- a/server/auth/auth_oauth.py +++ b/server/auth/auth_oauth.py @@ -218,22 +218,24 @@ class AuthTypeOAuth(AuthTypeClientBase): try: if self.session_cookie: - tokensdict = session.get(self.CXG_TOKENS) - if tokensdict: - g.tokens = Tokens(**tokensdict) + value = session.get(self.CXG_TOKENS) + if value: + g.tokens = Tokens(**value) else: return None else: value = request.cookies.get(self.cookie_params["key"]) - value = base64.b64decode(value) - try: - tokensdict = json.loads(value) - g.tokens = Tokens(**tokensdict) - except (TypeError, KeyError, json.decoder.JSONDecodeError): - g.pop("tokens", None) + if value is None: return None + value = base64.b64decode(value) + value = json.loads(value) + g.tokens = Tokens(**value) - except (TypeError, KeyError): + except Exception: + # there are many types of exceptions that can be raise in the above section. + # It is impractical to list all the exceptions here, since that would be brittle. + # If an exception occurs, then return None, meaning that no token could be retrieved. + current_app.logger.warning(f"auth cookie is in the wrong format: {str(value)}") g.pop("tokens", None) return None @@ -331,6 +333,7 @@ class AuthTypeOAuth(AuthTypeClientBase): # if there is no id_token, return None (user is not authenticated) tokens = self.get_tokens() + if tokens is None or tokens.id_token is None: return None diff --git a/server/test/unit/auth/test_oauth.py b/server/test/unit/auth/test_oauth.py index 1096ef1a..43bdf1c2 100644 --- a/server/test/unit/auth/test_oauth.py +++ b/server/test/unit/auth/test_oauth.py @@ -59,12 +59,7 @@ def logout(): @mock_oauth_app.route("/.well-known/jwks.json") def jwks(): - data = dict( - alg="RS256", - kty="RSA", - use="sig", - kid="fake_kid", - ) + data = dict(alg="RS256", kty="RSA", use="sig", kid="fake_kid",) return make_response(jsonify(dict(keys=[data]))) @@ -152,18 +147,25 @@ class AuthTest(unittest.TestCase): self.assertNotEqual(access_token_before, access_token_after) self.assertNotEqual(id_token_before, id_token_after) - # invalid cookie fails - # (THIS CURRENTLY 500's ON THE SERVER, BUT SHOULD RETURN EMPTY USERINFO OR 401) + # invalid cookie is rejected session.cookies.set(cookie_key, "TEST_" + cookie) - userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json() - self.assertIsNone(userinfo.get("userinfo")) + self.assertTrue(cookie_key in session.cookies) + response = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo") + # this is not an error, the invalid cookie is just ignored. + self.assertEqual(response.status_code, 200) + userinfo = response.json() + self.assertFalse(userinfo["userinfo"]["is_authenticated"]) + self.assertIsNone(userinfo["userinfo"]["username"]) - # invalid id_token fails + # invalid id_token is rejected test_token = token test_token["id_token"] = "TEST_" + id_token_after encoded_cookie = base64.b64encode(json.dumps(test_token).encode()).decode() session.cookies.set(cookie_key, encoded_cookie) - userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json() + response = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo") + # this is not an error, the invalid id_token is just ignored. + self.assertEqual(response.status_code, 200) + userinfo = response.json() self.assertFalse(userinfo["userinfo"]["is_authenticated"]) self.assertIsNone(userinfo["userinfo"]["username"]) @@ -181,9 +183,7 @@ class AuthTest(unittest.TestCase): # test with session cookies app_config = AppConfig() app_config.update_server_config(app__flask_secret_key="secret") - app_config.update_server_config( - authentication__params_oauth__session_cookie=True, - ) + app_config.update_server_config(authentication__params_oauth__session_cookie=True,) self.auth_flow(app_config) def test_auth_oauth_cookie(self):