From 82a7c48af1d2dada287b15f61cd037179aaf0b9b Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Wed, 5 Apr 2023 09:51:31 -0700 Subject: [PATCH 1/5] Version bump helm chart to 0.0.12 Publishes changes from #149 Signed-off-by: Jeremy Ho --- charts/coms/Chart.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/coms/Chart.yaml b/charts/coms/Chart.yaml index b65dc3a4..8155a355 100644 --- a/charts/coms/Chart.yaml +++ b/charts/coms/Chart.yaml @@ -3,7 +3,7 @@ name: common-object-management-service # This is the chart version. This version number should be incremented each time you make changes # to the chart and its templates, including the app version. # Versions are expected to follow Semantic Versioning (https://semver.org/) -version: 0.0.11 +version: 0.0.12 kubeVersion: ">= 1.13.0" description: A microservice for managing access control to S3 Objects # A chart can be either an 'application' or a 'library' chart. From 4dac9d4088141f7d580520e355780621acf65ba8 Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Tue, 4 Apr 2023 15:04:29 -0700 Subject: [PATCH 2/5] Add handler for errors with defined statusCode attributes This will end up allowing situations where the resource is not found to be returned as a 404 instead of current 502 behavior. Signed-off-by: Jeremy Ho Temp Signed-off-by: Jeremy Ho --- app/src/components/errorToProblem.js | 12 ++++- .../unit/components/errorToProblem.spec.js | 49 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/app/src/components/errorToProblem.js b/app/src/components/errorToProblem.js index 41f7583c..f9a60402 100644 --- a/app/src/components/errorToProblem.js +++ b/app/src/components/errorToProblem.js @@ -2,6 +2,13 @@ const Problem = require('api-problem'); const log = require('./log')(module.filename); +/** + * @function errorToProblem + * Attempts to interpret and infer the type of Problem to respond with + * @param {string} service A string representing which service the error occured at + * @param {Error} e The raw error exception object + * @returns {Problem} A problem error type + */ function errorToProblem(service, e) { // If already problem type, just return as is if (e instanceof Problem) { @@ -24,7 +31,10 @@ function errorToProblem(service, e) { }); } // Something else happened but there's a response - return new Problem(e.response.status, { detail: e.response.data.toString() }); + return new Problem(e.response.status, { detail: e.response.data }); + } else if (e.statusCode) { + // Handle errors with Status Codes + return new Problem(e.statusCode, { detail: e.message }); } else if (e.$metadata && e.$metadata.httpStatusCode) { // Handle S3 promise rejections if (e.$response && e.$response.body) delete e.$response.body; diff --git a/app/tests/unit/components/errorToProblem.spec.js b/app/tests/unit/components/errorToProblem.spec.js index ccbdf629..ad418e9b 100644 --- a/app/tests/unit/components/errorToProblem.spec.js +++ b/app/tests/unit/components/errorToProblem.spec.js @@ -35,6 +35,55 @@ describe('errorToProblem', () => { expect(result.errors).toBeUndefined(); }); + it('should return a 409 problem given an error', () => { + const e = { + response: { + data: { detail: 'detail' }, + status: 409 + } + }; + const result = errorToProblem(SERVICE, e); + + expect(result).toBeTruthy(); + expect(result instanceof Problem).toBeTruthy(); + expect(result.title).toMatch('Conflict'); + expect(result.status).toBe(409); + expect(result.detail).toEqual(expect.objectContaining(e.response.data)); + expect(result.errors).toBeUndefined(); + }); + + it('should return a problem given an error with statusCode', () => { + const e = { + statusCode: 404, + message: 'NotFoundError' + }; + const result = errorToProblem(SERVICE, e); + + expect(result).toBeTruthy(); + expect(result instanceof Problem).toBeTruthy(); + expect(result.title).toMatch('Not Found'); + expect(result.status).toBe(404); + expect(result.detail).toMatch(e.message); + expect(result.errors).toBeUndefined(); + }); + + it('should return a problem given an error with s3 metadata', () => { + const e = { + $metadata: { + httpStatusCode: 404, + }, + message: 'NotFoundError' + }; + const result = errorToProblem(SERVICE, e); + + expect(result).toBeTruthy(); + expect(result instanceof Problem).toBeTruthy(); + expect(result.title).toMatch('Not Found'); + expect(result.status).toBe(404); + expect(result.detail).toEqual(expect.objectContaining({ message: e.message })); + expect(result.errors).toBeUndefined(); + }); + it('should return a 422 problem with a supplied string response', () => { const e = { response: { From 1920949fbfcf660e65391b3952219c41a5304475 Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Tue, 4 Apr 2023 15:31:46 -0700 Subject: [PATCH 3/5] Modify _validateCredentials function signature to only throw the problem By simplifying this function to behave more like a predicate, we can use the existing errorToProblem plumbing to bubble up the error response instead of short circuiting the response call chain. Signed-off-by: Jeremy Ho --- app/src/controllers/bucket.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/app/src/controllers/bucket.js b/app/src/controllers/bucket.js index 095baa4a..39122c7b 100644 --- a/app/src/controllers/bucket.js +++ b/app/src/controllers/bucket.js @@ -68,17 +68,18 @@ const controller = { /** * @function _validateCredentials * Guard against creating or update a bucket with invalid creds - * @param {object} requestBody The body of the request + * @param {object} credentials The body of the request + * @throws A conflict error problem if the bucket is not reachable */ - async _validateCredentials(requestBody, res) { + async _validateCredentials(credentials) { try { const bucketSettings = { - accessKeyId: requestBody.accessKeyId, - bucket: requestBody.bucket, - endpoint: requestBody.endpoint, - key: requestBody.key, - region: requestBody.region || DEFAULTREGION, - secretAccessKey: requestBody.secretAccessKey, + accessKeyId: credentials.accessKeyId, + bucket: credentials.bucket, + endpoint: credentials.endpoint, + key: credentials.key, + region: credentials.region || DEFAULTREGION, + secretAccessKey: credentials.secretAccessKey, }; await storageService.headBucket(bucketSettings); } catch (e) { @@ -87,9 +88,8 @@ const controller = { function: '_validateCredentials', }); throw new Problem(409, { - details: - 'Unable to validate supplied key/password for the supplied object store or bucket', - }).send(res); + details: 'Unable to validate supplied credentials for the bucket', + }); } }, From bc388ce4c4440aaf445b0b6c7731e09782a13e39 Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Tue, 4 Apr 2023 16:12:51 -0700 Subject: [PATCH 4/5] Re-nest _validateCredentials to use the try/catch blocks in createObject This allows us to more reliably report other error situations without using a direct escape hatch to the express response object. Signed-off-by: Jeremy Ho --- app/src/controllers/bucket.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/src/controllers/bucket.js b/app/src/controllers/bucket.js index 39122c7b..53304a05 100644 --- a/app/src/controllers/bucket.js +++ b/app/src/controllers/bucket.js @@ -105,10 +105,9 @@ const controller = { const data = { ...req.body }; let response = undefined; - // Check for credential accessibility/validity first - await controller._validateCredentials(data, res); - try { + // Check for credential accessibility/validity first + await controller._validateCredentials(data); data.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER)); response = await bucketService.create(data); From fcd82330bd7f8a446d958e970881425ddefc0d6a Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Tue, 4 Apr 2023 16:13:51 -0700 Subject: [PATCH 5/5] Implement object merge logic for updateBucket By mixing in both the existing data as well as the incoming patch data, we can check if the new combination still yields a valid and reachable bucket. Signed-off-by: Jeremy Ho --- app/src/controllers/bucket.js | 12 ++++++++---- app/tests/unit/controllers/bucket.spec.js | 10 ++++++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/app/src/controllers/bucket.js b/app/src/controllers/bucket.js index 53304a05..c0a71056 100644 --- a/app/src/controllers/bucket.js +++ b/app/src/controllers/bucket.js @@ -215,13 +215,17 @@ const controller = { * @returns {function} Express middleware function */ async updateBucket(req, res, next) { - // Check for credential accessibility/validity first - await controller._validateCredentials(req.body, res); - try { + const bucketId = addDashesToUuid(req.params.bucketId); + const currentBucket = await bucketService.read(bucketId); + + // Check for credential accessibility/validity first + // Need to cross reference with existing data when partial patch data is provided + await controller._validateCredentials({ ...currentBucket, ...req.body }); + const userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER), SYSTEM_USER); const response = await bucketService.update({ - bucketId: req.params.bucketId, + bucketId: bucketId, bucketName: req.body.bucketName, accessKeyId: req.body.accessKeyId, bucket: req.body.bucket, diff --git a/app/tests/unit/controllers/bucket.spec.js b/app/tests/unit/controllers/bucket.spec.js index ba2c4bc5..eab748c7 100644 --- a/app/tests/unit/controllers/bucket.spec.js +++ b/app/tests/unit/controllers/bucket.spec.js @@ -107,7 +107,7 @@ describe('createBucket', () => { ); }); - it('excepts if invalid bucket head', async () => { + it('responds with error when invalid bucket head', async () => { // request object const req = { body: { bucket: REQUEST_BUCKET, region: 'test' }, @@ -126,13 +126,15 @@ describe('createBucket', () => { throw new Error(); }); - await expect(controller.createBucket(req, res, next)).rejects.toThrow(); + await controller.createBucket(req, res, next); expect(headBucketSpy).toHaveBeenCalledTimes(1); expect(headBucketSpy).toHaveBeenCalledWith(req.body); expect(getCurrentIdentitySpy).toHaveBeenCalledTimes(0); expect(getCurrentUserIdSpy).toHaveBeenCalledTimes(0); expect(createSpy).toHaveBeenCalledTimes(0); + expect(next).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledWith(expect.any(Problem)); }); it('nexts an error if the bucket service fails to create', async () => { @@ -167,7 +169,7 @@ describe('createBucket', () => { expect(getCurrentUserIdSpy).toHaveBeenCalledWith(USR_IDENTITY); expect(createSpy).toHaveBeenCalledTimes(1); expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID }); - + expect(next).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledWith( new Problem(502, 'Unknown BucketService Error') @@ -213,7 +215,7 @@ describe('createBucket', () => { expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID }); expect(checkGrantPermissionsSpy).toHaveBeenCalledTimes(1); expect(checkGrantPermissionsSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID }); - + expect(res.status).toHaveBeenCalledWith(201); }); });