Skip to content

Fix OWNER role to handle multiple relations #3140

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
Sep 27, 2017
Merged

Fix OWNER role to handle multiple relations #3140

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
126 changes: 112 additions & 14 deletions common/models/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ module.exports = function(Role) {
* @promise
*/
Role.isOwner = function isOwner(modelClass, modelId, userId, principalType, options, callback) {
var _this = this;

if (!callback && typeof options === 'function') {
callback = options;
options = {};
Expand All @@ -238,14 +240,31 @@ module.exports = function(Role) {
debug('isOwner(): %s %s userId: %s principalType: %s',
modelClass && modelClass.modelName, modelId, userId, principalType);

// Return false if userId is missing
// Resolve isOwner false if userId is missing
if (!userId) {
process.nextTick(function() {
callback(null, false);
});
return callback.promise;
}

// At this stage, principalType is valid in one of 2 following condition:
// 1. the app has a single user model and principalType is 'USER'
// 2. the app has multiple user models and principalType is not 'USER'
Copy link
Member

@bajtos bajtos Mar 8, 2017

Choose a reason for hiding this comment

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

Can principalType be APP or ROLE here?

Copy link
Contributor

@ebarault ebarault Mar 8, 2017

Choose a reason for hiding this comment

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

no i don't think so, isOwner is mean to test relations with users only
see this from the legacy code

Copy link
Member

@bajtos bajtos Mar 9, 2017

Choose a reason for hiding this comment

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

I agree it doesn't make sense to call isOwner passing APP or ROLE as a principal type.

However, are we sure that no code path can lead to such call? Is there any safe-guard check in isOwner to ensure we still return a sensible result in such case?

// multiple user models
var isMultipleUsers = _isMultipleUsers();
var isPrincipalTypeValid =
(!isMultipleUsers && principalType === Principal.USER) ||
(isMultipleUsers && principalType !== Principal.USER);

// Resolve isOwner false if principalType is invalid
if (!isPrincipalTypeValid) {
process.nextTick(function() {
callback(null, false);
});
return callback.promise;
}

Copy link
Contributor

@ebarault ebarault Mar 7, 2017

Choose a reason for hiding this comment

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

we should resolve false if the principalType is invalid given the user models config (single/multiple) : this way enables for example to resolve false if principalType is 'User' but the config is users:multiple
for this we use an helper which i comment later on

// Is the modelClass User or a subclass of User?
if (isUserClass(modelClass)) {
var userModelName = modelClass.modelName;
Expand All @@ -265,10 +284,25 @@ module.exports = function(Role) {
}
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 ownerRelations = modelClass.settings.ownerRelations;
if (!ownerRelations) {
return legacyOwnershipCheck(inst);
} else {
return checkOwnership(inst);
}
});
return callback.promise;

// NOTE 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.
// Additionaly, the original implementation did not support the
// possibility for a model to have multiple related users: when
// testing belongsTo relations, the first related user failing the
// ownership check induced the whole isOwner() to resolve as false.
// This behaviour will be pruned at next LoopBack major release.
function legacyOwnershipCheck(inst) {
var ownerId = inst.userId || inst.owner;
if (principalType === Principal.USER && ownerId && 'function' !== typeof ownerId) {
return callback(null, matches(ownerId, userId));
Expand All @@ -282,9 +316,16 @@ module.exports = function(Role) {
if (!belongsToUser) {
continue;
}

// checking related user
var userModelName = rel.modelTo.modelName;
if (principalType === Principal.USER || principalType === userModelName) {
var relatedUser = rel.modelTo;
var userModelName = relatedUser.modelName;
var isMultipleUsers = _isMultipleUsers(relatedUser);
// a relation can be considered for isOwner resolution if:
// 1. the app has a single user model and principalType is 'USER'
// 2. the app has multiple user models and principalType is the related user model name
if ((!isMultipleUsers && principalType === Principal.USER) ||
(isMultipleUsers && principalType === userModelName)) {
debug('Checking relation %s to %s: %j', r, userModelName, rel);
Copy link
Contributor

@ebarault ebarault Mar 7, 2017

Choose a reason for hiding this comment

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

although we already checked earlier if the config is user:multi/single in a general manner, we test it for each given user through relation.
It is a failsafe in case the user config is not 100% consistent and we did not detect it earlier

Copy link
Member

@bajtos bajtos Mar 8, 2017

Choose a reason for hiding this comment

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

TBH, I don't understand how the condition can be false. Is there a way how to write a unit-test reproducing this case? Not a big deal if not, I am just cautious about changing the legacy check implementation - we don't want to introduce an accidental regressions here.

Also the three lines (var isMultipleUsers..., !isMultipleUsers..., isMultipleUsers...) is duplicated in three places, can you extract a shared method?

Copy link
Contributor

@ebarault ebarault Mar 8, 2017

Choose a reason for hiding this comment

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

consider the following case which is not a valid configuration obviously:

  1. app is configured with multiple users
  2. one user is configured correctly (uses polymorphic relation with accessToken)
  3. other user is badly configured (uses standard hasMany relation with accessToken)

-> the condition will be false for the model in step 3.

Q. do you think we should instead do some sanity check during app boot so we warn or stop the app in case of incorrect config, and then get rid of some tests of the like?

i'll see to extract a test method and a unit test

Copy link
Member

@bajtos bajtos Mar 9, 2017

Choose a reason for hiding this comment

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

do you think we should instead do some sanity check during app boot so we warn or stop the app in case of incorrect config, and then get rid of some tests of the like?

I think that would be better - report problems to the user early.

Although we may still want to keep some of the sanity checks here, perhaps as assert calls.

inst[r](processRelatedUser);
return;
Expand All @@ -295,15 +336,72 @@ module.exports = function(Role) {
callback(null, false);

function processRelatedUser(err, user) {
if (!err && user) {
debug('User found: %j', user.id);
callback(null, matches(user.id, userId));
} else {
callback(err, false);
if (err || !user) return callback(err, false);

debug('User found: %j', user.id);
callback(null, matches(user.id, userId));
}
}

function checkOwnership(inst) {
var ownerRelations = inst.constructor.settings.ownerRelations;
// collecting related users
var relWithUsers = [];
for (var r in modelClass.relations) {
var rel = modelClass.relations[r];
// relation should be belongsTo and target a User based class
if (rel.type !== 'belongsTo' || !isUserClass(rel.modelTo)) {
continue;
}

// checking related user
var relatedUser = rel.modelTo;
var userModelName = relatedUser.modelName;
var isMultipleUsers = _isMultipleUsers(relatedUser);
// a relation can be considered for isOwner resolution if:
// 1. the app has a single user model and principalType is 'USER'
// 2. the app has multiple user models and principalType is the related user model name
// In addition, if an array of relations if provided with the ownerRelations option,
// then the given relation name is further checked against this array
if ((!isMultipleUsers && principalType === Principal.USER) ||
(isMultipleUsers && principalType === userModelName)) {
debug('Checking relation %s to %s: %j', r, userModelName, rel);
Copy link
Contributor

@ebarault ebarault Mar 7, 2017

Choose a reason for hiding this comment

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

same behavior than commented just before, but for the new isOwner behavior this time

if (ownerRelations === true) {
relWithUsers.push(r);
} else if (Array.isArray(ownerRelations) && ownerRelations.indexOf(r) !== -1) {
relWithUsers.push(r);
}
}
}
});
return callback.promise;
if (relWithUsers.length === 0) {
debug('No matching belongsTo relation found for model %j and user: %j principalType: %j',
modelId, userId, principalType);
return callback(null, false);
}

// check related users: someSeries is used to avoid spamming the db
async.someSeries(relWithUsers, processRelation, callback);

function processRelation(r, cb) {
inst[r](function processRelatedUser(err, user) {
if (err || !user) return cb(err, false);

debug('User found: %j (through %j)', user.id, r);
cb(null, matches(user.id, userId));
});
}
}

// A helper function to check if the app user config is multiple users or
// single user. It can be used with or without a reference user model.
// In case no user model is provided, we use the registry to get any of the
// user model by type. The relation with AccessToken is used to check
// if polymorphism is used, and thus if multiple users.
function _isMultipleUsers(userModel) {
var oneOfUserModels = userModel || _this.registry.getModelByType('User');
var accessTokensRel = oneOfUserModels.relations.accessTokens;
return !!(accessTokensRel && accessTokensRel.polymorphic);
}
};
Copy link
Contributor

@ebarault ebarault Mar 7, 2017

Choose a reason for hiding this comment

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

this is the helper to check if the app user config is multiple users or single user
it can be used with or without a reference user model.
in case no user model is provided, we use the registry to get any of the user model by type
the relation with AccessToken is used to check if polymorphism is used, and thus if multiple users

Copy link
Member

@bajtos bajtos Mar 8, 2017

Choose a reason for hiding this comment

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

Could you please capture this description in a code comment, preferably using jsdoc format?

Copy link
Contributor

@ebarault ebarault Mar 8, 2017

Choose a reason for hiding this comment

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

👍


Role.registerResolver(Role.AUTHENTICATED, function(role, context, callback) {
Expand Down
10 changes: 10 additions & 0 deletions lib/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,16 @@ app._verifyAuthModelRelations = function() {

function verifyUserRelations(Model) {
const hasManyTokens = Model.relations && Model.relations.accessTokens;

// display a temp warning message for users using multiple users config
if (hasManyTokens.polymorphic) {
console.warn(
'The app configuration follows the multiple user models setup ' +
'as described in http://ibm.biz/setup-loopback-auth',
'The built-in role resolver $owner is not currently compatible ' +
'with this configuration and should not be used in production.');
}

if (hasManyTokens) return;

const relationsConfig = Model.settings.relations || {};
Expand Down
Loading