Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions packages/authentication/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# @baseapp-frontend/authentication

## 4.0.3

### Patch Changes

- Use jotai global state and cookies to keep track of the current profile. CurrentProfileProvider is removed, use "const { currentProfile, setCurrentProfile } = useCurrentProfile()" to read and modify the current profile, respectively.

## 4.0.2

### Patch Changes
Expand Down
1 change: 1 addition & 0 deletions packages/authentication/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export * from './modules/access'
export * from './modules/mfa'
export * from './modules/profile'
export * from './modules/user'

export { default as AuthApi } from './services/auth'
Expand Down
14 changes: 14 additions & 0 deletions packages/authentication/modules/access/useLogin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { useState } from 'react'
import {
ACCESS_KEY_NAME,
REFRESH_KEY_NAME,
decodeJWT,
setFormApiErrors,
setTokenAsync,
} from '@baseapp-frontend/utils'
Expand All @@ -21,11 +22,13 @@ import type {
LoginMfaRequest,
LoginRequest,
} from '../../../types/auth'
import { User } from '../../../types/user'
import {
isLoginChangeExpiredPasswordRedirectResponse,
isLoginMfaResponse,
} from '../../../utils/login'
import { CODE_VALIDATION_INITIAL_VALUES, CODE_VALIDATION_SCHEMA } from '../../mfa/constants'
import { useCurrentProfile } from '../../profile'
import { DEFAULT_INITIAL_VALUES, DEFAULT_VALIDATION_SCHEMA } from './constants'
import type { UseLoginOptions } from './types'

