Skip to content
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

fix: tokens not cleared in DB when session times out #115

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

aversini
Copy link
Collaborator

@aversini aversini commented Jul 18, 2024

PR Type

Bug fix


Description

  • Added a new utility function getUserIdFromToken to extract user ID from a JWT token in utilities.ts.
  • Updated AuthProvider.tsx to use the new getUserIdFromToken function to retrieve the user ID if it is not present in the state.
  • Added logging for cases where the user ID cannot be identified during the logout process.

Changes walkthrough 📝

Relevant files
Enhancement
utilities.ts
Add utility function to extract user ID from token             

packages/auth-provider/src/common/utilities.ts

  • Added a new function getUserIdFromToken to extract user ID from a JWT
    token.
  • Imported decodeToken from @versini/auth-common.
  • +10/-0   
    Bug fix
    AuthProvider.tsx
    Use token to get user ID if not in state                                 

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx

  • Integrated getUserIdFromToken function to retrieve user ID if not
    present in state.
  • Added logging for cases where user ID cannot be identified.
  • +8/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The function getUserIdFromToken silently catches all exceptions and returns an empty string, which might suppress important errors that should be handled or logged.

    Logging Enhancement
    Consider enhancing the logging in invalidateAndLogout to include error details when the user ID cannot be identified, which could help in debugging issues related to user identification.

    Copy link

    Bundle Size

    Status File Size (Gzip) Limits
    index.js 17.75 KB (+227 B +1.26%) 18 kb

    Overall bundle size: 17.75 KB (+227 B +1.26%)
    Overall status: ✅

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Add error handling for invalid token formats in decodeToken usage

    Ensure that the decodeToken function is properly handling all expected token formats
    and errors, as it's critical for security and functionality.

    packages/auth-provider/src/common/utilities.ts [19-20]

     const jwt = decodeToken(token);
    -return jwt ? (jwt[JWT.USER_ID_KEY] as string) : "";
    +if (!jwt) {
    +  throw new Error("Invalid token format");
    +}
    +return jwt[JWT.USER_ID_KEY] as string;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Ensuring that decodeToken handles all expected token formats and errors is crucial for security and functionality, making this a high-impact improvement.

    9
    Enhancement
    Add error logging in the catch block to aid in debugging

    Consider adding error logging in the catch block of getUserIdFromToken to help with
    debugging issues related to token decoding failures.

    packages/auth-provider/src/common/utilities.ts [17-23]

     try {
       const jwt = decodeToken(token);
       return jwt ? (jwt[JWT.USER_ID_KEY] as string) : "";
    -} catch (_error) {
    +} catch (error) {
    +  console.error("Failed to decode token:", error);
       return "";
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error logging in the catch block is a good practice for debugging and maintaining code, especially when dealing with token decoding which can fail for various reasons.

    8
    Maintainability
    Simplify the conditional check for userId by integrating it directly into the logoutUser function call

    Replace the conditional check for userId with a direct check in the logoutUser
    function call to simplify the code and avoid unnecessary conditional branching.

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx [110-120]

     const userId = user?.userId || getUserIdFromToken(idToken);
    -if (!userId) {
    -  logger(
    -    "invalidateAndLogout: user cannot be identified, logging out without userId",
    -  );
    -}
     await logoutUser({
    -  userId,
    +  userId: userId || "unknown",
       idToken,
       accessToken,
       refreshToken,
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion simplifies the code by reducing unnecessary conditional branching, making it more readable and maintainable without changing the functionality.

    7
    Best practice
    Simplify the return statement using nullish coalescing

    Use TypeScript's nullish coalescing operator to simplify the return statement in
    getUserIdFromToken.

    packages/auth-provider/src/common/utilities.ts [20]

    -return jwt ? (jwt[JWT.USER_ID_KEY] as string) : "";
    +return jwt?.[JWT.USER_ID_KEY] as string ?? "";
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using the nullish coalescing operator simplifies the return statement, improving code readability and adhering to best practices, though the functional improvement is minor.

    6

    @aversini aversini merged commit be6a5e3 into main Jul 18, 2024
    4 checks passed
    @aversini aversini deleted the fix-tokens-not-cleared-in-DB-when-session-times-out branch July 18, 2024 16:20
    @aversini aversini mentioned this pull request Jul 18, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant