Auth context #6
Auth context #6
Conversation
… inline with frontend for auth with JWT
…frontend routes not tested
WalkthroughThis pull request introduces secure storage and authentication enhancements. In the mobile configuration, the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant AuthProvider
participant SecureStore
participant Server
App->>AuthProvider: Initialize authentication
AuthProvider->>SecureStore: getToken()
SecureStore-->>AuthProvider: Return token or null
alt Token exists
AuthProvider->>Server: GET /api/auth/me
Server-->>AuthProvider: Return user data
else No token
Note over AuthProvider: Skip user data fetching
end
AuthProvider-->>App: Update auth state
sequenceDiagram
participant User
participant AuthContext
participant Server
participant SecureStore
User->>AuthContext: Submit login credentials
AuthContext->>Server: POST /api/auth/login
Server-->>AuthContext: Return token and user data
AuthContext->>SecureStore: storeToken(token)
AuthContext-->>User: Update authentication state
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
mobile/store/auth/secureStore.ts (1)
1-2: Remove unused importThe
createfunction is imported from Zustand but never used in this file.-import { create } from 'zustand'; import * as SecureStore from 'expo-secure-store';mobile/store/auth/tokenStorage.ts (2)
3-3: Consider using a more specific TOKEN_KEY name.The current key 'auth_token' is generic and could potentially clash with other applications using SecureStore. Consider using a more app-specific prefix to ensure uniqueness.
-const TOKEN_KEY = 'auth_token'; +const TOKEN_KEY = 'your_app_name_auth_token';
26-33: Inconsistent error handling approach.The
getTokenmethod returns null on error whilestoreTokenandremoveTokenthrow exceptions. Consider standardizing the error handling approach across all methods for consistency.Either make all methods throw exceptions:
getToken: async (): Promise<string | null> => { try { return await SecureStore.getItemAsync(TOKEN_KEY); } catch (error) { console.error('Error retrieving token:', error); - return null; + throw new Error('Failed to retrieve authentication token'); } },Or make all methods return a success/failure status:
storeToken: async (token: string): Promise<void> => { try { await SecureStore.setItemAsync(TOKEN_KEY, token); } catch (error) { console.error('Error storing token:', error); - throw new Error('Failed to store authentication token'); + // Just log the error, similar to getToken } },mobile/store/auth/types.ts (2)
1-6: Consider separating input and output user types.The current
Usertype includes an optionalpasswordfield. It's typically better to have separate types for API requests and responses to avoid accidentally exposing sensitive fields.-export type User = { +export type UserResponse = { id: string; name?: string; email: string; - password?: string; }; + +export type UserInput = { + name?: string; + email: string; + password: string; +};
13-20: Consider simplifying action function signatures.The nested function return types for login and signup are unusual and might be harder to understand. Consider simplifying these to make the API more intuitive.
- login: () => (email: string, password: string) => Promise<User>; + login: (email: string, password: string) => Promise<User>; - signup: () => ( - email: string, - password: string, - name: string - ) => Promise<User>; + signup: (email: string, password: string, name: string) => Promise<User>;If the current approach is necessary for your state management pattern (like Zustand), consider adding a comment explaining why.
server/controllers/user.controller.ts (2)
137-142: Consider using exclude instead of select for better security.Using select is good, but exclude might be more defensive against future schema changes. If new sensitive fields are added to the user model in the future, they would be automatically excluded.
const user = await prisma.user.findUnique({ where: { id: req.user.id }, - select: { - id: true, - name: true, - email: true, - // Don't include password in the response - }, + select: { + password: false, + }, });
152-152: Improve error logging with more details.For better debugging, include the actual error in the log, similar to other controller functions.
- console.log('Error in Get Me Controller', error); + console.log('Error in Get Me Controller:', error);mobile/store/auth/authContext.ts (4)
14-37: Assess security implications of storing user data in Secure Storage.
Whileexpo-secure-storeis typically safe, ensure that storing user data in JSON (viapersist) meets your security requirements, especially if the returned user object contains sensitive information beyond email or name.
79-111: Provide loading feedback on login.
Currently, there is no visual or state-based indication to the user that an async login process is underway. Adding a loading state could improve UX.Consider applying a diff like:
login: () => async (email: string, password: string) => { + set({ loading: true }); const headers = new Headers(); headers.append('Content-Type', 'application/json'); const response = await fetch(`${API_URL}/api/auth/login`, { ... }); ... + set({ loading: false }); return user; },
220-227: Consider persisting additional auth states.
You are currently persisting only theuserfield. Storing additional fields (e.g.isAuthenticated) could avoid re-initializing them on each session load.partialize: (state) => ({ - user: state.user + user: state.user, + isAuthenticated: state.isAuthenticated }),
229-233: Show loading or error states during initialization.
Calls tobootstraphappen silently, which might mask delays or errors. Consider surfacing loading/error states to the user for a better experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mobile/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
mobile/app.json(1 hunks)mobile/package.json(2 hunks)mobile/store/auth/authContext.ts(1 hunks)mobile/store/auth/secureStore.ts(1 hunks)mobile/store/auth/tokenStorage.ts(1 hunks)mobile/store/auth/types.ts(1 hunks)server/controllers/user.controller.ts(1 hunks)server/index.ts(1 hunks)server/routes/users.routes.ts(2 hunks)
🔇 Additional comments (10)
mobile/app.json (1)
41-42: Correctly added expo-secure-store pluginThe addition of the expo-secure-store plugin is necessary for using SecureStore functionality for token storage in the authentication system.
mobile/store/auth/secureStore.ts (1)
5-16: Well implemented secure storage adapterThe secure storage implementation is well-structured with proper typing and error handling. This approach correctly wraps Expo's SecureStore API to be used with Zustand's persistence middleware.
mobile/package.json (2)
51-51: Good addition of React QueryAdding React Query is appropriate for managing API requests and state for authentication.
61-61: Consistent addition of expo-secure-storeThe expo-secure-store package is correctly added with version ~14.0.1, consistent with other Expo packages in the project.
server/routes/users.routes.ts (2)
5-5: Correctly updated import for the new controller functionThe
getMefunction is properly imported alongside the existing authentication functions.
15-16: Well-placed auth state restoration endpointThe
/meendpoint is correctly placed after the protect middleware, ensuring it's only accessible to authenticated users. The comment clearly explains its purpose for restoring authentication state.mobile/store/auth/tokenStorage.ts (1)
5-47: LGTM! Well-structured token storage implementation.This is a well-organized implementation for secure token storage with proper error handling and documentation. The use of expo-secure-store is appropriate for storing sensitive authentication tokens.
server/index.ts (1)
26-26: LGTM! Better semantic organization.Changing to '/api/auth' is a good architectural decision as it more accurately reflects that these routes handle authentication functionality rather than general user resource operations.
mobile/store/auth/types.ts (1)
7-21: LGTM! Well-structured auth state interface.The AuthState interface provides a comprehensive structure for managing authentication state with appropriate properties and actions.
server/controllers/user.controller.ts (1)
131-155: LGTM! Secure implementation for retrieving user profile.The implementation is secure and well-structured. Good practice explicitly excluding the password field from the response and including proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
mobile/store/auth/authContext.ts (5)
11-12: Consider initializing queryClient where it's neededCreating a global singleton QueryClient instance outside of React component hierarchy might cause issues with testing and server-side rendering.
Consider initializing the QueryClient in a React context provider and passing it to the store, or use React Query's built-in mechanisms for managing the cache:
-// Define a queryClient for direct usage within the store -const queryClient = new QueryClient(); +// Import and use queryClient from a central location +import { queryClient } from '../queryClient';
28-216: Improve error handling consistency across authentication methodsThe error handling approaches vary across different methods. Some use thorough error handling while others are more basic.
Standardize error handling by implementing a consistent approach across all methods. Consider creating a utility function for API calls that handles common error patterns:
const apiRequest = async (url: string, options: RequestInit) => { try { const response = await fetch(url, options); if (!response.ok) { const errorData = await response.json().catch(() => ({})); throw new Error(errorData.error || `Request failed with status ${response.status}`); } return await response.json(); } catch (error) { // Log error or handle specific error types if (error instanceof TypeError) { throw new Error("Network error. Please check your connection."); } throw error; } };
224-233: Consider adding loading indicator in AuthProviderThe current AuthProvider doesn't handle the loading state, which could lead to rendering authenticated content before authentication is verified.
Enhance the AuthProvider to show a loading indicator:
export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children, }) => { + const { loading } = useAuthStore(); React.useEffect(() => { initializeAuth(); }, []); + if (loading) { + return <LoadingIndicator />; // Create or import a loading component + } return children; };
60-65: Add error logging in catch blocksThe catch blocks currently only remove the token without logging what went wrong, which could make debugging difficult.
Add proper error logging:
try { // existing code } catch (error) { // Token invalid or expired + console.error('Authentication bootstrap failed:', error); await tokenStorage.removeToken(); } finally { set({ loading: false }); }
36-66: Consider adding token refresh mechanismThe current implementation removes the token if it's invalid but doesn't attempt to refresh it if expired.
Consider implementing a token refresh mechanism to enhance the user experience by automatically refreshing expired tokens instead of forcing re-login.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mobile/store/auth/authContext.ts(1 hunks)
🔇 Additional comments (3)
mobile/store/auth/authContext.ts (3)
87-91:❓ Verification inconclusive
Handle user data mapping more robustly
Current implementation manually maps user data and includes a comment about backend not returning email.
Consider normalizing the user data format between frontend and backend, or implementing a more robust mapping layer:
🏁 Script executed:
#!/bin/bash # Check backend user response structure to confirm schema grep -r "res.json({" --include="*.js" --include="*.ts" server/controllers/Length of output: 76
Robust User Data Mapping Verification Needed
The current manual mapping of the user object in
mobile/store/auth/authContext.ts(lines 87-91) explicitly assigns theres.json({inserver/controllers/did not return any output, so it’s unclear if the backend consistently omits the email or if the search missed alternative patterns.
- Please manually verify the backend response to confirm whether the email is truly absent.
- Consider normalizing the user data between frontend and backend, for example, by implementing a dedicated mapping/transformation layer.
- Revisit this mapping if the backend schema evolves to include the email field.
9-9:❓ Verification inconclusive
Update environment variable usage for Expo
Good job updating to use
EXPO_PUBLIC_API_URLas recommended in the previous review. However, consider implementing a more robust configuration system rather than defaulting to localhost.For a more robust approach, consider implementing environment-specific configurations:
🏁 Script executed:
#!/bin/bash # Check if EXPO_PUBLIC_API_URL is correctly set in environment files find . -name ".env*" -type f -exec grep -l "EXPO_PUBLIC_API_URL" {} \;Length of output: 73
Action Required: Verify Environment Variable Configuration
Good job updating the code to use
EXPO_PUBLIC_API_URLas recommended. However, the recent check for environment files (using.env*) returned no matches, suggesting that the variable isn't defined in any such files. This leaves the fallback to'http://localhost:3009'always in effect.
- File Concern:
mobile/store/auth/authContext.tsat line 9.- Recommendation:
- Verify that your production environment provides a value for
EXPO_PUBLIC_API_URL.- Consider implementing a configuration system (e.g., using different
.envfiles or a dedicated configuration module) that sets this variable explicitly for each environment, reducing reliance on a hardcoded fallback.Please ensure that these environment configurations are intentionally managed, and manually verify that your deployment process sets
EXPO_PUBLIC_API_URLappropriately.
183-208:❓ Verification inconclusive
Confirm delete endpoint URL structure
The delete account endpoint uses a URL structure that might be inconsistent with other auth endpoints.
Check if the delete endpoint URL format matches the API structure:
🏁 Script executed:
#!/bin/bash # Look for the delete user route definition grep -r "router.*delete.*user" --include="*.routes.js" --include="*.routes.ts" server/Length of output: 88
I'm not completely confident about the deletion endpoint URL structure in the API. The initial grep command returned no output, so I'll run a broader search for any "router.delete" occurrences related to auth routes in the server code. Please run the following script to help verify:
🏁 Script executed:
#!/bin/bash # Look for any deletion routes in auth routes grep -r "router\.delete" --include="*.routes.js" --include="*.routes.ts" server/ | grep "api/auth"Length of output: 98
Action: Manually verify the delete endpoint URL structure
Our automated searches did not reveal a clear matching
router.deletedefinition for an/api/authdeletion route. Please double-check against the API documentation or the backend implementation to confirm that using${API_URL}/api/auth/${userId}for account deletion is correct and consistent with the other auth endpoints.
- File:
mobile/store/auth/authContext.ts, Lines: 183-208- Confirm that the API route for deleting a user is defined as expected in the server code.
| // Update profile method (you'll need to implement this endpoint in your backend) | ||
| updateProfile: () => async (profileData: Partial<User>) => { | ||
| const token = await tokenStorage.getToken(); | ||
| if (!token) throw new Error('Not authenticated'); | ||
|
|
||
| const headers = new Headers(); | ||
| headers.append('Content-Type', 'application/json'); | ||
| headers.append('Authorization', `Bearer ${token}`); | ||
|
|
||
| const response = await fetch(`${API_URL}/api/auth/profile`, { | ||
| method: 'PUT', | ||
| headers, | ||
| body: JSON.stringify(profileData), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error('Failed to update profile'); | ||
| } | ||
|
|
||
| const updatedUser = await response.json(); | ||
|
|
||
| // Update store | ||
| set({ user: updatedUser }); | ||
|
|
||
| // Update React Query cache | ||
| queryClient.setQueryData(['user'], updatedUser); | ||
|
|
||
| return updatedUser; | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify the profile update endpoint exists on the backend
Comment indicates "you'll need to implement this endpoint in your backend" which suggests this endpoint might not exist yet.
Check if the /api/auth/profile endpoint is implemented on the backend:
🏁 Script executed:
#!/bin/bash
# Search for profile update endpoint implementation in server code
grep -r "\/api\/auth\/profile" --include="*.js" --include="*.ts" server/Length of output: 74
Profile update endpoint is missing in the backend
Our automated search in the server/ directory found no evidence of an implementation for the /api/auth/profile endpoint. Since the frontend’s updateProfile method relies on this endpoint, please either implement the corresponding backend handler or update the comment/documentation to clearly indicate that this endpoint is pending implementation.
- The grep search for
/api/auth/profile(and related router calls) returned no results. - Ensure that your backend (for example, in an Express setup) includes a PUT handler like:
router.put('/api/auth/profile', async (req, res) => { // Add logic to update user profile here });
- If this endpoint is intentionally omitted for now, please document it as a TODO and handle the error case appropriately.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/controllers/user.controller.ts (1)
131-131: Fix typo in comment.There's a small typo in the comment: "authetication" should be "authentication".
-//when user refreshes app and returns we restore their authetication state using this call +//when user refreshes app and returns we restore their authentication state using this callmobile/store/auth/authContext.ts (1)
40-47: Remove unnecessary empty lineThere's an empty line after the comment which breaks the flow slightly.
- // Manually fetch user data using JWT token - + // Manually fetch user data using JWT token const response = await fetch(`${API_URL}/api/auth/me`, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mobile/store/auth/authContext.ts(1 hunks)server/controllers/user.controller.ts(1 hunks)
🔇 Additional comments (6)
server/controllers/user.controller.ts (3)
135-137: Add null check for req.user.The code assumes that
req.useris always defined, but to prevent runtime errors, you should add a check to ensure it exists before accessing its properties.try { // req.user is set by the protect middleware + if (!req.user) { + res.status(401).json({ error: 'Not authenticated' }); + return; + } const user = await prisma.user.findUnique({ where: { id: req.user.id },
137-142: Good security practice: excluding password from response.Properly excluding the password field from the response is an excellent security practice. Consider using Prisma's exclude helper for cleaner code if needed in the future.
131-155: Well-implemented authentication state retrieval function.The
getMefunction is well-structured with proper error handling for both user not found cases and server errors. It correctly uses Prisma to fetch only the necessary user data and provides appropriate status codes for different scenarios.mobile/store/auth/authContext.ts (3)
8-9: LGTM - environment variable naming follows Expo's patternYou've correctly used the
EXPO_PUBLIC_prefix for the environment variable, which follows Expo's recommended pattern for client-side environment variables.
153-181: Profile update endpoint is missing in the backendThe comment on line 153 indicates that this endpoint needs to be implemented in the backend. This issue was previously identified in an earlier review.
Our automated search in the
server/directory found no evidence of an implementation for the/api/auth/profileendpoint. Since the frontend'supdateProfilemethod relies on this endpoint, please either implement the corresponding backend handler or update the comment/documentation to clearly indicate that this endpoint is pending implementation.
232-234: LGTM - clean initialization patternThe
AuthProvidercomponent has a clean implementation that initializes the auth state once when the application starts.
| const userId = get().user?.id; | ||
| const response = await fetch(`${API_URL}/api/auth/${userId}`, { | ||
| method: 'DELETE', | ||
| headers, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for userId before making the DELETE request
The current implementation might attempt to make a request with an undefined userId if the user object is null, which would result in a 404 error.
const userId = get().user?.id;
+ if (!userId) {
+ throw new Error('User ID not found. Cannot delete account.');
+ }
const response = await fetch(`${API_URL}/api/auth/${userId}`, {
method: 'DELETE',
headers,
});📝 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 userId = get().user?.id; | |
| const response = await fetch(`${API_URL}/api/auth/${userId}`, { | |
| method: 'DELETE', | |
| headers, | |
| }); | |
| const userId = get().user?.id; | |
| if (!userId) { | |
| throw new Error('User ID not found. Cannot delete account.'); | |
| } | |
| const response = await fetch(`${API_URL}/api/auth/${userId}`, { | |
| method: 'DELETE', | |
| headers, | |
| }); |
Summary by CodeRabbit
New Features
Chores