Merge pull request #150 from bcgov/bugfix/updateBucketDiff

Allow updateBucket to handle differential patch value checking

Author: kamorel

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;

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',
+      });
     }
   },
 
@@ -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);
@@ -216,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,

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: {

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);
   });
 });

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.