Expand All @@ -39,6 +42,7 @@ const useLogin = ({
enableFormApiErrors = true,
}: UseLoginOptions = {}) => {
const [mfaEphemeralToken, setMfaEphemeralToken] = useState<string | null>(null)
const { setCurrentProfile } = useCurrentProfile()

/*
* Handles login success with the auth token in response
Expand All @@ -49,6 +53,16 @@ const useLogin = ({
if (isLoginChangeExpiredPasswordRedirectResponse(response)) {
return
}
const user = decodeJWT<User>(response.access)
if (user) {
// TODO: handle the absolute image path on the backend
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the evidence gathered, I can now provide the final response:

Image path handling needs to be moved to the backend as planned

The TODO comment is valid as the current implementation in multiple files shows client-side image path construction:

  • packages/authentication/modules/access/useLogin/index.ts
  • packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx

Both files use NEXT_PUBLIC_API_BASE_URL to construct image URLs on the client side. While there are image URL implementations in the Wagtail module, they appear to be for a different part of the system and already handle complete URLs properly.

🔗 Analysis chain

Backend should handle image path construction.

The TODO comment indicates that image path handling should be moved to the backend. This is still being handled on the client side, which is not aligned with the security best practices.

Let's check if this is being addressed in other parts of the codebase:

Consider creating a dedicated API endpoint for profile data that returns fully qualified image URLs.


Let me verify if there are any backend implementations or ongoing work related to this image path handling.


Let me check one more thing to see if there are any backend API implementations or configurations related to image handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar image path handling patterns
rg -A 3 "NEXT_PUBLIC_API_BASE_URL.*image" 

# Check for any backend-related changes
rg -A 3 "TODO.*handle.*backend"

Length of output: 1098


Script:

#!/bin/bash
# Check backend-related files for image URL handling
ast-grep --pattern 'function $FUNC($$$) {
  $$$
  url: $$$
  $$$
}'

# Look for API endpoints related to profile or image handling
rg -l "router.(get|post).*profile"

# Check if there are any recent changes or PRs addressing this
rg -l "image.*url"

Length of output: 1433


Script:

#!/bin/bash
# Look for backend API configurations or implementations
rg -A 5 "createUrl|getImageUrl|imageUrl" 

# Check for any image-related configurations in the backend
fd -t f "config|settings" -x rg -l "image.*path|image.*url" {}

Length of output: 6251

const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
Comment on lines +59 to +60
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate image path construction

The current image path construction could be vulnerable to path traversal if the image path from the profile contains ../ or similar sequences.

Add path validation:

-      const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
+      const absoluteImagePath = user.profile.image 
+        ? `${baseUrl}${user.profile.image.replace(/\.{2,}/g, '')}` 
+        : null
📝 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.

Suggested change
const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
const absoluteImagePath = user.profile.image
? `${baseUrl}${user.profile.image.replace(/\.{2,}/g, '')}`
: null

setCurrentProfile({
...user.profile,
image: absoluteImagePath,
})
}
await setTokenAsync(accessKeyName, response.access, {
secure: process.env.NODE_ENV === 'production',
})
Expand Down
5 changes: 5 additions & 0 deletions packages/authentication/modules/profile/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export { default as useCurrentProfile } from './useCurrentProfile'
export type { MinimalProfile } from '../../types/profile'

export { InitialProfileProvider } from './useCurrentProfile/__tests__/__utils__/InitialProfileProvider'
export type { InitialProfileProp } from './useCurrentProfile/__tests__/__utils__/InitialProfileProvider'
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { faker } from '@faker-js/faker'

export const mockUserProfileFactory = (id: string) => {
return {
id,
name: faker.person.fullName(),
image: faker.image.avatar(),
urlPath: faker.internet.url(),
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { FC, PropsWithChildren } from 'react'

import { useHydrateAtoms } from 'jotai/utils'

import { MinimalProfile } from '../../../../../../types/profile'
import { profileAtom } from '../../../index'

export type InitialProfileProp = {
initialProfile: MinimalProfile | null
}

// Use as:
// <JotaiProvider>
// <InitialProfileProvider initialProfile={props.initialProfile}>
// // You're component goes here, it is passed the initialProfile
// </InitialProfileProvider>
// </JotaiProvider>

export const InitialProfileProvider: FC<PropsWithChildren & InitialProfileProp> = ({
initialProfile,
children,
}) => {
useHydrateAtoms([[profileAtom, initialProfile]])
return children
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { act, renderHook } from '@baseapp-frontend/test'
import { LOGOUT_EVENT, eventEmitter, removeCookie, setCookie } from '@baseapp-frontend/utils'

import useCurrentProfile from '..'
import { CURRENT_PROFILE_KEY } from '../constants'
import { mockUserProfileFactory } from './__mock__/profiles'

jest.mock('@baseapp-frontend/utils', () => ({
...jest.requireActual('@baseapp-frontend/utils'),
removeCookie: jest.fn(),
setCookie: jest.fn(),
}))

describe('useCurrentProfile', () => {
afterEach(() => {
jest.clearAllMocks()
})

it('changes current profile state and sets cookie', () => {
const profile1 = mockUserProfileFactory('profile-id-1')
const profile2 = mockUserProfileFactory('profile-id-2')
const { result } = renderHook(() => useCurrentProfile())

act(() => result.current.setCurrentProfile(profile1))
expect(result.current.currentProfile!.id).toEqual('profile-id-1')
expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true })

act(() => result.current.setCurrentProfile(profile2))
expect(result.current.currentProfile!.id).toEqual('profile-id-2')
expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true })
})
Comment on lines +19 to +31
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect profile in cookie assertion

There's a bug in the test case where the wrong profile is being used in the cookie assertion after setting profile2:

     act(() => result.current.setCurrentProfile(profile2))
     expect(result.current.currentProfile!.id).toEqual('profile-id-2')
-    expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true })
+    expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile2, { stringfyValue: true })
📝 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.

Suggested change
it('changes current profile state and sets cookie', () => {
const profile1 = mockUserProfileFactory('profile-id-1')
const profile2 = mockUserProfileFactory('profile-id-2')
const { result } = renderHook(() => useCurrentProfile())
act(() => result.current.setCurrentProfile(profile1))
expect(result.current.currentProfile!.id).toEqual('profile-id-1')
expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true })
act(() => result.current.setCurrentProfile(profile2))
expect(result.current.currentProfile!.id).toEqual('profile-id-2')
expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true })
})
it('changes current profile state and sets cookie', () => {
const profile1 = mockUserProfileFactory('profile-id-1')
const profile2 = mockUserProfileFactory('profile-id-2')
const { result } = renderHook(() => useCurrentProfile())
act(() => result.current.setCurrentProfile(profile1))
expect(result.current.currentProfile!.id).toEqual('profile-id-1')
expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true })
act(() => result.current.setCurrentProfile(profile2))
expect(result.current.currentProfile!.id).toEqual('profile-id-2')
expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile2, { stringfyValue: true })
})


it('erases current profile when user logs out', async () => {
const profile = mockUserProfileFactory('user-profile-1')
const { result } = renderHook(() => useCurrentProfile())
act(() => result.current.setCurrentProfile(profile))

act(() => eventEmitter.emit(LOGOUT_EVENT))
expect(result.current.currentProfile).toEqual(null)
expect(removeCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY)
})
})
Comment on lines +33 to +42
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add test coverage for critical scenarios

The current test suite is missing coverage for several key scenarios introduced in this PR:

  1. Initial profile loading from JWT claims:
it('should initialize profile from JWT claims', () => {
  const mockClaims = { profileId: 'jwt-profile-id' }
  parseJwt.mockReturnValue(mockClaims)
  const { result } = renderHook(() => useCurrentProfile())
  expect(result.current.currentProfile).toEqual(expect.objectContaining({ id: 'jwt-profile-id' }))
})
  1. SSR behavior with cookie:
it('should handle SSR profile initialization', () => {
  isSSR.mockReturnValue(true)
  const profile = mockUserProfileFactory('ssr-profile')
  // Mock cookie retrieval for SSR
  const { result } = renderHook(() => useCurrentProfile({ noSSR: false }))
  expect(result.current.currentProfile).toEqual(profile)
})
  1. Profile updates with active check:
it('should only update if profile is active', () => {
  const profile = mockUserProfileFactory('active-profile')
  const { result } = renderHook(() => useCurrentProfile())
  act(() => result.current.updateProfileIfActive(profile))
  expect(result.current.currentProfile).toEqual(profile)
})

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const CURRENT_PROFILE_KEY = 'CurrentProfile'
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
'use client'

import { useCallback, useEffect } from 'react'

import {
LOGOUT_EVENT,
ServerSideRenderingOption,
eventEmitter,
getCookie,
removeCookie,
setCookie,
} from '@baseapp-frontend/utils'

import { atom, useAtom } from 'jotai'

import { MinimalProfile } from '../../../types/profile'
import { CURRENT_PROFILE_KEY } from './constants'

export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => {
const settings =
getCookie<MinimalProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null

return settings
}

const initialProfile = getProfileFromCookie()

export const profileAtom = atom<MinimalProfile | null>(initialProfile)

/**
* By using `useCurrentProfile` with the `noSSR` option set to `false`, causes Next.js to dynamically render the affected pages, instead of statically rendering them.
*/
const useCurrentProfile = ({ noSSR = true }: ServerSideRenderingOption = {}) => {
const [currentProfile, setProfile] = useAtom(profileAtom)
const isSSR = typeof window === typeof undefined

const setCurrentProfile = (newProfile: MinimalProfile | null) => {
if (newProfile === null) {
setProfile(() => {
removeCookie(CURRENT_PROFILE_KEY)
return null
})
} else {
setProfile(() => {
setCookie(CURRENT_PROFILE_KEY, newProfile, { stringfyValue: true })
return newProfile
})
}
}

const updateProfileIfActive = (newProfile: MinimalProfile) => {
if (currentProfile?.id === newProfile.id) {
setCurrentProfile(newProfile)
}
}

const removeCurrentProfile = useCallback(() => setCurrentProfile(null), [])

Check warning on line 57 in packages/authentication/modules/profile/useCurrentProfile/index.ts

View workflow job for this annotation

GitHub Actions / Lint

React Hook useCallback has a missing dependency: 'setCurrentProfile'. Either include it or remove the dependency array
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing dependencies to React hooks

The useCallback and useEffect hooks are missing dependencies.

-  const removeCurrentProfile = useCallback(() => setCurrentProfile(null), [])
+  const removeCurrentProfile = useCallback(() => setCurrentProfile(null), [setCurrentProfile])

   useEffect(() => {
     eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile)
     return () => {
       eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile)
     }
-  }, [])
+  }, [removeCurrentProfile])

