Log out from other tabs. #1863
Log out from other tabs. #1863
Conversation
|
|
|
I have just noticed that this does not work as intended, since the csrf token will also be invalid even if the user logs back.. drafting, maybe tonight I can look for a fix or see if I need the broadcasting.. |
|
This seems the way to go! Thanks.
|
|
The cleanup is left over code. It has to be deleted. The reason it is there is that the notification code went through many iterations and this created some garbage in local storage. So I made a PR to delete this garbage. But I forgot to revert it. |
|
DEV updated, I'm not able to logout. |
|
Afk, I will look later today. |
ppigazzini
added
enhancement
server
|
DEV updated with "Fix prettier" 64564bd |
|
DEV updated with "Try also this ..." 65007cf |
|
Okay I should stop pushing commits from mobile, I will fix later when not afk |
|
DEV updated with "Update application.js" ac17f07 |
Logs users out from other tabs if they try to use them while they have logged-out or lost cookies. Worth noting that the page was already unusable and the user couldn't do actions due to an expired CSRF token. steps in the issue cannot reproduce the error. I can only see this scenario causing it. Fixes: #1839
|
Should be fixed. application.js definitely need refactoring. |
|
DEV updated, it looks good. |
|
PROD updated, thank you @peregrineshahin :) |
|
@vdbergh I just want to make sure you are okay with the change in the broadcast function to broadcast before calling the function for notifications? Because I need to broadcast to other tabs before I reload the page or else I will lose excutionof code that comes afterwards |
|
Well not actually broadcasting before, I only set the storage before executing the function so it should be okay? |
|
I guess it is ok. I think in normal circumstances the order should not matter. |
|
BTW the dual thing does not work yet. Logging in in one tab requires a refresh in the other tab. |
That's expected for logins, the PR only touches logout. |
|
I wonder about a possible issue with this PR. We are calling an Comments? If this is indeed an issue then we can extend the broadcast data structure with a field to indicate that the target function is |
|
Alternatively, since I think awaiting a non-async function is harmless (the await has no effect), we could always treat the target function as async. |
|
Well, we are writing pure vanilla Javascript in fishtest. async function someAsyncFunction() {
return 'someValue';
}
someAsyncFunction().catch(() => { });Check the link below, as using |
|
Of course you cannot use The problem with calling an |
|
Sure I'm all in for Error handling.. so I think that the correct way of dealing with such a case and other cases is to catch these errors with |
|
Actually in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function there are many examples of |
It's not just okay it's necessary if the main function of all is the async function. or else you will fall into infinite loop trying to call it with await, since using await not inside an async function is bad syntax? |
|
trying to call it with await* |
This situation occurs in Promise.all([spsa_promise, diff_promise, renderOnLoad])
.then(() => {
% if show_task >= 0:
scroll_to(${show_task});
document.getElementById("enclosure").style="visibility:visible;";
document.documentElement.style="overflow:scroll;";
% endif
});But I agree now that if one does not need the result and does not care about errors, and the order of things happening is not important, then calling an async function without await is ok... I think there are proposals to add toplevel await to javascript. This would make things more consistent. EDIT: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await#top_level_await (only for modules though). |
|
So what would, according to you, be the correct way to deal with a failing |
That's indeed an interesting topic! now since we use an httpOnly token for authTkt we cannot access the token with JavaScript so we cannot remove it from the client if the request fails and thus we cannot log out the user! Now in such case, we should handle the fetch error and the response not being ok indeed. I think that redirecting elsewhere if the request fails should not happen (i.e. needs fixing), additionally, I have seen people in such cases asking the user with an alert to clear his/her browser cookies for the website (I'm neutral about this one) or implementing a mechanism to keep trying to log out with a time interval (I'm also not so sure about this). |
Logs users out from other tabs if they try to use them while they have logged-out or lost cookies. Worth noting that the page was already unusable and the user couldn't do actions due to an expired CSRF token.
steps in the issue cannot reproduce the error. I can only see this scenario causing it. Fixes: #1839
Needs Dev testing.