Copilot commented on code in PR #11799: URL: https://github.com/apache/gravitino/pull/11799#discussion_r3473122551
########## web-v2/web/src/lib/hooks/useAbsoluteSessionTimeout.js: ########## @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +'use client' + +import { useEffect, useRef, useState, useCallback } from 'react' + +/** + * localStorage key for the session start timestamp. + * Stored in localStorage (not sessionStorage) so all tabs from the same + * browser share the same session start time. + */ +const SESSION_START_KEY = 'grtv-session-start' + +/** + * Default maximum session duration: 5 hours in milliseconds. + * Overridable via NEXT_PUBLIC_MAX_SESSION_DURATION_MS environment variable. + */ +const DEFAULT_MAX_SESSION_DURATION_MS = (() => { + const envVal = process.env.NEXT_PUBLIC_MAX_SESSION_DURATION_MS + const parsed = envVal ? Number(envVal) : NaN + + return Number.isFinite(parsed) && parsed > 0 ? parsed : 5 * 60 * 60 * 1000 +})() + +/** + * Custom hook for absolute session duration enforcement. + * + * Unlike idle timeout (which resets on user activity), the absolute session + * timer starts when the user logs in and never resets. After the configured + * duration (default: 5 hours), the session is forcibly terminated. + * + * The session start time is stored in localStorage so all tabs from the same + * browser share the same absolute deadline. + * + * @param {Object} options + * @param {boolean} options.isAuthenticated - Whether the user is currently authenticated. + * When true and no session start exists, the current time is recorded. + * When false, the session start is cleared. + * @param {number} [options.maxDurationMs] - Maximum session duration in milliseconds. + * Defaults to NEXT_PUBLIC_MAX_SESSION_DURATION_MS env var or 5 hours. + * @returns {{ isExpired: boolean, remainingMs: number, clearSession: () => void }} + */ +export function useAbsoluteSessionTimeout({ isAuthenticated, maxDurationMs = DEFAULT_MAX_SESSION_DURATION_MS } = {}) { + const [isExpired, setIsExpired] = useState(false) + const [remainingMs, setRemainingMs] = useState(maxDurationMs) + const intervalRef = useRef(null) + + // Record session start time when user becomes authenticated + useEffect(() => { + if (isAuthenticated) { + const existing = localStorage.getItem(SESSION_START_KEY) + + if (existing) { + // Session start exists (e.g., another tab logged in). Recalculate + // expiry so that if a new login refreshed the timestamp, this tab + // clears its stale isExpired flag. + const start = Number(existing) + const elapsed = Date.now() - start + const remaining = Math.max(0, maxDurationMs - elapsed) + + setRemainingMs(remaining) + if (remaining > 0) { + setIsExpired(false) + } + } else { + localStorage.setItem(SESSION_START_KEY, String(Date.now())) + } + } else { + // Clear session start when user is not authenticated + localStorage.removeItem(SESSION_START_KEY) + setIsExpired(false) + setRemainingMs(maxDurationMs) + } + }, [isAuthenticated, maxDurationMs]) + + // Poll every second to check if the absolute timeout has been reached + useEffect(() => { + if (!isAuthenticated) { + return + } + + const check = () => { + const startStr = localStorage.getItem(SESSION_START_KEY) + + if (!startStr) { + return + } + + const start = Number(startStr) + const now = Date.now() + const elapsed = now - start + const remaining = Math.max(0, maxDurationMs - elapsed) + + setRemainingMs(remaining) + + if (remaining <= 0) { + setIsExpired(true) + } + } Review Comment: `check()` only ever sets `isExpired` to `true` when `remaining <= 0`, but never clears it when `remaining` becomes positive again (e.g., another tab refreshes `grtv-session-start` after a re-login). This can leave a tab permanently "expired" even though a new session started. ########## web-v2/web/src/lib/provider/IdleSessionProvider.js: ########## @@ -84,12 +85,16 @@ export default function IdleSessionProvider({ const authToken = useAppSelector(state => state.auth.authToken) // Only enable idle timeout when the user is authenticated and not on login page. - // During bootstrap (authType === null), fall back to persisted token in localStorage - // to avoid prematurely treating the user as unauthenticated. + // Fall back to persisted token in localStorage to avoid prematurely treating + // the user as unauthenticated during the bootstrap window where authType has + // been set (via getAuthConfigs) but authToken has not yet been dispatched + // (initAuth is still running). Without this fallback, a race between the two + // Redux updates causes isAuthenticated to briefly flip to false, which + // triggers a cross-tab logout via BroadcastChannel. const isAuthenticated = authType === 'simple' ? !!sessionStorage.getItem('simpleAuthUser') - : !!(authToken || (authType === null && localStorage.getItem('accessToken'))) + : !!(authToken || localStorage.getItem('accessToken')) const isLoginPage = pathname === '/login' Review Comment: `usePathname()` can include the configured `basePath` (e.g. `/ui/login`), but this check only matches `/login`. That can cause the warning modal to render on the login page and can also affect other login-page gating logic. ########## web-v2/web/src/lib/hooks/useAbsoluteSessionTimeout.js: ########## @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +'use client' + +import { useEffect, useRef, useState, useCallback } from 'react' + +/** + * localStorage key for the session start timestamp. + * Stored in localStorage (not sessionStorage) so all tabs from the same + * browser share the same session start time. + */ +const SESSION_START_KEY = 'grtv-session-start' + +/** + * Default maximum session duration: 5 hours in milliseconds. + * Overridable via NEXT_PUBLIC_MAX_SESSION_DURATION_MS environment variable. + */ +const DEFAULT_MAX_SESSION_DURATION_MS = (() => { + const envVal = process.env.NEXT_PUBLIC_MAX_SESSION_DURATION_MS + const parsed = envVal ? Number(envVal) : NaN + + return Number.isFinite(parsed) && parsed > 0 ? parsed : 5 * 60 * 60 * 1000 +})() + +/** + * Custom hook for absolute session duration enforcement. + * + * Unlike idle timeout (which resets on user activity), the absolute session + * timer starts when the user logs in and never resets. After the configured + * duration (default: 5 hours), the session is forcibly terminated. + * + * The session start time is stored in localStorage so all tabs from the same + * browser share the same absolute deadline. + * + * @param {Object} options + * @param {boolean} options.isAuthenticated - Whether the user is currently authenticated. + * When true and no session start exists, the current time is recorded. + * When false, the session start is cleared. + * @param {number} [options.maxDurationMs] - Maximum session duration in milliseconds. + * Defaults to NEXT_PUBLIC_MAX_SESSION_DURATION_MS env var or 5 hours. + * @returns {{ isExpired: boolean, remainingMs: number, clearSession: () => void }} + */ +export function useAbsoluteSessionTimeout({ isAuthenticated, maxDurationMs = DEFAULT_MAX_SESSION_DURATION_MS } = {}) { + const [isExpired, setIsExpired] = useState(false) + const [remainingMs, setRemainingMs] = useState(maxDurationMs) + const intervalRef = useRef(null) + + // Record session start time when user becomes authenticated + useEffect(() => { + if (isAuthenticated) { + const existing = localStorage.getItem(SESSION_START_KEY) + + if (existing) { + // Session start exists (e.g., another tab logged in). Recalculate + // expiry so that if a new login refreshed the timestamp, this tab + // clears its stale isExpired flag. + const start = Number(existing) + const elapsed = Date.now() - start + const remaining = Math.max(0, maxDurationMs - elapsed) + + setRemainingMs(remaining) + if (remaining > 0) { + setIsExpired(false) + } Review Comment: When `grtv-session-start` exists, the code assumes it parses to a valid timestamp. If the value is corrupted/non-numeric, `remaining` becomes `NaN` and state updates will propagate `NaN` through the UI/logic. Consider validating the parsed value and resetting the key when invalid. ########## web-v2/web/src/app/login/page.js: ########## @@ -83,6 +84,16 @@ const LoginContent = () => { /> )} + {maxDurationReason && ( + <Alert + message='Your session has reached its maximum duration. Please sign in again.' + type='warning' + showIcon + closable + className='mb-6' + /> + )} Review Comment: The PR metadata says this does not introduce a user-facing change, but this adds a new visible login-page alert/message for the `max_duration` logout reason. Please update the PR description/template fields accordingly so reviewers and release notes are accurate. ########## web-v2/web/src/lib/provider/IdleSessionProvider.js: ########## @@ -193,6 +218,15 @@ export default function IdleSessionProvider({ if (!loggedOutRef.current) { loggedOutRef.current = true dispatch(logoutAction({ router, reason: message.reason })) + + // Same safety net as handleLogout — ensures navigation even if + // logoutAction's internal router.push never executes. + const loginUrl = message.reason ? `/login?reason=${encodeURIComponent(message.reason)}` : '/login' + setTimeout(() => { + if (window.location.pathname !== '/login') { + router.push(loginUrl) + } + }, 1500) Review Comment: Same basePath issue as the local logout path: the guard `window.location.pathname !== '/login'` doesn't match `/ui/login`, so this fallback redirect can re-run unnecessarily on basePath deployments. ########## web-v2/web/src/lib/provider/IdleSessionProvider.js: ########## @@ -145,21 +155,36 @@ export default function IdleSessionProvider({ }, [state, idleTimeRemaining]) // Handle logout (either from timeout or "Sign out now") - const handleLogout = useCallback(() => { - if (loggedOutRef.current) { - return - } - - loggedOutRef.current = true - setState('expired') + const handleLogout = useCallback( + (reason = 'inactive') => { + if (loggedOutRef.current) { + return + } - // Broadcast logout with reason to other tabs - sendMessage({ type: 'logout', reason: 'inactive', timestamp: Date.now() }) + loggedOutRef.current = true + setState('expired') - // Dispatch logout action (handles both OAuth and simple auth) - // Pass reason to show inactivity message on login page - dispatch(logoutAction({ router, reason: 'inactive' })) - }, [dispatch, router, sendMessage]) + // Broadcast logout with reason to other tabs + sendMessage({ type: 'logout', reason, timestamp: Date.now() }) + + // Dispatch logout action (handles both OAuth and simple auth) + // Pass reason to show appropriate message on login page + dispatch(logoutAction({ router, reason })) + + // Safety net: navigate to login page after a short delay. + // logoutAction is an async thunk — if it rejects (e.g., OIDC + // signoutRedirect throws) the error is silently swallowed and + // router.push inside the thunk never executes. This fallback + // guarantees the user lands on /login regardless. + const loginUrl = reason ? `/login?reason=${encodeURIComponent(reason)}` : '/login' + setTimeout(() => { + if (window.location.pathname !== '/login') { + router.push(loginUrl) + } + }, 1500) Review Comment: The safety-net redirect checks `window.location.pathname !== '/login'`, which won't be true when the app is served under a basePath (e.g. `/ui/login`). This can cause redundant redirects and makes the guard ineffective on the actual login route. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
