Skip to content

Commit

Permalink
Fix Role.isOwner() for multiple user models
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ebarault @bajtos
ebarault authored and bajtos committed Feb 17, 2017
1 parent 0dbfe89 commit f0e70dd
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 49 deletions.
78 changes: 51 additions & 27 deletions common/models/role.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
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 and user: %j', modelId, userId);
if (callback) callback(null, false);
}
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);
}
}
});
Expand Down
14 changes: 11 additions & 3 deletions lib/access-context.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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;
};

/**
Expand Down
76 changes: 57 additions & 19 deletions test/multiple-user-principal-types.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand All @@ -214,21 +214,27 @@ 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([
{type: 'AccessToken'},
{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,
});
});
});

Expand All @@ -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,
});
});
});
});
Expand Down Expand Up @@ -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();
});
});
});
Expand Down
44 changes: 44 additions & 0 deletions test/role.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

0 comments on commit f0e70dd

Please sign in to comment.