Skip to content

Correct PrincipalType for multiple extension of User models #3823

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
akki-ng wants to merge 3 commits into from
Closed

Correct PrincipalType for multiple extension of User models #3823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0da144
Correct pricipalType to be added into principals of access context
akki-ng Jan 19, 2018
b4cfc24
Temp fix for
akki-ng Jan 19, 2018
a42c241
Test case added to Check correct principalType for users belonging to…
Mar 8, 2018
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
4 changes: 4 additions & 0 deletions common/models/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,10 @@ module.exports = function(Role) {
process.nextTick(function() {
callback(null, matches(modelId, userId));
});
}else {
process.nextTick(function() {
callback(null, false);
});
Copy link
Member

@bajtos bajtos May 18, 2018

Choose a reason for hiding this comment

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

This will not work in the following case: one user model owns another user mode, e.g. a Seller owns the business account of Customer.

}
return callback.promise;
}
Expand Down
5 changes: 3 additions & 2 deletions lib/access-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ function AccessContext(context) {

var token = this.accessToken || {};

if (token.userId != null) {
this.addPrincipal(Principal.USER, token.userId);
if (token.userId) {
const userPrincipalType = token.principalType || Principal.USER;
this.addPrincipal(userPrincipalType, token.userId);
Copy link
Member

@bajtos bajtos Mar 20, 2018

Choose a reason for hiding this comment

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

I find this change suspicions - I would expect that token.principalType was already added on line 84 above.

Having said that, this change does fix certain bugs like e.g. #3829, so it's not without merit. I need to build a better understanding of this part of our codebase to be able to comprehend impacts of this change in a wider context.

Copy link
Member

@bajtos bajtos Mar 20, 2018

Choose a reason for hiding this comment

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

Never mind, the line 84 was added back in 2013, before support for multiple user models were introduced. I suspect that code path was actually never used.

Copy link
Author

@akki-ng akki-ng Mar 20, 2018

Choose a reason for hiding this comment

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

@bajtos You are right about line 84, but this block of code is neglecting the principal type associated with the accessToken,

loopback/lib/access-context.js

Lines 47 to 85 in 6ddf268

context = context || {};
assert(context.registry,
'Application registry is mandatory in AccessContext but missing in provided context');
this.registry = context.registry;
this.principals = context.principals || [];
var model = context.model;
model = ('string' === typeof model) ? this.registry.getModel(model) : model;
this.model = model;
this.modelName = model && model.modelName;
this.modelId = context.id || context.modelId;
this.property = context.property || AccessContext.ALL;
this.method = context.method;
this.sharedMethod = context.sharedMethod;
this.sharedClass = this.sharedMethod && this.sharedMethod.sharedClass;
if (this.sharedMethod) {
this.methodNames = this.sharedMethod.aliases.concat([this.sharedMethod.name]);
} else {
this.methodNames = [];
}
if (this.sharedMethod) {
this.accessType = this.model._getAccessTypeForMethod(this.sharedMethod);
}
this.accessType = context.accessType || AccessContext.ALL;
assert(loopback.AccessToken,
'AccessToken model must be defined before AccessContext model');
this.accessToken = context.accessToken || loopback.AccessToken.ANONYMOUS;
var principalType = context.principalType || Principal.USER;
var principalId = context.principalId || undefined;
var principalName = context.principalName || undefined;
if (principalId != null) {
this.addPrincipal(principalType, principalId, principalName);
}

I think we will need to correct the whole block for a holistic solution

Copy link
Author

@akki-ng akki-ng Mar 20, 2018

Choose a reason for hiding this comment

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

My initial thought is, if line 84 was supposed to set the correct principal type then line 47 is dumping the empty object in some corner case.

}
if (token.appId != null) {
this.addPrincipal(Principal.APPLICATION, token.appId);
Expand Down
46 changes: 45 additions & 1 deletion test/multiple-user-principal-types.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('Multiple users with custom principalType', function() {

var commonCredentials = {email: 'foo@bar.com', password: 'bar'};
var app, OneUser, AnotherUser, AccessToken, Role,
userFromOneModel, userFromAnotherModel, userRole, userOneBaseContext;
userFromOneModel, userFromAnotherModel, accessTokenForUserFromOneModel, accessTokenForUserFromAnotherModel, userRole, userOneBaseContext;

beforeEach(function setupAppAndModels() {
// create a local app object that does not share state with other tests
Expand Down Expand Up @@ -213,6 +213,50 @@ describe('Multiple users with custom principalType', function() {
});

describe('getUser()', function() {
it("Check correct principalType for users belonging to different user models", function() {
Promise.all([
OneUser.login(commonCredentials),
AnotherUser.login(commonCredentials)
]).spread(function(t1, t2) {

accessTokenForUserFromOneModel = t1;
accessTokenForUserFromAnotherModel = t2;


const accessContextForUserFromOneModel = new AccessContext({registry: OneUser.registry, accessToken: accessTokenForUserFromOneModel});
const accessContextForUserFromAnotherModel = new AccessContext({registry: AnotherUser.registry, accessToken: accessTokenForUserFromAnotherModel});

var user1 = accessContextForUserFromOneModel.getUser();
expect(user1).to.eql({
id: userFromOneModel.id,
principalType: OneUser.modelName,
});

var user2 = accessContextForUserFromAnotherModel.getUser();
expect(user2).to.eql({
id: userFromAnotherModel.id,
principalType: AnotherUser.modelName,
});

})


return Promise.try(function() {
addToAccessContext([
{type: Principal.ROLE},
{type: Principal.APP},
{type: Principal.SCOPE},
{type: OneUser.modelName, id: userFromOneModel.id},
]);
var user = accessContext.getUser();
expect(user).to.eql({
id: userFromOneModel.id,
principalType: OneUser.modelName,
});
});

})

it('returns user although principals contain non USER principals',
function() {
return Promise.try(function() {
Expand Down