Fix/keycloak timeout #445

MichalJura · 2025-01-13T22:01:00Z

Your checklist for this pull request

[ x] I've read the contributing guideline.
[ x] I've tested my changes by building and running mquery, and testing changed functionality (if applicable)
I've added automated tests for my change (if applicable, optional)
I've updated documentation to reflect my change (if applicable)

What is the current behaviour?

Currently mquery does not revoke token by itself therefore forcing relogin.
Also after token expiration user is forced to log in even if instance allows anonymous access,
What is the new behaviour?

Test plan

Token is auto-refreshing itself now.
Also when the token expires, user get anonymous access.
Also User is not logged out so often.

Closing issues

fixes #411
fixes #425

msm-cert · 2025-02-04T12:44:42Z

src/app.py

+        "client_id": db.config.openid_client_id,
+        "client_secret": db.config.openid_secret,
+    }
+    url = "http://mquery-keycloak-1:8080/auth/realms/myrealm/protocol/openid-connect/token"


Shouldn't be hardcoded - use url from config instead

msm-cert · 2025-02-04T12:46:27Z

src/app.py

@@ -70,6 +74,24 @@ def with_plugins() -> Iterable[PluginManager]:
        plugins.cleanup()


+def get_new_tokens(refresh_token):


Please add typing (I guess get_new_tokens(refresh_token: str) -> Tuple[str, str], but recheck)

msm-cert

Ping about hardcoded URL and style nitpicks.

I feel verifying OIDC correctness here is out of my depth, so I asked @psrok1 to help with the review of this aspect.

msm-cert · 2025-02-25T04:12:08Z

src/app.py

@@ -1,6 +1,8 @@
 from contextlib import asynccontextmanager
 import os

+import requests  # type: ignore
+


Suggested change

(I accidentaly removed this comment now, but it still applies - unnecessary empty line.

msm-cert

I did a more in-depth review and found a few more stylistic problems. Also a few design comments (especially cookie usage).

msm-cert · 2025-02-25T04:17:02Z

src/mqueryfront/src/utils.js

+    const rawToken = localStorage.getItem("rawToken");
+    if (rawToken) {
+        const expiresAt = localStorage.getItem("expiresAt");
+        if (Date.now() > expiresAt) {


Probably it would be better to check if token expires in the next ~30 seconds? To avoid situations where token is 0.1 second before expiration during the check, but server gets an expired token already. That, and clock desync.

msm-cert · 2025-02-25T04:19:01Z

src/mqueryfront/src/utils.js

+
+export const tokenExpired = () => {
+    const rawToken = localStorage.getItem("rawToken");
+    if (rawToken) {


nitpick - if possible prefer early exit and avoid nested ifs. So please change the flow to

if (!rawToken) { return false; }

msm-cert · 2025-02-25T04:19:40Z

src/mqueryfront/src/utils.js

+        if (response.data["token"]) {
+            storeTokenData(response.data["token"]);
+        } else {
+            return;


Does this return here do anything? If not, please remove it.

msm-cert · 2025-02-25T04:20:47Z

src/mqueryfront/src/utils.js

+        const response = await axios.request("/api/token/refresh", {
+            method: "POST",
+            headers: headers,
+            withCredentials: true,


Why is withCredentials here necessary? If it's not needed, please remove it.

Nevermind, I just noticed the code uses cookies.

msm-cert · 2025-02-25T04:22:41Z

src/mqueryfront/src/App.js

+        if (location_href) {
+            window.location.href = location_href;
+        } else {
+            window.location.href = "/";


Nitpick: this is technically incorrect, since mquery doesn't have to be hosted at / (it won't work with, for example, a web proxy that serves mquery on some.website.pl/mquery/). But I think this problem is safe to ignore, just wanted to point that out.

msm-cert · 2025-02-25T04:32:18Z

src/mqueryfront/src/App.js

+    const login = async (token_data) => {
+        token_data.not_before_policy = token_data["not-before-policy"];
+        delete token_data["not-before-policy"];
+        const response = await api.post("/login", token_data);


Suggested change

const response = await api.post("/login", token_data);

await api.post("/login", token_data);

since response is not used anywhere

msm-cert · 2025-02-25T04:34:18Z

src/mqueryfront/src/config/ConfigPage.js

@@ -14,6 +14,7 @@ class ConfigPage extends Component {
    }

    componentDidMount() {
+        localStorage.setItem("currentLocation", window.location.href);


Are you sure there is no way to solve this in a more generic way? Adding the code to set a variable on every subpage feels suboptimal and error-prone.

msm-cert · 2025-02-25T04:35:27Z

src/mqueryfront/src/utils.js

+export const storeTokenData = (token) => {
+    localStorage.setItem("rawToken", token);
+    const decodedToken = parseJWT(token);
+    localStorage.setItem("expiresAt", decodedToken.exp * 1000);


Please don't keep this in localStorage. It's better to avoid having two separate variables when one is necessary. Instead parse rawToken when necessary.

msm-cert · 2025-02-25T04:35:57Z

src/mqueryfront/src/utils.js

+
+export const refreshAccesToken = async () => {
+    const rawToken = localStorage.getItem("rawToken");
+    const expiresAt = localStorage.getItem("expiresAt");


msm-cert · 2025-02-25T04:37:48Z

src/app.py

+@app.post("/api/login", response_model=LoginSchema, tags=["stable"])
+async def login(request: Request, response: Response) -> LoginSchema:
+    token = await request.json()
+    if token["refresh_token"]:


The code intentionally doesn't use cookies and passes all parameters by the API. Please change this to keep refresh_token in local storage instead of adding a cookie.

msm-cert · 2025-02-25T04:46:06Z

(also please rebase this onto master, since there are merge conflicts)

Michał Jura added 6 commits January 10, 2025 15:28

If token expires user has anonymous access

81bf3ef

added current path just in case of relogging in

9bd2256

added refresh token feature

b5da8fa

cosmetics

30293f2

Moved keycloak token handling to backend

1dbd7af

moved token handling to backend

18a104f

MichalJura requested a review from msm-cert January 13, 2025 22:01

Michał Jura added 6 commits January 14, 2025 11:56

moved refresh_token to http only cookies

28b296c

cosmetics

2fb6e08

flake8 adjustments

f2e1dd9

added more typing

6588c63

removed unneccessary logging

0982f89

changed refresh_token max_age

69992d4

msm-cert reviewed Feb 4, 2025

View reviewed changes

msm-cert reviewed Feb 25, 2025

View reviewed changes

msm-cert requested changes Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/keycloak timeout #445

Fix/keycloak timeout #445

MichalJura commented Jan 13, 2025

msm-cert Feb 4, 2025

msm-cert Feb 4, 2025

msm-cert left a comment

msm-cert Feb 25, 2025

msm-cert left a comment

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert Feb 25, 2025

msm-cert commented Feb 25, 2025

		@@ -70,6 +74,24 @@ def with_plugins() -> Iterable[PluginManager]:
		plugins.cleanup()


		def get_new_tokens(refresh_token):

	const response = await api.post("/login", token_data);
	await api.post("/login", token_data);

Fix/keycloak timeout #445

Are you sure you want to change the base?

Fix/keycloak timeout #445

Conversation

MichalJura commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msm-cert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msm-cert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msm-cert commented Feb 25, 2025