Conversation
| "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" |
There was a problem hiding this comment.
Shouldn't be hardcoded - use url from config instead
| plugins.cleanup() | ||
|
|
||
|
|
||
| def get_new_tokens(refresh_token): |
There was a problem hiding this comment.
Please add typing (I guess get_new_tokens(refresh_token: str) -> Tuple[str, str], but recheck)
| import os | ||
|
|
||
| import requests # type: ignore | ||
|
|
There was a problem hiding this comment.
(I accidentaly removed this comment now, but it still applies - unnecessary empty line.
msm-cert
left a comment
msm-cert
left a comment
There was a problem hiding this comment.
I did a more in-depth review and found a few more stylistic problems. Also a few design comments (especially cookie usage).
| const rawToken = localStorage.getItem("rawToken"); | ||
| if (rawToken) { | ||
| const expiresAt = localStorage.getItem("expiresAt"); | ||
| if (Date.now() > expiresAt) { |
There was a problem hiding this comment.
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.
|
|
||
| export const tokenExpired = () => { | ||
| const rawToken = localStorage.getItem("rawToken"); | ||
| if (rawToken) { |
There was a problem hiding this comment.
nitpick - if possible prefer early exit and avoid nested ifs. So please change the flow to
if (!rawToken) {
return false;
}
| if (response.data["token"]) { | ||
| storeTokenData(response.data["token"]); | ||
| } else { | ||
| return; |
There was a problem hiding this comment.
Does this return here do anything? If not, please remove it.
| const response = await axios.request("/api/token/refresh", { | ||
| method: "POST", | ||
| headers: headers, | ||
| withCredentials: true, |
There was a problem hiding this comment.
Why is withCredentials here necessary? If it's not needed, please remove it.
There was a problem hiding this comment.
Nevermind, I just noticed the code uses cookies.
| if (location_href) { | ||
| window.location.href = location_href; | ||
| } else { | ||
| window.location.href = "/"; |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
| const response = await api.post("/login", token_data); | |
| await api.post("/login", token_data); |
since response is not used anywhere
| } | ||
|
|
||
| componentDidMount() { | ||
| localStorage.setItem("currentLocation", window.location.href); |
There was a problem hiding this comment.
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.
| export const storeTokenData = (token) => { | ||
| localStorage.setItem("rawToken", token); | ||
| const decodedToken = parseJWT(token); | ||
| localStorage.setItem("expiresAt", decodedToken.exp * 1000); |
There was a problem hiding this comment.
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.
|
|
||
| export const refreshAccesToken = async () => { | ||
| const rawToken = localStorage.getItem("rawToken"); | ||
| const expiresAt = localStorage.getItem("expiresAt"); |
| @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"]: |
There was a problem hiding this comment.
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.
|
(also please rebase this onto master, since there are merge conflicts) |
2 participants
Your checklist for this pull request
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