propagate resolved roles in remoting context #2957

ebarault · 2016-11-19T00:33:54Z

Description

For some advanced CRUD access control in remote methods, it is often required to get/check roles resolved (i.e. allowed) for a user against a method with Role.getRoles(context) or Role.isInRole(role, context).
In addition to clutter the remote methods / hooks code, it forces the app to run again through all static roles and registered dynamic role resolvers, inducing performance loss and extra calls to db.

Meanwhile, the exact same course of action is performed right when the Model.checkAccess() method is called to verify if a principal is allowed to access a protected resource :

Model.checkAccess(context) -> ACL.checkAccessForContext(context) -> acls.forEach(acl, ...) ->roleModel.isInRole(acl.principalId, context)

This PR proposes to add a new param (Array) in remoting context (name proposed: resolvedRoles, to be discussed) which is filled during Model Access Permission Checking phase with the roles for which the user is allowed to run the remote method.

an example of resolvedRoles is: ['admin', '$owner'] where admin is a custom role statically mapped to a principal, and $owner is a built-in smart role

resolvedRoles param can then be accessed in remote method and remote method hooks as ctx.req.remotingContext.resolvedRoles

To be noted: the resolvedRoles param is set in a way that the calling user cannot temper the value to fool the additional access control policies added by user in remote methods

Reviewers: please review/discuss the general idea before i provide the required tests

Related issues

None

Checklist

Code conforms with the style
guide
New tests added or existing tests modified to cover all changes

slnode · 2016-11-19T00:33:55Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2016-11-19T00:33:55Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:33:55Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:33:55Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:33:55Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:49:14Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:49:14Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:49:14Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:49:14Z

Can one of the admins verify this patch?

slnode · 2016-11-19T00:54:48Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

bajtos · 2016-11-22T14:42:21Z

@ebarault thank you for the pull request!

@raymondfeng I feel you are the best person to review this proposal at high level, could you PTAL?

bajtos · 2016-11-22T14:42:52Z

@slnode ok to test

ebarault · 2016-11-22T17:38:43Z

just rebased, but PR linter states 'PR out of date, needs to be rebased'

bajtos

@ebarault I am afraid something went wrong during your rebase. The patch contains a bunch of eslint-related changes now, see https://github.com/strongloop/loopback/pull/2957/files

It may be best to simply hard-reset your feature branch to the latest master and then add your changes again on top of that.

$ git fetch strongloop
$ git reset --hard strongloop/master
$ git push -f

Could you PTAL?

bajtos · 2016-12-01T14:57:20Z

.eslintrc

@@ -5,6 +5,7 @@
      "ignoreComments": true,
      "ignoreUrls": true,
      "ignorePattern": "^\\s*var\\s.+=\\s*(require\\s*\\()|(/)"
-    }]
+    }],
+  "no-unused-expressions": "off",


Please revert.

bajtos · 2016-12-01T14:57:41Z

Gruntfile.js

@@ -4,6 +4,7 @@
 // License text available at https://opensource.org/licenses/MIT

 /*global module:false*/
+'use strict';


Changes in this file looks unrelated to me, could you please revert?

bajtos · 2016-12-01T15:03:49Z

@ebarault

This PR proposes to add a new param (Array) in remoting context (name proposed: resolvedRoles, to be discussed) which is filled during Model Access Permission Checking phase with the roles for which the user is allowed to run the remote method.

I am little bit confused here. As far as I understand LoopBack's ACL system, there may be several different role arrays:

a list of roles the current user belongs to
a list of roles allowed to run the remote method
the intersection of the above two lists

What is stored in your new context property resolvedRoles?

ebarault · 2016-12-01T15:29:23Z

@bajtos
sorry about the mess with rebase, it went through at the moment of the large eslint refactor, it must have slipped in, i'll scratch and start over
about

about the pr intention: you're right, I should have made it clearer: the one i'm focusing here is the 3rd one: intersection of roles allowed to run the remote method and roles the current user belongs to

-or- rephrasing in my terms, to make sure we get each other: the roles for which the user is authorized to run a given remote method, which is in essence the exact purpose of the following course of actions:
Model.checkAccess(context) -> ACL.checkAccessForContext(context) -> acls.forEach(acl, ...) ->roleModel.isInRole(acl.principalId, context) (i can detail the exact points of references of each call in the codebase if you will)

