feat: user auth #6

typeWolffo · 2024-07-09T23:27:36Z

No description provided.

typeWolffo · 2024-07-09T23:28:35Z

examples/common_nestjs_remix/apps/api/src/common/index.ts

  });
 }

 export function nullResponse() {
  return Type.Null();
 }
+
+export function setupValidation() {


sinclairzx81/typebox#654

thanks for info <3

k1eu · 2024-07-10T09:20:53Z

examples/common_nestjs_remix/apps/api/src/app.module.ts

-    ManagementModule,
+    JwtModule.register({
+      global: true,
+      secret: process.env.JWT_SECRET,


should we register it async to get the env from convig service @mikoscz ?

k1eu · 2024-07-10T09:21:37Z

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

+    response: baseResponse(accountSchema),
+    request: [{ type: "body", schema: createAccountSchema }],


nitpick but could we swap those in order for the easier read? Top down -> request response

k1eu · 2024-07-10T09:22:39Z

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

+    request: [{ type: "body", schema: createAccountSchema }],
+  })
+  async register(
+    data: Static<typeof createAccountSchema>,


wdyt if we should also export Static as type from the schema file?

k1eu · 2024-07-10T09:23:43Z

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

+      secure: process.env.NODE_ENV === "production",
+      sameSite: "strict",
+      maxAge: ACCESS_TOKEN_EXPIRATION_TIME,
+    });
+
+    response.cookie("refresh_token", refreshToken, {
+      httpOnly: true,
+      secure: process.env.NODE_ENV === "production",


we're using the reverseproxy so we can always use secure I think?

k1eu · 2024-07-10T09:24:52Z

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

+    response.cookie("access_token", accessToken, {
+      httpOnly: true,
+      secure: process.env.NODE_ENV === "production",
+      sameSite: "strict",
+      maxAge: ACCESS_TOKEN_EXPIRATION_TIME,
+    });
+
+    response.cookie("refresh_token", newRefreshToken, {
+      httpOnly: true,
+      secure: process.env.NODE_ENV === "production",
+      sameSite: "strict",
+      maxAge: REFRESH_TOKEN_EXPIRATION_TIME,
+    });


maybe a method for that in a service since it's in the login and refresh?

k1eu · 2024-07-10T09:32:56Z

examples/common_nestjs_remix/apps/api/src/users/api/users.controller.ts

+  }
+
+  @UseGuards(JwtAuthGuard)
+  @Patch(":id/update")


that can be just a :id so the request would be

PATCH users/:id

k1eu · 2024-07-10T09:36:07Z

examples/common_nestjs_remix/apps/api/src/users/api/users.controller.ts

+  async updateUser(
+    @Param("id") id: string,
+    @Body() data: UpdateUserBody,
+    @CurrentUser() currentUser: { userId: string },
+  ): Promise<BaseResponse<Static<typeof userSchema>>> {
+    {
+      if (currentUser.userId !== id) {
+        throw new ForbiddenException("You can only update your own account");
+      }
+
+      const updatedUser = await this.usersService.updateUser(id, data);
+
+      return new BaseResponse(updatedUser);
+    }
+  }


okay it's self update , so maybe we can have another task that'd add some RBAC and allow people with admin privilages to update all users eg. @mikoscz wdyt?

I think this is out of the scope of this one. But definitely, we want something like that.

k1eu · 2024-07-10T09:37:30Z

examples/common_nestjs_remix/apps/api/src/users/users.service.ts

+    try {
+      const allUsers = await this.db
+        .select(omit(getTableColumns(users), ["password", "refreshToken"]))
+        .from(users);
+
+      return allUsers;
+    } catch (error) {
+      throw new InternalServerErrorException("Error fetching users");
+    }


in array gets you can leave the try catchI believe - it'd return empty array if nothing is found and throw 500 either way if anything goes bad

k1eu · 2024-07-10T09:37:47Z

examples/common_nestjs_remix/apps/api/src/users/users.service.ts

+      const [user] = await this.db
+        .select(omit(getTableColumns(users), ["password", "refreshToken"]))
+        .from(users)
+        .where(eq(users.id, id));
+
+      if (!user) {
+        throw new NotFoundException("User not found");
+      }
+
+      return user;


same here I think try catch is not necessary for the case here

k1eu · 2024-07-10T09:39:35Z

examples/common_nestjs_remix/apps/api/src/utils/guards/jwt-auth-guard.ts

+    const token = request.cookies["access_token"];
+
+    if (!token) {
+      throw new UnauthorizedException("Access token not found");


cool, just to leave it up there . Remember then to use Unauthorized to actions that should log out user on the frontend also, and If used doesn't have access to something like a resource - to return ForbiddenException

mikoscz · 2024-07-10T09:45:19Z

Remove the trys and use error filtering instead.

mikoscz · 2024-07-10T09:46:17Z

We should do 180 on the auth, instead of marking every endpoint with guard we want to guard all endpoints and whitelist only the public.

mikoscz · 2024-07-10T09:50:26Z

examples/common_nestjs_remix/apps/api/src/users/api/users.controller.ts

+    response: baseResponse(userSchema),
+  })
+  async getUserById(
+    @Param() params: { id: string },


I think that with nestjs-typebox @Param decorator is unnecessary, params are passed in the same order as validate params(they are missing)

typeWolffo · 2024-07-11T00:17:19Z

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

+  @Public()
+  @UseGuards(AuthGuard("local"))
+  @Post("login")


@mikoscz is this global jwt auth guard properly implemented and @Public can it be used together with @UseGuards(AuthGuard("local"))?

Write some tests and we will see :D

mikoscz · 2024-07-11T08:46:43Z

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

+    const [user] = await this.db
+      .select()
+      .from(users)
+      .where(eq(users.email, email));
+
+    if (!user) return null;
+
+    const [userCredentials] = await this.db
+      .select()
+      .from(credentials)
+      .where(eq(credentials.userId, user.id));
+
+    if (!userCredentials) return null;


mikoscz · 2024-07-11T08:48:21Z

examples/common_nestjs_remix/apps/api/src/utils/decorators/public.decorator.ts

@@ -0,0 +1,3 @@
+import { SetMetadata } from "@nestjs/common";


Move it to common both guards and decorators

mikoscz · 2024-07-11T08:49:28Z

examples/common_nestjs_remix/apps/api/src/storage/schema/index.ts

-  name: text("name").notNull(),
-  description: text("description"),
+  userId: uuid("user_id")
+    .references(() => users.id)


add cascade

mikoscz · 2024-07-11T08:50:40Z

examples/common_nestjs_remix/apps/api/src/users/users.service.ts

+
+  public async deleteUser(id: string) {
+    const deleted = await this.db.transaction(async (trx) => {
+      await trx.delete(credentials).where(eq(credentials.userId, id));


Set the onDelete cascade on the db level and then you can remove that

mikoscz · 2024-07-11T08:55:31Z

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

+    await this.updateRefreshToken(userId, null);
+  }
+
+  public async refreshTokens(userId: string, refreshToken: string) {


Can you explain me how we would like to use refresh token? I am thinking why do we need to store it in the db(probably I am missing something)

mikoscz · 2024-07-11T09:01:41Z

examples/common_nestjs_remix/apps/api/src/users/api/users.controller.ts

+  async updateUser(
+    id: string,
+    @Body() data: UpdateUserBody,
+    @CurrentUser() currentUser: { userId: string },


I think we should unify user type and create common type for it

I would go to something similar to user currentUser.id since currentUser.userId says the same info twice, just a nitpick

mikoscz · 2024-07-11T09:12:31Z

examples/common_nestjs_remix/apps/api/src/common/index.ts

@@ -23,3 +29,7 @@ export function baseResponse(data: TObject) {
 export function nullResponse() {
  return Type.Null();
 }
+
+export function setupValidation() {


This one looks like util since we are using it only during the bootstrap. Probably we need more formats eg. email I saw it is used but IDK if it is registered.

mikoscz · 2024-07-11T09:14:00Z

examples/common_nestjs_remix/apps/api/src/auth/token.service.ts

+      maxAge: ACCESS_TOKEN_EXPIRATION_TIME,
+    });
+
+    response.cookie("refresh_token", refreshToken, {


For the refresh I would limit the path to only endpoint that handles refresh.

mikoscz · 2024-07-11T09:22:01Z

examples/common_nestjs_remix/apps/api/src/utils/guards/jwt-auth-guard.ts

+
+    try {
+      const payload = await this.jwtService.verifyAsync(token, {
+        secret: process.env.JWT_SECRET,


Can we use configService?

mikoscz · 2024-07-11T09:22:15Z

examples/common_nestjs_remix/apps/api/src/utils/guards/refresh-token-guard.ts

+
+    try {
+      const payload = await this.jwtService.verifyAsync(refreshToken, {
+        secret: process.env.REFRESH_SECRET,


Can we use configService?

mikoscz

GJ overall, we are still missing few pieces:

tests
support for Bearer(for mobile apps)
idk if we need email validation?

k1eu · 2024-07-11T10:00:21Z

For the email validation I'd say we need it, but maybe we can add it as separate task that will handle Email integration and update the auth flow with email confirmation?

typeWolffo · 2024-07-11T20:51:50Z

support for Bearer(for mobile apps)

@mikoscz I've added the Bearer support - what you think?

* feat: configure jest for TypeScript and test database setup * feat: expand e2e tests for auth.controller and add new migration file * feat: update password change functionality and schema * feat: initialize test context and services * feat: create unit test setup function * feat: initialize test context and services for e2e tests * feat: create e2e test setup function * refactor: improve user and credential factories, add userWithCredentialFactory * feat: update dependencies and add JWT_REFRESH_SECRET * refactor: refactor user factory and add hashPassword function * fix: update password handling in user controller e2e tests * feat: refactor userFactory and authService.register * chore: update assertions in auth and users service tests

typeWolffo added 4 commits July 9, 2024 20:43

feat: implement user authentication and authorization

da13ad3

feat: add user management and access control

d899d9c

feat: implement user data management

a16c5f9

feat: improve error handling and remove unnecessary logs

69c28c6

typeWolffo commented Jul 9, 2024

View reviewed changes

typeWolffo force-pushed the jw/user-auth branch from afe0ef4 to d763c93 Compare July 9, 2024 23:52

feat: add uuid dependency and setup validation

a43add0

typeWolffo force-pushed the jw/user-auth branch from d763c93 to a43add0 Compare July 9, 2024 23:54

k1eu requested review from mikoscz and k1eu July 10, 2024 08:29

k1eu assigned typeWolffo Jul 10, 2024

k1eu requested changes Jul 10, 2024

View reviewed changes

mikoscz reviewed Jul 10, 2024

View reviewed changes

typeWolffo added 7 commits July 11, 2024 01:34

feat: add credentials table and user_id foreign key

78b96df

refactor: update JwtModule configuration

a9b5a7a

fix: update route endpoints in users controller

7ec41b7

feat: add token service for setting and clearing cookies

0755a18

feat: add JwtAuthGuard to AppModule and use Public decorator

a2039e0

refactor: remove JwtAuthGuard usage from controllers

083da80

refactor: remove JwtAuthGuard usage from UsersController methods

28b97ca

typeWolffo commented Jul 11, 2024

View reviewed changes

typeWolffo requested review from mikoscz and k1eu July 11, 2024 00:21

refactor: update JwtAuthGuard constructor and canActivate method

ff805e4

typeWolffo force-pushed the jw/user-auth branch from 2a2689e to ff805e4 Compare July 11, 2024 00:40

mikoscz reviewed Jul 11, 2024

View reviewed changes

mikoscz requested changes Jul 11, 2024

View reviewed changes

typeWolffo added 5 commits July 11, 2024 18:32

feat: implement JWT secret configuration

2c40cbe

refactor: update response schemas in AuthController

5eac8cc

chore: remove unused code and imports

50c440f

fix: update user deletion logic in UsersService

086bf94

feat: add Bearer support

26173dc

typeWolffo force-pushed the jw/user-auth branch from 69c8138 to 26173dc Compare July 11, 2024 20:45

typeWolffo requested a review from mikoscz July 11, 2024 20:53

k1eu approved these changes Jul 18, 2024

View reviewed changes

typeWolffo force-pushed the jw/user-auth branch from 0cff62a to 03fcabb Compare July 18, 2024 12:12

typeWolffo merged commit 6dbc6d0 into main Jul 18, 2024

typeWolffo deleted the jw/user-auth branch July 18, 2024 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: user auth #6

feat: user auth #6

typeWolffo commented Jul 9, 2024

typeWolffo Jul 9, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

mikoscz Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

k1eu Jul 10, 2024

mikoscz commented Jul 10, 2024

mikoscz commented Jul 10, 2024

mikoscz Jul 10, 2024

typeWolffo Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz Jul 11, 2024

mikoscz left a comment

k1eu commented Jul 11, 2024

typeWolffo commented Jul 11, 2024

		response: baseResponse(accountSchema),
		request: [{ type: "body", schema: createAccountSchema }],

		@@ -0,0 +1,3 @@
		import { SetMetadata } from "@nestjs/common";

feat: user auth #6

feat: user auth #6

Conversation

typeWolffo commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikoscz commented Jul 10, 2024

mikoscz commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikoscz left a comment

Choose a reason for hiding this comment

k1eu commented Jul 11, 2024

typeWolffo commented Jul 11, 2024