Merge pull request #1294 from strongloop/feature/conflict-resolution-access-control

Conflict resolution and Access control

common/models/change.js

@@ -617,9 +617,8 @@ module.exports = function(Change) {
     ], done);
 
     function getSourceChange(cb) {
-      conflict.SourceChange.findOne({where: {
+      var SourceModel = conflict.SourceModel;
-        modelId: conflict.modelId
+      SourceModel.findLastChange(conflict.modelId, function(err, change) {
-      }}, function(err, change) {
         if (err) return cb(err);
         sourceChange = change;
         cb();
@@ -627,9 +626,8 @@ module.exports = function(Change) {
     }
 
     function getTargetChange(cb) {
-      conflict.TargetChange.findOne({where: {
+      var TargetModel = conflict.TargetModel;
-        modelId: conflict.modelId
+      TargetModel.findLastChange(conflict.modelId, function(err, change) {
-      }}, function(err, change) {
         if (err) return cb(err);
         targetChange = change;
         cb();
@@ -658,11 +656,15 @@ module.exports = function(Change) {
 
   Conflict.prototype.resolve = function(cb) {
     var conflict = this;
-    conflict.changes(function(err, sourceChange, targetChange) {
+    conflict.TargetModel.findLastChange(
-      if (err) return cb(err);
+      this.modelId,
-      sourceChange.prev = targetChange.rev;
+      function(err, targetChange) {
-      sourceChange.save(cb);
+        if (err) return cb(err);
-    });
+        conflict.SourceModel.updateLastChange(
+          conflict.modelId,
+          { prev: targetChange.rev },
+          cb);
+      });
   };
 
   /**
@@ -692,7 +694,9 @@ module.exports = function(Change) {
       if (target === null) {
         return conflict.SourceModel.deleteById(conflict.modelId, done);
       }
-      var inst = new conflict.SourceModel(target);
+      var inst = new conflict.SourceModel(
+        target.toObject(),
+        { persisted: true });
       inst.save(done);
     });
 
@@ -702,6 +706,23 @@ module.exports = function(Change) {
     }
   };
 
+  /**
+   * Return a new Conflict instance with swapped Source and Target models.
+   *
+   * This is useful when resolving a conflict in one-way
+   * replication, where the source data must not be changed:
+   *
+   * ```js
+   * conflict.swapParties().resolveUsingTarget(cb);
+   * ```
+   *
+   * @returns {Conflict} A new Conflict instance.
+   */
+  Conflict.prototype.swapParties = function() {
+    var Ctor = this.constructor;
+    return new Ctor(this.modelId, this.TargetModel, this.SourceModel);
+  };
+
   /**
    * Resolve the conflict using the supplied instance data.
    *

lib/persisted-model.js

@@ -705,6 +705,37 @@ module.exports = function(registry) {
         accepts: {arg: 'updates', type: 'array'},
         http: {verb: 'post', path: '/bulk-update'}
       });
+
+      setRemoting(PersistedModel, 'findLastChange', {
+        description: 'Get the most recent change record for this instance.',
+        accessType: 'READ',
+        accepts: {
+          arg: 'id', type: 'any', required: true, http: { source: 'path' },
+          description: 'Model id'
+        },
+        returns: { arg: 'result', type: this.Change.modelName, root: true },
+        http: { verb: 'get', path: '/:id/changes/last' }
+      });
+
+      setRemoting(PersistedModel, 'updateLastChange', {
+        description: [
+          'Update the properties of the most recent change record',
+          'kept for this instance.'
+        ],
+        accessType: 'WRITE',
+        accepts: [
+          {
+            arg: 'id', type: 'any', required: true, http: { source: 'path' },
+            description: 'Model id'
+          },
+          {
+            arg: 'data', type: 'object', http: {source: 'body'},
+            description: 'An object of Change property name/value pairs'
+          },
+        ],
+        returns: { arg: 'result', type: this.Change.modelName, root: true },
+        http: { verb: 'put', path: '/:id/changes/last' }
+      });
     }
 
     if (options.trackChanges) {
@@ -1423,10 +1454,15 @@ module.exports = function(registry) {
 
     if (this.dataSource) {
       attachRelatedModels(this);
-    } else {
-      this.once('dataSourceAttached', attachRelatedModels);
     }
 
+    // We have to attach related model whenever the datasource changes,
+    // this is a workaround for autoAttach called by loopback.createModel
+    var self = this;
+    this.on('dataSourceAttached', function() {
+      attachRelatedModels(self);
+    });
+
     return this.Change;
 
     function attachRelatedModels(self) {
@@ -1464,6 +1500,26 @@ module.exports = function(registry) {
     Change.rectifyModelChanges(this.modelName, [id], callback);
   };
 
+  PersistedModel.findLastChange = function(id, cb) {
+    var Change = this.getChangeModel();
+    Change.findOne({ where: { modelId: id } }, cb);
+  };
+
+  PersistedModel.updateLastChange = function(id, data, cb) {
+    var self = this;
+    this.findLastChange(id, function(err, inst) {
+      if (err) return cb(err);
+      if (!inst) {
+        err = new Error('No change record found for ' +
+          self.modelName + ' with id ' + id);
+        err.statusCode = 404;
+        return cb(err);
+      }
+
+      inst.updateAttributes(data, cb);
+    });
+  };
+
   PersistedModel.setup();
 
   return PersistedModel;

test/replication.rest.test.js

@@ -13,6 +13,7 @@ describe('Replication over REST', function() {
   var serverApp, serverUrl, ServerUser, ServerCar, serverCars;
   var aliceId, peterId, aliceToken, peterToken, emeryToken, request;
   var clientApp, LocalUser, LocalCar, RemoteUser, RemoteCar, clientCars;
+  var conflictedCarId;
 
   before(setupServer);
   before(setupClient);
@@ -154,9 +155,137 @@ describe('Replication over REST', function() {
         });
       });
     });
+  });
 
-    // TODO conflict resolution
+  describe('conflict resolution with model-level permissions', function() {
-    // TODO verify permissions of individual methods
+    var LocalConflict, RemoteConflict;
+
+    before(function setupConflictModels() {
+      LocalConflict = LocalCar.getChangeModel().Conflict;
+      RemoteConflict = RemoteCar.getChangeModel().Conflict;
+    });
+
+    beforeEach(seedConflict);
+
+    describe('as anonymous user', function() {
+      it('rejects resolve() on the client', function(done) {
+        // simulate replication Client->Server
+        var conflict = new LocalConflict(
+          conflictedCarId,
+          LocalCar,
+          RemoteCar);
+        conflict.resolveUsingSource(expectHttpError(401, done));
+      });
+
+      it('rejects resolve() on the server', function(done) {
+        // simulate replication Server->Client
+        var conflict = new RemoteConflict(
+          conflictedCarId,
+          RemoteCar,
+          LocalCar);
+        conflict.resolveUsingSource(expectHttpError(401, done));
+      });
+    });
+
+    describe('as user with READ-only permissions', function() {
+      beforeEach(function() {
+        setAccessToken(emeryToken);
+      });
+
+      it('allows resolve() on the client', function(done) {
+        // simulate replication Client->Server
+        var conflict = new LocalConflict(
+          conflictedCarId,
+          LocalCar,
+          RemoteCar);
+        conflict.resolveUsingSource(done);
+      });
+
+      it('rejects resolve() on the server', function(done) {
+        // simulate replication Server->Client
+        var conflict = new RemoteConflict(
+          conflictedCarId,
+          RemoteCar,
+          LocalCar);
+        conflict.resolveUsingSource(expectHttpError(401, done));
+      });
+    });
+
+    describe('as user with REPLICATE-only permissions', function() {
+      beforeEach(function() {
+        setAccessToken(aliceToken);
+      });
+
+      it('allows reverse resolve() on the client', function(done) {
+        RemoteCar.replicate(LocalCar, function(err, conflicts) {
+          if (err) return done(err);
+          expect(conflicts, 'conflicts').to.have.length(1);
+
+          // By default, conflicts are always resolved by modifying
+          // the source datasource, so that the next replication run
+          // replicates the resolved data.
+          // However, when replicating changes from a datasource that
+          // users are not authorized to change, the users have to resolve
+          // conflicts at the target, discarding any changes made in
+          // the target datasource only.
+          conflicts[0].swapParties().resolveUsingTarget(function(err) {
+            if (err) return done(err);
+
+            RemoteCar.replicate(LocalCar, function(err, conflicts) {
+              if (err) return done(err);
+              if (conflicts.length) return done(conflictError(conflicts));
+              done();
+            });
+          });
+        });
+      });
+
+      it('rejects resolve() on the server', function(done) {
+        RemoteCar.replicate(LocalCar, function(err, conflicts) {
+          if (err) return done(err);
+          expect(conflicts, 'conflicts').to.have.length(1);
+          conflicts[0].resolveUsingSource(expectHttpError(401, done));
+        });
+      });
+    });
+
+    describe('as user with READ and WRITE permissions', function() {
+      beforeEach(function() {
+        setAccessToken(peterToken);
+      });
+
+      it('allows resolve() on the client', function(done) {
+        LocalCar.replicate(RemoteCar, function(err, conflicts) {
+          if (err) return done(err);
+          expect(conflicts).to.have.length(1);
+
+          conflicts[0].resolveUsingSource(function(err) {
+            if (err) return done(err);
+            LocalCar.replicate(RemoteCar, function(err, conflicts) {
+              if (err) return done(err);
+              if (conflicts.length) return done(conflictError(conflicts));
+              done();
+            });
+          });
+        });
+      });
+
+      it('allows resolve() on the server', function(done) {
+        RemoteCar.replicate(LocalCar, function(err, conflicts) {
+          if (err) return done(err);
+          expect(conflicts).to.have.length(1);
+
+          conflicts[0].resolveUsingSource(function(err) {
+            if (err) return done(err);
+            RemoteCar.replicate(LocalCar, function(err, conflicts) {
+              if (err) return done(err);
+              if (conflicts.length) return done(conflictError(conflicts));
+              done();
+            });
+          });
+        });
+      });
+    });
   });
 
   describe.skip('sync with instance-level permissions', function() {
@@ -222,7 +351,7 @@ describe('Replication over REST', function() {
       ], done);
     });
 
-    // TODO verify conflict resolution
+    // TODO(bajtos) verify conflict resolution
 
     function setupModifiedLocalCopyOfAlice(done) {
       // Replicate directly, bypassing REST+AUTH layers
@@ -245,7 +374,7 @@ describe('Replication over REST', function() {
     base: 'User',
     plural: 'Users', // use the same REST path in all models
     trackChanges: true,
-    strict: true,
+    strict: 'throw',
     persistUndefinedAsNull: true
   };
 
@@ -259,7 +388,7 @@ describe('Replication over REST', function() {
     base: 'PersistedModel',
     plural: 'Cars', // use the same REST path in all models
     trackChanges: true,
-    strict: true,
+    strict: 'throw',
     persistUndefinedAsNull: true,
     acls: [
       // disable anonymous access
@@ -390,9 +519,6 @@ describe('Replication over REST', function() {
       function(next) {
         serverApp.dataSources.db.automigrate(next);
       },
-      function(next) {
-        ServerUser.deleteAll(next);
-      },
       function(next) {
         ServerUser.create([ALICE, PETER, EMERY], function(err, created) {
           if (err) return next(err);
@@ -421,8 +547,8 @@ describe('Replication over REST', function() {
       function(next) {
         ServerCar.create(
           [
-            { maker: 'Ford', model: 'Mustang' },
+            { id: 'Ford-Mustang',  maker: 'Ford', model: 'Mustang' },
-            { maker: 'Audi', model: 'R8' }
+            { id: 'Audi-R8', maker: 'Audi', model: 'R8' }
           ],
           function(err, cars) {
             if (err) return next(err);
@@ -434,16 +560,38 @@ describe('Replication over REST', function() {
   }
 
   function seedClientData(done) {
-    LocalUser.deleteAll(function(err) {
+    async.series([
-      if (err) return done(err);
+      function(next) {
-      LocalCar.deleteAll(function(err) {
+        clientApp.dataSources.db.automigrate(next);
-        if (err) return done(err);
+      },
+      function(next) {
         LocalCar.create(
           [{ maker: 'Local', model: 'Custom' }],
           function(err, cars) {
-            if (err) return done(err);
+            if (err) return next(err);
             clientCars = cars.map(carToString);
-            done();
+            next();
+          });
+      },
+    ], done);
+  }
+
+  function seedConflict(done) {
+    LocalCar.replicate(ServerCar, function(err, conflicts) {
+      if (err) return done(err);
+      if (conflicts.length) return done(conflictError(conflicts));
+      ServerCar.replicate(LocalCar, function(err, conflicts) {
+        if (err) return done(err);
+        if (conflicts.length) return done(conflictError(conflicts));
+
+        // Hard-coded, see the seed data above
+        conflictedCarId = 'Ford-Mustang';
+
+        new LocalCar({ id: conflictedCarId })
+          .updateAttributes({ model: 'Client' }, function(err, c) {
+            if (err) return done(err);
+            new ServerCar({ id: conflictedCarId })
+              .updateAttributes({ model: 'Server' }, done);
           });
       });
     });