so when this set of tasks is achieved, for each acl covering the method the question has been solved "is the user in a role that can run the method?" : the general idea is to keep track of this result for further use in methods code or hooks to perform advanced CRUD.

A very basic example of when this can be useful: say a user is allowed to run a method by 2 roles, those 2 roles having conceptually very different access levels (custom role $organization:owner on one hand, and built-in smart role $authenticated) : inside that single method implementation you might want to treat the user differently from just an authenticated user, compared to an organization admin. Keeping track of the roleResolving status and making it accessible in the methods code greatly simplifies this process by avoiding running the role resolvers again for each role of interest for the method.

slnode · 2016-12-02T08:04:23Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2016-12-02T08:04:28Z

Can one of the admins verify this patch?

slnode · 2016-12-02T08:04:28Z

Can one of the admins verify this patch?

bajtos

Reviewed, PTAL.

bajtos · 2017-02-09T14:29:15Z

common/models/acl.js

+   */
+  ACL.prototype.isAllowed = function(defaultPermission) {
+    var permission = this.permission;
+    if (permission === ACL.DEFAULT) {


What a mess :(

I am afraid we don't have any specification (either as a documentation or a executable test) for what's the correct behaviour, therefore we have to rely on what we can see elsewhere in the code and try to not break it.

Could you please capture your reasoning in a code comment, so that future readers understand better why isAllowed is implemented this way?

bajtos · 2017-02-09T14:31:21Z

common/models/acl.js

@@ -399,8 +411,16 @@ module.exports = function(ACL) {
    if (!(context instanceof AccessContext)) {
      context = new AccessContext(context);
    }
+    // initialize the authorizedRoles array in RemotingContext args options object
+    var remotingContext = context && context.remotingContext;


I think context is always set, see L410-412 above, therefore the check context && can be removed.

bajtos · 2017-02-09T14:35:06Z

common/models/acl.js

+    var remotingContext = context && context.remotingContext;
+    if (remotingContext) {
+      var options = remotingContext.args.options || {};
+      remotingContext.args.options = extend(options, {authorizedRoles: []});


I think we shouldn't be adding args.options property if the invoked method does not have any options argument. Also what if the remote method expects options of a different type, e.g. an array or an object?

I am proposing the following:

const authorizedRoles = []; if (remotingContext) { const options = remotingContext.args.options; if (options && typeof options === 'object') { // null is object too options.authorizedRoles = authorizedRoles; } }

Maybe move the code modifying remotingContext to the very end of this method?

fair enough, minus the next comment with regard to the standalone authorizedRoles array

bajtos · 2017-02-09T14:35:51Z

common/models/acl.js

+      var options = remotingContext.args.options || {};
+      remotingContext.args.options = extend(options, {authorizedRoles: []});
+    }
+    var authorizedRoles = [];


I think this should be set to the same array as in remotingContext.args.options. Unless you want to prevent remote method from tampering with the array value used inside ACL.checkAccessForContext?

actually yes, it's some sort of security failsafe : i prepare an array of permissions on one side and initialize the content of options.authorizedRoles to [] on the other side.
Then only if the result of checkAccessForContext is ALLOW and set options.authorizedRoles to the value of the array of permissions we've computed. I captured the rationale in a comment

bajtos · 2017-02-09T14:37:44Z

common/models/acl.js

@@ -419,13 +439,19 @@ module.exports = function(ACL) {
    var effectiveACLs = [];
    var staticACLs = self.getStaticACLs(model.modelName, property);

+    // push unique roles in provided  array
+    var addRole = function(roles, role) {
+      if (role && roles.indexOf(role) === -1) {


Should we perhaps use an object (a key/value map) instead of an array? That way this can be inlined at calling places as authorizedRoles[role] = true. I think it all depends on how do you expect applications to consume this API. If they will be looking up authorized roles by name, then a key/value map is probably easier to use.

bajtos · 2017-02-09T15:21:32Z

test/acl.test.js

 var loopback = require('../index');
 var Scope = loopback.Scope;
 var ACL = loopback.ACL;
 var request = require('supertest');
+var Promise = require('bluebird');
+var supertest = require('supertest-as-promised')(Promise);


FWIW, the recent versions of supertest provide Promise API natively, supertest-as-promised was deprecated. Let's wait for #3196 to be landed.

#3196 was landed.

bajtos · 2017-02-09T15:25:27Z

test/acl.test.js

+    request = supertest(app);
+
+    // attaching base models
+    ['User', 'Role', 'RoleMapping', 'ACL', 'AccessToken'].map(setupModel);


app.enableAuth({ dataSource: 'db' }) automatically attaches all required models to the given datasource.

any app model can be accessed via app.models, so it should be enough to set var models = app.models;

app.enableAuthentication({dataSource: 'db'}); models = app.models;

This way, setupModel shouldn't be needed at all.

bajtos · 2017-02-09T15:29:49Z

test/acl.test.js

+    app.model(models.MyTestModel, {dataSource: 'db'});
+
+    // beforeRemote hook on MyTestModel, as a promise
+    models.MyTestModel.onBeforeRemote = new Promise(function(resolve, reject) {


Uff, I find this very confusing. Can you simplify to the following?

models.MyTestModel.beforeRemote('find', function(ctx, notUsed,next) { models.MyTestModel.lastRemotingContext = ctx; next(); });

Unless I am missing something here?

bajtos · 2017-02-09T15:31:12Z

test/acl.test.js

+
+    // creating a user, a role and a rolemapping binding that user with that role
+    return Promise.all([
+      models.User.create({username: 'myUser', email: 'myuser@email.com', password: 'password'}),


We use @example.com addresses, and usually set password to pass in the tests.

See http://loopback.io/doc/en/contrib/style-guide.html#email-examples, although it shows password set to bar (don't know why).

bajtos · 2017-02-09T15:32:07Z

test/model.test.js

@@ -897,7 +897,7 @@ describe.onServer('Remote Methods', function() {
      request(app).get('/TestModels/saveOptions')
        .expect(204, function(err) {
          if (err) return done(err);
-          expect(actualOptions).to.eql({accessToken: null});


Why is this change needed? The original code should produce a more helpful error message on failure.

so... that's funny : you asked 29 days ago why this change was required, and i could not remember why. It happens that the test as-is fails since actualOptions is no longer equal to {accessToken: null}but to {accessToken: null, authorizedRoles: {...}}.
So i reverted my fix, and then you had the error you mentioned in your comment here

i rolled the fix back, testing if accessToken is null is actually what we are testing here

Sounds good.

Is it worth adding a comment explaining that actualOptions contains other properties too?

Alternatively, can we use expect(actualOptions).to.include({accessToken: null})?

👍 used this latter form

bajtos · 2017-03-06T17:09:38Z

@ebarault ping - I am just wondering, what's the status of this patch?

ebarault · 2017-03-06T17:11:33Z

i still plan to finish it in the coming days as part of the in-remote-method access control toolbox

ebarault

@bajtos: I reviewed your comments/proposal, PTAL

ebarault · 2017-03-07T09:54:04Z

common/models/acl.js

+   */
+  ACL.prototype.isAllowed = function(defaultPermission) {
+    var permission = this.permission;
+    if (permission === ACL.DEFAULT) {


ebarault · 2017-03-07T09:54:07Z

common/models/acl.js

@@ -399,8 +411,16 @@ module.exports = function(ACL) {
    if (!(context instanceof AccessContext)) {
      context = new AccessContext(context);
    }
+    // initialize the authorizedRoles array in RemotingContext args options object
+    var remotingContext = context && context.remotingContext;


ebarault · 2017-03-07T10:01:42Z

common/models/acl.js

+    var remotingContext = context && context.remotingContext;
+    if (remotingContext) {
+      var options = remotingContext.args.options || {};
+      remotingContext.args.options = extend(options, {authorizedRoles: []});


fair enough, minus the next comment with regard to the standalone authorizedRoles array

ebarault · 2017-03-07T10:06:59Z

common/models/acl.js

+      var options = remotingContext.args.options || {};
+      remotingContext.args.options = extend(options, {authorizedRoles: []});
+    }
+    var authorizedRoles = [];


actually yes, it's some sort of security failsafe : i prepare an array of permissions on one side and initialize the content of options.authorizedRoles to [] on the other side.
Then only if the result of checkAccessForContext is ALLOW and set options.authorizedRoles to the value of the array of permissions we've computed. I captured the rationale in a comment

ebarault · 2017-03-07T10:19:49Z

common/models/acl.js

+        // add authorizedRoles to remotingContext if overall resolved permission status is ALLOW
+        if (resolved.permission === ACL.ALLOW && remotingContext) {
+          remotingContext.args.options.authorizedRoles = authorizedRoles;
+        };
        return callback(null, resolved);


ebarault · 2017-03-07T12:00:03Z

common/models/acl.js

+        // checkAccessForContext resolved permission status is ALLOW,
+        // else set it to empty object
+        authorizedRoles = permission === ACL.ALLOW ? authorizedRoles : {};
+        setAuthorizedRoles(authorizedRoles);


reduced down the whole setAuthorizedRoles logic to just these 2 lines in the main code block

ebarault · 2017-03-07T12:01:06Z

common/models/acl.js

+      if (options && typeof options === 'object') { // null is object too
+        options.authorizedRoles = authorizedRoles;
+      }
+    }


the related helper to perform check ups on context.remotingContext.args.options before proceeding

ebarault · 2017-03-07T12:01:14Z

test/acl.test.js

@@ -5,10 +5,13 @@

 'use strict';
 var assert = require('assert');
+var expect = require('./helpers/expect');


ebarault · 2017-03-07T12:12:15Z

common/models/acl.js

        if (resolved && resolved.permission === ACL.DEFAULT) {
-          resolved.permission = (model && model.settings.defaultPermission) || ACL.ALLOW;
+          permission = defaultPermission || ACL.ALLOW;
        }


you were proposing to share here the same logic as in ACL.isAllowed() but here were are not quite doing the same thing and not testing the same type of object:

resolved is not an instance of ACL but an instance of AccessRequest. I'm reluctant in using ACL.isAllowed() on the permission part of an AccessRequest instance. Proper way would be to use the existing AccessRequest.prototype.isAllowed after adapting it to accept a default permission

we are not testing if allowed, we are changing the permission type if actually DEFAULT, because it is not done either here or there which is used in model.js. This could be avoided and the code simplified/unified if were to align the implementation of AccessRequest.prototype.isAllowed

If you agree, i can track this as a different issue and address later

Uff, this goes too deep, beyond my current understanding. I'll leave this decision up to you to make.

Perhaps add a code comment explaining why we cannot use ACL.isAllowed() here?

👍 actually goes too deep to be discussed async i'm afraid ah ah
i'll figure out something

ebarault · 2017-03-07T12:14:58Z

common/models/acl.js

+        // set authorizedRoles in remotingContext options argument if
+        // checkAccessForContext resolved permission status is ALLOW,
+        // else set it to empty object
+        authorizedRoles = permission === ACL.ALLOW ? authorizedRoles : {};


then if we commit the changes discussed before, we can change this to
authorizedRoles = resolved.isAllowed() ? authorizedRoles : {};
which would be better

(no need to even pass the defaultPermission since the accessRequest has already the targeted model and hence the defaultPermission)

bajtos

The updated code looks pretty good! I left few more comments to address, I hope they should be minor and easy to fix.

Please fix npm test, right now it's failing:

Remote Methods Model.createOptionsFromRemotingContext sets empty options.accessToken for anonymous requests:
      Uncaught AssertionError: expected { Object (accessToken, authorizedRoles) } to deeply equal { accessToken: null }
      + expected - actual
       {
         "accessToken": [null]
      -  "authorizedRoles": {}
       }

(See e.g. https://travis-ci.org/strongloop/loopback/jobs/208560030)

bajtos · 2017-03-08T12:38:59Z

common/models/acl.js

@@ -456,15 +489,34 @@ module.exports = function(ACL) {
        if (err) return callback(err, null);

        var resolved = self.resolvePermission(effectiveACLs, req);
+        var permission = resolved && resolved.permission;
        if (resolved && resolved.permission === ACL.DEFAULT) {


I think this can be simplified to if (permission === ACL.DEFAULT, thoughts?

bajtos · 2017-03-08T12:40:40Z

common/models/acl.js

        if (resolved && resolved.permission === ACL.DEFAULT) {
-          resolved.permission = (model && model.settings.defaultPermission) || ACL.ALLOW;
+          permission = defaultPermission || ACL.ALLOW;
        }


Uff, this goes too deep, beyond my current understanding. I'll leave this decision up to you to make.

Perhaps add a code comment explaining why we cannot use ACL.isAllowed() here?

bajtos · 2017-03-08T12:47:45Z

common/models/acl.js

        return callback(null, resolved);
      });
    });
    return callback.promise;
+
+    // helpers
+    function setAuthorizedRoles(authorizedRoles) {


I am proposing to move this function out of ACL.checkAccessForContext and add remotingContext as a new parameter, so that the function does not require a closure from ACL.checkAccessForContext to access that reference.

There are two reasons for my proposal:

I personally consider such code easier to reason about, because it's easier to see what data is read/touched by setAuthorizedRoles

It's a also a (micro) performance optimisation. In the current implementation, V8 has to create a new function every time ACL.checkAccessForContext is called. I don't know what's the current status, but V8 was not able to optimise such short-living functions in the past. By moving the function out, there is only one function that's called many times, and therefore V8 can optimise the heck out of it.

I would also like to propose to use a different name, e.g. saveAuthorizedRolesToRemotingContext, and to omit the //helpers comment.

wouahh, thanks for the optimization trick, done

bajtos · 2017-03-08T12:48:28Z

common/models/acl.js

+    // helpers
+    function setAuthorizedRoles(authorizedRoles) {
+      var remotingContext = context.remotingContext;
+      const options = remotingContext && remotingContext.args.options;


Should we check that remotingContext.args is set too, just to be 100% sure?

bajtos · 2017-03-08T12:51:12Z

test/acl.test.js

+    .then(function() {
+      return request.get('/MyTestModels')
+        .set('X-Access-Token', accessToken.id)
+        .expect(200);


This code is repeated in all tests, can you extract a shared helper function please? E.g.

// usage .then(makeAuthorizedHttpRequest) // implementation function makeAuthorizedHttpRequest() { return request.get('/MyTestModels') .set(...) .expect(200); }

bajtos · 2017-03-08T12:52:21Z

test/acl.test.js

+    app = loopback({localRegistry: true, loadBuiltinModels: true});
+    app.use(loopback.rest());
+    app.set('remoting', {errorHandler: {debug: true, log: true}});
+    app.enableAuth();


Please remove in favour of L563.

bajtos · 2017-03-08T12:53:36Z

test/acl.test.js

+
+    // creating a custom model
+    models.MyTestModel = app.registry.createModel('MyTestModel');
+    app.model(models.MyTestModel, {dataSource: 'db'});


Uff, modifying app.models directly is not a good practice. I am proposing the following:

const MyTestModel = app.registry.createModel('MyTestModel'); app.model(MyTestModel, {dataSource: 'db'}); // models.MyTestModel was set by app.model() by now

bajtos · 2017-03-08T12:54:00Z

test/acl.test.js

+    app.model(models.MyTestModel, {dataSource: 'db'});
+
+    // capturing the value of the last remoting context
+    models.MyTestModel.beforeRemote('find', function(ctx, notUsed, next) {


Nitpick (feel free to ignore): unused instead of notUsed?

ebarault · 2017-03-13T08:05:17Z

@bajtos : i applied changes from last code review

NOTE

I also fixed a number of related erroneous jsdoc comments
I isolated in a dedicated commit an additional change that adds a new method settleDefaultPermission on AccessRequest instances, that helps keeping the code tidier. The method replaces the messy code used to set the AccessRequest's permission to the correct default value when initial value is DEFAULT.

More comments inlined.

ebarault · 2017-03-13T08:07:09Z

common/models/acl.js

   */
  ACL.resolvePermission = function resolvePermission(acls, req) {
    if (!(req instanceof AccessRequest)) {
+      req.registry = this.registry;
      req = new AccessRequest(req);


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

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.

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.

ebarault · 2017-03-13T08:10:40Z

common/models/acl.js

+      property: req.property,
+      accessType: req.accessType,
+      permission: permission || ACL.DEFAULT,
+      registry: this.registry});
    return res;


switching to one object param declaration when adding the registry as it reduces chances for further errors (adding things like (param1, param2, null, null, param5))

Note: this should the default documented option, as for AccessContext

ebarault · 2017-03-13T08:11:54Z

common/models/acl.js

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


the former definition is very vague

ebarault · 2017-03-13T08:12:24Z

common/models/acl.js

@@ -332,10 +337,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});



adding registry to AccessRequest, going to one object param

ebarault · 2017-03-13T08:13:46Z

common/models/acl.js


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

+    // resolved is an instance of AccessRequest
    var resolved = this.resolvePermission(acls, req);


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

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?

ebarault · 2017-03-13T08:32:19Z

lib/access-context.js

 * @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}


jsdoc fixes

ebarault · 2017-03-13T08:32:36Z

lib/access-context.js

@@ -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 line is wrong

ebarault · 2017-03-13T08:33:00Z

lib/access-context.js

  } 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;


adding registry, as in L291

ebarault · 2017-03-13T08:33:24Z

lib/access-context.js

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


asserting we have a registry in accessrequest instances

ebarault · 2017-03-13T08:34:11Z

lib/access-context.js

+  this.permission = defaultPermission || 'ALLOW';
+};
+
+/**
 * Is the request for access allowed?


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

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?

no idea TBH

bajtos

Thank you for the update, I think this is 99% ready.

Should we include getValueForRoleInAuthorizedRoles in this pull request?
PTAL at the last three comments related to tests

The rest can be fixed later in a new pull request.

For posterity, after online chat, we decided with @ebarault to land these changes in a single pull request, even though they are mixing refactoring/code cleanup with adding new features, which makes future code archaeology difficult.

bajtos · 2017-03-17T14:16:19Z

common/models/acl.js


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

+    // resolved is an instance of AccessRequest
    var resolved = this.resolvePermission(acls, req);


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?

bajtos · 2017-03-17T14:18:37Z

common/models/acl.js

-          var modelClass = self.registry.findModel(model);
-          resolved.permission = (modelClass && modelClass.settings.defaultPermission) || ACL.ALLOW;
-        }
+        resolved.settleDefaultPermission();


I am wondering why settleDefaultPermission is not called by resolePermission? What is the reasoning why a caller of resolvePermission would want to see unresolved ALC.DEFAULT?

AFAICT, L339/L345 is one of those places. I am not sure whether that's intentional.

Anyhow, let's leave this improvement out of scope of this pull request.

bajtos · 2017-03-17T14:22:04Z

common/models/acl.js

  /**
   * Check if the request has the permission to access.
-   * @options {Object} context See below.
+   * @options {AccessContext} context An AccessContext instance.


I think this is not correct, the method allows a plain object to be passed in this parameter. How about this?

@options {AccessContext|Object} context An AccessContext instance or a plain object with the following properties.

I think this whole jsdoc block should be mostly the same as we have for AccessContext constructor, where you have {AccessContext|Object} type AFAICT.

bajtos · 2017-03-17T14:23:06Z

common/models/acl.js

-   * @property {String} accessType The access type:
-   *   READ, REPLICATE, WRITE, or EXECUTE.
-   * @param {Function} callback Callback function
+   * @property {String} accessType The access type: READ, REPLICATE, WRITE, or EXECUTE.


Nitpick: Isn't this line too long? What is the benefit compared to the original solution? AFAIK, strong-docs supports multi-line parameter descriptions.

just rolled back

bajtos · 2017-03-17T14:25:50Z

common/models/acl.js

   * @property {Object[]} principals An array of principals.
-   * @property {String|Model} model The model name or model class.


I think we should revert this back to {String|Model} model. If the caller is passing AccessContext instance then they don't need a description of properties duplicated here. The property descriptions should be used when passing a plain data object for constructing a new AccessContext, in which case only model should be provided (see

loopback/lib/access-context.js

Lines 39 to 42 in 960e118

var model = context.model;

model = ('string' === typeof model) ? this.registry.getModel(model) : model;

this.model = model;

this.modelName = model && model.modelName;

).

did it and also removed modelName then
will come back to this latter, we should not be confusing one property for two different things

bajtos · 2017-03-17T14:33:58Z

common/models/acl.js

+  ACL.getValueForRoleInAuthorizedRoles = function(context) {
+    return true;
+  };
+


I am proposing to defer async version of this method until there are users asking for it.

bajtos · 2017-03-17T14:39:01Z

lib/access-context.js

+  this.permission = defaultPermission || 'ALLOW';
+};
+
+/**
 * Is the request for access allowed?


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?

bajtos · 2017-03-17T14:46:02Z

test/acl.test.js

@@ -157,6 +160,8 @@ describe('security ACLs', function() {
    acls = acls.map(function(a) { return new ACL(a); });

    var perm = ACL.resolvePermission(acls, req);
+    // remove the registry from AccessRequest instance to ease asserting
+    delete perm.registry;
    assert.deepEqual(perm, {model: 'account',


Instead of modifying the resolved object, how about using include assertion from chai to verify only a subset of model properties?

expect(perm).to.include({ model: 'account', // ... methodNames: [] });

It may not work because of issues that were fixed by chaijs/chai#761 (which was not released in a stable version yet), in which case it's ok to keep your implementation, but please add a TODO comment so that we can eventually clean this code.

👍 yeap this does not work ATM, i left a comment

bajtos · 2017-03-17T14:47:56Z

test/acl.test.js

+          },
+        }
+      );
+      // fix back the original method


This won't work when the expect call above fails.

Ideally, we should be creating a fresh app instance for each test and thus it should not matter whether a test changes built-in models. I think you are already doing that, aren't you?

The worser alternative is to move this fix from .then into .finally block.

bajtos · 2017-03-17T14:49:50Z

test/model.test.js

@@ -897,7 +897,7 @@ describe.onServer('Remote Methods', function() {
      request(app).get('/TestModels/saveOptions')
        .expect(204, function(err) {
          if (err) return done(err);
-          expect(actualOptions).to.eql({accessToken: null});


Sounds good.

Is it worth adding a comment explaining that actualOptions contains other properties too?

Alternatively, can we use expect(actualOptions).to.include({accessToken: null})?

bajtos · 2017-03-20T10:13:01Z

lib/access-context.js

  if (!(this instanceof AccessRequest)) {
-    return new AccessRequest(model, property, accessType);
+    return new AccessRequest(model, property, accessType, permission, methodNames);


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

bajtos · 2017-03-20T10:17:27Z

coverage/coveralls — Coverage decreased (-0.006%) to 89.295%

Looks like this decrease comes from the changes in lib/access-context.js, see https://coveralls.io/builds/10674265/source?filename=lib%2Faccess-context.js. I am not able to identify the cause of this decrease, it may be caused by new lines added to a branch that was not covered by any tests before (e.g. AccessContext/AccessRequest constructor).

I am proposing to ignore this tiny decrease in code coverage numbers.

ebarault

final review

ebarault · 2017-03-20T10:38:01Z

common/models/acl.js

-          var modelClass = self.registry.findModel(model);
-          resolved.permission = (modelClass && modelClass.settings.defaultPermission) || ACL.ALLOW;
-        }
+        resolved.settleDefaultPermission();


👍 very valid point, my initial intention was to put it in the constructor, but i leave this for later

ebarault · 2017-03-20T10:40:18Z

common/models/acl.js

  /**
   * Check if the request has the permission to access.
-   * @options {Object} context See below.
+   * @options {AccessContext} context An AccessContext instance.


ebarault · 2017-03-20T10:55:54Z

common/models/acl.js

+   * @property {String} accessType The access type: READ, REPLICATE, WRITE, or EXECUTE.
+   * @callback {Function} callback Callback function
+   * @param {String|Error} err The error object.
+   * @param {AccessRequest} result The resolved access request.
   */


👍 arff, you're right, TBH i find this so ugly... modelName should have been used when seeking to build an accesscontext by the model name...
I also removed modelName then, it's of no use here

ebarault · 2017-03-20T11:09:00Z

test/acl.test.js

@@ -157,6 +160,8 @@ describe('security ACLs', function() {
    acls = acls.map(function(a) { return new ACL(a); });

    var perm = ACL.resolvePermission(acls, req);
+    // remove the registry from AccessRequest instance to ease asserting
+    delete perm.registry;
    assert.deepEqual(perm, {model: 'account',


👍 yeap this does not work ATM, i left a comment

ebarault · 2017-03-20T11:12:22Z

test/model.test.js

@@ -897,7 +897,7 @@ describe.onServer('Remote Methods', function() {
      request(app).get('/TestModels/saveOptions')
        .expect(204, function(err) {
          if (err) return done(err);
-          expect(actualOptions).to.eql({accessToken: null});


👍 used this latter form

ebarault · 2017-03-20T11:14:07Z

common/models/acl.js

-          var modelClass = self.registry.findModel(model);
-          resolved.permission = (modelClass && modelClass.settings.defaultPermission) || ACL.ALLOW;
-        }
+        resolved.settleDefaultPermission();


ebarault · 2017-03-20T11:15:08Z

common/models/acl.js

   * @property {Object[]} principals An array of principals.
-   * @property {String|Model} model The model name or model class.


did it and also removed modelName then
will come back to this latter, we should not be confusing one property for two different things

ebarault · 2017-03-20T11:15:59Z

common/models/acl.js

-   * @property {String} accessType The access type:
-   *   READ, REPLICATE, WRITE, or EXECUTE.
-   * @param {Function} callback Callback function
+   * @property {String} accessType The access type: READ, REPLICATE, WRITE, or EXECUTE.


just rolled back

ebarault · 2017-03-20T11:16:54Z

lib/access-context.js

+  this.permission = defaultPermission || 'ALLOW';
+};
+
+/**
 * Is the request for access allowed?


no idea TBH

Adds an authorizedRoles object to remotingContext.args.options which contains all the roles (static and dynamic) that are granted to the user when performing a request through strong-remoting to an app with authentication enabled. The authorizedRoles object for example looks like: { $everyone: true, $authenticated: true, myRole: true } NOTE: this pr also covers a number of jsdoc fixes as well as refactoring in ACL.js and access-context.js

bajtos · 2017-03-20T12:34:21Z

Landed 🎉

crandmck added the #community contribution label Nov 19, 2016

ebarault closed this Nov 19, 2016

ebarault force-pushed the propagate-resolved-roles-in-context branch from 4139d7f to 61c0c85 Compare November 19, 2016 00:49

crandmck removed the #community contribution label Nov 19, 2016

ebarault reopened this Nov 19, 2016

crandmck added the #community contribution label Nov 19, 2016

bajtos assigned raymondfeng and bajtos Nov 22, 2016

bajtos added the loopback-auth label Nov 23, 2016

bajtos reviewed Dec 1, 2016

View reviewed changes

ebarault closed this Dec 2, 2016

ebarault force-pushed the propagate-resolved-roles-in-context branch from 5f1b896 to a34f321 Compare December 2, 2016 08:04

bajtos suggested changes Feb 9, 2017

View reviewed changes

ebarault force-pushed the propagate-resolved-roles-in-context branch 2 times, most recently from 3fa19b4 to 6738cf4 Compare March 7, 2017 11:54

ebarault commented Mar 7, 2017

View reviewed changes

bajtos suggested changes Mar 8, 2017

View reviewed changes

bajtos mentioned this pull request Mar 8, 2017

Fix OWNER role to handle multiple relations #3140

Merged

ebarault force-pushed the propagate-resolved-roles-in-context branch 9 times, most recently from 990a608 to 7906fb3 Compare March 10, 2017 18:21

ebarault commented Mar 13, 2017

View reviewed changes

ebarault mentioned this pull request Mar 13, 2017

enhance context passing in OWNER role resolver #3250

Closed

bajtos suggested changes Mar 17, 2017

View reviewed changes

ebarault force-pushed the propagate-resolved-roles-in-context branch from 7906fb3 to 8a34495 Compare March 20, 2017 08:22

bajtos reviewed Mar 20, 2017

View reviewed changes

ebarault commented Mar 20, 2017

View reviewed changes

ebarault force-pushed the propagate-resolved-roles-in-context branch from 8a34495 to 8aa98a8 Compare March 20, 2017 11:29

bajtos approved these changes Mar 20, 2017

View reviewed changes

bajtos merged commit cb7e211 into strongloop:master Mar 20, 2017

		* @property {Object[]} principals An array of principals.
		* @property {String\|Model} model The model name or model class.

	var model = context.model;
	model = ('string' === typeof model) ? this.registry.getModel(model) : model;
	this.model = model;
	this.modelName = model && model.modelName;

propagate resolved roles in remoting context #2957

propagate resolved roles in remoting context #2957

Conversation

ebarault commented Nov 19, 2016 • edited

Description

Related issues

Checklist

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

slnode commented Nov 19, 2016

bajtos commented Nov 22, 2016

bajtos commented Nov 22, 2016

ebarault commented Nov 22, 2016

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Dec 1, 2016

ebarault commented Dec 1, 2016 • edited

slnode commented Dec 2, 2016

slnode commented Dec 2, 2016

slnode commented Dec 2, 2016

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Mar 6, 2017

ebarault commented Mar 6, 2017 • edited

ebarault left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebarault commented Mar 13, 2017 • edited

Choose a reason for hiding this comment

ejvaitkus Apr 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebarault commented Nov 19, 2016 •

edited

ebarault commented Dec 1, 2016 •

edited

ebarault commented Mar 6, 2017 •

edited

ebarault left a comment •

edited

ebarault commented Mar 13, 2017 •

edited

ejvaitkus Apr 28, 2017 •

edited

bajtos Mar 17, 2017 •

edited