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
    muntaxir4 pushed a commit to muntaxir4/keyshade that referenced this pull request Jan 1, 2025
    rajdip-b pushed a commit that referenced this pull request Jan 23, 2025
    ## [2.9.0](v2.8.0...v2.9.0) (2025-01-23)
    
    ### 🚀 Features
    
    * **api-client:** Get all workspace invitation ([#619](#619)) ([8a23850](8a23850))
    * **api,cli,api-client,schema:** Enhance permissions to allow filtering by environments through project roles ([#599](#599)) ([030b539](030b539))
    * **api:** Add `ADMIN` authority for API keys ([#609](#609)) ([fb6aba7](fb6aba7))
    * **api:** Add email template for inviting user to workspace ([#480](#480)) ([f5ddf7a](f5ddf7a))
    * **api:** Add email template for sending OTP to the user ([#582](#582)) ([cb6bbcb](cb6bbcb))
    * **api:** Add endpoint to fetch all workspace invitations for a user ([#586](#586)) ([d45417a](d45417a))
    * **api:** Add logout endpoint to clear token cookie ([#581](#581)) ([27f81ba](27f81ba))
    * **api:** Add slack integration ([#531](#531)) ([fe124d8](fe124d8))
    * **cli:** Add CLI command to check version flag using `--version` or `-v` ([#650](#650)) ([31b5efe](31b5efe))
    * **cli:** Add functionality to operate on Workspace Membership ([#589](#589)) ([0fde62b](0fde62b))
    * **cli:** Add import sub commmand for project. ([#594](#594)) ([9896f27](9896f27))
    * **cli:** Add List-invitation command ([#633](#633)) ([874f8c2](874f8c2))
    * **cli:** Added keyshade command to cli ([cf260ae](cf260ae))
    * **cli:** Api health probe ([#645](#645)) ([dd854f4](dd854f4))
    * **cli:** Create basic README.md ([a1b74e9](a1b74e9))
    * **cli:** Log publicKey, privacyKey, & accessLevel after project creation ([#623](#623)) ([5d5b329](5d5b329))
    * **cli:** Supports to specify environment(s) and its optional description ([#634](#634)) ([62083b1](62083b1))
    * **cli:** update README with feature and installation ([#644](#644)) ([a4d2a6a](a4d2a6a))
    * **platform:** Add a new [secure] and added loader on project screen ([#603](#603)) ([c3a08cc](c3a08cc))
    * **platform:** Add new variables to a project ([#593](#593)) ([d6c6252](d6c6252))
    * **platform:** Delete variable from a project ([#600](#600)) ([e64a738](e64a738))
    * **platform:** Edit existing variables in a project ([#602](#602)) ([bb48f6c](bb48f6c))
    * **platform:** Show all the existing variables inside a project ([#591](#591)) ([5276bb8](5276bb8))
    * **platofrm:** Added online/offline status detection in the platform ([#585](#585)) ([89aa84f](89aa84f))
    * **schema:** Add workspace invitation schema ([#612](#612)) ([1a5721b](1a5721b))
    * **web:** Add Google Analytics integration ([#649](#649)) ([397d6da](397d6da))
    
    ### 🐛 Bug Fixes
    
    * **api:** Empty name `""` accepted as a valid name while creating environments ([#583](#583)) ([a33f4b5](a33f4b5))
    * **api:** Enable global project access ([#580](#580)) ([b3a0309](b3a0309))
    * **api:** Update build command ([0ddfa59](0ddfa59))
    * **cli:** Add keywords for improved package discoverability ([#641](#641)) ([57ce10b](57ce10b))
    * **cli:** Check for .keyshade dir if profile isn't found ([#636](#636)) ([a69665d](a69665d))
    * **cli:** Create project --store-private-key option default value removed ([#638](#638)) ([20f16c6](20f16c6))
    * **cli:** Fixed binary path in package.json ([e531af0](e531af0))
    * **cli:** Fixed binary path in package.json ([81d674d](81d674d))
    * **cli:** Incorrect message on listing projects ([#624](#624)) ([eeffa42](eeffa42))
    * **cli:** Module errors ([d3432c5](d3432c5))
    * **cli:** Module errors ([a639065](a639065))
    * **cli:** Module errors ([a7742b1](a7742b1))
    * **cli:** Module errors ([e96300e](e96300e))
    * **cli:** Profile name now can use - and _ and updated error message ([#639](#639)) ([00dd66a](00dd66a))
    * **cli:** Prompt users for all values if no option set and show default values ([#640](#640)) ([fe862ab](fe862ab))
    * **docker:** Update build script ([40ef3e2](40ef3e2))
    * **platform:** Check if `Env. Name` is left empty ([#646](#646)) ([5f3fac8](5f3fac8))
    * **platform:** Clickable Workspaces combobox options ([#630](#630)) ([acc96f7](acc96f7))
    * **platform:** Optimized user update request body ([#605](#605)) ([ee1adf0](ee1adf0))
    * **platform:** Type error in navbar ([8199de8](8199de8))
    * **README:** Update Discord badge ([6f9382e](6f9382e))
    * **schema:** Add versions field to project [secure]s and variables response ([#590](#590)) ([755ea46](755ea46))
    
    ### 📚 Documentation
    
    * **cli:** Update changelog to include missed out changes ([8910c5c](8910c5c))
    * Updated alignment of pictures in API Testing page ([5d69223](5d69223))
    * Updated alignment of pictures in API Testing page ([e31eeca](e31eeca))
    
    ### 🔧 Miscellaneous Chores
    
    * Add Sentry and update CI ([#653](#653)) ([ca96862](ca96862))
    * **ci:** Add CLI deployment script ([51de9d1](51de9d1))
    * **ci:** Add internal package dependencies to existing workflows ([#592](#592)) ([a9fc39e](a9fc39e))
    * **ci:** Add scope ([8ef89a8](8ef89a8))
    * **ci:** Bug fix in workflow ([d583a46](d583a46))
    * **ci:** Bug fix in workflow ([eb9d60f](eb9d60f))
    * **ci:** Bump version to 2.2.0 ([951bd14](951bd14))
    * **ci:** Deploy DB migrations ([ea1beed](ea1beed))
    * **ci:** Fixed chaining and scripts ([6a009eb](6a009eb))
    * **ci:** Fixed environment name ([172c348](172c348))
    * **ci:** Fixed errors ([f28e3b5](f28e3b5))
    * **ci:** Minor fixes ([c7f05a0](c7f05a0))
    * **ci:** Push docker images of platform and API to ACR ([5f79dd7](5f79dd7))
    * **ci:** Remove npm ci ([3d45a4c](3d45a4c))
    * **ci:** Remove pnpm cache ([f45970c](f45970c))
    * **ci:** Update app redeployment ([18cf765](18cf765))
    * **ci:** Update web deployment to push to ACR ([26d4bed](26d4bed))
    * **cli:** Bumped CLI to v2.4.0 ([09efcd9](09efcd9))
    * **cli:** Rearranged dependency ([b6e344d](b6e344d))
    * **cli:** Removed changeset ([6c436de](6c436de))
    * **cli:** Update package name ([23552a1](23552a1))
    * **cli:** Update package name ([480cf54](480cf54))
    * **cli:** Update package.json ([871679a](871679a))
    * **cli:** Updated build scripts ([2e2b42d](2e2b42d))
    * **docker:** Update port of platform docker build ([c79d886](c79d886))
    * Housekeeping ([2ed31c0](2ed31c0))
    * **platoform:** Swapped all legacy API calls with `@keyshade/api-client` ([#584](#584)) ([c600db7](c600db7))
    * Removed .postman folder ([4b2b675](4b2b675))
    * Reverted back registry ([1699a89](1699a89))
    * **schema:** Add describe blocks in tests for each kind of schema ([#577](#577)) ([c0763f3](c0763f3))
    * Update npmrc ([9f7f495](9f7f495))
    * Update pipelines; fixed api docker ([3f36a17](3f36a17))
    * Update platform build command ([83a1851](83a1851))
    * **web:** Fix CI ([d1bc740](d1bc740))
    * **web:** Update dockerfile and ci to include google analytics env ([f2df4f4](f2df4f4))
    
    ### 🔨 Code Refactoring
    
    * **cli:** Replace arguments with options ([#615](#615)) ([498f44e](498f44e))
    * **platform:** Optimized codebase ([#629](#629)) ([d411081](d411081))
    * **platform:** Refactor project components ([#626](#626)) ([5b70805](5b70805))
    * **web:** Changed the text in the hero section of the web application ([#579](#579)) ([a92925f](a92925f))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 2.9.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    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