From ec444317b3f3fc4fefab46191051c3a887b7eb21 Mon Sep 17 00:00:00 2001 From: Faruk AYDIN Date: Mon, 26 Feb 2024 01:06:54 +0100 Subject: [PATCH 1/5] feat: Catch not found error message for objection --- packages/backend/src/helpers/error-handler.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/helpers/error-handler.js b/packages/backend/src/helpers/error-handler.js index a4a738a7..07862a64 100644 --- a/packages/backend/src/helpers/error-handler.js +++ b/packages/backend/src/helpers/error-handler.js @@ -2,12 +2,12 @@ import logger from './logger.js'; // Do not remove `next` argument as the function signature will not fit for an error handler middleware // eslint-disable-next-line no-unused-vars -const errorHandler = (err, req, res, next) => { - if (err.message === 'Not Found') { - res.status(404).end(); +const errorHandler = (error, request, response, next) => { + if (error.message === 'Not Found' || error.message === 'NotFoundError') { + response.status(404).end(); } else { - logger.error(err.message + '\n' + err.stack); - res.status(err.statusCode || 500).send(err.message); + logger.error(error.message + '\n' + error.stack); + response.status(error.statusCode || 500).send(error.message); } }; From 58658c6b1a6b48a6e862bfb1a1c81571d76f6ae5 Mon Sep 17 00:00:00 2001 From: Faruk AYDIN Date: Mon, 26 Feb 2024 01:07:31 +0100 Subject: [PATCH 2/5] feat: Do not expose unknown error message to client --- packages/backend/src/helpers/error-handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/helpers/error-handler.js b/packages/backend/src/helpers/error-handler.js index 07862a64..30bebeab 100644 --- a/packages/backend/src/helpers/error-handler.js +++ b/packages/backend/src/helpers/error-handler.js @@ -7,7 +7,7 @@ const errorHandler = (error, request, response, next) => { response.status(404).end(); } else { logger.error(error.message + '\n' + error.stack); - response.status(error.statusCode || 500).send(error.message); + response.status(error.statusCode || 500); } }; From 4afe7c6b467c44468c85c5f8622220e87ee445cc Mon Sep 17 00:00:00 2001 From: Faruk AYDIN Date: Mon, 26 Feb 2024 01:26:04 +0100 Subject: [PATCH 3/5] feat: Handle bad request for invalid UUID --- packages/backend/src/helpers/error-handler.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/helpers/error-handler.js b/packages/backend/src/helpers/error-handler.js index 30bebeab..49a74956 100644 --- a/packages/backend/src/helpers/error-handler.js +++ b/packages/backend/src/helpers/error-handler.js @@ -1,14 +1,19 @@ import logger from './logger.js'; +import { NotFoundError, DataError } from 'objection'; // Do not remove `next` argument as the function signature will not fit for an error handler middleware // eslint-disable-next-line no-unused-vars const errorHandler = (error, request, response, next) => { - if (error.message === 'Not Found' || error.message === 'NotFoundError') { + if (error.message === 'Not Found' || error instanceof NotFoundError) { response.status(404).end(); - } else { - logger.error(error.message + '\n' + error.stack); - response.status(error.statusCode || 500); } + + if (error instanceof DataError) { + response.status(400).end(); + } + + logger.error(error.message + '\n' + error.stack); + response.status(error.statusCode || 500); }; export default errorHandler; From 404ea94dd25c636b8ec3eec9bc092b4264fcaf16 Mon Sep 17 00:00:00 2001 From: Faruk AYDIN Date: Mon, 26 Feb 2024 01:40:20 +0100 Subject: [PATCH 4/5] test: Cover not found responses for API endpoint tests --- .../get-app-auth-client.test.js | 10 ++++++++++ .../api/v1/admin/roles/get-role.ee.test.js | 14 +++++++++++++- .../get-saml-auth-provider.ee.test.js | 12 ++++++++++++ .../api/v1/admin/users/get-user.ee.test.js | 12 ++++++++++++ .../get-app-auth-client.test.js | 10 ++++++++++ .../controllers/api/v1/flows/get-flow.test.js | 17 +++++++++++++++++ packages/backend/src/helpers/error-handler.js | 3 ++- 7 files changed, 76 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js b/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js index 191aafe0..51fa840b 100644 --- a/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js +++ b/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js @@ -1,5 +1,6 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import request from 'supertest'; +import Crypto from 'crypto'; import app from '../../../../../app.js'; import createAuthTokenByUserId from '../../../../../helpers/create-auth-token-by-user-id.js'; import { createUser } from '../../../../../../test/factories/user.js'; @@ -31,5 +32,14 @@ describe('GET /api/v1/admin/app-auth-clients/:appAuthClientId', () => { const expectedPayload = getAdminAppAuthClientMock(currentAppAuthClient); expect(response.body).toEqual(expectedPayload); }); + + it('should return not found response for not existing app auth client ID', async () => { + const invalidAppAuthClientId = Crypto.randomUUID(); + + await request(app) + .get(`/api/v1/admin/app-auth-clients/${invalidAppAuthClientId}`) + .set('Authorization', token) + .expect(404); + }); }); }); diff --git a/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js b/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js index 9a4dca01..d083b171 100644 --- a/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js @@ -1,5 +1,6 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import request from 'supertest'; +import Crypto from 'crypto'; import app from '../../../../../app.js'; import createAuthTokenByUserId from '../../../../../helpers/create-auth-token-by-user-id.js'; import { createRole } from '../../../../../../test/factories/role.js'; @@ -20,7 +21,7 @@ describe('GET /api/v1/admin/roles/:roleId', () => { token = createAuthTokenByUserId(currentUser.id); }); - it('should return roles', async () => { + it('should return role', async () => { vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); const response = await request(app) @@ -35,4 +36,15 @@ describe('GET /api/v1/admin/roles/:roleId', () => { expect(response.body).toEqual(expectedPayload); }); + + it('should return not found response for not existing role ID', async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + const invalidRoleId = Crypto.randomUUID(); + + await request(app) + .get(`/api/v1/admin/roles/${invalidRoleId}`) + .set('Authorization', token) + .expect(404); + }); }); diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js index c1cc2aca..d78efc43 100644 --- a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js @@ -1,5 +1,6 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import request from 'supertest'; +import Crypto from 'crypto'; import app from '../../../../../app.js'; import createAuthTokenByUserId from '../../../../../helpers/create-auth-token-by-user-id.js'; import { createRole } from '../../../../../../test/factories/role.js'; @@ -31,4 +32,15 @@ describe('GET /api/v1/admin/saml-auth-provider/:samlAuthProviderId', () => { expect(response.body).toEqual(expectedPayload); }); + + it('should return not found response for not existing saml auth provider ID', async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + const invalidSamlAuthProviderId = Crypto.randomUUID(); + + await request(app) + .get(`/api/v1/admin/saml-auth-providers/${invalidSamlAuthProviderId}`) + .set('Authorization', token) + .expect(404); + }); }); diff --git a/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js b/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js index 918a9499..c9696cf2 100644 --- a/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js @@ -1,5 +1,6 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import request from 'supertest'; +import Crypto from 'crypto'; import app from '../../../../../app.js'; import createAuthTokenByUserId from '../../../../../helpers/create-auth-token-by-user-id'; import { createUser } from '../../../../../../test/factories/user'; @@ -31,4 +32,15 @@ describe('GET /api/v1/admin/users/:userId', () => { const expectedPayload = getUserMock(anotherUser, anotherUserRole); expect(response.body).toEqual(expectedPayload); }); + + it('should return not found response for not existing user ID', async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + const invalidUserId = Crypto.randomUUID(); + + await request(app) + .get(`/api/v1/admin/users/${invalidUserId}`) + .set('Authorization', token) + .expect(404); + }); }); diff --git a/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js b/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js index 2fb90ce0..4894b01f 100644 --- a/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js +++ b/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js @@ -1,5 +1,6 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import request from 'supertest'; +import Crypto from 'crypto'; import app from '../../../../app.js'; import createAuthTokenByUserId from '../../../../helpers/create-auth-token-by-user-id.js'; import { createUser } from '../../../../../test/factories/user.js'; @@ -28,4 +29,13 @@ describe('GET /api/v1/app-auth-clients/:id', () => { const expectedPayload = getAppAuthClientMock(currentAppAuthClient); expect(response.body).toEqual(expectedPayload); }); + + it('should return not found response for not existing app auth client ID', async () => { + const invalidAppAuthClientId = Crypto.randomUUID(); + + await request(app) + .get(`/api/v1/app-auth-clients/${invalidAppAuthClientId}`) + .set('Authorization', token) + .expect(404); + }); }); diff --git a/packages/backend/src/controllers/api/v1/flows/get-flow.test.js b/packages/backend/src/controllers/api/v1/flows/get-flow.test.js index 91bafffb..6bfc46d0 100644 --- a/packages/backend/src/controllers/api/v1/flows/get-flow.test.js +++ b/packages/backend/src/controllers/api/v1/flows/get-flow.test.js @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach } from 'vitest'; import request from 'supertest'; +import Crypto from 'crypto'; import app from '../../../../app.js'; import createAuthTokenByUserId from '../../../../helpers/create-auth-token-by-user-id'; import { createUser } from '../../../../../test/factories/user'; @@ -68,4 +69,20 @@ describe('GET /api/v1/flows/:flowId', () => { expect(response.body).toEqual(expectedPayload); }); + + it('should return not found response for not existing flow id', async () => { + await createPermission({ + action: 'read', + subject: 'Flow', + roleId: currentUserRole.id, + conditions: [], + }); + + const invalidFlowId = Crypto.randomUUID(); + + await request(app) + .get(`/api/v1/flows/${invalidFlowId}`) + .set('Authorization', token) + .expect(404); + }); }); diff --git a/packages/backend/src/helpers/error-handler.js b/packages/backend/src/helpers/error-handler.js index 49a74956..e9131370 100644 --- a/packages/backend/src/helpers/error-handler.js +++ b/packages/backend/src/helpers/error-handler.js @@ -1,5 +1,6 @@ import logger from './logger.js'; -import { NotFoundError, DataError } from 'objection'; +import objection from 'objection'; +const { NotFoundError, DataError } = objection; // Do not remove `next` argument as the function signature will not fit for an error handler middleware // eslint-disable-next-line no-unused-vars From d74b215169e5176634c3bb3d35041ce53aa46b6b Mon Sep 17 00:00:00 2001 From: Faruk AYDIN Date: Mon, 26 Feb 2024 13:30:30 +0100 Subject: [PATCH 5/5] test: Cover bad request responses for API endpoint tests --- .../get-app-auth-client.test.js | 13 +++++++++--- .../api/v1/admin/roles/get-role.ee.test.js | 15 +++++++++++--- .../get-saml-auth-provider.ee.test.js | 17 +++++++++++++--- .../api/v1/admin/users/get-user.ee.test.js | 15 +++++++++++--- .../get-app-auth-client.test.js | 11 ++++++++-- .../controllers/api/v1/flows/get-flow.test.js | 20 ++++++++++++++++--- 6 files changed, 74 insertions(+), 17 deletions(-) diff --git a/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js b/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js index 51fa840b..fcd6d842 100644 --- a/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js +++ b/packages/backend/src/controllers/api/v1/admin/app-auth-clients/get-app-auth-client.test.js @@ -33,13 +33,20 @@ describe('GET /api/v1/admin/app-auth-clients/:appAuthClientId', () => { expect(response.body).toEqual(expectedPayload); }); - it('should return not found response for not existing app auth client ID', async () => { - const invalidAppAuthClientId = Crypto.randomUUID(); + it('should return not found response for not existing app auth client UUID', async () => { + const notExistingAppAuthClientUUID = Crypto.randomUUID(); await request(app) - .get(`/api/v1/admin/app-auth-clients/${invalidAppAuthClientId}`) + .get(`/api/v1/admin/app-auth-clients/${notExistingAppAuthClientUUID}`) .set('Authorization', token) .expect(404); }); + + it('should return bad request response for invalid UUID', async () => { + await request(app) + .get('/api/v1/admin/app-auth-clients/invalidAppAuthClientUUID') + .set('Authorization', token) + .expect(400); + }); }); }); diff --git a/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js b/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js index d083b171..bf2b7451 100644 --- a/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/roles/get-role.ee.test.js @@ -37,14 +37,23 @@ describe('GET /api/v1/admin/roles/:roleId', () => { expect(response.body).toEqual(expectedPayload); }); - it('should return not found response for not existing role ID', async () => { + it('should return not found response for not existing role UUID', async () => { vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); - const invalidRoleId = Crypto.randomUUID(); + const notExistingRoleUUID = Crypto.randomUUID(); await request(app) - .get(`/api/v1/admin/roles/${invalidRoleId}`) + .get(`/api/v1/admin/roles/${notExistingRoleUUID}`) .set('Authorization', token) .expect(404); }); + + it('should return bad request response for invalid UUID', async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + await request(app) + .get('/api/v1/admin/roles/invalidRoleUUID') + .set('Authorization', token) + .expect(400); + }); }); diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js index d78efc43..1f0b63e1 100644 --- a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.test.js @@ -33,14 +33,25 @@ describe('GET /api/v1/admin/saml-auth-provider/:samlAuthProviderId', () => { expect(response.body).toEqual(expectedPayload); }); - it('should return not found response for not existing saml auth provider ID', async () => { + it('should return not found response for not existing saml auth provider UUID', async () => { vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); - const invalidSamlAuthProviderId = Crypto.randomUUID(); + const notExistingSamlAuthProviderUUID = Crypto.randomUUID(); await request(app) - .get(`/api/v1/admin/saml-auth-providers/${invalidSamlAuthProviderId}`) + .get( + `/api/v1/admin/saml-auth-providers/${notExistingSamlAuthProviderUUID}` + ) .set('Authorization', token) .expect(404); }); + + it('should return bad request response for invalid UUID', async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + await request(app) + .get('/api/v1/admin/saml-auth-providers/invalidSamlAuthProviderUUID') + .set('Authorization', token) + .expect(400); + }); }); diff --git a/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js b/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js index c9696cf2..6a3976b9 100644 --- a/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/users/get-user.ee.test.js @@ -33,14 +33,23 @@ describe('GET /api/v1/admin/users/:userId', () => { expect(response.body).toEqual(expectedPayload); }); - it('should return not found response for not existing user ID', async () => { + it('should return not found response for not existing user UUID', async () => { vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); - const invalidUserId = Crypto.randomUUID(); + const notExistingUserUUID = Crypto.randomUUID(); await request(app) - .get(`/api/v1/admin/users/${invalidUserId}`) + .get(`/api/v1/admin/users/${notExistingUserUUID}`) .set('Authorization', token) .expect(404); }); + + it('should return bad request response for invalid UUID', async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + await request(app) + .get('/api/v1/admin/users/invalidUserUUID') + .set('Authorization', token) + .expect(400); + }); }); diff --git a/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js b/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js index 4894b01f..6fcd494f 100644 --- a/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js +++ b/packages/backend/src/controllers/api/v1/app-auth-clients/get-app-auth-client.test.js @@ -31,11 +31,18 @@ describe('GET /api/v1/app-auth-clients/:id', () => { }); it('should return not found response for not existing app auth client ID', async () => { - const invalidAppAuthClientId = Crypto.randomUUID(); + const notExistingAppAuthClientUUID = Crypto.randomUUID(); await request(app) - .get(`/api/v1/app-auth-clients/${invalidAppAuthClientId}`) + .get(`/api/v1/app-auth-clients/${notExistingAppAuthClientUUID}`) .set('Authorization', token) .expect(404); }); + + it('should return bad request response for invalid UUID', async () => { + await request(app) + .get('/api/v1/app-auth-clients/invalidAppAuthClientUUID') + .set('Authorization', token) + .expect(400); + }); }); diff --git a/packages/backend/src/controllers/api/v1/flows/get-flow.test.js b/packages/backend/src/controllers/api/v1/flows/get-flow.test.js index 6bfc46d0..a3746d48 100644 --- a/packages/backend/src/controllers/api/v1/flows/get-flow.test.js +++ b/packages/backend/src/controllers/api/v1/flows/get-flow.test.js @@ -70,7 +70,7 @@ describe('GET /api/v1/flows/:flowId', () => { expect(response.body).toEqual(expectedPayload); }); - it('should return not found response for not existing flow id', async () => { + it('should return not found response for not existing flow UUID', async () => { await createPermission({ action: 'read', subject: 'Flow', @@ -78,11 +78,25 @@ describe('GET /api/v1/flows/:flowId', () => { conditions: [], }); - const invalidFlowId = Crypto.randomUUID(); + const notExistingFlowUUID = Crypto.randomUUID(); await request(app) - .get(`/api/v1/flows/${invalidFlowId}`) + .get(`/api/v1/flows/${notExistingFlowUUID}`) .set('Authorization', token) .expect(404); }); + + it('should return bad request response for invalid UUID', async () => { + await createPermission({ + action: 'read', + subject: 'Flow', + roleId: currentUserRole.id, + conditions: [], + }); + + await request(app) + .get('/api/v1/flows/invalidFlowUUID') + .set('Authorization', token) + .expect(400); + }); });