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: logout accepts idToken for a more resilient process #171

Merged

Conversation

aversini
Copy link
Collaborator

@aversini aversini commented Aug 26, 2024

PR Type

enhancement, bug fix


Description

  • Enhanced the logout process by adding an optional idToken parameter to the LogoutProps type.
  • Updated the logoutUser function to accept and utilize the idToken parameter for a more resilient logout process.
  • Modified the AuthProvider component to pass the idToken to the logoutUser function.

Changes walkthrough 📝

Relevant files
Enhancement
types.d.ts
Add optional `idToken` to `LogoutProps` type                         

packages/auth-provider/src/common/types.d.ts

  • Added optional idToken property to LogoutProps type.
+1/-0     
utilities.ts
Enhance `logoutUser` function with `idToken` parameter     

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

  • Added idToken parameter to logoutUser function.
  • Updated restCall parameters to include idToken.
  • +2/-0     
    AuthProvider.tsx
    Pass `idToken` in `AuthProvider` logout call                         

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

    • Passed idToken to logoutUser function in AuthProvider.
    +1/-0     

    💡 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

    Optional Parameter
    The idToken parameter is set to an empty string by default. Consider if this is the intended behavior or if it should be set to undefined instead.

    Potential Undefined Value
    The idToken is passed to the logoutUser function without checking if it exists. This might lead to passing an undefined value.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Conditionally include non-empty parameters in API requests

    Consider adding a check to ensure that idToken is not an empty string before
    including it in the params object. This can prevent sending unnecessary empty
    parameters to the API.

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

     params: {
     	userId,
     	domain,
    -	idToken,
    +	...(idToken && { idToken }),
     },
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion improves the API request by avoiding sending unnecessary empty parameters, which is a best practice and can prevent potential issues with the API.

    9
    Possible issue
    Conditionally include optional parameters in function calls

    Consider adding a check to ensure that idToken is not undefined before passing it to
    the logoutUser function. This can prevent potential issues if idToken is not
    available.

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx [145-150]

     await logoutUser({
     	userId,
     	clientId,
     	domain,
    -	idToken,
    +	...(idToken && { idToken }),
     });
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to check if idToken is defined before passing it to the function is a good practice, as it prevents potential issues if idToken is not available, ensuring more robust code.

    9
    Enhancement
    Use object destructuring with default value in function parameters

    Consider using object destructuring with a default value for the idToken parameter
    in the function signature. This can make the code more concise and align with modern
    JavaScript practices.

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

    +export const logoutUser = async ({
    +	userId,
    +	clientId,
    +	domain,
    +	idToken = "",
    +}: LogoutProps): Promise<LogoutResponse> => {
     
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion is already implemented in the PR code, which uses object destructuring with a default value for idToken. This aligns with modern JavaScript practices and improves code conciseness.

    8

    Copy link

    Bundle Size

    Status File Size (Gzip) Limits
    index.js 14.32 KB (+18 B +0.12%) 15 kb

    Overall bundle size: 14.32 KB (+18 B +0.12%)
    Overall status: ✅

    @aversini aversini merged commit a0beea5 into main Aug 26, 2024
    4 checks passed
    @aversini aversini deleted the fix-logout-accepts-idToken-for-a-more-resilient-process branch August 26, 2024 22:48
    @aversini aversini mentioned this pull request Aug 26, 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