Refactor: Improve session management with jose #528
Conversation
WalkthroughReplaces synchronous session() with a useSession React hook (async JWT verification via jose), makes getAuthToken() async and propagates async token retrieval through Apollo client/provider and layout, swaps jsonwebtoken for jose, adds Cypress E2E tests, and updates Cypress config/workflow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser (React UI)
participant Hook as useSession (hook)
participant Cookies as CookieStore
participant Verifier as jose (importSPKI / jwtVerify)
participant Apollo as Apollo Client (authLink)
participant GraphQL as GraphQL Server
Browser->>Hook: mount -> useEffect
Hook->>Cookies: read "_datacite" cookie
Cookies-->>Hook: token?
alt token present
Hook->>Verifier: importSPKI & jwtVerify(token, pubkey)
Verifier-->>Hook: payload / error
alt verified
Hook-->>Browser: set user, loading=false
else verification failed
Hook-->>Browser: set user=null, loading=false
end
else no token
Hook-->>Browser: set user=null, loading=false
end
Browser->>Apollo: GraphQL request
Apollo->>Hook: await getAuthToken() (async)
Hook->>Cookies: read "_datacite"
Hook-->>Apollo: token or null
Apollo->>GraphQL: attach Authorization header if token present and send request
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
cypress/e2e/session.test.ts (1)
6-10: Consider making the expected user name configurable.The test hardcodes the expected user name as 'Martin Fenner', which creates a tight coupling to the test environment's
userCookie. Consider extracting the expected name from the same environment variable or making the assertion more flexible.Apply this diff to make the test more maintainable:
it('shows logged in state with valid token', () => { cy.setCookie('_datacite', Cypress.env('userCookie'), { log: false }); cy.visit('/'); - cy.get('#sign-in').should('contain.text', 'Martin Fenner'); // Adjust to expected user name from env cookie + cy.get('#sign-in').should('not.contain.text', 'Sign In'); // Verify user is signed in + // Or extract expected name: cy.get('#sign-in').should('contain.text', Cypress.env('expectedUserName')); });src/components/Claim/Claim.tsx (1)
83-83: Consider handling session loading state for better UX.The component ignores the
loadingstate fromuseSession(), causing buttons to be disabled during the initial session load even for authenticated users. While the component already handles its own claim query loading, the session loading could create a brief moment where buttons appear disabled when they shouldn't be.Consider destructuring
loadingand handling it appropriately:- const { user } = useSession() + const { user, loading: sessionLoading } = useSession()Then use
sessionLoadingin the button disabled logic or show a loading state.Also applies to: 153-153, 164-164
src/components/DiscoverWorksAlert/DiscoverWorksAlert.tsx (1)
17-17: Consider using the loading state for better UX.The
useSessionhook provides aloadingstate, but it's not currently used. During the loading phase,userisnull, causing the component to render nothing (line 21). This may cause a brief flash of no content while the session is being verified.Apply this diff to handle the loading state:
- const { user } = useSession() + const { user, loading } = useSession() const [show, setShow] = useState(true); - if (!user) return + if (loading || !user) return nullsrc/utils/apolloClient/apolloClient.ts (1)
4-8: Async cookies() migration is correct for Next.js 15.The migration to
await cookies()aligns with Next.js 15's async Request APIs requirement. The token extraction logic remains functionally equivalent.However, line 6 uses
as anywhich bypasses type safety. Consider defining a proper type for the cookie structure:type DataCiteCookie = { authenticated?: { access_token?: string } } export async function getAuthToken() { const cookieStore = await cookies() const cookieValue = cookieStore.get('_datacite')?.value const sessionCookie: DataCiteCookie = cookieValue ? JSON.parse(cookieValue) : {} return sessionCookie?.authenticated?.access_token }src/utils/session.ts (2)
15-28: Token retrieval logic is correct but won't update on cookie changes.The implementation correctly retrieves the token from cookies and handles missing JWT_KEY or token gracefully. However, the empty dependency array on line 43 means the effect only runs once on mount. If the user logs in/out in another tab, this component won't detect the cookie change.
Consider adding a mechanism to detect cookie changes, such as a periodic check or window focus listener:
useEffect(() => { const checkSession = () => { // existing fetchUser logic } checkSession() // Re-check on window focus window.addEventListener('focus', checkSession) return () => window.removeEventListener('focus', checkSession) }, [])
35-35: Replace console.log with proper error logging.Using
console.logfor error messages is not suitable for production. Consider using a proper logging service or at minimum useconsole.error.Apply this diff:
- console.log('JWT verification error: ' + error.message) + console.error('JWT verification error:', error.message)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
cypress.config.ts(1 hunks)cypress/e2e/session.test.ts(1 hunks)package.json(1 hunks)src/app/(main)/layout.tsx(2 hunks)src/components/Claim/Claim.tsx(2 hunks)src/components/DiscoverWorksAlert/DiscoverWorksAlert.tsx(1 hunks)src/components/Header/ClientButtons.tsx(2 hunks)src/components/Header/Dropdown.tsx(1 hunks)src/components/Header/NavRight.tsx(1 hunks)src/utils/apolloClient/apolloClient.ts(1 hunks)src/utils/apolloClient/builder.ts(1 hunks)src/utils/session.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/utils/apolloClient/builder.ts (2)
src/utils/apolloClient/provider.tsx (1)
ApolloProvider(5-11)src/app/(main)/Providers.tsx (1)
Providers(18-36)
src/components/Header/ClientButtons.tsx (1)
src/utils/session.ts (1)
useSession(11-46)
src/app/(main)/layout.tsx (3)
src/utils/apolloClient/apolloClient.ts (1)
getAuthToken(4-8)src/app/(main)/Providers.tsx (2)
Providers(18-36)Props(12-14)src/utils/apolloClient/provider.tsx (1)
ApolloProvider(5-11)
src/components/Header/NavRight.tsx (1)
src/utils/session.ts (1)
useSession(11-46)
src/components/Claim/Claim.tsx (1)
src/utils/session.ts (1)
useSession(11-46)
src/components/DiscoverWorksAlert/DiscoverWorksAlert.tsx (1)
src/utils/session.ts (1)
useSession(11-46)
src/components/Header/Dropdown.tsx (1)
src/utils/session.ts (1)
useSession(11-46)
src/utils/session.ts (4)
src/data/constants.ts (1)
JWT_KEY(51-51)src/utils/apolloClient/provider.tsx (1)
ApolloProvider(5-11)src/data/queries/searchPersonQuery.ts (1)
useSearchPersonQuery(50-54)src/data/queries/claimQuery.ts (1)
useGetClaimQuery(5-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (5)
🔇 Additional comments (5)
cypress.config.ts (1)
9-9: LGTM - cleanup of unused parameters.Removing unused parameters from
setupNodeEventsimproves code clarity.Also applies to: 14-14
cypress/e2e/session.test.ts (1)
17-21: Good test coverage for invalid token scenario.The test correctly validates that an invalid JWT signature results in the logged-out state, which verifies the jose JWT verification logic.
src/app/(main)/layout.tsx (1)
45-45: LGTM - efficient token retrieval pattern.Fetching the auth token once and reusing it across the component tree is the correct approach, avoiding redundant async calls.
Also applies to: 58-58
src/utils/session.ts (2)
11-13: LGTM: Proper initialization of hook state.The initial state correctly sets
usertonullandloadingtotrue, following React best practices for async data fetching.
42-45: LGTM: Hook execution and return value.The
useEffectcorrectly executesfetchUseron mount with an empty dependency array, and the hook returns the expected{ user, loading }object.
| const { user } = useSession() | ||
| if (!user) throw new Error("User not signed in") |
There was a problem hiding this comment.
Handle loading state to prevent premature error throwing.
Both UserCommonsPageButton and UserOrcidButton throw errors when user is null, but useSession() returns null during 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
In src/components/Header/ClientButtons.tsx around lines 13-14 and 24-25, the
components currently throw an Error when user is falsy but useSession() can be
null during initial loading; update the components to handle the loading state
by checking the session status (or user presence) before throwing: if session is
undefined or status is "loading" return null or a lightweight loading
placeholder, only throw or render error UI when status is "unauthenticated" or
when you explicitly know loading finished and user is still null; replace direct
throw new Error("User not signed in") with guarded logic that first checks
loading and returns early to avoid premature errors.
| const { user } = useSession() | ||
|
|
||
| if (!user) return signedOutContent | ||
| return signedInContent |
There was a problem hiding this comment.
Handle loading state to prevent UI flicker.
The component ignores the loading state from useSession(), causing it to render signedOutContent during the initial load even when a user is authenticated. This creates a visible flash of the "Sign In" UI before the user information appears.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { user } = useSession() | |
| if (!user) return signedOutContent | |
| return signedInContent | |
| const { user, loading } = useSession() | |
| if (loading) return signedOutContent | |
| if (!user) return signedOutContent | |
| return signedInContent |
🤖 Prompt for AI Agents
In src/components/Header/NavRight.tsx around lines 12 to 15, the component
currently only checks user and returns signedOutContent which causes a flash;
update the logic to also read the loading flag from useSession() and while
loading return a neutral placeholder (e.g., null or a spinner) to avoid
rendering signedOutContent, then when loading is false return signedInContent if
user exists otherwise signedOutContent; ensure you destructure loading from
useSession(), use it first in the conditional, and keep existing
signedInContent/signedOutContent flows intact.
| * in the authLink setContext. I'm not sure why | ||
| */ | ||
| export default function apolloClientBuilder(getToken: () => string) { | ||
| export default function apolloClientBuilder(getToken: () => Promise<string | null>) { |
There was a problem hiding this comment.
🧩 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: provider.tsx passes synchronous function where async function is expected.
The getToken parameter in apolloClientBuilder (line 11) now requires () => Promise<string | null>, but provider.tsx (line 7) passes () => token which returns a synchronous string. This will cause a TypeScript type error.
The call in apolloClient.ts (line 10) correctly passes getAuthToken, an async function that matches the expected signature. However, the provider.tsx implementation must be updated to wrap the token in a Promise to maintain type compatibility.
🤖 Prompt for AI Agents
In src/utils/apolloClient/builder.ts around line 11, the builder currently
expects getToken: () => Promise<string | null> but provider.tsx passes a
synchronous function; update provider.tsx (around line 7) to return a Promise by
wrapping the existing synchronous token return in a Promise (e.g., return
Promise.resolve(token) or make the function async and return token) so its
signature matches () => Promise<string | null>, ensuring the types align with
apolloClientBuilder.
| 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) |
There was a problem hiding this comment.
Validate JWT payload structure before casting to User.
Line 33 casts the JWT payload to User without validation. If the JWT contains unexpected fields or is missing required fields (uid, name), this could cause runtime errors in components that depend on these properties.
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
In src/utils/session.ts around lines 30 to 36, the JWT payload is being cast
directly to User after verification without checking required fields; validate
the payload shape before calling setUser by confirming required properties
(e.g., uid and name) exist and have the expected types, and only then call
setUser(payload as User); if validation fails, log a descriptive message and
call setUser(null) to avoid runtime errors in components.
akita
|
||||||||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
jwt-and-cookies
|
| Run status |
|
| Run duration | 01m 41s |
| Commit |
|
| Committer | Joseph Rhoads |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
3
|
Skipped
|
0
|
Passing
|
49
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/session.test.ts • 1 failed test • Tests
| Test | Artifacts | |
|---|---|---|
| useSession behavior > shows logged in state with valid token |
Test Replay
Screenshots
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cypress/e2e/session.test.ts (1)
6-22: Test logic is correct; consider removing diagnostic logging.The test correctly validates the logged-in state. However, the diagnostic logging block (lines 9-16) appears to be debugging code that could be removed unless it's intentionally kept for CI troubleshooting.
If the diagnostics aren't needed, apply this diff:
it('shows logged in state with valid token', () => { - cy.then(() => { - const userCookie = Cypress.env('userCookie'); - Cypress.log({ - name: 'diagnostics:userCookie', - message: [ - `type=${typeof userCookie}`, - `present=${Boolean(userCookie)}`, - `stringLength=${typeof userCookie === 'string' ? userCookie.length : 'n/a'}`, - ], - }); - }); - cy.setCookie('_datacite', Cypress.env('userCookie'), { log: false }); cy.visit('/'); cy.get('#sign-in').should('contain.text', 'Martin Fenner'); // Adjust to expected user name from env cookie
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/session.test.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
cypress/e2e/session.test.ts
[high] 30-30: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (3)
🔇 Additional comments (3)
cypress/e2e/session.test.ts (3)
1-4: LGTM! Test suite setup follows established patterns.The test structure and consent cookie setup align with other Cypress tests in the codebase.
24-27: LGTM! Clean test case for logged-out state.The test correctly validates that users see "Sign In" when no authentication token is present.
29-33: LGTM! Valid test with intentionally invalid token.The test correctly validates that users with invalid tokens are treated as logged out. The hardcoded JWT with an invalid signature is appropriate test data.
Note on static analysis warning: The Gitleaks warning flagging line 30 as a "Generic API Key" is a false positive. This is intentionally invalid test data with a clearly fake signature ("invalid.signature"), not a real credential.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cypress/e2e/session.test.ts (2)
7-17: Remove or comment out diagnostic logging.The diagnostic logging block appears to be debug code. Consider removing it to keep the test clean, or comment it out if needed for troubleshooting.
Apply this diff:
- cy.then(() => { - const userCookie = Cypress.env('userCookie'); - Cypress.log({ - name: 'diagnostics:userCookie', - message: [ - `type=${typeof userCookie}`, - `present=${Boolean(userCookie)}`, - `stringLength=${typeof userCookie === 'string' ? userCookie.length : 'n/a'}`, - ], - }); - }); - cy.setCookie('_datacite', Cypress.env('userCookie'));
19-19: Add{ log: false }option for consistency and security.To prevent sensitive cookie values from appearing in Cypress logs, add the
{ log: false }option. This is consistent with line 30 and follows the pattern used in other test files.Apply this diff:
- cy.setCookie('_datacite', Cypress.env('userCookie')); + cy.setCookie('_datacite', Cypress.env('userCookie'), { log: false });Based on relevant code snippet from cypress/e2e/claim.test.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/session.test.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
cypress/e2e/session.test.ts
[high] 30-30: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (4)
🔇 Additional comments (1)
cypress/e2e/session.test.ts (1)
30-30: Static analysis false positive.The static analysis tool flagged this line as containing a potential API key. However, this is a test JWT token with an intentionally invalid signature (note the "invalid.signature" suffix), used specifically to test the handling of malformed tokens. This warning can be safely ignored.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cypress_tests.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (1)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cypress/e2e/session.test.ts (3)
6-10: Reduce coupling to a specific user name / env shapeThe assertion on Line 9 hard-codes
"DataCite"and the inline comment reads like a TODO. This couples the spec to a particular cookie payload and CI user. Consider asserting a more generic logged‑in signal (e.g., that the element no longer contains"Sign In"or uses a stable data attribute), or deriving the expected label from configuration/env instead of a literal.
12-21: Make logged‑out tests robust to future cookie‑preservation configThe “logged out” tests rely on Cypress clearing cookies between tests. If cookie‑preservation is ever enabled, these could become order‑dependent. To keep them self‑contained, consider explicitly clearing
_dataciteat the start of the logged‑out specs:it('shows logged out state without token', () => { + cy.clearCookie('_datacite'); 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":"…"}}', { log: false }); + cy.clearCookie('_datacite'); + cy.setCookie('_datacite', '{"authenticated":{"access_token":"…"}}', { log: false }); cy.visit('/'); cy.get('#sign-in').should('contain.text', 'Sign In'); });
17-21: Clarify dummy JWT to avoid secret‑scanner noiseStatic analysis (Gitleaks) is flagging the hard‑coded JWT‑like value on Line 18 as a generic API key. It’s clearly an invalid, fixture‑style token (ending in
.invalid.signature), so it’s not a real secret, but you may want to either shorten/obfuscate it or add a brief comment like// dummy JWT for invalid-token testto avoid recurring scanner noise or future confusion about whether this is sensitive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/session.test.ts(1 hunks)src/components/Header/Dropdown.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Header/Dropdown.tsx
🧰 Additional context used
🪛 Gitleaks (8.30.0)
cypress/e2e/session.test.ts
[high] 18-18: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (4)
akita
|
||||||||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
jwt-and-cookies
|
| Run status |
|
| Run duration | 01m 40s |
| Commit |
|
| Committer | jrhoads |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
1
|
|
Flaky
|
0
|
|
Pending
|
3
|
|
Skipped
|
0
|
|
Passing
|
49
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/session.test.ts • 1 failed test • Tests
| Test | Artifacts | |
|---|---|---|
| useSession behavior > shows logged in state with valid token |
Test Replay
Screenshots
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cypress/e2e/session.test.ts (1)
7-17: Consider removing diagnostic logging.The diagnostic logging block is useful for debugging but adds noise to the test output. Consider removing it once the tests are stable, or make it conditional based on a debug flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/session.test.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
cypress/e2e/session.test.ts
[high] 29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (2)
🔇 Additional comments (4)
cypress/e2e/session.test.ts (4)
1-4: LGTM!The test suite structure and consent cookie setup follow the established pattern across other Cypress test files in the codebase.
18-20: Verify environment variable and expected assertion value.Two verification points:
Line 18: The test uses
Cypress.env('userCookie')without validating it's set. If the environment variable is missing, the test will fail in a confusing way.Line 20: The comment suggests the assertion value might need adjustment. Verify that 'DataCite' is indeed the correct expected text for a logged-in state, or if it should dynamically match the user name from the environment cookie.
Consider adding a validation step to ensure the environment is properly configured:
it('shows logged in state with valid token', () => { + const userCookie = Cypress.env('userCookie'); + expect(userCookie, 'userCookie env variable must be set').to.exist; + cy.then(() => { - const userCookie = Cypress.env('userCookie'); Cypress.log({ name: 'diagnostics:userCookie', message: [ `type=${typeof userCookie}`, `present=${Boolean(userCookie)}`, `stringLength=${typeof userCookie === 'string' ? userCookie.length : 'n/a'}`, ], }); }); - cy.setCookie('_datacite', Cypress.env('userCookie'), { log: false }); + cy.setCookie('_datacite', userCookie, { log: false }); cy.visit('/'); cy.get('#sign-in').should('contain.text', 'DataCite'); // Adjust to expected user name from env cookie });
23-26: LGTM!The test correctly verifies that the UI shows the logged-out state when no authentication cookie is present.
28-32: Test logic is correct; static analysis warning is a false positive.The test appropriately validates that an invalid JWT token results in a logged-out state. The Gitleaks warning about an API key on line 29 is a false positive—this is a dummy JWT token used as a test fixture, not a real secret.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cypress/e2e/session.test.ts (3)
7-21: Consider simplifying the diagnostic logging.The diagnostic block is verbose and could potentially fail on line 18 if
userCookieis not a string after the conditional check. While helpful for debugging, consider either:
- Removing this block once tests are stable
- Adding a type guard before line 18 to prevent potential errors
Apply this diff to add a type guard:
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)}...`, + `preview=${typeof userCookie === 'string' ? userCookie.substring(0, 50) + '...' : 'n/a'}`, ], });
25-25: Consider removing or clarifying the TODO comment.The inline comment "Match your JWT payload name" appears to be a reminder. Consider either removing it if the assertion is correct or updating it to be more specific about what needs to be matched.
25-25: Optionally add explicit visibility checks for more robust assertions.While Cypress's
.should()automatically retries, explicitly verifying visibility before checking text content can make tests more reliable and provide clearer failure messages.Example for line 25:
- cy.get('#sign-in').should('contain.text', 'DataCite Test User'); + cy.get('#sign-in').should('be.visible').and('contain.text', 'DataCite Test User');Apply similar changes to lines 30 and 36.
Also applies to: 30-30, 36-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/session.test.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
cypress/e2e/session.test.ts
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (5)
🔇 Additional comments (2)
cypress/e2e/session.test.ts (2)
1-4: LGTM!The test suite setup is clean and appropriately sets the consent cookie for session testing.
34-34: Static analysis false positive: This is intentionally invalid test data.The static analysis tool flagged this line as containing an API key. However, this is clearly a test JWT token with an intentionally invalid signature (ending with ".invalid.signature"). The payload contains standard example data from jwt.io and poses no security risk. This warning can be safely ignored.
Purpose
This PR introduces significant improvements to session management and authentication, including the implementation of a new JWT verification library and updated Cypress tests.
Approach
The core of this update involves replacing the
jsonwebtokenlibrary withjosefor JWT handling, which is a more modern and secure approach. This change is accompanied by modifications to how authentication tokens are retrieved and processed, particularly in theuseSessionhook and Apollo Client setup. Additionally, new Cypress tests have been added to specifically cover session behavior.Key Modifications
jsonwebtokenwithjose: Thejsonwebtokenpackage has been removed, andjosehas been added as a dependency for JWT operations.useSessionhook: TheuseSessionhook now utilizesjosefor JWT verification, handling the retrieval of the token from cookies and its subsequent decoding. It also introduces aloadingstate for better user experience.getAuthToken: This function now explicitly awaits thecookies()fromnext/headersto ensure correct asynchronous behavior.apolloClientBuildernow expects an asynchronous function forgetTokento accommodate the asynchronous nature ofgetAuthToken.cypress/e2e/session.test.ts, has been added to thoroughly test various session states (logged in, logged out, invalid token) within the Cypress environment.setupNodeEvents: ThesetupNodeEventsfunctions incypress.config.tshave been simplified by removing unused parameters.RootLayout: TheRootLayoutcomponent now awaits theauthTokenbefore passing it toProviders, ensuring that the authentication status is correctly determined on initial render.Claim,DiscoverWorksAlert,ClientButtons,UserDropdown, andNavRightnow correctly destructure theuserfrom theuseSessionhook, along with itsloadingstate.Important Technical Details
joselibrary for JWT verification is a key security enhancement, leveraging modern cryptographic standards.getAuthTokenand its integration with theuseSessionhook and Apollo Client are crucial for correct client-side authentication state management.Types of changes
Reviewer, please remember our guidelines:
Summary by CodeRabbit
Tests
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.