Initial relation auth implementation

[ritch]

/to @bajtos
/cc @raymondfeng

WIP: DO NOT MERGE

This could be considered a controversial change. It changes the behavior for access control of the dynamically defined relation remote methods.

Here is a basic example:

• /customers/1/reviews?include=foo => Customers.prototype.__get__reviews()
• checkAccess() for customer with id 1
• checkAccess() for Reviews.find()
• checkAccess() for Foo.find()

This same concept is applied to all other relation methods.

Where I am a bit concerned about this change... what if you don't want to check access in the way mentioned above?

....well technically you can modify the auth hook (or just remove it) like this

var sharedMethod = Customer.sharedClass.find('__get__reviews');
sharedMethod.authorization = function(ctx, done) {
  // same signature as the `Model.checkAccess()` callback
  done(null, true); // allow all
};

// or extend the built in logic
var defaultCheck = sharedMethod.authorization;

sharedMethod.authorization = function myAuthorizationExtension(ctx, done) {
  if(ctx.args.myArg == 'someValue') {
    return done(null, true);
  }

  defaultCheck(ctx, done);
}

// or just disable the hook
delete sharedMethod.authorization;

@bajtos do you share the concern above? I don't want to break existing apps unless it is in a helpful way (eg. patching a security gap). Also can you look at the implementation and suggest how to test it or any major changes? I am going to actually start over since I am not entirely happy with how it is implemented (very duplicative).

Alternative Impl

I'm thinking of passing all the hooks through Model.checkRelationAccess which would allow you to change the way the check is performed just by overriding that function.

Connect #1362

[ritch]

This doesn't seem like the right place for this, but it is required to be able to just pass the ctx to the auth hook.

[bajtos]

Well, is it necessary for the auth hook to share the ctx object with strong-remoting? I mean you don't have to pass strong-remoting context to the auth hook, in my opinion you are free to create your own auth-hook-specific context.

[bajtos]

do you share the concern above?

Yes, I agree we must be very careful here. I think it's very likely that there are projects which actually depend on the current behaviour. Can we hide this new behaviour behind a "compat" flag, which would be disabled by default, but enabled in the project template in loopback-workspace?

[bajtos]

I don't remember how exactly does strong-remoting handle arguments of prototype method. I think ctx.args.id is set for the shared constructor only, the method itself does not necessarily have to have an id argument.

[bajtos]

... In which case this may be a breaking change.

[ritch]

Hmmm... thats right! I can add in the sharedCtor args to the remote context but I'm not sure that is useful. But I'm not happy with the current mess of param checking (which is http specific for no reason).

[bajtos]

Also can you look at the implementation and suggest how to test it or any major changes?

In general, it would be great to write several tests for each relation method and check that

1. an authorized user can access the method
2. an unauthorized user cannot access the method

Preferably one test for each cause of "unauthorized user".

Using your basic example outlined in the PR description, there are three permissions being checked:

• (1) checkAccess() for customer with id 1
• (2) checkAccess() for Reviews.find()
• (3) checkAccess() for Foo.find()

So you need four tests for this particular relation method:

1. user is allowed to do (1), (2), (3) -> requests is allowed
2. user is allowed (1), (2), but not (3) -> 403
3. user is allowed (1),(3) but not (2) -> 403
4. user is allowed (2),(3) but not (1) -> 403

I think the important part is to come up with a good DSL that will allow you to write this tests with minimum amount of duplicated code. Perhaps something similar to what we have in test/access-control.integration.js. Or you can invent a new language, for example:

describe('hasMany relation methods', function() {
  describe('find', function() {
    it('allows users with all permissions', function(done) {
      requestWithPermissions({ Customer: 'READ', Review: 'READ', FOO: 'READ' })
        .get('/customers/1/reviews?include=foo')
        .expect(200).end(done);
    });
    it('rejects user without permission to read related model', function(done) {
      requestWithPermissions({ Customer: 'READ', Review: null, FOO: 'READ' })
         .get('/customers/1/reviews?include=foo')
         .expect(403).end(done);
    });
   // etc.
  });
});

function requestWithPermissions(map) {
  // create a new user
  // setup ACLs to give the user permissions from "map"
  // login the new user
  // setup supertest object with Authorization header set to user's access token
}

Or you can even push this further:

describe('hasMany relation methods', function() {
  describe('find', function() {
    describe('/customers/1/reviews?include=foo', function() {
      verifyAccessCheck({ Customer: 'READ', Review: 'READ', FOO: 'READ' }, 200);
      verifyAccessCheck({ Customer: 'READ', Review: null, FOO: 'READ' }, 403);
    });
  });
});

[ritch]

The testing comment is really useful thanks @bajtos

[fabien]

@ritch looking forward to this - are there any remaining issues?

[ritch]

Yes... We need to re-work this once #1495 is implemented as a core feature.

[zanemcca]

Is there an update on this?