refactor(api): Polish api custom-widget controller, service and autor…

…ization
Vizzuality · Nov 8, 2024 · 89ff63f · 89ff63f
1 parent 72c2a74
commit 89ff63f
Show file tree

Hide file tree

Showing 9 changed files with 109 additions and 44 deletions.
diff --git a/api/src/decorators/check-authorization.decorator.ts b/api/src/decorators/check-authorization.decorator.ts
@@ -0,0 +1,21 @@
+import { SetMetadata } from '@nestjs/common';
+
+/**
+ * @description Decorator to inject a IS_PUBLIC_KEY metadata to the handler, which will be read by the JwtAuthGuard to allow public access to the handler.
+ */
+
+export const AUTHORIZATION_CHECKS = 'AUTHORIZATION_CHECKS';
+
+export const AUTHORIZATION_STRATEGIES = {
+  USER_ID: 'userId',
+  ROLE: 'role',
+} as const;
+
+export type AuthorizationCheck = [
+  (typeof AUTHORIZATION_STRATEGIES)[keyof typeof AUTHORIZATION_STRATEGIES],
+  params?: unknown[],
+];
+
+export const CheckAuthorization = (
+  ...authorizationChecks: AuthorizationCheck[]
+) => SetMetadata(AUTHORIZATION_CHECKS, authorizationChecks);
diff --git a/api/src/guards/check-authorization.guard.ts b/api/src/guards/check-authorization.guard.ts
@@ -0,0 +1,40 @@
+// auth-user.guard.ts
+import {
+  Injectable,
+  CanActivate,
+  ExecutionContext,
+  ForbiddenException,
+} from '@nestjs/common';
+import { Reflector } from '@nestjs/core';
+
+@Injectable()
+export class CheckAuthentication implements CanActivate {
+  constructor(private reflector: Reflector) {}
+
+  public canActivate(context: ExecutionContext): boolean {
+    const request = context.switchToHttp().getRequest();
+    const user = request.user;
+    const userIdFromParams = request.params.userId;
+
+    // Retrieve roles metadata set by @CheckAuthorization
+    const requiredRoles = this.reflector.get<string[]>(
+      'roles',
+      context.getHandler(),
+    );
+
+    // Check if the user ID from the request matches the one in the params
+    if (userIdFromParams && user.id !== userIdFromParams) {
+      throw new ForbiddenException('Access denied: User ID mismatch.');
+    }
+
+    // If roles are required, check that the user has at least one of the required roles
+    if (requiredRoles && requiredRoles.length > 0) {
+      const hasRole = requiredRoles.some((role) => user.roles?.includes(role));
+      if (!hasRole) {
+        throw new ForbiddenException('Access denied: Insufficient role.');
+      }
+    }
+
+    return true;
+  }
+}
diff --git a/api/src/guards/jwt-auth.guard.ts b/api/src/guards/jwt-auth.guard.ts
@@ -2,11 +2,17 @@ import {
   Injectable,
   ExecutionContext,
   UnauthorizedException,
+  ForbiddenException,
 } from '@nestjs/common';
 import { AuthGuard } from '@nestjs/passport';
 import { Reflector } from '@nestjs/core';
 import { Observable } from 'rxjs';
 import { IS_PUBLIC_KEY } from '../decorators/is-public.decorator';
