verdnatura-mirror
/
loopback
mirror of https://github.com/strongloop/loopback.git
0
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix Role.isOwner() for multiple user models 

Fix `Role.isOwner()` to check both principalId and principalType.
This fixes a bug where users from different User model were treated
as owners as long as their user id was the same as owner's id.
This commit is contained in:
ebarault 2017-02-03 21:53:27 +01:00 committed by Miroslav Bajtoš
parent 0dbfe8970f
commit f0e70dd8a9
4 changed files with 164 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

80
common/models/role.js
View File

@ -179,8 +179,10 @@ module.exports = function(Role) {
}
var modelClass = context.model;
var modelId = context.modelId;
var userId = context.getUserId();
Role.isOwner(modelClass, modelId, userId, callback);
var user = context.getUser();
var userId = user && user.id;
var principalType = user && user.principalType;
Role.isOwner(modelClass, modelId, userId, principalType, callback);
});
function isUserClass(modelClass) {
@ -210,18 +212,26 @@ module.exports = function(Role) {
* @param {Function} modelClass The model class
* @param {*} modelId The model ID
* @param {*} userId The user ID
* @param {String} principalType The user principalType (optional)
* @callback {Function} [callback] The callback function
* @param {String|Error} err The error string or object
* @param {Boolean} isOwner True if the user is an owner.
* @promise
*/
Role.isOwner = function isOwner(modelClass, modelId, userId, callback) {
Role.isOwner = function isOwner(modelClass, modelId, userId, principalType, callback) {
if (!callback && typeof principalType === 'function') {
callback = principalType;
principalType = undefined;
}
principalType = principalType || Principal.USER;
assert(modelClass, 'Model class is required');
if (!callback) callback = utils.createPromiseCallback();
debug('isOwner(): %s %s userId: %s', modelClass && modelClass.modelName, modelId, userId);
debug('isOwner(): %s %s userId: %s principalType: %s',
modelClass && modelClass.modelName, modelId, userId, principalType);
// No userId is present
// Return false if userId is missing
if (!userId) {
process.nextTick(function() {
callback(null, false);
@ -231,44 +241,58 @@ module.exports = function(Role) {
// Is the modelClass User or a subclass of User?
if (isUserClass(modelClass)) {
process.nextTick(function() {
callback(null, matches(modelId, userId));
});
var userModelName = modelClass.modelName;
// matching ids is enough if principalType is USER or matches given user model name
if (principalType === Principal.USER || principalType === userModelName) {
process.nextTick(function() {
callback(null, matches(modelId, userId));
});
}
return callback.promise;
}
modelClass.findById(modelId, function(err, inst) {
if (err || !inst) {
debug('Model not found for id %j', modelId);
if (callback) callback(err, false);
return;
return callback(err, false);
}
debug('Model found: %j', inst);
// Historically, for principalType USER, we were resolving isOwner()
// as true if the model has "userId" or "owner" property matching
// id of the current user (principalId), even though there was no
// belongsTo relation set up.
var ownerId = inst.userId || inst.owner;
// Ensure ownerId exists and is not a function/relation
if (ownerId && 'function' !== typeof ownerId) {
if (callback) callback(null, matches(ownerId, userId));
return;
} else {
// Try to follow belongsTo
for (var r in modelClass.relations) {
var rel = modelClass.relations[r];
if (rel.type === 'belongsTo' && isUserClass(rel.modelTo)) {
debug('Checking relation %s to %s: %j', r, rel.modelTo.modelName, rel);
inst[r](processRelatedUser);
return;
}
}
debug('No matching belongsTo relation found for model %j and user: %j', modelId, userId);
if (callback) callback(null, false);
if (principalType === Principal.USER && ownerId && 'function' !== typeof ownerId) {
return callback(null, matches(ownerId, userId));
}
// Try to follow belongsTo
for (var r in modelClass.relations) {
var rel = modelClass.relations[r];
// relation should be belongsTo and target a User based class
var belongsToUser = rel.type === 'belongsTo' && isUserClass(rel.modelTo);
if (!belongsToUser) {
continue;
}
// checking related user
var userModelName = rel.modelTo.modelName;
if (principalType === Principal.USER || principalType === userModelName) {
debug('Checking relation %s to %s: %j', r, userModelName, rel);
inst[r](processRelatedUser);
return;
}
}
debug('No matching belongsTo relation found for model %j - user %j principalType %j',
modelId, userId, principalType);
callback(null, false);
function processRelatedUser(err, user) {
if (!err && user) {
debug('User found: %j', user.id);
if (callback) callback(null, matches(user.id, userId));
callback(null, matches(user.id, userId));
} else {
if (callback) callback(err, false);
callback(err, false);
}
}
});

14
lib/access-context.js
View File

@ -128,6 +128,15 @@ AccessContext.prototype.addPrincipal = function(principalType, principalId, prin
* @returns {*}
*/
AccessContext.prototype.getUserId = function() {
var user = this.getUser();
return user && user.id;
};
/**
* Get the user
* @returns {*}
*/
AccessContext.prototype.getUser = function() {
var BaseUser = this.registry.getModel('User');
for (var i = 0; i < this.principals.length; i++) {
var p = this.principals[i];
@ -138,17 +147,16 @@ AccessContext.prototype.getUserId = function() {
// the principalType must either be 'USER'
if (p.type === Principal.USER) {
return p.id;
return {id: p.id, principalType: p.type};
}
// or permit to resolve a valid user model
var userModel = this.registry.findModel(p.type);
if (!userModel) continue;
if (userModel.prototype instanceof BaseUser) {
return p.id;
return {id: p.id, principalType: p.type};
}
}
return null;
};
/**

76
test/multiple-user-principal-types.test.js
View File

@ -204,8 +204,8 @@ describe('Multiple users with custom principalType', function() {
accessContext = new AccessContext({registry: OneUser.registry});
});
describe('getUserId()', function() {
it('returns userId although principals contain non USER principals',
describe('getUser()', function() {
it('returns user although principals contain non USER principals',
function() {
return Promise.try(function() {
addToAccessContext([
@ -214,12 +214,15 @@ describe('Multiple users with custom principalType', function() {
{type: Principal.SCOPE},
{type: OneUser.modelName, id: userFromOneModel.id},
]);
var userId = accessContext.getUserId();
expect(userId).to.equal(userFromOneModel.id);
var user = accessContext.getUser();
expect(user).to.eql({
id: userFromOneModel.id,
principalType: OneUser.modelName,
});
});
});
it('returns userId although principals contain invalid principals',
it('returns user although principals contain invalid principals',
function() {
return Promise.try(function() {
addToAccessContext([
@ -227,8 +230,11 @@ describe('Multiple users with custom principalType', function() {
{type: 'invalidModelName'},
{type: OneUser.modelName, id: userFromOneModel.id},
]);
var userId = accessContext.getUserId();
expect(userId).to.equal(userFromOneModel.id);
var user = accessContext.getUser();
expect(user).to.eql({
id: userFromOneModel.id,
principalType: OneUser.modelName,
});
});
});
@ -238,8 +244,11 @@ describe('Multiple users with custom principalType', function() {
return ThirdUser.create(commonCredentials)
.then(function(userFromThirdModel) {
accessContext.addPrincipal(ThirdUser.modelName, userFromThirdModel.id);
var userId = accessContext.getUserId();
expect(userId).to.equal(userFromThirdModel.id);
var user = accessContext.getUser();
expect(user).to.eql({
id: userFromThirdModel.id,
principalType: ThirdUser.modelName,
});
});
});
});
@ -373,17 +382,46 @@ describe('Multiple users with custom principalType', function() {
return Album.create({name: 'album', userId: userFromOneModel.id})
.then(function(album) {
return Role.isInRole(
Role.OWNER,
{
principalType: OneUser.modelName,
principalId: userFromOneModel.id,
model: Album,
id: album.id,
});
var validContext = {
principalType: OneUser.modelName,
principalId: userFromOneModel.id,
model: Album,
id: album.id,
};
return Role.isInRole(Role.OWNER, validContext);
})
.then(function(isInRole) {
expect(isInRole).to.be.true();
.then(function(isOwner) {
expect(isOwner).to.be.true();
});
});
it('expects OWNER to resolve false if owner has incorrect principalType', function() {
var Album = app.registry.createModel('Album', {
name: String,
userId: Number,
}, {
relations: {
user: {
type: 'belongsTo',
model: 'OneUser',
foreignKey: 'userId',
},
},
});
app.model(Album, {dataSource: 'db'});
return Album.create({name: 'album', userId: userFromOneModel.id})
.then(function(album) {
var invalidContext = {
principalType: AnotherUser.modelName,
principalId: userFromOneModel.id,
model: Album,
id: album.id,
};
return Role.isInRole(Role.OWNER, invalidContext);
})
.then(function(isOwner) {
expect(isOwner).to.be.false();
});
});
});

44
test/role.test.js
View File

@ -489,6 +489,50 @@ describe('role model', function() {
});
});
it('resolves OWNER via "userId" property with no relation', function() {
var Album = app.registry.createModel('Album', {
name: String,
userId: Number,
});
app.model(Album, {dataSource: 'db'});
var user;
return User.create({email: 'test@example.com', password: 'pass'})
.then(u => {
user = u;
return Album.create({name: 'Album 1', userId: user.id});
})
.then(album => {
return Role.isInRole(Role.OWNER, {
principalType: ACL.USER, principalId: user.id,
model: Album, id: album.id,
});
})
.then(isInRole => expect(isInRole).to.be.true());
});
it('resolves OWNER via "owner" property with no relation', function() {
var Album = app.registry.createModel('Album', {
name: String,
owner: Number,
});
app.model(Album, {dataSource: 'db'});
var user;
return User.create({email: 'test@example.com', password: 'pass'})
.then(u => {
user = u;
return Album.create({name: 'Album 1', owner: user.id});
})
.then(album => {
return Role.isInRole(Role.OWNER, {
principalType: ACL.USER, principalId: user.id,
model: Album, id: album.id,
});
})
.then(isInRole => expect(isInRole).to.be.true());
});
describe('isMappedToRole', function() {
var user, app, role;