Skip to content

[US-17.1 / OBT-254] fix(auth): commit session state atomically to stop Access Denied flash #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
levigtri wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[US-17.1 / OBT-254] fix(auth): commit session state atomically to stop Access Denied flash #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d379f98
fix(auth): commit session state atomically to stop Access Denied flash
levigtri Jul 13, 2026
aebbc9c
chore(ci): empty commit to re-trigger CI checks
levigtri Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ import PhasesPage from "@/components/pages/PhasesPage"
import MapPage from "@/components/pages/MapPage"
import NotFoundPage from "@/components/pages/NotFoundPage"
import AccessDeniedPage from "@/components/pages/AccessDeniedPage"
import { LoadingSpinner } from "@/components/common/LoadingSpinner"

function AdminRoute({ children }: { children: React.ReactNode }) {
const { isPlatformAdmin } = useAuth()
const { isPlatformAdmin, isLoading } = useAuth()
if (isLoading) return <LoadingSpinner size="lg" />
if (!isPlatformAdmin) return <AccessDeniedPage />
return <>{children}</>
}
Expand Down
88 changes: 53 additions & 35 deletions src/contexts/AuthContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,16 @@ import { createContext, useCallback, useContext, useEffect, useMemo, useState }
import { useNavigate } from "react-router-dom"
import { authAPI } from "@/services/api"
import { ACCESS_TOKEN_KEY, REFRESH_TOKEN_KEY } from "@/constants/app"
import { useLanguagesStore } from "@/stores/languagesStore"
import { usePhasesStore } from "@/stores/phasesStore"
import type { User, MyRoleResponse } from "@/types"

interface Session {
user: User
appRoles: MyRoleResponse[]
managedOrgIds: string[]
}

