Skip to content

Fix Role.isOwner() when multiple user models #3180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bajtos merged 1 commit into from
Feb 17, 2017
Merged

Fix Role.isOwner() when multiple user models #3180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 51 additions & 27 deletions common/models/role.js
View file
Open in desktop
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);
});

Copy link
Contributor Author

@ebarault ebarault Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that context.getUser() can return null, we need to test user before getting user.id and user.principalType

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;

Copy link
Contributor Author

@ebarault ebarault Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while principalType is an optional parameter, it defaults internally to USER if undefined for backward compatibility

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)) {
Copy link
Member

@bajtos bajtos Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, I think it's better to keep the second argument as false? This can happen when err is falsy and the second condition !inst was triggered.

Copy link
Contributor Author

@ebarault ebarault Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, too quickly assumed err wasn't ever falsy in that case (overlooked the !inst)

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;
Copy link
Contributor Author

@ebarault ebarault Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need now to check if the principalType is either USER (standard, single user model) or equal to user's model name (multiple user models)

}

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;
Copy link
Contributor Author

@ebarault ebarault Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, principalType must either be equal to USER, or to related user model name

}
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);
Copy link
Contributor Author

@ebarault ebarault Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just removing useless test on callback now it's always defined thanks to utils.createPromiseCallback()

}
}
});
Expand Down
14 changes: 11 additions & 3 deletions lib/access-context.js
View file
Open in desktop
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;
};

Copy link
Contributor Author

@ebarault ebarault Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shrinked version of getUserId(), leveraging getUser()

/**
* 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};
}
}
Copy link
Contributor Author

@ebarault ebarault Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context.getUser() now returns null instead of {} to enable developers to more easily detect issues when accessContext misses the user

return null;
};

/**
Expand Down
76 changes: 57 additions & 19 deletions test/multiple-user-principal-types.test.js
View file
Open in desktop
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,
});
});
});
Copy link
Contributor Author

@ebarault ebarault Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adapted new tests to check getUser instead of getUserId


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,
});
});
});
Copy link
Contributor Author

@ebarault ebarault Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here


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,
});
});
});
});
Copy link
Contributor Author

@ebarault ebarault Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

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();
});
Copy link
Contributor Author

@ebarault ebarault Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split the OWNER test in 2 to check apart if resolver returns false when principalType is incorrect

});
});
Expand Down
44 changes: 44 additions & 0 deletions test/role.test.js
View file
Open in desktop
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