Merge pull request #159 from Kashoo/issue156_unauthorized_delete

Issue #156 unauthorized delete
OldSneerJaw · Jan 2, 2018 · 984e0ce · 984e0ce
2 parents d3df4b8 + a097137
commit 984e0ce
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased]
 
 ### Changed
+- [#156](https://github.com/Kashoo/synctos/issues/156): Fix for an edge case where users assigned a replace role may gain the privilege of removing a document erroneously under certain document definition conditions
 - [#157](https://github.com/Kashoo/synctos/issues/157): Swap in Chai as the assertion library used in specs throughout the project
 
 ## [1.9.3] - 2017-10-23

diff --git a/etc/sync-function-authorization-module.js b/etc/sync-function-authorization-module.js
@@ -57,9 +57,11 @@ function() {
       appendToAuthorizationList(requiredAuthorizations, authorizationMap.write);
     }
 
-    if (doc._deleted && authorizationMap.remove) {
-      writeAuthorizationFound = true;
-      appendToAuthorizationList(requiredAuthorizations, authorizationMap.remove);
+    if (doc._deleted) {
+      if (authorizationMap.remove) {
+        writeAuthorizationFound = true;
+        appendToAuthorizationList(requiredAuthorizations, authorizationMap.remove);
+      }
     } else if (!isDocumentMissingOrDeleted(oldDoc) && authorizationMap.replace) {
       writeAuthorizationFound = true;
       appendToAuthorizationList(requiredAuthorizations, authorizationMap.replace);

diff --git a/etc/test-helper.js b/etc/test-helper.js
@@ -645,11 +645,11 @@ function verifyAccessDenied(doc, oldDoc, expectedAuthorization) {
     } else if (countAuthorizationTypes(expectedAuthorization) > 1) {
       assert.equal(ex.forbidden, generalAuthFailedMessage, 'Expected authorization exception not met: ' + ex.forbidden);
     } else if (expectedAuthorization.expectedChannels) {
-      assert.ok(ex instanceof Error && ex.message === channelAccessDenied.message, 'Expected channel authorization error not triggered');
+      assert.equal(ex, channelAccessDenied, 'Expected channel authorization error not triggered, got this instead: ' + ex);
     } else if (expectedAuthorization.expectedRoles) {
-      assert.ok(ex instanceof Error && ex.message === roleAccessDenied.message, 'Expected role authorization error not triggered');
+      assert.equal(ex, roleAccessDenied, 'Expected role authorization error not triggered, got this instead: ' + ex);
     } else if (expectedAuthorization.expectedUsers) {
-      assert.ok(ex instanceof Error && ex.message === userAccessDenied.message, 'Expected user authorization error not triggered');
+      assert.equal(ex, userAccessDenied, 'Expected user authorization error not triggered, got this instead: ' + ex);
     }
   }
 

diff --git a/test/authorization-spec.js b/test/authorization-spec.js
@@ -232,4 +232,78 @@ describe('Authorization:', function() {
       testHelper.verifyAccessDenied(doc, oldDoc, { });
     });
   });
+
+  describe('for a document with statically-assigned replace role and nothing else', function() {
+    it('rejects document creation', function() {
+      var doc = {
+        _id: 'replaceOnlyRoleDoc',
+        stringProp: 'foobar'
+      };
+
+      testHelper.verifyAccessDenied(doc, null, []);
+    });
+
+    it('allows document replacement', function() {
+      var doc = {
+        _id: 'replaceOnlyRoleDoc',
+        stringProp: 'foobar'
+      };
+      var oldDoc = {
+        _id: 'replaceOnlyRoleDoc',
+        stringProp: 'barfoo'
+      };
+
+      testHelper.verifyDocumentReplaced(doc, oldDoc, { expectedRoles: [ 'replace' ] });
+    });
+
+    it('rejects document deletion for a user with only replace role', function() {
+      var doc = {
+        _id: 'replaceOnlyRoleDoc',
+        _deleted: true
+      };
+      var oldDoc = {
+        _id: 'replaceOnlyRoleDoc',
+        stringProp: 'foobar'
+      };
+
+      testHelper.verifyAccessDenied(doc, oldDoc, []);
+    });
+  });
+
+  describe('for a document with statically-assigned add role and nothing else', function() {
+    it('allows document creation', function() {
+      var doc = {
+        _id: 'addOnlyRoleDoc',
+        stringProp: 'foobar'
+      };
+
+      testHelper.verifyDocumentCreated(doc, { expectedRoles: [ 'add' ] });
+    });
+
+    it('rejects document replacement', function() {
+      var doc = {
+        _id: 'addOnlyRoleDoc',
+        stringProp: 'foobar'
+      };
+      var oldDoc = {
+        _id: 'addOnlyRoleDoc',
+        stringProp: 'barfoo'
+      };
+
+      testHelper.verifyAccessDenied(doc, oldDoc, []);
+    });
+
+    it('rejects document deletion for a user with only add role', function() {
+      var doc = {
+        _id: 'addOnlyRoleDoc',
+        _deleted: true
+      };
+      var oldDoc = {
+        _id: 'addOnlyRoleDoc',
+        stringProp: 'foobar'
+      };
+
+      testHelper.verifyAccessDenied(doc, oldDoc, []);
+    });
+  });
 });
diff --git a/test/resources/authorization-doc-definitions.js b/test/resources/authorization-doc-definitions.js
@@ -109,5 +109,31 @@
         type: 'string'
       }
     }
+  },
+  replaceOnlyRoleDoc: {
+    authorizedRoles: {
+      replace: 'replace'
+    },
+    typeFilter: function(doc) {
+      return doc._id === 'replaceOnlyRoleDoc';
+    },
+    propertyValidators: {
+      stringProp: {
+        type: 'string'
+      }
+    }
+  },
+  addOnlyRoleDoc: {
+    authorizedRoles: {
+      add: 'add'
+    },
+    typeFilter: function(doc) {
+      return doc._id === 'addOnlyRoleDoc';
+    },
+    propertyValidators: {
+      stringProp: {
+        type: 'string'
+      }
+    }
   }
 }