Skip to content

Commit f0e70dd

Browse files
ebaraultbajtos
ebarault
authored and
bajtos
committed
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.
1 parent 0dbfe89 commit f0e70dd

4 files changed

Lines changed: 163 additions & 49 deletions

File tree

‎common/models/role.js‎

Lines changed: 51 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -179,8 +179,10 @@ module.exports = function(Role) {
179179
}
180180
var modelClass = context.model;
181181
var modelId = context.modelId;
182-
var userId = context.getUserId();
183-
Role.isOwner(modelClass, modelId, userId, callback);
182+
var user = context.getUser();
183+
var userId = user && user.id;
184+
var principalType = user && user.principalType;
185+
Role.isOwner(modelClass, modelId, userId, principalType, callback);
184186
});
185187

186188
function isUserClass(modelClass) {
@@ -210,18 +212,26 @@ module.exports = function(Role) {
210212
* @param {Function} modelClass The model class
211213
* @param {*} modelId The model ID
212214
* @param {*} userId The user ID
215+
* @param {String} principalType The user principalType (optional)
213216
* @callback {Function} [callback] The callback function
214217
* @param {String|Error} err The error string or object
215218
* @param {Boolean} isOwner True if the user is an owner.
216219
* @promise
217220
*/
218-
Role.isOwner = function isOwner(modelClass, modelId, userId, callback) {
221+
Role.isOwner = function isOwner(modelClass, modelId, userId, principalType, callback) {
222+
if (!callback && typeof principalType === 'function') {
223+
callback = principalType;
224+
principalType = undefined;
225+
}
226+
principalType = principalType || Principal.USER;
227+
219228
assert(modelClass, 'Model class is required');
220229
if (!callback) callback = utils.createPromiseCallback();
221230

222-
debug('isOwner(): %s %s userId: %s', modelClass && modelClass.modelName, modelId, userId);
231+
debug('isOwner(): %s %s userId: %s principalType: %s',
232+
modelClass && modelClass.modelName, modelId, userId, principalType);
223233

224-
// No userId is present
234+
// Return false if userId is missing
225235
if (!userId) {
226236
process.nextTick(function() {
227237
callback(null, false);
@@ -231,44 +241,58 @@ module.exports = function(Role) {
231241

232242
// Is the modelClass User or a subclass of User?
233243
if (isUserClass(modelClass)) {
234-
process.nextTick(function() {
235-
callback(null, matches(modelId, userId));
236-
});
244+
var userModelName = modelClass.modelName;
245+
// matching ids is enough if principalType is USER or matches given user model name
246+
if (principalType === Principal.USER || principalType === userModelName) {
247+
process.nextTick(function() {
248+
callback(null, matches(modelId, userId));
249+
});
250+
}
237251
return callback.promise;
238252
}
239253

240254
modelClass.findById(modelId, function(err, inst) {
241255
if (err || !inst) {
242256
debug('Model not found for id %j', modelId);
243-
if (callback) callback(err, false);
244-
return;
257+
return callback(err, false);
245258
}
246259
debug('Model found: %j', inst);
260+
261+
// Historically, for principalType USER, we were resolving isOwner()
262+
// as true if the model has "userId" or "owner" property matching
263+
// id of the current user (principalId), even though there was no
264+
// belongsTo relation set up.
247265
var ownerId = inst.userId || inst.owner;
248-
// Ensure ownerId exists and is not a function/relation
249-
if (ownerId && 'function' !== typeof ownerId) {
250-
if (callback) callback(null, matches(ownerId, userId));
251-
return;
252-
} else {
253-
// Try to follow belongsTo
254-
for (var r in modelClass.relations) {
255-
var rel = modelClass.relations[r];
256-
if (rel.type === 'belongsTo' && isUserClass(rel.modelTo)) {
257-
debug('Checking relation %s to %s: %j', r, rel.modelTo.modelName, rel);
258-
inst[r](processRelatedUser);
259-
return;
260-
}
266+
if (principalType === Principal.USER && ownerId && 'function' !== typeof ownerId) {
267+
return callback(null, matches(ownerId, userId));
268+
}
269+
270+
// Try to follow belongsTo
271+
for (var r in modelClass.relations) {
272+
var rel = modelClass.relations[r];
273+
// relation should be belongsTo and target a User based class
274+
var belongsToUser = rel.type === 'belongsTo' && isUserClass(rel.modelTo);
275+
if (!belongsToUser) {
276+
continue;
277+
}
278+
// checking related user
279+
var userModelName = rel.modelTo.modelName;
280+
if (principalType === Principal.USER || principalType === userModelName) {
281+
debug('Checking relation %s to %s: %j', r, userModelName, rel);
282+
inst[r](processRelatedUser);
283+
return;
261284
}
262-
debug('No matching belongsTo relation found for model %j and user: %j', modelId, userId);
263-
if (callback) callback(null, false);
264285
}
286+
debug('No matching belongsTo relation found for model %j - user %j principalType %j',
287+
modelId, userId, principalType);
288+
callback(null, false);
265289

266290
function processRelatedUser(err, user) {
267291
if (!err && user) {
268292
debug('User found: %j', user.id);
269-
if (callback) callback(null, matches(user.id, userId));
293+
callback(null, matches(user.id, userId));
270294
} else {
271-
if (callback) callback(err, false);
295+
callback(err, false);
272296
}
273297
}
274298
});

‎lib/access-context.js‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,15 @@ AccessContext.prototype.addPrincipal = function(principalType, principalId, prin
128128
* @returns {*}
129129
*/
130130
AccessContext.prototype.getUserId = function() {
131+
var user = this.getUser();
132+
return user && user.id;
133+
};
134+
135+
/**
136+
* Get the user
137+
* @returns {*}
138+
*/
139+
AccessContext.prototype.getUser = function() {
131140
var BaseUser = this.registry.getModel('User');
132141
for (var i = 0; i < this.principals.length; i++) {
133142
var p = this.principals[i];
@@ -138,17 +147,16 @@ AccessContext.prototype.getUserId = function() {
138147

139148
// the principalType must either be 'USER'
140149
if (p.type === Principal.USER) {
141-
return p.id;
150+
return {id: p.id, principalType: p.type};
142151
}
143152

144153
// or permit to resolve a valid user model
145154
var userModel = this.registry.findModel(p.type);
146155
if (!userModel) continue;
147156
if (userModel.prototype instanceof BaseUser) {
148-
return p.id;
157+
return {id: p.id, principalType: p.type};
149158
}
150159
}
151-
return null;
152160
};
153161

154162
/**

‎test/multiple-user-principal-types.test.js‎

Lines changed: 57 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,8 @@ describe('Multiple users with custom principalType', function() {
204204
accessContext = new AccessContext({registry: OneUser.registry});
205205
});
206206

207-
describe('getUserId()', function() {
208-
it('returns userId although principals contain non USER principals',
207+
describe('getUser()', function() {
208+
it('returns user although principals contain non USER principals',
209209
function() {
210210
return Promise.try(function() {
211211
addToAccessContext([
@@ -214,21 +214,27 @@ describe('Multiple users with custom principalType', function() {
214214
{type: Principal.SCOPE},
215215
{type: OneUser.modelName, id: userFromOneModel.id},
216216
]);
217-
var userId = accessContext.getUserId();
218-
expect(userId).to.equal(userFromOneModel.id);
217+
var user = accessContext.getUser();
218+
expect(user).to.eql({
219+
id: userFromOneModel.id,
220+
principalType: OneUser.modelName,
221+
});
219222
});
220223
});
221224

222-
it('returns userId although principals contain invalid principals',
225+
it('returns user although principals contain invalid principals',
223226
function() {
224227
return Promise.try(function() {
225228
addToAccessContext([
226229
{type: 'AccessToken'},
227230
{type: 'invalidModelName'},
228231
{type: OneUser.modelName, id: userFromOneModel.id},
229232
]);
230-
var userId = accessContext.getUserId();
231-
expect(userId).to.equal(userFromOneModel.id);
233+
var user = accessContext.getUser();
234+
expect(user).to.eql({
235+
id: userFromOneModel.id,
236+
principalType: OneUser.modelName,
237+
});
232238
});
233239
});
234240

@@ -238,8 +244,11 @@ describe('Multiple users with custom principalType', function() {
238244
return ThirdUser.create(commonCredentials)
239245
.then(function(userFromThirdModel) {
240246
accessContext.addPrincipal(ThirdUser.modelName, userFromThirdModel.id);
241-
var userId = accessContext.getUserId();
242-
expect(userId).to.equal(userFromThirdModel.id);
247+
var user = accessContext.getUser();
248+
expect(user).to.eql({
249+
id: userFromThirdModel.id,
250+
principalType: ThirdUser.modelName,
251+
});
243252
});
244253
});
245254
});
@@ -373,17 +382,46 @@ describe('Multiple users with custom principalType', function() {
373382

374383
return Album.create({name: 'album', userId: userFromOneModel.id})
375384
.then(function(album) {
376-
return Role.isInRole(
377-
Role.OWNER,
378-
{
379-
principalType: OneUser.modelName,
380-
principalId: userFromOneModel.id,
381-
model: Album,
382-
id: album.id,
383-
});
385+
var validContext = {
386+
principalType: OneUser.modelName,
387+
principalId: userFromOneModel.id,
388+
model: Album,
389+
id: album.id,
390+
};
391+
return Role.isInRole(Role.OWNER, validContext);
384392
})
385-
.then(function(isInRole) {
386-
expect(isInRole).to.be.true();
393+
.then(function(isOwner) {
394+
expect(isOwner).to.be.true();
395+
});
396+
});
397+
398+
it('expects OWNER to resolve false if owner has incorrect principalType', function() {
399+
var Album = app.registry.createModel('Album', {
400+
name: String,
401+
userId: Number,
402+
}, {
403+
relations: {
404+
user: {
405+
type: 'belongsTo',
406+
model: 'OneUser',
407+
foreignKey: 'userId',
408+
},
409+
},
410+
});
411+
app.model(Album, {dataSource: 'db'});
412+
413+
return Album.create({name: 'album', userId: userFromOneModel.id})
414+
.then(function(album) {
415+
var invalidContext = {
416+
principalType: AnotherUser.modelName,
417+
principalId: userFromOneModel.id,
418+
model: Album,
419+
id: album.id,
420+
};
421+
return Role.isInRole(Role.OWNER, invalidContext);
422+
})
423+
.then(function(isOwner) {
424+
expect(isOwner).to.be.false();
387425
});
388426
});
389427
});

‎test/role.test.js‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,50 @@ describe('role model', function() {
489489
});
490490
});
491491

492+
it('resolves OWNER via "userId" property with no relation', function() {
493+
var Album = app.registry.createModel('Album', {
494+
name: String,
495+
userId: Number,
496+
});
497+
app.model(Album, {dataSource: 'db'});
498+
499+
var user;
500+
return User.create({email: 'test@example.com', password: 'pass'})
501+
.then(u => {
502+
user = u;
503+
return Album.create({name: 'Album 1', userId: user.id});
504+
})
505+
.then(album => {
506+
return Role.isInRole(Role.OWNER, {
507+
principalType: ACL.USER, principalId: user.id,
508+
model: Album, id: album.id,
509+
});
510+
})
511+
.then(isInRole => expect(isInRole).to.be.true());
512+
});
513+
514+
it('resolves OWNER via "owner" property with no relation', function() {
515+
var Album = app.registry.createModel('Album', {
516+
name: String,
517+
owner: Number,
518+
});
519+
app.model(Album, {dataSource: 'db'});
520+
521+
var user;
522+
return User.create({email: 'test@example.com', password: 'pass'})
523+
.then(u => {
524+
user = u;
525+
return Album.create({name: 'Album 1', owner: user.id});
526+
})
527+
.then(album => {
528+
return Role.isInRole(Role.OWNER, {
529+
principalType: ACL.USER, principalId: user.id,
530+
model: Album, id: album.id,
531+
});
532+
})
533+
.then(isInRole => expect(isInRole).to.be.true());
534+
});
535+
492536
describe('isMappedToRole', function() {
493537
var user, app, role;
494538

0 commit comments

Comments
 (0)