Skip to content
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.

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

Basic access control for change replication #1270

Merged
merged 2 commits into from
Apr 7, 2015

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Apr 2, 2015

  1. Add integration tests running change replication over REST to verify
    that access control at model level is correctly enforced.

  2. Implement a new access type "REPLICATE" that allows principals
    to create new checkpoints, even though they don't have full WRITE
    access to the model. Together with the "READ" permission, these
    two types allow principals to replicate (pull) changes from the server.

Note that anybody having "WRITE" access type is automatically
granted "REPLICATE" type too.

  1. Add a new model option "enableRemoteReplication" that exposes
    replication methods via strong remoting, but does not configure
    change rectification. This option should be used the clients
    when setting up Remote models attached to the server via the remoting
    connector.

The scope of this pull request is to implement access control checks for replication at model-level, i.e. when a user can either access (read, write) all mode instances or cannot access any model instance. Instance-level access control checks (where e.g. a user can read & write only his own user profile) are out of scope of this pull request.

Connect to #1166

@bajtos bajtos self-assigned this Apr 2, 2015
@bajtos bajtos added the #review label Apr 2, 2015
@bajtos bajtos added this to the #Epic: Offline Sync V1 milestone Apr 2, 2015
@bajtos bajtos force-pushed the feature/replication-access-control branch from 5660413 to 166245e Compare April 2, 2015 12:47
@bajtos
Copy link
Member Author

bajtos commented Apr 2, 2015

As I am thinking about this issue more, I see two "feature" levels we can implement:

Level1 - model-level access control. A user can either read/write all model instances, or no model instance at all. This should be rather easy to implement, as the ACL check can stay at method-level.

Level2 - instance-level access control. A user can read/write only a subset of model instances. This is more difficult to implement, partially because we don't support this fine-grained access control in existing code either (e.g. Model.find does not filter instances to those that the current user can access). I want to leave this out of Sync v1.0 release.

@bajtos
Copy link
Member Author

bajtos commented Apr 2, 2015

@ritch could you please comment with a code snippet showing how to configure ACLs so that user Alice can read & write all Car instances, user Peter can only read Car instances, and user Jane cannot read nor write any Car instances.

@bajtos bajtos force-pushed the feature/replication-access-control branch from 166245e to af3b505 Compare April 3, 2015 14:44
@bajtos bajtos changed the title WIP: replication + ACL v2 [DO NOT MERGE] WIP: basic access control for change replication [DO NOT MERGE] Apr 3, 2015
@bajtos bajtos added #wip and removed #review labels Apr 3, 2015
@bajtos bajtos force-pushed the feature/replication-access-control branch from af3b505 to 6f0cd70 Compare April 3, 2015 15:07
@bajtos bajtos mentioned this pull request Apr 3, 2015
@bajtos bajtos force-pushed the feature/replication-access-control branch from 6f0cd70 to a2fadae Compare April 7, 2015 13:52
@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2015

@ritch please review. I have implemented the first part of what we have discussed on Friday: the new setting "enableRemoteReplication" and a new access type "REPLICATE".

If we manage to land this PR soon, then I'll send the second part (conflict resolution fixes) in a new pull request. Otherwise I'll add more commits to this one.

Let me know what works best for you.

Two sets of tests that I left out, let me know if you think they are important and I should add them:

  • Tests covering ACLs per each individual PersistedModel method.
  • Tests checking the new "REPLICATE" access type on a lower level, i.e. directly via AccessContext and/or ACL methods.

/cc @raymondfeng you may want to review this PR too, as it is adding a new feature to access control.

@bajtos bajtos changed the title WIP: basic access control for change replication [DO NOT MERGE] Basic access control for change replication Apr 7, 2015
@bajtos bajtos assigned ritch and unassigned bajtos Apr 7, 2015
beforeEach(seedServerData);
beforeEach(seedClientData);

describe('scenario under test', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Missing describe?

"scenario under test" seems like your editors default...?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intentional. The test suite is checking that I the before/beforeEach hooks have correctly set up all models and permissions. You would not believe how much are the tests intertwined through the global model registry and how easy it is to mess up the setup.

Can you come up with a better name that will make my intent more clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps describe('the replication scenario scaffolded for the tests')?

Copy link
Member

Choose a reason for hiding this comment

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

ya that would be better

@ritch
Copy link
Member

ritch commented Apr 7, 2015

I'm happy with the changes. A bit concerned about the 3-4 TODO's... But if those are captured by other stories than this LGTM. 👍

1) Add integration tests running change replication over REST to verify
that access control at model level is correctly enforced.

2) Implement a new access type "REPLICATE" that allows principals
to create new checkpoints, even though they don't have full WRITE
access to the model. Together with the "READ" permission, these
two types allow principals to replicate (pull) changes from the server.

Note that anybody having "WRITE" access type is automatically
granted "REPLICATE" type too.

3) Add a new model option "enableRemoteReplication" that exposes
replication methods via strong remoting, but does not configure
change rectification. This option should be used the clients
when setting up Remote models attached to the server via the remoting
connector.
@bajtos bajtos force-pushed the feature/replication-access-control branch from a2fadae to 9c5fe08 Compare April 7, 2015 17:54
bajtos added a commit that referenced this pull request Apr 7, 2015
…ontrol

Basic access control for change replication
@bajtos bajtos merged commit b18d251 into master Apr 7, 2015
@bajtos bajtos deleted the feature/replication-access-control branch April 7, 2015 18:00
@bajtos bajtos removed the #wip label Apr 7, 2015
@bajtos bajtos mentioned this pull request Apr 9, 2015
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants