From 28acffd7dd6e6ababcf5f0c06f9facf7b136356e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Wed, 8 Apr 2015 11:52:58 +0200 Subject: [PATCH 1/2] Fix PersistedModel._defineChangeModel Correctly handle the case when the model is attached multiple times during the lifecycle, this happens because `loopback.createModel` always makes an attempt to auto-attach. --- lib/persisted-model.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/persisted-model.js b/lib/persisted-model.js index 53f8cc62..89b3b9e4 100644 --- a/lib/persisted-model.js +++ b/lib/persisted-model.js @@ -1423,10 +1423,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) { From cf2acb3cd23f9bcd8736a703093f6c7ca6b9aee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 9 Apr 2015 12:44:29 +0200 Subject: [PATCH 2/2] Conflict resolution and Access control Add end-to-end unit-tests verifying enforcement of access control during conflict resolution. Implement two facade methods providing REST API for Change methods used by conflict resolution: PersistedModel.findLastChange GET /api/{model.pluralName}/{id}/changes/last PersistedModel.updateLastChange PUT /api/{model.pluralName}/{id}/changes/last By providing these two methods on PersistedModel, replication users don't have to expose the Change model via the REST API. What's even more important, these two methods use the same set of ACL rules as other (regular) PersistedModel methods. Rework `Conflict.prototype.changes()` and `Conflict.prototype.resolve()` to use these new facade methods. Implement a new method `Conflict.prototype.swapParties()` that provides better API for the situation when a conflict detected in Remote->Local replication should be resolved locally (i.e. in the replication target). --- common/models/change.js | 45 ++++++--- lib/persisted-model.js | 51 ++++++++++ test/replication.rest.test.js | 180 +++++++++++++++++++++++++++++++--- 3 files changed, 248 insertions(+), 28 deletions(-) diff --git a/common/models/change.js b/common/models/change.js index ecb0a267..daac4054 100644 --- a/common/models/change.js +++ b/common/models/change.js @@ -617,9 +617,8 @@ module.exports = function(Change) { ], done); function getSourceChange(cb) { - conflict.SourceChange.findOne({where: { - modelId: conflict.modelId - }}, function(err, change) { + var SourceModel = conflict.SourceModel; + SourceModel.findLastChange(conflict.modelId, 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: { - modelId: conflict.modelId - }}, function(err, change) { + var TargetModel = conflict.TargetModel; + TargetModel.findLastChange(conflict.modelId, 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) { - if (err) return cb(err); - sourceChange.prev = targetChange.rev; - sourceChange.save(cb); - }); + conflict.TargetModel.findLastChange( + this.modelId, + function(err, targetChange) { + 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. * diff --git a/lib/persisted-model.js b/lib/persisted-model.js index 89b3b9e4..00a17434 100644 --- a/lib/persisted-model.js +++ b/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) { @@ -1469,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; diff --git a/test/replication.rest.test.js b/test/replication.rest.test.js index a1ae6765..6316457f 100644 --- a/test/replication.rest.test.js +++ b/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 - // TODO verify permissions of individual methods + describe('conflict resolution with model-level permissions', function() { + 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' }, - { maker: 'Audi', model: 'R8' } + { id: 'Ford-Mustang', maker: 'Ford', model: 'Mustang' }, + { 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) { - if (err) return done(err); - LocalCar.deleteAll(function(err) { - if (err) return done(err); + async.series([ + function(next) { + clientApp.dataSources.db.automigrate(next); + }, + 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); }); }); });