Conversation
There was a problem hiding this comment.
Is there a specific reason for 15 min? If not, why don't we make this configurable? + docs would be welcome :)
There was a problem hiding this comment.
@Chaho12 mostly pentester recommend 15 minutes for UI session. That's why. otherwise I can make it configurable. and which docs I should update?
There was a problem hiding this comment.
Since Tomcat, Spring is 30 min, i thought 30 min was default.
https://trinodb.github.io/trino-gateway/security/ this page sounds fit.
There was a problem hiding this comment.
@Chaho12 okay, I will make this 30. and do I make this configurable as well?
There was a problem hiding this comment.
also, please use airlift's Duration class
There was a problem hiding this comment.
let's not use reflection for this
| public class LbFormAuthManager | ||
| { | ||
| private static final Logger log = Logger.get(LbFormAuthManager.class); | ||
| private static final long SERVER_START_TIME = System.currentTimeMillis(); |
There was a problem hiding this comment.
if this is intended to be shared by multiple classes, we can move it out from this class and then create a separate config class. OR we can create a getter for this field
There was a problem hiding this comment.
also, please use airlift's Duration class
There was a problem hiding this comment.
can we wrap it in Optional so it's clear that it's nullable?
There was a problem hiding this comment.
can we wrap it in Optional so it's clear that it's nullable?
There was a problem hiding this comment.
@andythsu I have resolved other comments in this forced pushed pr.
| } | ||
| } catch (error) { | ||
| // Token validation failed, user will be logged out | ||
| throw error; |
There was a problem hiding this comment.
Are we doing anything in catch? Otherwise it may be unnecessary to catch the error
There was a problem hiding this comment.
@andythsu yes, we are throwing error as token validation failed and user will be logged out in that case
| } | ||
| } catch (error) { | ||
| console.error('Error checking server info:', error); | ||
| // Don't logout on API error, just continue |
There was a problem hiding this comment.
should we log out here? technically we should never end up in this state, but if we do, it means the server is having issues.
There was a problem hiding this comment.
@andythsu No, we are not logging out here, we are logging the server error here
|
@andythsu can you please review once? |
|
As it stands this change is not useful. Users should NOT be logged out just because one Trino Gateway server shuts down. Trino Gateway can run as a cluster and users should not be logged out but instead remain active and continue to be able to use everything. |
Hello @mosabua , understand, will it be helpful if I discard the changes related to logout in server detection and keep the changes of 30 minute session timeout if user remains idle. Can this be taken if Trino Gateway Run as a cluster? |
|
Yes .. I think an idle logout might still be a good thing .. as long as it also works if Trino Gateway runs as cluster |
|
Hello @mosabua if you suggesting Gateway run as a cluster means, HA proxy above gateway cluster, then the above changes works fine. I have already validated that. If one of the server restarts it does not throw away users logged off. only if HA cluster is restarted then only it logged off the user for a new session. |
|
@sug-ghosh Can you rebase so that it get be ready for final review? |
|
This pull request has gone a while without any activity. Ask for help on #trino-gateway-dev on Trino slack. |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
4 participants
Description
fixes #717
Features Implemented
Backend: JWT tokens now expire after 15 minutes instead of 24 hours
Frontend: Client-side session management tracks user activity and automatically logs out after 15 minutes of inactivity
Activity Detection: Mouse movements, clicks, keyboard input, scrolling, and touch events reset the timeout
Backend: JWT tokens include a server start timestamp claim
Frontend: Checks server start time on every API call to detect server restarts immediately
Automatic Logout: Users are automatically logged out on any action after server restart
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required, with the following suggested text:
* Fix some things.