#3881 - Validate user account for all routes #3985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sources/packages/backend/apps/api/src/auth/guards/requires-user-account.guard.ts
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/testHelpers/auth/student-user-helpers.ts
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * Test route which requires a user to be present as default authorization. |
There was a problem hiding this comment.
Please review the comment.
| return request(app.getHttpServer()) | ||
| .get("/auth-test/default-requires-user-route") | ||
| .auth(studentAccessToken, { type: "bearer" }) | ||
| .expect(HttpStatus.FORBIDDEN); |
There was a problem hiding this comment.
Even not being consistent in this file we have been validating the full error including the message in the expect.
If possible, I would recommend adding it to these new methods.
sources/packages/backend/apps/api/src/route-controllers/user/user.aest.controller.ts
Show resolved
Hide resolved
| const axiosError = error as AxiosError; | ||
| if (axiosError.response?.status === StatusCodes.FORBIDDEN) { | ||
| if ( | ||
| error instanceof AxiosError && |
There was a problem hiding this comment.
Is it not possible to check using if (error instanceof ApiProcessError)?
|
E2E Workflow Workers Coverage Report
|
andrewsignori-aot
approved these changes
Nov 23, 2024
Backend Unit Tests Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
guru-aot
reviewed
Nov 25, 2024
| userToken.groups?.some((userGroup) => userGroup.startsWith(group)), | ||
| ); | ||
|
|
||
| // Throw custom error to be handled by the consumer. | ||
| if (!hasGroupAccess) { |
guru-aot
reviewed
Nov 25, 2024
| const { user } = context.switchToHttp().getRequest(); | ||
| const userToken = user as IUserToken; | ||
|
|
||
| if (!userToken?.userId) { |
guru-aot
reviewed
Nov 25, 2024
| [context.getHandler(), context.getClass()], | ||
| ); | ||
|
|
||
| if (requiresUserAccount === false) { |
guru-aot
reviewed
Nov 25, 2024
| * This could used to reset the mock after each test. | ||
| * @param testingModule testing module. | ||
| */ | ||
| export async function resetMockUserLoginInfo(testingModule: TestingModule) { |
guru-aot
approved these changes
Nov 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Validate user account for all routes
New global guard and decorator
RequiresUserAccountGuardhas been introduced globally to ensure that routes are authorized with the user token which belongs to valid SIMS user. There are exceptional routes like public routes and routes used that setup the user itself are skipped from this validation.@RequiresUserAccount()is introduced to get the metadata context for the guard.Student page container
E2E Tests
mockUserLoginInfo()does not have a way to restore the mock, if the mock needs to be restored for other tests in same suite.Hence refactored the code to use
jest.spyOn()to mock the userService method implementation and also created a reset mock method to reset the mock as required in the test suite.Here is an example.
Mock applied
Mock Reset
Volar extension