interface AuthContextValue {
user: User | null
isPlatformAdmin: boolean
Expand All @@ -20,26 +28,47 @@ interface AuthContextValue {

const AuthContext = createContext<AuthContextValue | null>(null)

const EMPTY_ROLES: MyRoleResponse[] = []
const EMPTY_IDS: string[] = []

// eslint-disable-next-line react-refresh/only-export-components
export function useAuth() {
const ctx = useContext(AuthContext)
if (!ctx) throw new Error("useAuth must be used within AuthProvider")
return ctx
}

function clearTokens() {
localStorage.removeItem(ACCESS_TOKEN_KEY)
localStorage.removeItem(REFRESH_TOKEN_KEY)
}

async function loadSession(knownUser?: User): Promise<Session> {
const [user, rolesRes, managedRes] = await Promise.all([
knownUser ? Promise.resolve(knownUser) : authAPI.me().then((res) => res.data),
authAPI.myRoles(),
authAPI.myManagedOrgs(),
])

return {
user,
appRoles: rolesRes.data,
managedOrgIds: managedRes.data.managed_org_ids,
}
}

export function AuthProvider({ children }: { children: React.ReactNode }) {
const [user, setUser] = useState<User | null>(null)
const [appRoles, setAppRoles] = useState<MyRoleResponse[]>([])
const [managedOrgIds, setManagedOrgIds] = useState<string[]>([])
const [session, setSession] = useState<Session | null>(null)
const [isLoading, setIsLoading] = useState(true)
const navigate = useNavigate()

const isPlatformAdmin = useMemo(() => user?.is_platform_admin ?? false, [user])
const isManager = useMemo(() => managedOrgIds.length > 0, [managedOrgIds])
const managedOrgId = useMemo(
() => (managedOrgIds.length === 1 ? managedOrgIds[0] : null),
[managedOrgIds],
)
const user = session?.user ?? null
const appRoles = session?.appRoles ?? EMPTY_ROLES
const managedOrgIds = session?.managedOrgIds ?? EMPTY_IDS

const isPlatformAdmin = user?.is_platform_admin ?? false
const isManager = managedOrgIds.length > 0
const managedOrgId = managedOrgIds.length === 1 ? managedOrgIds[0] : null

const isAppAdmin = useCallback(
(appKey: string) =>
Expand All @@ -52,23 +81,21 @@ export function AuthProvider({ children }: { children: React.ReactNode }) {
const { data } = await authAPI.login(email, password)
localStorage.setItem(ACCESS_TOKEN_KEY, data.tokens.access_token)
localStorage.setItem(REFRESH_TOKEN_KEY, data.tokens.refresh_token)
setUser(data.user)

const [rolesRes, managedRes] = await Promise.all([
authAPI.myRoles(),
authAPI.myManagedOrgs(),
])
setAppRoles(rolesRes.data)
setManagedOrgIds(managedRes.data.managed_org_ids)
try {
setSession(await loadSession(data.user))
} catch (error) {
clearTokens()
throw error
}

navigate("/app/dashboard")
},
[navigate],
)

const refreshUser = useCallback(async () => {
const { data } = await authAPI.me()
setUser(data)
setSession(await loadSession())
}, [])

const logout = useCallback(async () => {
Expand All @@ -80,17 +107,15 @@ export function AuthProvider({ children }: { children: React.ReactNode }) {
} catch {
// Ignore logout API errors — clear local state regardless
}
localStorage.removeItem(ACCESS_TOKEN_KEY)
localStorage.removeItem(REFRESH_TOKEN_KEY)
setUser(null)
setAppRoles([])
setManagedOrgIds([])
clearTokens()
setSession(null)
useLanguagesStore.getState().reset()
usePhasesStore.getState().reset()
navigate("/login")
}, [navigate])

useEffect(() => {
const token = localStorage.getItem(ACCESS_TOKEN_KEY)
if (!token) {
if (!localStorage.getItem(ACCESS_TOKEN_KEY)) {
setIsLoading(false)
return
}
Expand All @@ -99,20 +124,13 @@ export function AuthProvider({ children }: { children: React.ReactNode }) {

async function restoreSession() {
try {
const [meRes, rolesRes, managedRes] = await Promise.all([
authAPI.me(),
authAPI.myRoles(),
authAPI.myManagedOrgs(),
])
const restored = await loadSession()
if (!cancelled) {
setUser(meRes.data)
setAppRoles(rolesRes.data)
setManagedOrgIds(managedRes.data.managed_org_ids)
setSession(restored)
}
} catch {
if (!cancelled) {
localStorage.removeItem(ACCESS_TOKEN_KEY)
localStorage.removeItem(REFRESH_TOKEN_KEY)
clearTokens()
}
} finally {
if (!cancelled) {
Expand Down
5 changes: 5 additions & 0 deletions src/stores/languagesStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ interface LanguagesStore {
lastFetched: number | null
fetch: () => Promise<LanguageResponse[]>
invalidate: () => void
reset: () => void

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

reset() doesn't guard against an in-flight fetch() clobbering the reset state.

If reset() (called from logout()) runs while a fetch() is mid-flight, the pending call's .then still unconditionally commits { languages: data, lastFetched: Date.now(), loading: false } once it resolves — overwriting the just-reset store with data fetched under the previous session/account. Any caller queued on the "duplicate fetch" subscriber (Lines 30-37) also resolves early with the emptied languages array instead of the real result.

♻️ Proposed fix using a generation counter 
+let generation = 0
+
 export const useLanguagesStore = create<LanguagesStore>((set, get) => ({
   languages: [],
   loading: false,
   lastFetched: null,

   fetch: async () => {
     const state = get()
     if (state.lastFetched && Date.now() - state.lastFetched < CACHE_TTL && state.languages.length > 0) {
       return state.languages
     }
     if (state.loading) {
       return new Promise<LanguageResponse[]>((resolve) => {
         const unsub = useLanguagesStore.subscribe((s) => {
           if (!s.loading) {
             unsub()
             resolve(s.languages)
           }
         })
       })
     }
+    const myGeneration = generation
     set({ loading: true })
     try {
       const { data } = await languagesAPI.list()
-      set({ languages: data, lastFetched: Date.now(), loading: false })
+      if (myGeneration !== generation) return get().languages
+      set({ languages: data, lastFetched: Date.now(), loading: false })
       return data
     } catch {
-      set({ loading: false })
+      if (myGeneration === generation) set({ loading: false })
       return state.languages
     }
   },

   invalidate: () => {
     set({ lastFetched: null })
   },

   reset: () => {
+    generation++
     set({ languages: [], loading: false, lastFetched: null })
   },

Also applies to: 54-57

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/stores/languagesStore.ts` at line 11, Protect the languages store from
stale in-flight requests by adding a generation counter shared by reset() and
fetch(). Increment the generation in reset(), and have each fetch capture its
generation before awaiting; only the matching request may commit state or
resolve duplicate-fetch subscribers. Invalidate or retain those subscribers
until a current-generation fetch completes so they do not receive reset’s empty
languages result.

getLanguageName: (langId: string) => string
}

Expand Down Expand Up @@ -50,6 +51,10 @@ export const useLanguagesStore = create<LanguagesStore>((set, get) => ({
set({ lastFetched: null })
},

reset: () => {
set({ languages: [], loading: false, lastFetched: null })
},

getLanguageName: (langId: string) => {
const lang = get().languages.find((l) => l.id === langId)
return lang ? `${lang.name} (${lang.code})` : langId
Expand Down
5 changes: 5 additions & 0 deletions src/stores/phasesStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ interface PhasesStore {
lastFetched: number | null
fetch: () => Promise<void>
invalidate: () => void
reset: () => void

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

reset() doesn't guard against an in-flight fetch() clobbering the reset state.

Same issue as languagesStore.ts: if a fetch() is in flight when reset() (called from logout()) runs, the pending call still unconditionally commits { phases: data, dependencies: depsMap, lastFetched: Date.now(), loading: false } once it resolves, silently undoing the reset with data fetched under the previous session.

Also applies to: 50-53

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/stores/phasesStore.ts` at line 12, Update the phasesStore fetch/reset
flow so an in-flight fetch cannot commit results after reset() invalidates the
current session state. Add the same request-generation or cancellation guard
used by languagesStore.ts, and have the fetch completion update phases,
dependencies, lastFetched, and loading only when it still belongs to the active
generation; preserve reset()’s cleared state otherwise.

}

const CACHE_TTL = 2 * 60 * 1000 // 2 minutes
Expand Down Expand Up @@ -46,4 +47,8 @@ export const usePhasesStore = create<PhasesStore>((set, get) => ({
invalidate: () => {
set({ lastFetched: null })
},

reset: () => {
set({ phases: [], dependencies: new Map(), loading: false, lastFetched: null })
},
}))
Loading