Also applies to: 64-64

🧰 Tools
🪛 GitHub Check: Lint

[warning] 57-57:
React Hook useCallback has a missing dependency: 'setCurrentProfile'. Either include it or remove the dependency array


useEffect(() => {
eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile)
return () => {
eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile)
}
}, [])

Check warning on line 64 in packages/authentication/modules/profile/useCurrentProfile/index.ts

View workflow job for this annotation

GitHub Actions / Lint

React Hook useEffect has a missing dependency: 'removeCurrentProfile'. Either include it or remove the dependency array

if (isSSR) {
return {
currentProfile: getProfileFromCookie({ noSSR }),
setCurrentProfile,
updateProfileIfActive,
}
}
return {
currentProfile,
setCurrentProfile,
updateProfileIfActive,
}
}

export default useCurrentProfile
3 changes: 3 additions & 0 deletions packages/authentication/modules/user/useUpdateUser/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useMutation, useQueryClient } from '@tanstack/react-query'

import UserApi, { USER_API_KEY } from '../../../services/user'
import type { User, UserUpdateParams } from '../../../types/user'
import { useCurrentProfile } from '../../profile'
import type { UseUpdateUserOptions } from './types'

const useUpdateUser = <TUser extends Pick<User, 'id'>>({
Expand All @@ -13,6 +14,7 @@ const useUpdateUser = <TUser extends Pick<User, 'id'>>({
ApiClass = UserApi,
}: UseUpdateUserOptions<TUser> = {}) => {
const queryClient = useQueryClient()
const { setCurrentProfile } = useCurrentProfile()

const mutation = useMutation({
mutationFn: (params: UserUpdateParams<TUser>) => ApiClass.updateUser<TUser>(params),
Expand All @@ -23,6 +25,7 @@ const useUpdateUser = <TUser extends Pick<User, 'id'>>({
} catch (e) {
// silently fail
// eslint-disable-next-line no-console
setCurrentProfile(null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering why we would do it on catch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I put this here, is that if there occurs an error in refreshAccessToken this function removes the access and refresh tokens, but does not fire a logout event. So I guess we should also remove the current profile, too. I would have liked to do that in the error handling of the refreshAccessToken function directly, but since the function is defined in the utils package, it should better not import from authentication. I thought that passing an onError callback was too compicated, so settled for this easier solution.

console.error(e)
}
options?.onSettled?.(data, error, variables, context)
Expand Down
4 changes: 3 additions & 1 deletion packages/authentication/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@baseapp-frontend/authentication",
"description": "Authentication modules.",
"version": "4.0.2",
"version": "4.0.3",
"main": "./index.ts",
"types": "dist/index.d.ts",
"sideEffects": false,
Expand All @@ -15,6 +15,7 @@
"dependencies": {
"@hookform/resolvers": "catalog:",
"@tanstack/react-query": "catalog:",
"jotai": "catalog:",
"js-cookie": "catalog:",
"react-hook-form": "catalog:",
"zod": "catalog:"
Expand All @@ -28,6 +29,7 @@
"@baseapp-frontend/eslint-plugin": "workspace:*",
"@baseapp-frontend/test": "workspace:*",
"@baseapp-frontend/tsconfig": "workspace:*",
"@faker-js/faker": "catalog:test",
"@testing-library/jest-dom": "catalog:test",
"@testing-library/react": "catalog:test",
"@testing-library/user-event": "catalog:test",
Expand Down
6 changes: 6 additions & 0 deletions packages/authentication/types/profile.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export type MinimalProfile = {
id: string
name: string | null
image: string | null
urlPath: string | null
}
9 changes: 4 additions & 5 deletions packages/authentication/types/user.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,22 @@
import { MinimalProfile } from './profile'

export interface User {
id: number
email: string
isEmailVerified: boolean
newEmail: string
isNewEmailConfirmed: boolean
referralCode: string
avatar: {
fullSize: string
small: string
}
firstName: string
lastName: string
profile: MinimalProfile
phoneNumber: string
preferredLanguage: string
}

export interface UserUpdateParams<TUser extends Partial<User>> {
userId: TUser['id']
data: Partial<Omit<TUser, 'avatar' | 'id'>> & {
data: Partial<Omit<TUser, 'id'>> & {
avatar?: File | string
}
}
13 changes: 3 additions & 10 deletions packages/components/.storybook/__mocks__/nextFontMock.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,3 @@
module.exports = {
Public_Sans: () => ({
className: 'mock-public-sans',
style: {},
}),
Barlow: () => ({
className: 'mock-barlow',
style: {},
}),
}
module.exports = require('@baseapp-frontend/test/__mocks__/nextFontMock.ts')

export {}
26 changes: 13 additions & 13 deletions packages/components/.storybook/decorators/withProviders.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { NotificationProvider } from '@baseapp-frontend/utils'

import type { StoryContext, StoryFn } from '@storybook/react'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
import { Provider as JotaiProvider } from 'jotai'

import CurrentProfileProvider from '../../modules/profiles/context/CurrentProfileProvider'
import '../../styles/tailwind/globals.css'
import defaultTheme from '../__mocks__/theme'

Expand All @@ -34,19 +34,19 @@ const withProviders = (Story: StoryFn, context: StoryContext) => {
}, [mockData, resolveMostRecentOperation, queueOperationResolver, mockResolvers])

return (
<QueryClientProvider client={queryClient}>
<RelayTestProvider environment={environment}>
<React.Suspense fallback={<LoadingState />}>
<ThemeProvider {...defaultTheme}>
<NotificationProvider>
<CurrentProfileProvider>
<JotaiProvider>
<QueryClientProvider client={queryClient}>
<RelayTestProvider environment={environment}>
<React.Suspense fallback={<LoadingState />}>
<ThemeProvider {...defaultTheme}>
<NotificationProvider>
<Story />
</CurrentProfileProvider>
</NotificationProvider>
</ThemeProvider>
</React.Suspense>
</RelayTestProvider>
</QueryClientProvider>
</NotificationProvider>
</ThemeProvider>
</React.Suspense>
</RelayTestProvider>
</QueryClientProvider>
</JotaiProvider>
)
}

Expand Down
10 changes: 10 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# @baseapp-frontend/components

## 0.0.24

### Patch Changes

- Remove `useCurrentProfile` from the package.
- Use mocks from `@baseapp-frontend/test`.

- Updated dependencies
- @baseapp-frontend/authentication@4.0.3

## 0.0.23

### Patch Changes
Expand Down
Loading
Loading