Add JWT-based user authentication and registration #27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis PR implements user authentication functionality, including login, registration, and token management. The changes include adding JWT-based authentication, removing unused components, and updating the authentication UI forms. Sequence diagram for user login processsequenceDiagram
actor User
participant AuthLogin
participant useUserLogin
participant API
participant LocalStorage
participant Navigate
User->>AuthLogin: Submit login form
AuthLogin->>useUserLogin: Call mutateAsync with credentials
useUserLogin->>API: POST /user/login
API-->>useUserLogin: Return access and refresh tokens
useUserLogin-->>AuthLogin: Return tokens
AuthLogin->>LocalStorage: Store access_token and refresh_token
AuthLogin->>Navigate: Redirect to home page
Sequence diagram for user registration processsequenceDiagram
actor User
participant AuthRegister
participant useUserRegistration
participant API
participant Navigate
User->>AuthRegister: Submit registration form
AuthRegister->>useUserRegistration: Call mutateAsync with user data
useUserRegistration->>API: POST /user/register
API-->>useUserRegistration: Return success response
useUserRegistration-->>AuthRegister: Return success
AuthRegister->>Navigate: Redirect to login page
Sequence diagram for token refresh processsequenceDiagram
participant MainLayout
participant useRefreshAccessToken
participant API
participant LocalStorage
MainLayout->>LocalStorage: Get access_token
MainLayout->>useRefreshAccessToken: Call mutateAsync if token is expiring
useRefreshAccessToken->>API: POST /user/refresh
API-->>useRefreshAccessToken: Return new access_token
useRefreshAccessToken-->>LocalStorage: Update access_token
Class diagram for authentication mutationsclassDiagram
class useUserLogin {
+mutateAsync(user)
}
class useUserRegistration {
+mutateAsync(body)
}
class useRefreshAccessToken {
+mutateAsync()
}
class LocalStorage {
+getItem(key)
+setItem(key, value)
+removeItem(key)
}
useUserLogin --> API : POST /user/login
useUserRegistration --> API : POST /user/register
useRefreshAccessToken --> API : POST /user/refresh
useRefreshAccessToken --> LocalStorage : Update access_token
LocalStorage <.. useUserLogin : Store tokens
LocalStorage <.. useRefreshAccessToken : Get/Update tokens
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cbrianbet - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider moving the token refresh logic from MainLayout into a dedicated auth context/provider component to better centralize authentication state management and token handling
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/store/auth/mutations.js
Outdated
| } | ||
|
|
||
| try { | ||
| const response = await fetch(`${API_URL}/user/refresh`, { refresh_token: refreshToken }); |
There was a problem hiding this comment.
issue (bug_risk): The refresh token API call is missing proper request configuration
The API call should include Content-Type header and send the refresh token in the request body properly formatted as JSON.
|
|
||
| return newAccessToken; | ||
| } catch (error) { | ||
| console.error('Failed to refresh access token:', error); |
There was a problem hiding this comment.
issue: Token refresh error handling leaves application in invalid state
After clearing tokens on error, the application should redirect to login to ensure user session is properly terminated.
|
|
||
| // ==============================|| MAIN LAYOUT ||============================== // | ||
|
|
||
| const isAuthenticated = () => { |
There was a problem hiding this comment.
suggestion: Token validation logic is duplicated between isAuthenticated and token refresh
Consider extracting the token validation logic into a shared utility function to ensure consistent behavior.
import { validateToken } from 'utils/tokenUtils';
const isAuthenticated = () => validateToken();| }) | ||
| if(!res.ok){ | ||
| const error = await res.json() | ||
| throw Error(error.detail || 'Login Failed') |
There was a problem hiding this comment.
🚨 issue (security): Error handling exposes raw server error messages
Consider mapping server errors to user-friendly messages to avoid exposing implementation details.
| } | ||
| } | ||
|
|
||
|
|
||
| const MainLayout = () => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting authentication logic into a dedicated custom hook
The authentication logic should be extracted into a custom hook to reduce complexity and improve maintainability. Here's how:
// useAuth.js
export const useAuth = () => {
const refreshToken = useRefreshAccessToken();
useEffect(() => {
const checkAndRefreshToken = async () => {
const token = localStorage.getItem('access_token');
if (!token) return;
try {
const { exp } = jwtDecode(token);
const currentTime = Date.now() / 1000;
if (exp < currentTime + 60) {
await refreshToken.mutateAsync();
}
} catch (error) {
console.error('Token decoding failed:', error);
}
};
checkAndRefreshToken();
const interval = setInterval(checkAndRefreshToken, 30 * 60 * 1000);
return () => clearInterval(interval);
}, [refreshToken]);
const isAuthenticated = () => {
const token = localStorage.getItem('access_token');
if (!token) return false;
try {
const { exp } = jwtDecode(token);
return exp > Date.now() / 1000;
} catch {
return false;
}
};
return { isAuthenticated };
};
// MainLayout.js
const MainLayout = () => {
const { isAuthenticated } = useAuth();
// ... other existing layout logic ...
return isAuthenticated() ? (
<Box sx={{ display: 'flex', width: '100%' }}>
{/* ... existing layout JSX ... */}
</Box>
) : <Navigate to="/login" replace />;
};This change:
- Encapsulates all authentication logic in a reusable hook
- Eliminates duplicate token decoding logic
- Makes MainLayout more focused on layout concerns
- Improves testability by separating concerns
sourcery-ai
bot
changed the title
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add user management features including login, registration, and token refresh using JWT. Simplify registration form by requiring a username instead of first and last names. Clean up code by removing unused Firebase social login components and sample routes.
New Features:
Enhancements:
Chores: