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/delete users tokens #116

Merged
merged 24 commits into from
Mar 15, 2024
Merged

Feat/delete users tokens #116

merged 24 commits into from
Mar 15, 2024

Conversation

timDeHof
Copy link
Collaborator

@timDeHof timDeHof commented Mar 8, 2024

Description

This PR adds a DELETE endpoint to auth services that will revoke an user's refresh token when given a user's id or email.This endpoint has role guard attached so that only users with admin role can initiate the endpoint.

Also this PR tests the new functionality of revoking a refresh token. The tests cover scenarios where the refresh token is successfully revoked, where the email is invalid, and where the user is not permitted to revoke the token. These tests ensure that the revoking of refresh tokens is working correctly and that the appropriate responses are returned based on the different scenarios.

In addition, swagger UI api has been updated to allow for users to test the new endpoint.

Issue link

Fixes # (issue) "Create endpoint to revoke refresh token" from trello

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature updates / changes
  • Tests
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

You can run yarn test:e2e or yarn test:e2e:docker, then look for auth.e2e-spec.ts to pass.
Screenshot 2024-03-08 at 1 05 13 PM
Screenshot 2024-03-08 at 1 04 41 PM

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have updated the change log

…-modules

The .yarnrc.yml file is added with the nodeLinker configuration set to node-modules. This configuration specifies that Yarn should use the "node-modules" linker for managing dependencies.
…refresh tokens

The revoke method is added to the AuthService in order to provide functionality for revoking refresh tokens. It takes an optional parameter which can be either userId or email. If userId is provided, the refreshToken field of the user with that userId is set to null. If email is provided, the refreshToken field of the user with that email is set to null. If neither userId nor email is provided, a NotFoundException is thrown with a message indicating that the user was not found. This method is useful for implementing a logout functionality where the refresh token is invalidated.
…'s refresh token

feat(auth.controller.ts): add revoke endpoint to allow revoking user's refresh token with a valid user id
The revoke endpoint is added to the AuthController to allow revoking a user's refresh token. This endpoint requires a valid user id and removes the user's refresh token. It is protected by the RolesGuard and can only be accessed by users with the "Admin" role. The endpoint returns a success response if the refresh token is successfully revoked. If the user is not found, a NotFoundErrorResponse is returned. If the user doesn't have permission to perform the operation, a ForbiddenErrorResponse is returned.
…erties

The RevokeRTDTo class is added to the auth/dto directory. It includes two properties: userId and email. The userId property is optional and represents the user ID, while the email property is a string with an example value of "elza59@ethereal.email". This class is used for data transfer objects related to revoking user access.
…ality

This commit adds tests for the new functionality of revoking a refresh token. The tests cover scenarios where the refresh token is successfully revoked, where the email is invalid, and where the user is not permitted to revoke the token. These tests ensure that the revoking of refresh tokens is working correctly and that the appropriate responses are returned based on the different scenarios.
… method

The console.log statement in the revoke method has been removed as it was not providing any useful information and was unnecessary for the functionality of the method.
…operties for better validation flexibility

The IsOptional decorator has been added to the userId and email properties in the RevokeRTDTo class. This allows these properties to be optional during validation, providing better flexibility in the validation process.
➤ YN0002: │ chingu-dashboard-be@workspace:. doesn't provide express (pa980d), requested by swagger-ui-express
➤ YN0002: │ chingu-dashboard-be@workspace:. doesn't provide webpack (p8e44a), requested by ts-loader
➤ YN0000: │ Some peer dependencies are incorrectly met; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code
➤ YN0000: └ Completed in 0s 290ms
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 315ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed in 0s 442ms
➤ YN0000: Done with warnings in 1s 178ms after updating branch with  branch
…token from revoke

The filename change was for easier readability and two more class validator were added to the properties: userId and email
…t to the correct file

refactor(auth.controller.ts): rename revoke method parameter from revokeTRDTo to body for clarity
refactor(auth.controller.ts): rename revoke method to revokeRefreshToken for clarity
The import statement for the RevokeRTDTo DTO has been fixed to point to the correct file. The parameter name of the revoke method has been changed from revokeTRDTo to body for clarity. Additionally, the method name has been changed to revokeRefreshToken to better reflect its purpose.
… and add validation for userId and email parameters

