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

End of sprint 15 merge into main #120

Merged
merged 99 commits into from
Mar 25, 2024
Merged

End of sprint 15 merge into main #120

merged 99 commits into from
Mar 25, 2024

Conversation

cherylli
Copy link
Collaborator

Description

End of sprint 15 merge into main

JoshuaHinman and others added 30 commits February 28, 2024 21:44
…-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.
Merge pull request #111 from chingu-x/dev
…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.
@cherylli
Copy link
Collaborator Author

Not sure why tests are broken, they are all fine locally. Will have a look after other more urgent tasks are done. Or if anyone has any idea, please post here 🙏

@timDeHof
Copy link
Collaborator

timDeHof commented Mar 22, 2024

I checkouted into the 'dev' branch and ran yarn test. The same tests are failing there too. It seems that there a typescript error with the req variable in ideations.service.spec.ts and ideations.controller.spec.ts.

Do you think it could be that CustomRequest interface is requiring the user object in the req variable to have 'email', 'roles', and 'voyageTeams' too?

@cherylli
Copy link
Collaborator Author

cherylli commented Mar 23, 2024

I checkouted into the 'dev' branch and ran yarn test. The same tests are failing there too. It seems that there a typescript error with the req variable in ideations.service.spec.ts and ideations.controller.spec.ts.

Do you think it could be that CustomRequest interface is requiring the user object in the req variable to have 'email', 'roles', and 'voyageTeams' too?

i just pulled the latest dev branch and all tests passed 🤔

this is the existing code

export interface CustomRequest extends Request {
    user: {
        userId: string;
        email: string;
        roles: string[];
        voyageTeams: VoyageTeam[];
    };
}

Maybe i need to do intersection instead of extends 🤔
https://stackoverflow.com/questions/69478954/nestjs-request-type-in-controller-using-an-authguard-attaches-unknown-key

@cherylli
Copy link
Collaborator Author

when it worked it was any, maybe I extended the wrong base class?
Hard to know what the issue is when i'm not getting errors locally. I wonder if it was node version 🤔

@timDeHof
Copy link
Collaborator

when it worked it was any, maybe I extended the wrong base class? Hard to know what the issue is when i'm not getting errors locally. I wonder if it was node version 🤔

maybe. But here is what typescript is saying on my computer:

Argument of type '{ user: { userId: string; }; }' is not assignable to parameter of type 'CustomRequest'.
  Type '{ user: { userId: string; }; }' is missing the following properties from type 'CustomRequest': cache, credentials, destination, headers, and 17 more.ts(2345)

I think the issue is that its looking for variables that Request needs.

@cherylli
Copy link
Collaborator Author

when it worked it was any, maybe I extended the wrong base class? Hard to know what the issue is when i'm not getting errors locally. I wonder if it was node version 🤔

maybe. But here is what typescript is saying on my computer:

Argument of type '{ user: { userId: string; }; }' is not assignable to parameter of type 'CustomRequest'.
  Type '{ user: { userId: string; }; }' is missing the following properties from type 'CustomRequest': cache, credentials, destination, headers, and 17 more.ts(2345)

I think the issue is that its looking for variables that Request needs.

yeah that's exactly what the github actions says. I don't know why i'm not getting error on my machine. WHat's your node version? Mine is v21.6.1

but if I extends Request, it should have everything in Request right?

@cherylli
Copy link
Collaborator Author

github actions using node 18. I might make a fork and try changing it

@cherylli
Copy link
Collaborator Author

nope node 21 in github actions doesn't work.

@timDeHof
Copy link
Collaborator

I have node version 20 on my computer

@cherylli
Copy link
Collaborator Author

I think maybe this - I didn't import {Request} from express
could you try this adding this to the src/global/types/CustomRequest.ts file so it looks like

import { Request } from "express";

type VoyageTeam = {
    teamId: number;
    memberId: number;
};

export interface CustomRequest extends Request {
    user: {
        userId: string;
        email: string;
        roles: string[];
        voyageTeams: VoyageTeam[];
    };
}

I tried that on my fork still fail though

image

compared to

image

@timDeHof
Copy link
Collaborator

I think maybe this - I didn't import {Request} from express could you try this adding this to the src/global/types/CustomRequest.ts file so it looks like

import { Request } from "express";

type VoyageTeam = {
    teamId: number;
    memberId: number;
};

export interface CustomRequest extends Request {
    user: {
        userId: string;
        email: string;
        roles: string[];
        voyageTeams: VoyageTeam[];
    };
}

I tried that on my fork still fail though

image

compared to

image

I tried that and got a similar error with res looking for alot more properties.
Screenshot 2024-03-23 at 11 15 30 AM

Copy link
Collaborator

@curtwl curtwl left a comment

Choose a reason for hiding this comment

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

all passed

Copy link
Collaborator

@timDeHof timDeHof left a comment

Choose a reason for hiding this comment

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

looks ready!!! I have ran unit tests and e2e tests on dev branch and it passed.

@cherylli cherylli merged commit 5b23c76 into main Mar 25, 2024
3 checks passed
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.

4 participants