-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor: Improve session management with jose #528
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
a0a795d
4167dfb
7763589
d3389b6
6c505ed
f32043d
3070469
3438bf8
ff20ffd
839ab59
2a19b55
0df3a55
8305513
c7e67a2
e854a69
a02d475
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| describe('useSession behavior', () => { | ||
| beforeEach(() => { | ||
| cy.setCookie('_consent', 'true'); | ||
| }); | ||
|
|
||
| it('shows logged in state with valid token', () => { | ||
| cy.then(() => { | ||
| let userCookie = Cypress.env('userCookie'); | ||
| if (typeof userCookie === 'object') { | ||
| userCookie = JSON.stringify(userCookie); | ||
| } | ||
| Cypress.log({ | ||
| name: 'diagnostics:userCookie', | ||
| message: [ | ||
| `type=${typeof userCookie}`, | ||
| `present=${Boolean(userCookie)}`, | ||
| `stringLength=${typeof userCookie === 'string' ? userCookie.length : 'n/a'}`, | ||
| `preview=${userCookie.substring(0, 50)}...`, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| cy.setCookie('_datacite', String(Cypress.env('userCookie')), { log: false }); | ||
| cy.visit('/'); | ||
| cy.get('#sign-in').should('contain.text', 'DataCite Test User'); // Match your JWT payload name | ||
| }); | ||
|
|
||
| it('shows logged out state without token', () => { | ||
| cy.visit('/'); | ||
| cy.get('#sign-in').should('contain.text', 'Sign In'); | ||
| }); | ||
|
|
||
| it('shows logged out state with invalid token', () => { | ||
| cy.setCookie('_datacite', '{"authenticated":{"access_token":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.invalid.signature"}}', { log: false }); | ||
| cy.visit('/'); | ||
| cy.get('#sign-in').should('contain.text', 'Sign In'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,15 +1,15 @@ | ||||||||||||||||||||
| 'use client' | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||||
| import { session } from 'src/utils/session'; | ||||||||||||||||||||
| import { useSession } from 'src/utils/session'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| interface Props { | ||||||||||||||||||||
| signedInContent: React.ReactNode | ||||||||||||||||||||
| signedOutContent: React.ReactNode | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| export default function NavRight({ signedInContent, signedOutContent }: Props) { | ||||||||||||||||||||
| const user = session() | ||||||||||||||||||||
| const { user } = useSession() | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (!user) return signedOutContent | ||||||||||||||||||||
| return signedInContent | ||||||||||||||||||||
|
Comment on lines
+12
to
15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle loading state to prevent UI flicker. The component ignores the Apply this diff to handle the loading state: export default function NavRight({ signedInContent, signedOutContent }: Props) {
- const { user } = useSession()
+ const { user, loading } = useSession()
+ if (loading) return signedOutContent // Or a loading skeleton
if (!user) return signedOutContent
return signedInContent
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,16 +8,16 @@ import { DATACITE_API_URL } from 'src/data/constants' | |
| * this throws an error unless the token is returned from a function that is called | ||
| * in the authLink setContext. I'm not sure why | ||
| */ | ||
| export default function apolloClientBuilder(getToken: () => string) { | ||
| export default function apolloClientBuilder(getToken: () => Promise<string | null>) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's check the builder.ts file to see the function signature and implementation
cd src/utils/apolloClient && cat -n builder.ts | head -30Repository: datacite/akita Length of output: 1356 🏁 Script executed: # Now let's examine provider.tsx to see how apolloClientBuilder is being called
cd src/utils/apolloClient && cat -n provider.tsx | head -20Repository: datacite/akita Length of output: 574 🏁 Script executed: # Search for all calls to apolloClientBuilder throughout the codebase
rg -nP --type=ts --type=tsx 'apolloClientBuilder\s*\(' --no-heading -A 2Repository: datacite/akita Length of output: 85 🏁 Script executed: # Search for apolloClientBuilder calls with adjusted file types
rg -nP --type ts -A 2 'apolloClientBuilder' --no-headingRepository: datacite/akita Length of output: 1278 🏁 Script executed: # Also search in JS/JSX files just in case
rg -nP --type js -A 2 'apolloClientBuilder' --no-headingRepository: datacite/akita Length of output: 40 Type mismatch: The The call in 🤖 Prompt for AI Agents |
||
| // needed for CORS, see https://www.apollographql.com/docs/react/networking/authentication/#cookie | ||
| const httpLink = createHttpLink({ | ||
| uri: DATACITE_API_URL + '/graphql', | ||
| credentials: 'include' | ||
| }) | ||
|
|
||
| const authLink = setContext((_, { headers }) => { | ||
| const authLink = setContext(async (_, { headers }) => { | ||
| // return the headers to the context so httpLink can read them | ||
| const token = getToken() | ||
| const token = await getToken() | ||
|
|
||
| return { | ||
| headers: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,46 @@ | ||
| import { useState, useEffect } from 'react' | ||
| import { Cookies } from 'react-cookie-consent' | ||
| import JsonWebToken from 'jsonwebtoken' | ||
| import { jwtVerify, importSPKI } from 'jose' | ||
| import { JWT_KEY } from 'src/data/constants' | ||
|
|
||
| export type User = { | ||
| uid: string, | ||
| name: string | ||
| } | null | ||
|
|
||
| export const session = () => { | ||
| // RSA public key | ||
| if (!JWT_KEY) return null | ||
| export const useSession = () => { | ||
| const [user, setUser] = useState<User>(null) | ||
| const [loading, setLoading] = useState(true) | ||
|
|
||
| const sessionCookie = Cookies.getJSON('_datacite') | ||
| const token = sessionCookie?.authenticated?.access_token | ||
| if (!token) return null | ||
| useEffect(() => { | ||
| const fetchUser = async () => { | ||
| // RSA public key | ||
| if (!JWT_KEY) { | ||
| setLoading(false) | ||
| return | ||
| } | ||
|
|
||
| let user: any = null | ||
| function setUser(error: any, payload: any) { | ||
| if (error) { | ||
| console.log('JWT verification error: ' + error.message) | ||
| return | ||
| } | ||
| const sessionCookie = Cookies.getJSON('_datacite') | ||
| const token = sessionCookie?.authenticated?.access_token | ||
| if (!token) { | ||
| setLoading(false) | ||
| return | ||
| } | ||
|
|
||
| user = payload | ||
| } | ||
| try { | ||
| const publicKey = await importSPKI(JWT_KEY, 'RS256') | ||
| const { payload } = await jwtVerify(token, publicKey) | ||
| setUser(payload as User) | ||
| } catch (error: any) { | ||
| console.log('JWT verification error: ' + error.message) | ||
| setUser(null) | ||
|
Comment on lines
+30
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate JWT payload structure before casting to User. Line 33 casts the JWT payload to Apply this diff to add validation: try {
const publicKey = await importSPKI(JWT_KEY, 'RS256')
const { payload } = await jwtVerify(token, publicKey)
- setUser(payload as User)
+ // Validate payload structure
+ if (payload && typeof payload.uid === 'string' && typeof payload.name === 'string') {
+ setUser({ uid: payload.uid, name: payload.name })
+ } else {
+ console.error('Invalid JWT payload structure:', payload)
+ setUser(null)
+ }
} catch (error: any) {🤖 Prompt for AI Agents |
||
| } finally { | ||
| setLoading(false) | ||
| } | ||
| } | ||
|
|
||
| // verify asymmetric token, using RSA with SHA-256 hash algorithm | ||
| JsonWebToken.verify(token, JWT_KEY, { algorithms: ['RS256'] }, setUser) | ||
| fetchUser() | ||
| }, []) | ||
|
|
||
| return user as User | ||
| return { user, loading } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle loading state to prevent premature error throwing.
Both
UserCommonsPageButtonandUserOrcidButtonthrow errors whenuseris null, butuseSession()returnsnullduring the initial loading phase. This causes these components to throw errors during normal operation before the session is loaded.Apply this diff to handle the loading state properly:
export function UserCommonsPageButton() { - const { user } = useSession() + const { user, loading } = useSession() + if (loading) return null // Or a loading skeleton if (!user) throw new Error("User not signed in") const href = '/orcid.org/' + user.uid return <DropdownItem href={href} eventKey={3.3} data-cy="commons-page"> <FontAwesomeIcon icon={faAddressCard} /> Commons Page </DropdownItem> } export function UserOrcidButton() { - const { user } = useSession() + const { user, loading } = useSession() + if (loading) return null // Or a loading skeleton if (!user) throw new Error("User not signed in") const href = `${ORCID_URL}/${user.uid}`Also applies to: 24-25
🤖 Prompt for AI Agents