Merge pull request #73 from bcgov/AI-1153

AI-1153 Keycloak & Identity Check Cleanup

Author: dbarkowsky

apps/backend-services/src/api-key/api-key.controller.spec.ts

@@ -70,23 +70,6 @@ describe("ApiKeyController", () => {
       expect(apiKeyService.getApiKey).toHaveBeenCalledWith("group123");
     });
 
-    it("should throw ForbiddenException when user is not a group member", async () => {
-      await expect(
-        controller.getApiKey(
-          {
-            ...mockRequest,
-            resolvedIdentity: {
-              userId: "testuser",
-              isSystemAdmin: false,
-              groupRoles: {},
-            },
-          } as any,
-          "group123",
-        ),
-      ).rejects.toThrow(ForbiddenException);
-      expect(apiKeyService.getApiKey).not.toHaveBeenCalled();
-    });
-
     it("should throw BadRequestException when groupId is missing", async () => {
       await expect(
         controller.getApiKey(mockRequest as any, ""),
@@ -136,23 +119,6 @@ describe("ApiKeyController", () => {
       );
     });
 
-    it("should throw ForbiddenException when user is not a group member", async () => {
-      await expect(
-        controller.generateApiKey(
-          {
-            ...mockRequest,
-            resolvedIdentity: {
-              userId: "testuser",
-              isSystemAdmin: false,
-              groupRoles: {},
-            },
-          } as any,
-          { groupId: "group123" },
-        ),
-      ).rejects.toThrow(ForbiddenException);
-      expect(apiKeyService.generateApiKey).not.toHaveBeenCalled();
-    });
-
     it("should not throw when user has no email for regenerate", async () => {
       mockApiKeyService.regenerateApiKey.mockResolvedValue({});
       mockApiKeyService.getApiKeyGroupId.mockResolvedValue("group123");

apps/backend-services/src/api-key/api-key.controller.ts

@@ -60,11 +60,6 @@ export class ApiKeyController {
     if (!groupId) {
       throw new BadRequestException("groupId query parameter is required");
     }
-    await identityCanAccessGroup(
-      req.resolvedIdentity,
-      groupId,
-      GroupRole.ADMIN,
-    );
     const apiKey = await this.apiKeyService.getApiKey(groupId);
     return { apiKey };
   }
@@ -90,7 +85,6 @@ export class ApiKeyController {
         "User ID is required to generate an API key",
       );
     }
-    identityCanAccessGroup(req.resolvedIdentity, body.groupId, GroupRole.ADMIN);
     const apiKey = await this.apiKeyService.generateApiKey(
       userId,
       body.groupId,

apps/backend-services/src/auth/keycloak-jwt.strategy.ts

@@ -33,8 +33,6 @@ interface KeycloakJwtPayload {
  */
 @Injectable()
 export class KeycloakJwtStrategy extends PassportStrategy(Strategy, "jwt") {
-  private readonly clientId: string;
-
   constructor(configService: ConfigService) {
     const ssoAuthServerUrl = configService.get<string>("SSO_AUTH_SERVER_URL");
     const realm = configService.get<string>("SSO_REALM");
@@ -96,64 +94,19 @@ export class KeycloakJwtStrategy extends PassportStrategy(Strategy, "jwt") {
       // token signed with the public key as a symmetric secret.
       algorithms: ["RS256"],
     });
-
-    this.clientId = clientId;
   }
 
   /**
    * Called by Passport AFTER the JWT signature, issuer, audience, and expiry
    * have all been verified. The returned object is attached to `req.user`.
-   *
-   * We normalize roles here (via extractRoles) so that downstream handlers
-   * can check `req.user.roles` without caring about Keycloak's nested claim
-   * structure.
    */
   validate(payload: KeycloakJwtPayload): User {
-    const normalizedRoles = this.extractRoles(payload);
-
     return {
       sub: payload.sub,
       idir_username: payload.idir_username,
       display_name: payload.display_name,
       email: payload.email,
-      roles: normalizedRoles,
       ...payload,
     };
   }
-
-  // TODO: review. bcgov may store roles in a different structure,
-  // also determine if we need other roles from other sources.
-  /**
-   * Keycloak doesn't put roles in a single place — depending on how the realm
-   * and client are configured, roles can appear in up to three locations:
-   *
-   *  - `roles[]`                           — top-level claim (if client mappers add it)
-   *  - `realm_access.roles[]`              — realm-wide roles (e.g. "admin", "user")
-   *  - `resource_access.<clientId>.roles[]` — roles scoped to a specific client
-   *
-   * extractRoles merges all three into a flat, deduplicated array so the rest
-   * of the app (controllers) can just check `user.roles.includes("admin")`
-   * without knowing which Keycloak claim it came from.
-   */
-  private extractRoles(payload: KeycloakJwtPayload): string[] {
-    const roleSet = new Set<string>();
-
-    const pushRoles = (roles?: string[]) => {
-      roles?.forEach((role) => {
-        if (role) {
-          roleSet.add(role);
-        }
-      });
-    };
-
-    // Collect from all potential sources
-    pushRoles(payload.roles);
-    pushRoles(payload.realm_access?.roles);
-
-    const resourceRoles = payload.resource_access ?? {};
-    Object.values(resourceRoles).forEach((access) => pushRoles(access.roles));
-    pushRoles(resourceRoles[this.clientId]?.roles);
-
-    return Array.from(roleSet);
-  }
 }

apps/backend-services/src/azure/azure.controller.spec.ts

@@ -109,19 +109,6 @@ describe("AzureController", () => {
       expect(result).toEqual({ id: "1" });
       expect(classifierService.createClassifierModel).toHaveBeenCalled();
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      const body = {
-        name: "c1",
-        description: "desc",
-        source: ClassifierSource.AZURE,
-        status: ClassifierStatus.READY,
-        group_id: "g1",
-      };
-      await expect(controller.createClassifier(req, body)).rejects.toThrow(
-        ForbiddenException,
-      );
-    });
     it("should throw ForbiddenException if classifier exists", async () => {
       classifierService.findClassifierModel.mockResolvedValue({ id: "1" });
       const req = createMockReq("user1", ["g1"]);
@@ -173,17 +160,6 @@ describe("AzureController", () => {
         expect.any(Buffer),
       );
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      await expect(
-        controller.uploadClassifierDocuments(
-          req,
-          [],
-          { name: "c1", label: "l1" },
-          "g1",
-        ),
-      ).rejects.toThrow(ForbiddenException);
-    });
     it("should throw NotFoundException if classifier does not exist", async () => {
       classifierService.findClassifierModel.mockResolvedValue(null);
       const req = createMockReq("user1", ["g1"]);
@@ -210,15 +186,6 @@ describe("AzureController", () => {
         "classifier/g1/c1/",
       );
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      await expect(
-        controller.deleteClassifierDocuments(req, {
-          name: "c1",
-          group_id: "g1",
-        }),
-      ).rejects.toThrow(ForbiddenException);
-    });
     it("should throw NotFoundException if classifier does not exist", async () => {
       classifierService.findClassifierModel.mockResolvedValue(null);
       const req = createMockReq("user1", ["g1"]);
@@ -306,12 +273,6 @@ describe("AzureController", () => {
       );
       expect(result).toEqual({ result: "ok" });
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      await expect(
-        controller.requestClassification(req, { name: "c1" }, mockFile, "g1"),
-      ).rejects.toThrow(ForbiddenException);
-    });
     it("should throw NotFoundException if classifier does not exist", async () => {
       classifierService.findClassifierModel.mockResolvedValue(null);
       const req = createMockReq("user1", ["g1"]);
@@ -373,15 +334,6 @@ describe("AzureController", () => {
         }),
       ).rejects.toThrow();
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      await expect(
-        controller.getTrainingResult(req, {
-          name: "c1",
-          group_id: "g1",
-        }),
-      ).rejects.toThrow(ForbiddenException);
-    });
     it("should throw NotFoundException if classifier not found", async () => {
       classifierService.findClassifierModel.mockResolvedValue(null);
       const req = createMockReq("user1", ["g1"]);
@@ -436,12 +388,6 @@ describe("AzureController", () => {
         controller.getTrainingResult(req, { name: null, group_id: null }),
       ).rejects.toThrow();
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      await expect(
-        controller.getTrainingResult(req, { name: "c1", group_id: "g1" }),
-      ).rejects.toThrow(ForbiddenException);
-    });
     it("should throw NotFoundException if classifier not found", async () => {
       classifierService.findClassifierModel.mockResolvedValue(null);
       const req = createMockReq("user1", ["g1"]);
@@ -514,13 +460,6 @@ describe("AzureController", () => {
       expect(result).toEqual(["labelA/doc1", "labelB/doc2"]);
       expect(storageService.list).toHaveBeenCalledWith("classifier/g1/c1/");
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      const query = { name: "c1", group_id: "g1" };
-      await expect(
-        controller.getClassifierDocuments(req, query),
-      ).rejects.toThrow(ForbiddenException);
-    });
     it("should throw NotFoundException if classifier does not exist", async () => {
       classifierService.findClassifierModel.mockResolvedValue(null);
       const req = createMockReq("user1", ["g1"]);
@@ -548,13 +487,6 @@ describe("AzureController", () => {
         "user1",
       );
     });
-    it("should throw ForbiddenException if user not in group", async () => {
-      const req = createMockReq();
-      const body = { name: "c1", group_id: "g1", description: "desc" };
-      await expect(controller.updateClassifier(req, body)).rejects.toThrow(
-        ForbiddenException,
-      );
-    });
     it("should throw NotFoundException if classifier does not exist", async () => {
       classifierService.findClassifierModel.mockResolvedValue(null);
       const req = createMockReq("user1", ["g1"]);

apps/backend-services/src/azure/azure.controller.ts

@@ -77,7 +77,7 @@ export class AzureController {
   ) {}
 
   @Get("classifier")
-  @Identity({ minimumRole: GroupRole.MEMBER })
+  @Identity()
   @ApiOperation({
     summary: "Get classifiers for user groups",
     description:
@@ -123,7 +123,6 @@ export class AzureController {
   })
   async createClassifier(@Request() req, @Body() body: ClassifierCreationDto) {
     const { name, description, source, group_id } = body;
-    identityCanAccessGroup(req.resolvedIdentity, group_id);
 
     // Does this classifier already exist?
     const classifier = await this.classifierService.findClassifierModel(
@@ -167,8 +166,6 @@ export class AzureController {
   async updateClassifier(@Request() req, @Body() body: UpdateClassifierDto) {
     const { name, group_id, description, source } = body;
 
-    identityCanAccessGroup(req.resolvedIdentity, group_id);
-
     // Check if classifier exists
     const classifier = await this.classifierService.findClassifierModel(
       name,
@@ -238,7 +235,6 @@ export class AzureController {
     @Query("group_id") group_id: string,
   ): Promise<UploadClassifierDocumentsResponseDto> {
     const { name, label } = body;
-    identityCanAccessGroup(req.resolvedIdentity, group_id);
 
     const existingModelData = await this.classifierService.findClassifierModel(
       name,
@@ -281,7 +277,6 @@ export class AzureController {
     @Query() query: GetClassifierDocumentsQueryDto,
   ): Promise<string[]> {
     const { name, group_id } = query;
-    identityCanAccessGroup(req.resolvedIdentity, group_id);
 
     const existingModelData = await this.classifierService.findClassifierModel(
       name,
@@ -315,7 +310,6 @@ export class AzureController {
     @Query() query: DeleteClassifierDocumentsDto,
   ): Promise<void> {
     const { name, group_id, folder } = query;
-    identityCanAccessGroup(req.resolvedIdentity, group_id);
 
     const existingModelData = await this.classifierService.findClassifierModel(
       name,
@@ -366,7 +360,6 @@ export class AzureController {
   ): Promise<ClassifierModelResponseDto> {
     const { name, group_id } = body;
     const userId = req.user.sub;
-    identityCanAccessGroup(req.resolvedIdentity, group_id);
 
     // Respond immediately and run the heavy work in the background
     const model = await this.classifierService.updateClassifierModel(
@@ -454,7 +447,6 @@ export class AzureController {
   ): Promise<ClassifierResponseDto> {
     const { name } = body;
     const userId = req.user.sub;
-    identityCanAccessGroup(req.resolvedIdentity, group_id);
     // Is there a classifier trained for this group?
     const classifier = await this.classifierService.findClassifierModel(
       name,

apps/backend-services/src/benchmark/benchmark-project.controller.ts

@@ -39,6 +39,7 @@ import {
   getIdentityGroupIds,
   identityCanAccessGroup,
 } from "@/auth/identity.helpers";
+import { GroupRole } from "@/generated";
 import { BenchmarkProjectService } from "./benchmark-project.service";
 import { CreateProjectDto, ProjectDetailsDto, ProjectSummaryDto } from "./dto";
 
@@ -53,7 +54,11 @@ export class BenchmarkProjectController {
 
   @Post()
   @HttpCode(HttpStatus.CREATED)
-  @Identity({ allowApiKey: true })
+  @Identity({
+    allowApiKey: true,
+    groupIdFrom: { body: "groupId" },
+    minimumRole: GroupRole.MEMBER,
+  })
   @ApiOperation({ summary: "Create a benchmark project" })
   @ApiBody({ type: CreateProjectDto })
   @ApiCreatedResponse({
@@ -74,8 +79,6 @@ export class BenchmarkProjectController {
 
     const userId = req.user?.sub || req.resolvedIdentity?.userId || "anonymous";
 
-    identityCanAccessGroup(req.resolvedIdentity, createProjectDto.groupId);
-
     return this.benchmarkProjectService.createProject(createProjectDto, userId);
   }
 

apps/backend-services/src/benchmark/benchmark-run.controller.ts

@@ -64,7 +64,7 @@ export class BenchmarkRunController {
   ): Promise<void> {
     const project =
       await this.benchmarkProjectService.getProjectById(projectId);
-    await identityCanAccessGroup(req.resolvedIdentity, project.groupId);
+    identityCanAccessGroup(req.resolvedIdentity, project.groupId);
   }
 
   @Post("definitions/:definitionId/runs")

apps/backend-services/src/benchmark/dataset.controller.spec.ts

@@ -1,6 +1,6 @@
 jest.mock("@/auth/identity.helpers", () => ({
-  identityCanAccessGroup: jest.fn().mockResolvedValue(undefined),
-  getIdentityGroupIds: jest.fn().mockResolvedValue(["test-group"]),
+  identityCanAccessGroup: jest.fn().mockReturnValue(undefined),
+  getIdentityGroupIds: jest.fn().mockReturnValue(["test-group"]),
 }));
 
 import { BadRequestException, NotFoundException } from "@nestjs/common";