Skip to content

Allow multiple ownership relations #3106

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

Closed
pierreclr wants to merge 5 commits into from
Closed

Allow multiple ownership relations #3106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b091ea2
NEW fetch belongsTo relation first before resolving role
pierreclr Jan 16, 2017
39c68af
Remove console.log
pierreclr Jan 16, 2017
0c1e15b
NEW add unit test for handling multi owners role
pierreclr Jan 19, 2017
de8cc15
FIX deny owner resolve if no userId in request
pierreclr Jan 19, 2017
8d68b85
Remove describe only
pierreclr Jan 19, 2017
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
48 changes: 28 additions & 20 deletions common/models/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ module.exports = function(Role) {
Role.isOwner = function isOwner(modelClass, modelId, userId, callback) {
assert(modelClass, 'Model class is required');
debug('isOwner(): %s %s userId: %s', modelClass && modelClass.modelName, modelId, userId);
// No userId is present

// If no requester id, deny the resolver
Copy link
Member

@bajtos bajtos Jan 19, 2017

Choose a reason for hiding this comment

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

I think this block should be preserved. If there is no userId provided by the request context, then there is no point in running any checks - they must all fail.

Copy link
Contributor Author

@pierreclr pierreclr Jan 19, 2017

Choose a reason for hiding this comment

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

Sorry, done in de8cc15

if (!userId) {
process.nextTick(function() {
callback(null, false);
Expand All @@ -193,34 +194,41 @@ module.exports = function(Role) {
if (callback) callback(err, false);
return;
}
debug('Model found: %j', inst);
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;
}

// 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);
if (inst[r](processRelatedUser)) return callback(null, true);
Copy link
Member

@bajtos bajtos Jan 19, 2017

Choose a reason for hiding this comment

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

inst[r] is async function that may not return immediately, I am surprised this code actually works.

I think we should use async.some() to find whether any of the relation grants the owner role.

async.some(
  Object.keys(modelClass.relations),
  function(r, next) {
    var rel = modelClass.relations[r];
    // ...
    next(null, matches(user.id, userId));
  },
  function(err, matchFound) {
    if (err) return callback(err);
    if (matchFound) return callback(null, matchFound);
    // handle userId/owner properties
  });

Copy link
Contributor Author

@pierreclr pierreclr Jan 19, 2017

Choose a reason for hiding this comment

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

Indeed, something is wrong here

}

debug('No matching belongsTo relation found for model %j and user: %j', modelId, userId);
if (callback) callback(null, false);
}

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

// Checking the userId or owner field for resolving owner role
// is now done after fetching belongsTo relation to make possible
// to have multiple resolver role
var ownerId = inst.userId || inst.owner;
// Ensure ownerId exists and is not a function/relation
if (ownerId && 'function' !== typeof ownerId) {
Copy link
Contributor Author

@pierreclr pierreclr Jan 19, 2017

Choose a reason for hiding this comment

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

@bajtos Concerning your comment on backward compatibility, the fallback is here : I choose to first check the belongsTo relations and then fallback to userId / owner fields. So no problem of backward compatibility in case userId exist without belongsTo relation. However I feel not comfortable with that because it means that you can resolve the owner role without having any belongsTo relation defined (which was already the case)

Copy link
Member

@bajtos bajtos Jan 23, 2017

Choose a reason for hiding this comment

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

However I feel not comfortable with that because it means that you can resolve the owner role without having any belongsTo relation defined (which was already the case)

While the scenario may look wrong to us, it is supported by the current LoopBack version, thus there may be existing application relying on it, and therefore we need to preserve it for backwards compatibility.

if (callback && matches(ownerId, userId)) callback(null, true);
return;
}

// Finally DENY the owner role after sending
if (callback) callback(null, false);
});
};

Expand Down
52 changes: 52 additions & 0 deletions test/role.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,58 @@ describe('role model', function() {
});
});

it('should be able to handle multiple owners roles', function(done) {
var Message = app.registry.createModel('Message', {
name: String,
userId: Number,
senderId: Number,
}, {
relations: {
user: {
type: 'belongsTo',
model: 'User',
foreignKey: 'userId',
},
sender: {
type: 'belongsTo',
model: 'User',
foreignKey: 'senderId',
},
},
});

app.model(Message, {dataSource: 'db'});

User.create([
{name: 'pierre', email: 'pierre@xyz.com', password: 'foobar'},
{name: 'henry', email: 'henry@xyz.com', password: 'foobar'},
], function(err, users) {
if (err) return done(err);
Message.create([
{name: 'message1', userId: users[0].id, senderId: users[1].id},
{name: 'message2'},
], function(err, messages) {
var role1 = {
principalType: ACL.USER, principalId: users[0].id,
model: Message, id: messages[0].id,
};
var role2 = {
principalType: ACL.USER, principalId: users[1].id,
model: Message, id: messages[0].id,
};
Role.isInRole(Role.OWNER, role1, function(err, yes) {
if (err) return done(err);
assert(yes);
Role.isInRole(Role.OWNER, role2, function(err, yes) {
if (err) return done(err);
assert(yes);
done();
});
});
});
});
});

describe('isMappedToRole', function() {
var user, app, role;

Expand Down