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

feat(api): add logout endpoint to clear token cookie #581

Merged
merged 3 commits into from
Dec 8, 2024

Conversation

ShreyamKundu
Copy link
Contributor

@ShreyamKundu ShreyamKundu commented Dec 7, 2024

User description

Description

Added a logout endpoint to the auth.controller.ts file and the logout function in the auth.service.ts file. This endpoint clears the token cookie, ensuring the user is logged out securely.

Fixes #223

Dependencies

NA

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

@rajdip-b

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement


Description

  • Added a new logout endpoint in the auth.controller.ts file to handle user logout by clearing the token cookie.
  • Implemented a logout method in the auth.service.ts file that clears the token cookie and logs the action.
  • Ensured the logout process sends a success message upon completion.

Changes walkthrough 📝

Relevant files
Enhancement
auth.controller.ts
Add logout endpoint to AuthController                                       

apps/api/src/auth/controller/auth.controller.ts

  • Added a new POST endpoint logout to the AuthController.
  • The endpoint calls the logout method from the AuthService.
  • Sends a success message upon logout.
  • +7/-0     
    auth.service.ts
    Implement logout functionality in AuthService                       

    apps/api/src/auth/service/auth.service.ts

  • Introduced a logout method in AuthService.
  • Clears the token cookie from the response.
  • Logs the logout action.
  • +12/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    223 - Partially compliant

    Compliant requirements:

    • Added logout endpoint in auth controller
    • Created service function to clear token cookie

    Non-compliant requirements:

    • No explicit verification that @public decorator was not added
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Cookie Security:
    The cookie clearing does not explicitly set secure flags like httpOnly, secure, or sameSite which are important security measures for cookies. Consider adding these flags when clearing the cookie to prevent any residual security issues.

    ⚡ Recommended focus areas for review

    Code Coverage
    The logout endpoint is marked with istanbul ignore next which excludes it from code coverage. Consider adding tests instead of ignoring coverage.

    Error Handling
    The logout method lacks error handling for cases where clearing the cookie fails

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Dec 7, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Ensure cookie clearing matches cookie setting parameters for consistent security behavior

    Add the same cookie options used during token setting (httpOnly, secure, sameSite)
    when clearing the cookie to ensure proper cookie removal across all security
    contexts.

    apps/api/src/auth/service/auth.service.ts [229-231]

     res.clearCookie('token', {
    -  domain: process.env.DOMAIN ?? 'localhost'
    +  domain: process.env.DOMAIN ?? 'localhost',
    +  httpOnly: true,
    +  secure: process.env.NODE_ENV === 'production',
    +  sameSite: 'lax'
     })
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding security parameters when clearing cookies is crucial for maintaining consistent security across different contexts and preventing potential security vulnerabilities. Missing these parameters could lead to cookies not being properly cleared in all scenarios.

    9
    ✅ Ensure critical authentication endpoints have proper test coverage
    Suggestion Impact:The commit removed the 'istanbul ignore next' comment from the logout endpoint, as suggested.

    code diff:

    -  /* istanbul ignore next */

    Remove the istanbul ignore comment and add proper test coverage for the logout
    endpoint, as it handles critical authentication state.

    apps/api/src/auth/controller/auth.controller.ts [211-213]

    -/* istanbul ignore next */
     @Post('logout')
     async logout(@Res() res: Response): Promise<void> {
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Test coverage for authentication endpoints is important for security and reliability. Removing the istanbul ignore directive will ensure this critical functionality is properly tested.

    7
    Possible issue
    Add proper error handling for cookie operations to ensure reliable logout functionality

    Add error handling around the cookie clearing operation to ensure the operation
    completes successfully and provide appropriate feedback.

    apps/api/src/auth/service/auth.service.ts [228-233]

     async logout(res: Response): Promise<void> {
    -  res.clearCookie('token', {
    -    domain: process.env.DOMAIN ?? 'localhost'
    -  })
    -  this.logger.log('User logged out and token cookie cleared.')
    +  try {
    +    res.clearCookie('token', {
    +      domain: process.env.DOMAIN ?? 'localhost'
    +    })
    +    this.logger.log('User logged out and token cookie cleared.')
    +  } catch (error) {
    +    this.logger.error('Failed to clear token cookie during logout', error)
    +    throw new Error('Logout operation failed')
    +  }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for cookie operations is essential for system reliability and debugging. The suggestion provides proper error logging and propagation which helps maintain system stability.

    8

    💡 Need additional feedback ? start a PR chat

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Dec 7, 2024

    Can you also add the request under "Auth Controller" in Bruno?

    @ShreyamKundu
    Copy link
    Contributor Author

    okay...i will try that

    @ShreyamKundu
    Copy link
    Contributor Author

    @rajdip-b I have added the request in auth controller in Bruno

    @rajdip-b rajdip-b merged commit 27f81ba into keyshade-xyz:develop Dec 8, 2024
    2 checks passed
    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.

    API: Add logout endpoint
    2 participants