+import {
+  AUTHORIZATION_CHECKS,
+  AUTHORIZATION_STRATEGIES,
+  AuthorizationCheck,
+} from '@api/decorators/check-authorization.decorator';
 
 @Injectable()
 export class JwtAuthGuard extends AuthGuard('jwt') {
@@ -29,10 +35,28 @@ export class JwtAuthGuard extends AuthGuard('jwt') {
     return super.canActivate(context);
   }
 
-  handleRequest(err, user) {
+  handleRequest(err: any, user: any, info: any, ctx: ExecutionContext): any {
     if (err || !user) {
       throw err || new UnauthorizedException();
     }
-    return user;
+
+    const authorizationChecks = this.reflector.get<AuthorizationCheck[]>(
+      AUTHORIZATION_CHECKS,
+      ctx.getHandler(),
+    );
+    if (authorizationChecks === undefined) return user;
+
+    for (let idx = 0; idx < authorizationChecks.length; idx++) {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      const [strategy, params] = authorizationChecks[idx];
+      if (strategy === AUTHORIZATION_STRATEGIES.USER_ID) {
+        const userId = ctx.switchToHttp().getRequest().params.userId;
+        if (userId === user.id) {
+          return user;
+        }
+      }
+    }
+
+    throw new ForbiddenException();
   }
 }
diff --git a/api/src/modules/widgets/custom-widgets.controller.ts b/api/src/modules/widgets/custom-widgets.controller.ts
@@ -5,17 +5,21 @@ import { ControllerResponse } from '@api/types/controller.type';
 import { CustomWidgetService } from '@api/modules/widgets/custom-widgets.service';
 import { GetUser } from '@api/decorators/get-user.decorator';
 import { User } from '@shared/dto/users/user.entity';
+import {
+  AUTHORIZATION_STRATEGIES,
+  CheckAuthorization,
+} from '@api/decorators/check-authorization.decorator';
 
 @Controller()
 export class CustomWidgetsController {
   public constructor(
     private readonly customWidgetsService: CustomWidgetService,
   ) {}
 
+  @CheckAuthorization([AUTHORIZATION_STRATEGIES.USER_ID])
   @TsRestHandler(c.searchCustomWidgets)
   public async searchCustomWidgets(
-    @GetUser()
-    user: User,
+    @GetUser() user: User,
   ): Promise<ControllerResponse> {
     return tsRestHandler(c.searchCustomWidgets, async ({ params, query }) => {
       const data = await this.customWidgetsService.searchCustomWidgets(
@@ -29,6 +33,7 @@ export class CustomWidgetsController {
     });
   }
 
+  @CheckAuthorization([AUTHORIZATION_STRATEGIES.USER_ID])
   @TsRestHandler(c.findCustomWidget)
   public async findCustomWidget(
     @GetUser() user: User,
@@ -46,6 +51,7 @@ export class CustomWidgetsController {
     });
   }
 
+  @CheckAuthorization([AUTHORIZATION_STRATEGIES.USER_ID])
   @TsRestHandler(c.createCustomWidget)
   public async createCustomWidget(
     @GetUser() user: User,
@@ -62,6 +68,7 @@ export class CustomWidgetsController {
     });
   }
 
+  @CheckAuthorization([AUTHORIZATION_STRATEGIES.USER_ID])
   @TsRestHandler(c.updateCustomWidget)
   public async updateCustomWidget(
     @GetUser() user: User,
@@ -70,7 +77,7 @@ export class CustomWidgetsController {
       const { userId, id } = params;
       const data = await this.customWidgetsService.updateCustomWidget(
         userId,
-        Number.parseInt(id),
+        id,
         body,
         {
           authenticatedUser: user,
@@ -80,15 +87,16 @@ export class CustomWidgetsController {
     });
   }
 
+  @CheckAuthorization([AUTHORIZATION_STRATEGIES.USER_ID])
   @TsRestHandler(c.deleteCustomWidget)
   public async deleteCustomWidget(
     @GetUser() user: User,
   ): Promise<ControllerResponse> {
     return tsRestHandler(c.deleteCustomWidget, async ({ params }) => {
       const { userId, id } = params;
-      const data = await this.customWidgetsService.removeCustomWidget(
+      const data = await this.customWidgetsService.deleteCustomWidget(
         userId,
-        Number.parseInt(id),
+        id,
         {
           authenticatedUser: user,
         },

diff --git a/api/src/modules/widgets/custom-widgets.service.ts b/api/src/modules/widgets/custom-widgets.service.ts
@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/no-unused-vars */
 import { AppBaseService } from '@api/utils/app-base.service';
 import { AppInfoDTO } from '@api/utils/info.dto';
 import {
@@ -42,10 +43,6 @@ export class CustomWidgetService extends AppBaseService<
     fetchSpecification: FetchSpecification,
     info: AppInfoDTO,
   ): Promise<ApiPaginationResponse<CustomWidget>> {
-    if (userId !== info.authenticatedUser.id) {
-      throw new ForbiddenException();
-    }
-
     return this.findAllPaginated(fetchSpecification, { userId });
   }
 
@@ -66,10 +63,6 @@ export class CustomWidgetService extends AppBaseService<
     id: number,
     info: AppInfoDTO,
   ): Promise<CustomWidget> {
-    if (userId !== info.authenticatedUser.id) {
-      throw new ForbiddenException();
-    }
-
     const customWidget = await this.customWidgetRepository.findOne({
       where: {
         id,
@@ -89,12 +82,6 @@ export class CustomWidgetService extends AppBaseService<
     params: CreateCustomWidgetDTO,
     info: AppInfoDTO,
   ): Promise<CustomWidget> {
-    // TODO: If we go this way, we will have to make this check at service level everywhere, and besides duplicating code (or if we extract it, coupling different behaviours)
-    // it can get really messy really fast.
-    if (userId !== info.authenticatedUser.id) {
-      throw new ForbiddenException();
-    }
-
     const baseWidget = await this.baseWidgetRepository.findOneBy({
       indicator: params.widgetIndicator,
     });
@@ -108,8 +95,6 @@ export class CustomWidgetService extends AppBaseService<
       throw exception;
     }
 
-    // TODO: All kinds of entity/model/dto validations should be left to the framework, and not be done manually.
-    // Additionally, final safety nets should be implemented at the database level.
     if (
       WidgetUtils.isValidDefaultVisualization(
         params.defaultVisualization,
@@ -141,10 +126,6 @@ export class CustomWidgetService extends AppBaseService<
     params: UpdateCustomWidgetDTO,
     info: AppInfoDTO,
   ) {
-    if (userId !== info.authenticatedUser.id) {
-      throw new ForbiddenException();
-    }
-
     const { name, widgetIndicator, defaultVisualization, filters } = params;
 
     const customWidget = await this.customWidgetRepository.findOneBy({
@@ -170,8 +151,7 @@ export class CustomWidgetService extends AppBaseService<
       relatedBaseWidget = await this.baseWidgetRepository.findOneBy({
         indicator: widgetIndicator,
       });
-      // TODO: This can never be null, as it comes from the DB entity, and the relation is set as mandatory, it will always exists
-      //       as long as the main entity exists
+
       if (relatedBaseWidget === null) {
         const exception = new NotFoundException('BaseWidget not found');
         this.logger.error(
@@ -185,8 +165,7 @@ export class CustomWidgetService extends AppBaseService<
       customWidget.defaultVisualization =
         relatedBaseWidget.defaultVisualization;
     }
-    // TODO: Again, all this checks should be done at framework level. Plus nested if statements are a code smell. It is very important to remember
-    //       that we are in the early stages of the project, we should make sure that basic/easy operations are kept simple and clean.
+
     if (defaultVisualization) {
       if (
         WidgetUtils.isValidDefaultVisualization(
@@ -208,21 +187,14 @@ export class CustomWidgetService extends AppBaseService<
       customWidget.filters = filters;
     }
 
-    // TODO: Finally, instead of building the new object manually with the updated fields, TypeORM can take care of this by using the update method.
-    //       You can pass the id, and all the body params that are received. It will check which are truthy, and update only those. If some value breaks
-    //       any constraint
     return this.customWidgetRepository.save(customWidget);
   }
 
-  public async removeCustomWidget(
+  public async deleteCustomWidget(
     userId: string,
     id: number,
     info?: AppInfoDTO,
   ) {
-    if (userId !== info.authenticatedUser.id) {
-      throw new ForbiddenException();
-    }
-
     const customWidget = await this.customWidgetRepository.findOneBy({
       id,
       user: { id: info.authenticatedUser.id },

diff --git a/...lizations/table/columns/actions-button/actions-menu/delete-visualization-button/index.tsx b/...lizations/table/columns/actions-button/actions-menu/delete-visualization-button/index.tsx
@@ -25,7 +25,7 @@ import { getAuthHeader } from "@/utils/auth-header";
 
 import { CLASS } from "../";
 
-const DeleteVisualizationButton: FC<{ id: string }> = ({ id }) => {
+const DeleteVisualizationButton: FC<{ id: number }> = ({ id }) => {
   const { data: session } = useSession();
   const queryClient = useQueryClient();
   const { toast } = useToast();

diff --git a/...ntainers/profile/saved-visualizations/table/columns/actions-button/actions-menu/index.tsx b/...ntainers/profile/saved-visualizations/table/columns/actions-button/actions-menu/index.tsx
@@ -40,7 +40,7 @@ const ActionsMenu: FC<{
         </button>
       </li>
       <li>
-        <DeleteVisualizationButton id={String(id)} />
+        <DeleteVisualizationButton id={Number(id)} />
       </li>
     </ul>
   );

diff --git a/client/src/containers/profile/saved-visualizations/table/columns/cell-name/index.tsx b/client/src/containers/profile/saved-visualizations/table/columns/cell-name/index.tsx
@@ -46,7 +46,7 @@ const CellName: FC<CellContext<ColumnsTable, unknown>> = ({
         const response = await client.users.updateCustomWidget.mutation({
           params: {
             userId: session?.user.id as string,
-            id: String(row.original.id),
+            id: Number(row.original.id),
           },
           body: {
             name: evt.currentTarget.value,

diff --git a/shared/contracts/users.contract.ts b/shared/contracts/users.contract.ts
@@ -156,7 +156,7 @@ export const usersContract = contract.router({
   updateCustomWidget: {
     method: 'PATCH',
     path: '/users/:userId/widgets/:id',
-    pathParams: z.object({ userId: z.string().uuid(), id: z.string() }),
+    pathParams: z.object({ userId: z.string().uuid(), id: z.coerce.number() }),
     body: UpdateCustomWidgetSchema,
     responses: {
       200: contract.type<ApiResponse<CustomWidget>>(),
@@ -168,7 +168,7 @@ export const usersContract = contract.router({
   deleteCustomWidget: {
     method: 'DELETE',
     path: '/users/:userId/widgets/:id',
-    pathParams: z.object({ userId: z.string().uuid(), id: z.string() }),
+    pathParams: z.object({ userId: z.string().uuid(), id: z.coerce.number() }),
     body: null,
     responses: {
       200: contract.type<ApiResponse<CustomWidget>>(),