The revoke method in the AuthService class has been renamed to revokeRefreshToken to better reflect its purpose. Additionally, the method now includes validation to ensure that either the userId or email parameter is provided, but not both. If both parameters are provided, a BadRequestException is thrown with an appropriate error message. This change improves the clarity and reliability of the code.
…en email and user id are provided

The new test case checks if a 400 status code is returned when both an email and user id are provided. This is to ensure that the server handles this scenario correctly and denies access.
Copy link
Collaborator

@cherylli cherylli left a comment

Choose a reason for hiding this comment

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

this PR is very well done. Thank you

src/auth/auth.controller.ts Outdated Show resolved Hide resolved
src/auth/auth.controller.ts Outdated Show resolved Hide resolved
test/auth.e2e-spec.ts Show resolved Hide resolved
test/auth.e2e-spec.ts Show resolved Hide resolved
…ame in revoke method

The import statement for the RevokeRTDto was incorrect, it was importing RevokeRTDTo instead. The parameter name in the revoke method was also incorrect, it was using RevokeRTDTo instead of RevokeRTDto. This commit fixes these typos to ensure the correct import and parameter name are used.
…TDTo to RevokeRTDto for consistency and clarity

The class name has been corrected from RevokeRTDTo to RevokeRTDto to improve consistency and clarity in the codebase.
The RolesGuard import and usage in the auth.controller.ts file have been removed as it is no longer needed. This improves code cleanliness and removes unnecessary dependencies.
…r a user

The new test checks if the refresh token is null for a specific user. This is important to ensure that the refresh token is properly set to null when it should be.
@timDeHof timDeHof requested a review from cherylli March 10, 2024 17:07
Copy link
Collaborator

@cherylli cherylli left a comment

Choose a reason for hiding this comment

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

image
I think this is not accurate, since we can either use email or userID, maybe it should be auth/refresh/revoke or auth/revoke-refresh-token

You can look at this for reference https://restfulapi.net/resource-naming/


image
I got this when user is not found in the database, it's not very descriptive


200 - delete token successful works, but there's no response message
image

… revoke refresh token endpoint

feat(auth.controller.ts): add response message and status code to the revoke refresh token endpoint
The summary and description of the revoke refresh token endpoint have been improved to clarify that only admins can perform this action and that the user's id or email can be used to revoke the refresh token. Additionally, a response message and status code have been added to provide feedback to the client after the refresh token has been successfully revoked.
…n for non-existent user

Error handling has been added to handle the case when the user specified by  or  does not exist, throwing a  with an appropriate error message.
…h/userId" to "/auth/refresh/revoke" for better clarity and consistency

The revokeRTUrl endpoint has been updated to "/auth/refresh/revoke" to provide a more descriptive and consistent naming convention. This change improves the readability and maintainability of the code.
@timDeHof
Copy link
Collaborator Author

updates:

  1. changed endpoint to /refresh/revoke from auth/refresh/userid for better clarity and readability.
  2. changed the swagger description to include the 'email' option in the body and added an API response body for letting the user know refresh token revoking was successfully.
  3. added error handling that gives a not-found exception if user wasn't found either by id or email.

@timDeHof timDeHof requested a review from cherylli March 12, 2024 03:27
cherylli
cherylli previously approved these changes Mar 12, 2024
Copy link
Collaborator

@cherylli cherylli left a comment

Choose a reason for hiding this comment

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

thanks for the changes!

src/auth/auth.service.ts Outdated Show resolved Hide resolved
src/auth/auth.service.ts Outdated Show resolved Hide resolved
Co-authored-by: Cheryl M <webmaster@cherylli.com>
Co-authored-by: Cheryl M <webmaster@cherylli.com>
Copy link
Contributor

@JoshuaHinman JoshuaHinman left a comment

Choose a reason for hiding this comment

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

e2e tests passed , swagger responses work, code looks great!

@timDeHof timDeHof merged commit 3b26f6a into dev Mar 15, 2024
1 check passed
@timDeHof timDeHof deleted the feat/delete-users-tokens branch March 15, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants