Replication + ACLs

[ritch]

This is a fix for: #1166

Connect to #1166

@bajtos This test already passes. I must be missing something. Are you suggesting that we check before we start replicating?

Also can you comment on the tests I've added. I'd like to ensure I've got those right before I move much further.

[bajtos]

Perhaps use debug logs here? require('debug')('test')('%s %s', req.method, req.url)

[bajtos]

This test already passes. I must be missing something.

I don't know enough about our auth&auth implementation to be able to help you. Few things that stand out in the current code:

• You are calling app.enableAuth() for all tests, even for the test that verifies anonymous mode
• The restrictive ACLs are enabled after the application and the model was already set up. Is that valid approach, I mean will the auth code pick up these changes?

I don't know how to help you here without debugging your new tests, which is something you can do yourself too.

Are you suggesting that we check before we start replicating?

I am not following you here, what should we check before we start?

Also can you comment on the tests I've added. I'd like to ensure I've got those right before I move much further.

Yes, it looks good so far. Few more scenarios to consider:

• a happy path where replication succeeds for a logged-in user
• replication is rejected for a logged-in user due to permissions

It may be a good idea to test all three operations in all three scenarios: create, update, delete.

On more idea to consider: bulkUpdate should be as atomic as possible. If any of the updates is rejected by auth, then no updates should be applied at all.

[bajtos]

This test already passes. I must be missing something.

I think the test is passing because the ACL implementation reject anonymous requests when there are ACL configured for the operation, thus I guess this is mostly expected.

A more interesting scenario is described in my comment above, where the user is authenticated, but they don't have permissions to create/update/delete the specific model instance.

The built-in User model is a good example:

• regular users cannot create new User instances (accounts)
• a regular user can edit only his own account
• a regular user can either delete only his own account or cannot delete any account at all

I am not sure how exactly the current implementation works, please take the above items with a grain of salt.

[ritch]

I don't know how to help you here without debugging your new tests, which is something you can do yourself too.

I am more concerned with what exactly led you to create #1166.

[bajtos]

As discussed in chat, I didn't have any specific case where it should not work, just that we didn't have any tests and I had a feeling that a the current implementation of authentication may not work, because:

• auth is implemented as a beforeRemote, thus it verifies permissions at the granularity of a single method (bulkUpdate in this case)
• bulkUpdate makes possibly multiple create/update/delete calls, however these calls don't go through the authentication layer

What I would like to see is a suite of tests that will show that bulkUpdate respects permissions like I described in my comment above, i.e.g that when a user "Joe" can read all Products but cannot modify any of them, then a bulk update will reject a request containing updates of Product instances. When "Employee" record is viewable by all users but only owner can edit it, then a bulk update containing update of different employee than the logged in user will fail.

[bajtos]

Closing in favour of #1270