Skip to content

propagate resolved roles in remoting context #2957

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
Mar 20, 2017
Merged

propagate resolved roles in remoting context #2957

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
118 changes: 92 additions & 26 deletions common/models/acl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ var g = require('../../lib/globalize');
var loopback = require('../../lib/loopback');
var utils = require('../../lib/utils');
var async = require('async');
var extend = require('util')._extend;
var assert = require('assert');
var debug = require('debug')('loopback:security:acl');

Expand Down Expand Up @@ -204,11 +205,12 @@ module.exports = function(ACL) {
/*!
* Resolve permission from the ACLs
* @param {Object[]) acls The list of ACLs
* @param {Object} req The request
* @returns {AccessRequest} result The effective ACL
* @param {AccessRequest} req The access request
* @returns {AccessRequest} result The resolved access request
*/
ACL.resolvePermission = function resolvePermission(acls, req) {
if (!(req instanceof AccessRequest)) {
req.registry = this.registry;
req = new AccessRequest(req);
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

as I did with AccessContext objects, I propose to add the Registry instance to the AccessRequest definition, so we can access models from that class, this is required by the new settleDefaultPermission method

Copy link

@ejvaitkus ejvaitkus Apr 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

When specifying an "arg": "options" in a common/models/xxx.json 'accepts' section, we ran into problems, finding 'authorizedRoles: {}' added to what was intended to be a list of options to pass to a system call, causing problems since we are simply passing the options values forward (in which case authorizedRoles becomes an invalid option). Our workaround was to rename the arg from "options" to something else (eg "lsfOptions". It would be more convenient to not have to worry about this; without knowing the inner workings, having the authorizedRoles in some more uniquely named structure would help the situation.

Copy link
Contributor Author

@ebarault ebarault Apr 28, 2017

Choose a reason for hiding this comment

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

hi @ejvaitkus : i'm afraid it's not specific to the authorizedRoles property as this remote method argument is also used to pass remoting context properties downstream to hooks and methods. see https://loopback.io/doc/en/lb3/Using-current-context.html

it's on purpose that the generic arg options is used as is in LB3.

}
// Sort by the matching score in descending order
Expand Down Expand Up @@ -250,9 +252,16 @@ module.exports = function(ACL) {
debug('with score:', acl.score(req));
});
}
var res = new AccessRequest({
model: req.model,
property: req.property,
accessType: req.accessType,
permission: permission || ACL.DEFAULT,
registry: this.registry});

// Elucidate permission status if DEFAULT
res.settleDefaultPermission();

var res = new AccessRequest(req.model, req.property, req.accessType,
permission || ACL.DEFAULT);
return res;
};

