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

#3881 - Validate user account for all routes #3985

Merged
merged 21 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sims.code-workspace
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
],
"extensions": {
"recommendations": [
"vue.vscode-typescript-vue-plugin",
"Vue.volar",
"dbaeumer.vscode-eslint",
"esbenp.prettier-vscode",
"streetsidesoftware.code-spell-checker",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@ import { AuthTestController } from "../../../testHelpers/controllers/auth-test/a
import { DiscoveryModule } from "@golevelup/nestjs-discovery";
import { DataSource } from "typeorm";
import { JwtService } from "@nestjs/jwt";
import { INVALID_BETA_USER } from "../../../constants";
import { BEARER_AUTH_TYPE } from "../../../testHelpers";
import { INVALID_BETA_USER, MISSING_USER_ACCOUNT } from "../../../constants";
import {
BEARER_AUTH_TYPE,
mockUserLoginInfo,
resetMockUserLoginInfo,
} from "../../../testHelpers";
import * as dayjs from "dayjs";
import { Student, User } from "@sims/sims-db";

describe("Authentication (e2e)", () => {
// Nest application to be shared for all e2e tests
Expand All @@ -38,6 +43,7 @@ describe("Authentication (e2e)", () => {
let configService: ConfigService;
let db: E2EDataSources;
let studentDecodedToken: any;
let moduleFixture: TestingModule;

beforeAll(async () => {
await KeycloakConfig.load();
Expand All @@ -57,7 +63,7 @@ describe("Authentication (e2e)", () => {
);
aestAccessToken = aestToken.access_token;

const moduleFixture: TestingModule = await Test.createTestingModule({
moduleFixture = await Test.createTestingModule({
imports: [AppModule, createZeebeModuleMock(), DiscoveryModule],
// AuthTestController is used only for e2e tests and could be
// changed as needed to implement more test scenarios.
Expand All @@ -80,6 +86,8 @@ describe("Authentication (e2e)", () => {
jest
.spyOn(configService, "allowBetaUsersOnly", "get")
.mockReturnValue(false);
// Reset mock user login info.
await resetMockUserLoginInfo(moduleFixture);
});

it("Load publicKey from Keycloak", async () => {
Expand Down Expand Up @@ -254,6 +262,40 @@ describe("Authentication (e2e)", () => {
expect(resp.body.email).toBeTruthy();
});
});

it(
"Should return a HttpStatus FORBIDDEN(403) when there is no user account associated to the user token " +
"to a default route that requires a user account.",
async () => {
await mockUserLoginInfo(moduleFixture, {
id: null,
user: { id: null, isActive: null } as User,
} as Student);
return request(app.getHttpServer())
.get("/auth-test/default-requires-user-route")
.auth(studentAccessToken, { type: "bearer" })
.expect(HttpStatus.FORBIDDEN)
.expect({
message: "No user account has been associated to the user token.",
errorType: MISSING_USER_ACCOUNT,
});
},
);

it(
"Should return a HttpStatus OK(200) when there is no user account associated to the user token " +
"to a route that does not requires a user account.",
async () => {
await mockUserLoginInfo(moduleFixture, {
id: null,
user: { id: null, isActive: null } as User,
} as Student);
return request(app.getHttpServer())
.get("/auth-test/user-not-required-route")
.auth(studentAccessToken, { type: "bearer" })
.expect(HttpStatus.OK);
},
);
});

afterAll(async () => {
Expand Down
5 changes: 5 additions & 0 deletions sources/packages/backend/apps/api/src/auth/auth.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
RequiresStudentAccountGuard,
InstitutionBCPublicGuard,
InstitutionStudentDataAccessGuard,
RequiresUserAccountGuard,
} from "./guards";
import { RolesGuard } from "./guards/roles.guard";
import { ConfigModule } from "@sims/utilities/config";
Expand Down Expand Up @@ -99,6 +100,10 @@ const jwtModule = JwtModule.register({
provide: APP_GUARD,
useClass: InstitutionStudentDataAccessGuard,
},
{
provide: APP_GUARD,
useClass: RequiresUserAccountGuard,
},
],
exports: [jwtModule, KeycloakService],
})
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { Reflector } from "@nestjs/core";
/**
* Specifies when a user account must be already created in order to access a route.
*/
export const RequiresUserAccount = Reflector.createDecorator<boolean>();
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ export * from "./allow-inactive-user.decorator";
export * from "./groups.decorator";
export * from "./student/check-sin-status.decorator";
export * from "./student/requires-student-account.decorator";
export * from "./common.decorator";
23 changes: 21 additions & 2 deletions sources/packages/backend/apps/api/src/auth/guards/groups.guard.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
import { Injectable, CanActivate, ExecutionContext } from "@nestjs/common";
import {
Injectable,
CanActivate,
ExecutionContext,
ForbiddenException,
} from "@nestjs/common";
import { Reflector } from "@nestjs/core";
import { GROUPS_KEY } from "../decorators";
import { AuthorizedParties, UserGroups, IUserToken } from "..";
import { MISSING_GROUP_ACCESS } from "../../constants";
import { ApiProcessError } from "../../types";

/**
* Allow the authorization based on groups making use
Expand Down Expand Up @@ -34,8 +41,20 @@ export class GroupsGuard implements CanActivate {
// Check if the user has any of the groups required to have access to the resource.
// UserGroups 'aest/user/x' and 'aest/user' are valid, as they start with 'aest/user'
// we are here looking for matches, not the exact string.
return requiredGroups.some((group: string) =>
const hasGroupAccess = requiredGroups.some((group: string) =>
userToken.groups?.some((userGroup) => userGroup.startsWith(group)),
);

// Throw custom error to be handled by the consumer.
if (!hasGroupAccess) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

throw new ForbiddenException(
new ApiProcessError(
"Required group permission is missing for the user.",
MISSING_GROUP_ACCESS,
),
);
}

return true;
}
}
1 change: 1 addition & 0 deletions sources/packages/backend/apps/api/src/auth/guards/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ export * from "./active-user.guard";
export * from "./groups.guard";
export * from "./student/sin-validation.guard";
export * from "./student/requires-student-account.guard";
export * from "./requires-user-account.guard";
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import {
Injectable,
CanActivate,
ExecutionContext,
ForbiddenException,
} from "@nestjs/common";
import { Reflector } from "@nestjs/core";
import { IS_PUBLIC_KEY, RequiresUserAccount } from "../decorators";
import { IUserToken } from "apps/api/src/auth/userToken.interface";
import { MISSING_USER_ACCOUNT } from "../../constants";
import { ApiProcessError } from "../../types";

/**
* Validates that a user account must be already created in order to access a route.
* Public routes and routes that do not require a user account are skipped.
*/
@Injectable()
export class RequiresUserAccountGuard implements CanActivate {
constructor(private readonly reflector: Reflector) {}

canActivate(context: ExecutionContext): boolean {
// Check if the route is public.
const isPublic = this.reflector.getAllAndOverride<boolean>(IS_PUBLIC_KEY, [
context.getHandler(),
context.getClass(),
]);
// If the route is public, no validation is required.
if (isPublic) {
return true;
}

const requiresUserAccount = this.reflector.getAllAndOverride(
RequiresUserAccount,
[context.getHandler(), context.getClass()],
);

if (requiresUserAccount === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return true;
}

const { user } = context.switchToHttp().getRequest();
const userToken = user as IUserToken;

if (!userToken?.userId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

throw new ForbiddenException(
new ApiProcessError(
"No user account has been associated to the user token.",
MISSING_USER_ACCOUNT,
),
);
}

return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ export const OFFERING_START_DATE_ERROR = "OFFERING_START_DATE_ERROR";
export const INVALID_STUDY_DATES = "INVALID_STUDY_DATES";
export const OFFERING_INTENSITY_MISMATCH = "OFFERING_INTENSITY_MISMATCH";
export const MISSING_STUDENT_ACCOUNT = "MISSING_STUDENT_ACCOUNT";
export const MISSING_USER_ACCOUNT = "MISSING_USER_ACCOUNT";
export const MISSING_GROUP_ACCESS = "MISSING_GROUP_ACCESS";
export const MISSING_USER_INFO = "MISSING_USER_INFO";
export const INVALID_APPLICATION_NUMBER = "INVALID_APPLICATION_NUMBER";
/**
* An application can have only one pending appeal at a time.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe("AssessmentStudentsController(e2e)-getAssessmentAwardDetails", () => {
db = createE2EDataSources(dataSource);
// Shared student to be used for tests that would not need further student customization.
sharedStudent = await saveFakeStudent(db.dataSource);
mockUserLoginInfo(module, sharedStudent);
await mockUserLoginInfo(module, sharedStudent);
// Valid MSFAA Number that will be part of the expected returned values.
sharedMSFAANumber = await db.msfaaNumber.save(
createFakeMSFAANumber(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { Controller, Post, Req, Body } from "@nestjs/common";
import { ApiTags } from "@nestjs/swagger";
import { AuthorizedParties, IUserToken } from "../../auth";
import { AllowAuthorizedParty, UserToken } from "../../auth/decorators";
import {
AllowAuthorizedParty,
RequiresUserAccount,
UserToken,
} from "../../auth/decorators";
import BaseController from "../BaseController";
import { AuditService } from "../../services";
import { Request } from "express";
Expand All @@ -13,6 +17,7 @@ import { AuditAPIInDTO } from "./models/audit.dto";
AuthorizedParties.supportingUsers,
AuthorizedParties.aest,
)
@RequiresUserAccount(false)
@Controller("audit")
@ApiTags("audit")
export class AuditController extends BaseController {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { Controller, Get, Param } from "@nestjs/common";
import BaseController from "../BaseController";
import { FormService } from "../../services";
import { AllowAuthorizedParty } from "../../auth/decorators";
import {
AllowAuthorizedParty,
RequiresUserAccount,
} from "../../auth/decorators";
import { AuthorizedParties } from "../../auth/authorized-parties.enum";
import { ApiTags } from "@nestjs/swagger";
import { FormNameParamAPIInDTO } from "./models/form.dto";
Expand All @@ -12,6 +15,7 @@ import { FormNameParamAPIInDTO } from "./models/form.dto";
AuthorizedParties.supportingUsers,
AuthorizedParties.aest,
)
@RequiresUserAccount(false)
@Controller("dynamic-form")
@ApiTags("dynamic-form")
export class DynamicFormController extends BaseController {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
AllowAuthorizedParty,
AllowInactiveUser,
IsInstitutionAdmin,
RequiresUserAccount,
UserToken,
} from "../../auth/decorators";
import { InstitutionService, UserService, BCeIDService } from "../../services";
Expand Down Expand Up @@ -197,6 +198,7 @@ export class InstitutionUserInstitutionsController extends BaseController {
* @returns information to support the institution login process and
* the decisions that need happen to complete the process.
*/
@RequiresUserAccount(false)
@AllowInactiveUser()
@Get("status")
async getInstitutionUserStatus(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { AuthorizedParties } from "../../auth/authorized-parties.enum";
import {
AllowAuthorizedParty,
IsInstitutionAdmin,
RequiresUserAccount,
UserToken,
} from "../../auth/decorators";
import { InstitutionService, UserService } from "../../services";
Expand Down Expand Up @@ -58,6 +59,7 @@ export class InstitutionInstitutionsController extends BaseController {
@ApiUnprocessableEntityResponse({
description: "Institution user already exist",
})
@RequiresUserAccount(false)
@Post()
async createInstitutionWithAssociatedUser(
@Body() payload: CreateInstitutionAPIInDTO,
Expand Down Expand Up @@ -139,6 +141,7 @@ export class InstitutionInstitutionsController extends BaseController {
* list (key/value pair) schema.
* @returns institutions types in an option list (key/value pair) schema.
*/
@RequiresUserAccount(false)
@Get("type/options-list")
async getInstitutionTypeOptions(): Promise<OptionItemAPIOutDTO[]> {
return this.institutionControllerService.getInstitutionTypeOptions();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { AuthorizedParties } from "../../auth/authorized-parties.enum";
import {
AllowAuthorizedParty,
RequiresStudentAccount,
RequiresUserAccount,
UserToken,
} from "../../auth/decorators";
import BaseController from "../BaseController";
Expand Down Expand Up @@ -66,6 +67,7 @@ export class StudentAccountApplicationStudentsController extends BaseController
"There is already a student account application in progress or the user is already present.",
})
@Post()
@RequiresUserAccount(false)
@RequiresStudentAccount(false)
async create(
@UserToken() userToken: IUserToken,
Expand Down Expand Up @@ -110,6 +112,7 @@ export class StudentAccountApplicationStudentsController extends BaseController
* @returns true if there is a pending student account application
* to be assessed by the Ministry, otherwise, false.
*/
@RequiresUserAccount(false)
@RequiresStudentAccount(false)
@Get("has-pending-account-application")
async hasPendingAccountApplication(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { AuthorizedParties } from "../../auth/authorized-parties.enum";
import {
AllowAuthorizedParty,
RequiresStudentAccount,
RequiresUserAccount,
UserToken,
} from "../../auth/decorators";
import {
Expand Down Expand Up @@ -108,6 +109,7 @@ export class StudentStudentsController extends BaseController {
@ApiForbiddenResponse({
description: "User is not allowed to create a student account.",
})
@RequiresUserAccount(false)
@RequiresStudentAccount(false)
async create(
@UserToken() studentUserToken: StudentUserToken,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
} from "@nestjs/swagger";
import BaseController from "../BaseController";
import { WorkflowClientService } from "@sims/services";
import { RequiresUserAccount } from "../../auth/decorators";

@AllowAuthorizedParty(AuthorizedParties.supportingUsers)
@Controller("supporting-user")
Expand All @@ -70,6 +71,7 @@ export class SupportingUserSupportingUsersController extends BaseController {
* Application.
* @returns application details.
*/
@RequiresUserAccount(false)
@Post(":supportingUserType/application")
@HttpCode(HttpStatus.OK)
@ApiUnprocessableEntityResponse({
Expand Down Expand Up @@ -130,6 +132,7 @@ export class SupportingUserSupportingUsersController extends BaseController {
* the supporting data (e.g. parent/partner).
* @param payload data used for validation and to execute the update.
*/
@RequiresUserAccount(false)
@Patch(":supportingUserType")
@ApiUnprocessableEntityResponse({
description:
Expand Down
Loading