Expand Down Expand Up @@ -316,8 +325,8 @@ module.exports = function(ACL) {
* @param {String} property The property/method/relation name.
* @param {String} accessType The access type.
* @callback {Function} callback Callback function.
* @param {String|Error} err The error object
* @param {AccessRequest} result The access permission
* @param {String|Error} err The error object.
* @param {AccessRequest} result The resolved access request.
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

the former definition is very vague

*/
ACL.checkPermission = function checkPermission(principalType, principalId,
model, property, accessType,
Expand All @@ -332,10 +341,11 @@ module.exports = function(ACL) {
var accessTypeQuery = (accessType === ACL.ALL) ? undefined :
{inq: [accessType, ACL.ALL, ACL.EXECUTE]};

var req = new AccessRequest(model, property, accessType);
var req = new AccessRequest({model, property, accessType, registry: this.registry});

Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

adding registry to AccessRequest, going to one object param

var acls = this.getStaticACLs(model, property);

// resolved is an instance of AccessRequest
var resolved = this.resolvePermission(acls, req);
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

further to commenting, i'd like to ask if ok (here or latter) to switch the name of the variable: resolved is just vague. This should be called something with accessRequest in it to make further code maintenance easier

Copy link
Member

@bajtos bajtos Mar 17, 2017

Choose a reason for hiding this comment

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

Good idea. Can we leave it out of scope of this pull request though, to make it easier for me to review the already long list of changes?


if (resolved && resolved.permission === ACL.DENY) {
Expand All @@ -355,11 +365,8 @@ module.exports = function(ACL) {
return callback(err);
}
acls = acls.concat(dynACLs);
// resolved is an instance of AccessRequest
resolved = self.resolvePermission(acls, req);
if (resolved && resolved.permission === ACL.DEFAULT) {
var modelClass = self.registry.findModel(model);
resolved.permission = (modelClass && modelClass.settings.defaultPermission) || ACL.ALLOW;
}
return callback(null, resolved);
});
return callback.promise;
Expand All @@ -377,30 +384,63 @@ module.exports = function(ACL) {
}
};

// NOTE Regarding ACL.isAllowed() and ACL.prototype.isAllowed()
// Extending existing logic, including from ACL.checkAccessForContext() method,
// ACL instance with missing property `permission` are not promoted to
// permission = ACL.DEFAULT config. Such ACL instances will hence always be
// inefective

/**
* Test if ACL's permission is ALLOW
* @param {String} permission The permission to test, expects one of 'ALLOW', 'DENY', 'DEFAULT'
* @param {String} defaultPermission The default permission to apply if not providing a finite one in the permission parameter
* @returns {Boolean} true if ACL permission is ALLOW
*/
ACL.isAllowed = function(permission, defaultPermission) {
if (permission === ACL.DEFAULT) {
permission = defaultPermission || ACL.ALLOW;
}
return permission !== loopback.ACL.DENY;
};
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.

I changed the implem of this new helper following our discussion so to honour the defaultPermission setting for the given model

Copy link
Contributor Author

@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.

following your proposal I added a 2 params flavor of the isAllowed method on model itself
Although it is not used as of now, see my following comments
i also captured in a comment the rationale on how to treat undefined ACL permission param


/**
* Test if ACL's permission is ALLOW
* @param {String} defaultPermission The default permission to apply if missing in ACL instance
* @returns {Boolean} true if ACL permission is ALLOW
*/
ACL.prototype.isAllowed = function(defaultPermission) {
return this.constructor.isAllowed(this.permission, defaultPermission);
};

/**
* Check if the request has the permission to access.
* @options {Object} context See below.
* @options {AccessContext|Object} context
* An AccessContext instance or a plain object with the following properties.
* @property {Object[]} principals An array of principals.
* @property {String|Model} model The model name or model class.
* @property {*} id The model instance ID.
* @property {*} modelId The model instance ID.
* @property {String} property The property/method/relation name.
* @property {String} accessType The access type:
* READ, REPLICATE, WRITE, or EXECUTE.
* @param {Function} callback Callback function
* READ, REPLICATE, WRITE, or EXECUTE.
* @callback {Function} callback Callback function
* @param {String|Error} err The error object.
* @param {AccessRequest} result The resolved access request.
*/

ACL.checkAccessForContext = function(context, callback) {
if (!callback) callback = utils.createPromiseCallback();
var self = this;
self.resolveRelatedModels();
var roleModel = self.roleModel;

context.registry = this.registry;
if (!(context instanceof AccessContext)) {
context.registry = this.registry;
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

in fact there's no reason why to add the registry if context is already an instance of AccessContext, as we assert the presence of registry in the class constructor

context = new AccessContext(context);
}

var authorizedRoles = {};
var remotingContext = context.remotingContext;
var model = context.model;
var modelDefaultPermission = model && model.settings.defaultPermission;
var property = context.property;
var accessType = context.accessType;
var modelName = context.modelName;
Expand All @@ -414,7 +454,13 @@ module.exports = function(ACL) {
{inq: [ACL.REPLICATE, ACL.WRITE, ACL.ALL]} :
{inq: [accessType, ACL.ALL]};

var req = new AccessRequest(modelName, property, accessType, ACL.DEFAULT, methodNames);
var req = new AccessRequest({
model: modelName,
property,
accessType,
permission: ACL.DEFAULT,
methodNames,
registry: this.registry});

Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

dito

var effectiveACLs = [];
var staticACLs = self.getStaticACLs(model.modelName, property);
Expand Down Expand Up @@ -445,6 +491,9 @@ module.exports = function(ACL) {
function(err, inRole) {
if (!err && inRole) {
effectiveACLs.push(acl);
// add the role to authorizedRoles if allowed
if (acl.isAllowed(modelDefaultPermission))
authorizedRoles[acl.principalId] = true;
}
done(err, acl);
});
Expand All @@ -455,18 +504,31 @@ module.exports = function(ACL) {
async.parallel(inRoleTasks, function(err, results) {
if (err) return callback(err, null);

// resolved is an instance of AccessRequest
var resolved = self.resolvePermission(effectiveACLs, req);
if (resolved && resolved.permission === ACL.DEFAULT) {
resolved.permission = (model && model.settings.defaultPermission) || ACL.ALLOW;
}
debug('---Resolved---');
resolved.debug();

// set authorizedRoles in remotingContext options argument if
// resolved AccessRequest permission is ALLOW, else set it to empty object
authorizedRoles = resolved.isAllowed() ? authorizedRoles : {};
saveAuthorizedRolesToRemotingContext(remotingContext, authorizedRoles);
return callback(null, resolved);
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

call to helper to save the authorizedRoles to remoting context options

});
});
return callback.promise;
};

function saveAuthorizedRolesToRemotingContext(remotingContext, authorizedRoles) {
const options = remotingContext && remotingContext.args && remotingContext.args.options;
// authorizedRoles key/value map is added to the options argument only if
// the latter exists and is an object. This means that the feature's availability
// will depend on the app configuration
if (options && typeof options === 'object') { // null is object too
options.authorizedRoles = authorizedRoles;
}
}

Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

this method is not meant to be a class method, only a private helper

/**
* Check if the given access token can invoke the method
* @param {AccessToken} token The access token
Expand All @@ -489,9 +551,9 @@ module.exports = function(ACL) {
modelId: modelId,
});

this.checkAccessForContext(context, function(err, access) {
this.checkAccessForContext(context, function(err, accessRequest) {
if (err) callback(err);
else callback(null, access.permission !== ACL.DENY);
else callback(null, accessRequest.isAllowed());
});
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

the existing isAllowed instance method of AccessRequest should be used here for more clarity instead of checking value of nested permission

return callback.promise;
};
Expand All @@ -510,7 +572,9 @@ module.exports = function(ACL) {
* Resolve a principal by type/id
* @param {String} type Principal type - ROLE/APP/USER
* @param {String|Number} id Principal id or name
* @param {Function} cb Callback function
* @callback {Function} callback Callback function
* @param {String|Error} err The error object
* @param {Object} result An instance of principal (Role, Application or User)
*/
ACL.resolvePrincipal = function(type, id, cb) {
cb = cb || utils.createPromiseCallback();
Expand All @@ -530,7 +594,7 @@ module.exports = function(ACL) {
{where: {or: [{name: id}, {email: id}, {id: id}]}}, cb);
break;
default:
// try resolving a user model that matches principalType
// try resolving a user model with a name matching the principalType
var userModel = this.registry.findModel(type);
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

enhancing one of my own previous comment

if (userModel) {
userModel.findOne(
Expand All @@ -553,7 +617,9 @@ module.exports = function(ACL) {
* @param {String} principalType Principal type
* @param {String|*} principalId Principal id/name
* @param {String|*} role Role id/name
* @param {Function} cb Callback function
* @callback {Function} callback Callback function
* @param {String|Error} err The error object
* @param {Boolean} isMapped is the ACL mapped to the role
*/
ACL.isMappedToRole = function(principalType, principalId, role, cb) {
cb = cb || utils.createPromiseCallback();
Expand Down
68 changes: 57 additions & 11 deletions lib/access-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,29 @@ var debug = require('debug')('loopback:security:access-context');
* Access context represents the context for a request to access protected
* resources
*
* NOTE While the method expects an array of principals in the AccessContext instance/object,
* it also accepts a single principal defined with the following properties:
* ```js
* {
* // AccessContext instance/object
* // ..
* principalType: 'somePrincipalType', // APP, ROLE, USER, or custom user model name
* principalId: 'somePrincipalId',
* }
* ```
*
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

adding a comment to document the ability to pass a principal definition at root of the AccessContext instance
I did not referenced the prinicipalName property as i can't seem to find any usage of it in the entire codebase

* @class
* @options {Object} context The context object
* @options {AccessContext|Object} context An AccessContext instance or an object
* @property {Principal[]} principals An array of principals
* @property {Function} model The model class
* @property {String} modelName The model name
* @property {String} modelId The model id
* @property {*} modelId The model id
* @property {String} property The model property/method/relation name
* @property {String} method The model method to be invoked
* @property {String} accessType The access type
* @property {AccessToken} accessToken The access token
*
* @property {String} accessType The access type: READ, REPLICATE, WRITE, or EXECUTE.
* @property {AccessToken} accessToken The access token resolved for the request
* @property {RemotingContext} remotingContext The request's remoting context
* @property {Registry} registry The application or global registry
* @returns {AccessContext}
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

fixing the jsdoc (based on AccessContext definition) + Adding registry

* @constructor
*/
Expand Down Expand Up @@ -250,16 +262,23 @@ Principal.prototype.equals = function(p) {

/**
* A request to access protected resources.
* @param {String} model The model name
* @param {String} property
*
* The method can either be called with the following signature or with a single
* argument: an AccessRequest instance or an object containing all the required properties.
*
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

adding a comment to let know the one object param signature
(this should be the default doc, as for AccessContext)

* @class
* @options {String|AccessRequest|Object} model|req The model name,<br>
* or an AccessRequest instance/object.
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

this line is wrong

* @param {String} property The property/method/relation name
* @param {String} accessType The access type
* @param {String} permission The requested permission
* @param {String[]} methodNames The names of involved methods
* @param {Registry} registry The application or global registry
* @returns {AccessRequest}
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

jsdoc fixes

* @class
*/
function AccessRequest(model, property, accessType, permission, methodNames) {
function AccessRequest(model, property, accessType, permission, methodNames, registry) {
if (!(this instanceof AccessRequest)) {
return new AccessRequest(model, property, accessType);
return new AccessRequest(model, property, accessType, permission, methodNames);
Copy link
Member

@bajtos bajtos Mar 20, 2017

Choose a reason for hiding this comment

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

I think this will not work, because the new argument registry is not forwarded.

}
if (arguments.length === 1 && typeof model === 'object') {
// The argument is an object that contains all required properties
Expand All @@ -268,14 +287,19 @@ function AccessRequest(model, property, accessType, permission, methodNames) {
this.property = obj.property || AccessContext.ALL;
this.accessType = obj.accessType || AccessContext.ALL;
this.permission = obj.permission || AccessContext.DEFAULT;
this.methodNames = methodNames || [];
this.methodNames = obj.methodNames || [];
this.registry = obj.registry;
} else {
this.model = model || AccessContext.ALL;
this.property = property || AccessContext.ALL;
this.accessType = accessType || AccessContext.ALL;
this.permission = permission || AccessContext.DEFAULT;
this.methodNames = methodNames || [];
this.registry = registry;
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

adding registry, as in L291

}
// do not create AccessRequest without a registry
assert(this.registry,
'Application registry is mandatory in AccessRequest but missing in provided argument(s)');
}
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

asserting we have a registry in accessrequest instances


/**
Expand Down Expand Up @@ -308,6 +332,28 @@ AccessRequest.prototype.exactlyMatches = function(acl) {
return false;
};

/**
* Settle the accessRequest's permission if DEFAULT
* In most situations, the default permission can be resolved from the nested model
* config. An default permission can also be explicitly provided to override it or
* cope with AccessRequest instances without a nested model (e.g. model is '*')
*
* @param {String} defaultPermission (optional) the default permission to apply
*/

AccessRequest.prototype.settleDefaultPermission = function(defaultPermission) {
if (this.permission !== 'DEFAULT')
return;

var modelName = this.model;
if (!defaultPermission) {
var modelClass = this.registry.findModel(modelName);
defaultPermission = modelClass && modelClass.settings.defaultPermission;
}

this.permission = defaultPermission || 'ALLOW';
};

/**
* Is the request for access allowed?
Copy link
Contributor Author

@ebarault ebarault Mar 13, 2017

Choose a reason for hiding this comment

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

new accessrequest instance method to help keeping the code tidier.
see code jsdoc comment

Copy link
Member

@bajtos bajtos Mar 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

new accessrequest instance method to help keeping the code tidier.
see code jsdoc comment

This GitHub comment seems not relevant to me, am I missing something?

Copy link
Contributor Author

@ebarault ebarault Mar 20, 2017

Choose a reason for hiding this comment

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

no idea TBH

*
Expand Down
Loading