MODE-1920: Initial access management implementation #849

okulikov · 2013-06-05T14:49:23Z

This implementation contains following changes:
-AccessManager provides implementation for access list policy only where policy is associated with each node. Access list data is stored as special child node. Node types are defined in the file access-manager.cnd. Nodes defined protected;
-AccessManager also implements AdvancedAuthorization interface and SecurityContext interface what allows to plug it with minimal changes

Security context interface was extended with additional methods which allow to get/set subordinated security context. The purpose was from one hand plug access manager with minimal changes at the core level and from other hand to make security functions more flexible. For instance modeshape can provide role based servlet context with JCR defined access manager as subordinated for finer grained access control.
session.checkPermissions was modified to be able recursively check chain of primary/subordinated security contexts and to skip all checks when call is initiated by security context itself. it is required to prevent cyclic calls of hasPermission(..) when call is initiated by access manager itself.
Access list nodes are defined as protected. I was always trying to prevent changes at the methods signature to allow internal manipulation with protected properties or add new methods. Instead it uses session context to distinguish calls and allow validate protected fields if call was initiated by access manager.
-Unit tests for functional tests of the access list

rhauch · 2013-06-05T19:17:09Z

I'd like to merge this only after we release 3.3.

rhauch · 2013-06-20T13:32:49Z

modeshape-jcr/src/main/java/org/modeshape/jcr/AbstractJcrNode.java

@@ -982,7 +982,8 @@ final AbstractJcrNode addNode( String relPath,
                if (parent instanceof AbstractJcrNode) {
                    // delegate to the parent node ...
                    Name childName = path.getLastSegment().getName();
-                    session.checkPermission(absolutePathFor(parent.path(), path.getLastSegment()), ModeShapePermissions.ADD_NODE);
+                        //MODE-1920: check add_child_node permission on parent node
+                        session.checkPermission(parent.path(), ModeShapePermissions.ADD_NODE);


Why was this changed back to 'parent.path()' rather than absolutePathFor(parent.path(), path.getLastSegment())? If you look at the absolutePathFor method, it will simply return the parent path if it is absolute; otherwise, it constructs an absolute path. This was changed recently to fix a but where sometimes we would be passing relative paths to the authorization framework.

rhauch · 2013-06-20T13:39:28Z

A couple of things:

Are the ACL nodes are stored underneath each normal node, or are they stored in the system area?
Can you document what the persisted node structure look like? Actually, there needs to be a lot better documentation about how this system works.
What impact does this have on performance?
Is this enabled by default? Is there a way to run with the old/existing authorization mechanism?

rhauch · 2013-06-20T13:45:27Z

modeshape-jcr/src/test/java/org/modeshape/jcr/security/acl/PrivilegesTest.java

+
+    @After
+    public void tearDown() {
+    }


Generally, a test should not have any of these methods that are empty. Just remove them to keep the test as simple as possible.

okulikov · 2013-06-20T14:58:21Z

Briefly before preparing java doc with node structures.

-ACLs are stored as a special child nodes. As normal content.
-Regarding perfomance: current implementation is not optimal, node is
requested at least two times. This is definitely can be improved a lot. The
initial idea was to keep less changes in the code and simplify analysis of
changes.
-ACL support is enabled by default however if no ACL defined everything is
granted for this node and its childs
Old/existing authentication/authorization mechanism remains in place and
so-exist with ACL. ACLs are just used for finer access control. ACL acts
like a subset.

2013/6/20 Randall Hauch notifications@github.com

A couple of things:

Are the ACL nodes are stored underneath each normal node, or are they
stored in the system area?

Can you document what the persisted node structure look like?

What impact does this have on performance?

Is this enabled by default? Is there a way to run with the old/existing
authorization mechanism?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/849#issuecomment-19752790
.

okulikov · 2013-06-20T16:56:37Z

Implementation design
https://docs.google.com/file/d/0Byr0u9Cn0CSAN3hrZ3dZaTdsRzg/edit?usp=sharing

rhauch · 2013-06-20T17:23:48Z

I'm really not sure how I feel about the ACL-specifics being added below the regular content nodes. I understand the very real benefits, but it doesn't "smell" very good.

How does the reference implementation do it?

okulikov · 2013-06-20T18:00:35Z

It does the same.

2013/6/20 Randall Hauch notifications@github.com

I'm really not sure how I feel about the ACL-specifics being added below
the regular content nodes. I understand the benefits, but it doesn't
"smell" very good.

How does the reference implementation do it?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/849#issuecomment-19768203
.

rhauch · 2013-06-27T15:58:16Z

Any update on performance? Consider running https://github.com/ModeShape/modeshape-performance tests before and after these changes.

okulikov · 2013-06-27T16:43:47Z

I will do test

2013/6/27 Randall Hauch notifications@github.com

Any update on performance? Consider running
https://github.com/ModeShape/modeshape-performance tests before and after
these changes.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/849#issuecomment-20132316
.

rhauch · 2013-07-02T15:40:47Z

Am I missing an updated commit? I still don't see any documentation of the design inside the JavaDoc (ideally in the AccessControlManagerImpl class). You're Google doc looks good, but we still need some documentation inside the code. If you want to also create a wiki page with the design, that's fine. But I still prefer basic design documentation inside the code. For example, how are the ACLs managed? Are they visible to clients? Can they be edited without going through an admin API?

Also, I made some suggestions earlier that don't seem to be addressed.

okulikov · 2013-07-02T16:22:01Z

Sorry, I was waiting for your decision about ACLs added below the content node. If we agree on this way I will just add fixes by following comments otherwise it might require more changes.

rhauch · 2013-07-02T16:36:41Z

Ah, gotcha. I'm fine with storing the ACLs below the affected/controlled node. It might be nice, however, if we plan to support and groupnames, not just usernames. Maybe "principalName" is a generic term? Once you get to the documentation, it'd be good to include some sample code of how to create an ACL for a node.

rhauch · 2013-07-02T16:37:32Z

modeshape-jcr/src/test/java/org/modeshape/jcr/security/acl/JcrAccessControlListTest.java

+    }
+
+
+    private class User implements Principal {


Should we have a default implementation in API that does this? Seems useful if we're expecting clients to supply these instances.

rhauch · 2013-07-09T14:03:45Z

Status?

okulikov · 2013-07-09T15:10:14Z

Few things remains. Should be ready very soon.

2013/7/9 Randall Hauch notifications@github.com

Status?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/849#issuecomment-20675823
.

rhauch · 2013-07-09T15:32:32Z

Excellent. Just please post as you work.

okulikov · 2013-07-09T15:39:38Z

Now I think it is ready. Latest commit adds principals implementation to the API and fixes problem with absolute path mentioned above.

rhauch · 2013-07-15T15:55:10Z

modeshape-jcr/src/main/resources/org/modeshape/jcr/access-manager.cnd

+    + * (mode:Permission) protected
+[mode:Permission] 
+    - name (string) protected
+    - privileges (string) multiple protected


Why did you choose to put these in a separate file, rather than in modeshape_builtins.cnd?

rhauch · 2013-07-15T16:10:29Z

Nice work, @okulikov. I do like how this is designed and implemented, though I do have a number of small requests that I'd like to see addressed (see above). @hchiorean, please review this and post your thoughts.

This will be great to have in 3.4.

hchiorean · 2013-07-16T08:12:50Z

The Repository.OPTION_ACCESS_CONTROL_SUPPORTED should return true (via JcrRepository#initializeDescriptors). I suspect this is required for the ACL specific part of the TCK tests to "kick in". We should make sure we pass all of those.

hchiorean · 2013-07-16T08:20:13Z

Shouldn't the org.modeshape.jcr.AccessControlManagerImpl class be part of the security package ?

hchiorean · 2013-07-16T08:23:50Z

Are the ACL-related nodes reflected in the query results ? I'm guessing the mode:Acl, mode:Permission etc are indexed & stored as regular nodes, but does the query engine filter out results based on those nodes ? (i.e. the simplest case is a user executing a select * from type query, but the user only has read-permissions on a subset of nodes). This can potentially get a lot more complicated, when stuff like LIMITs or OFFSETs come into play.

Also, related to the above, do we really want to index those nodes as regular child nodes or would it be better to add some ACL-related information to the NodeInfo for example ?

hchiorean · 2013-07-16T08:37:53Z

Do we have unit tests for validating multiple session & workspace interaction in regard to nodes that have ACLs set ? To be more precise, I'm referring to unit tests which validate the following part of the spec:

A node which has had a policy set or removed is marked as modified until the changes are saved.

The access control modifications can be reverted by calling Session.refresh(false).

The changes are visible to sessions other than the session making the change no earlier than its being dispatched (i.e., saved if outside atransaction, committed if within a transaction).

Depending on the repository implementation, the changes may not be reflected in another session until that session reacquires the modified node

I'm aware that using child nodes assures pretty much all of the above, however since it's a JCR Spec Feature, we should have dedicated tests IMO.

hchiorean · 2013-07-16T08:47:43Z

We also need to think about federation, because that is not part of the JCR spec.

For example, for a node on which a user does not have the jcr:addChildNodes privilege, do we allow the FederationManager#createProjection call ?

rhauch · 2013-07-16T15:49:03Z

@hchiorean said:

Are the ACL-related nodes reflected in the query results ? I'm guessing the mode:Acl, mode:Permission etc are indexed & stored as regular nodes, but does the query engine filter out results based on those nodes ? (i.e. the simplest case is a user executing a select * from type query, but the user only has read-permissions on a subset of nodes). This can potentially get a lot more complicated, when stuff like LIMITs or OFFSETs come into play.

We should make sure that none of the nodes associated with ACLs are ever indexed, by setting the node types to have the 'noqueryorder' and 'nofulltext' attributes, and by ensuring that the nodes are never indexed (via the setQueryable(false) method on the ACL-related nodes).

However, the query system is already using the session's ability to see nodes before a node appears in the results ... as long as the existing session nodes now take into account the ACLs in addition to the older-style permissions.

rhauch · 2013-08-02T15:57:56Z

modeshape-jcr/src/main/java/org/modeshape/jcr/AccessControlManagerImpl.java

+    }
+
+    @Override
+    public Privilege[] getSupportedPrivileges(String path) throws PathNotFoundException, RepositoryException {


Remove the exceptions, since they're never thrown from the method.

rhauch · 2013-08-02T17:32:46Z

@okulikov, overall this PR is looking really good. I did have a number of line comments above, but they're all fairly minor - though I did have a few questions and I'm concerned about the AccessControlManagerImpl.hasPermission(...) implementation not being as efficient as possible considering it is called so frequently (see above).

Oh, building locally worked just fine for me.

rhauch · 2013-08-06T14:08:05Z

Rebased and merged into the 'master' branch. Also, fixed a number of JavaDoc errors and compiler warnings with an additional commit.

gregjan · 2013-08-28T14:08:45Z

I found a very minor issue in the ACM source code line 117. It passes path instead of username to the default JcrAccessControlList. I don't know how the default ACL is set, but I suppose it might be set differently, such that it references principals beyond "everyone". Anyway, hope this helps.

rhauch · 2013-08-28T14:10:29Z

@gregjan, can you log a separate JIRA for this? I think the line number has changed: https://github.com/ModeShape/modeshape/blob/master/modeshape-jcr/src/main/java/org/modeshape/jcr/AccessControlManagerImpl.java#L121

rhauch reviewed Jun 20, 2013
View reviewed changes

rhauch reviewed Jul 2, 2013
View reviewed changes

rhauch reviewed Jul 15, 2013
View reviewed changes

rhauch reviewed Aug 2, 2013
View reviewed changes

MODE-1920: Clean up code and java-doc

88e2858

rhauch closed this Aug 6, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MODE-1920: Initial access management implementation #849

MODE-1920: Initial access management implementation #849

okulikov commented Jun 5, 2013

rhauch commented Jun 5, 2013

rhauch Jun 20, 2013

rhauch commented Jun 20, 2013

rhauch Jun 20, 2013

okulikov commented Jun 20, 2013

okulikov commented Jun 20, 2013

rhauch commented Jun 20, 2013

okulikov commented Jun 20, 2013

rhauch commented Jun 27, 2013

okulikov commented Jun 27, 2013

rhauch commented Jul 2, 2013

okulikov commented Jul 2, 2013

rhauch commented Jul 2, 2013

rhauch Jul 2, 2013

rhauch commented Jul 9, 2013

okulikov commented Jul 9, 2013

rhauch commented Jul 9, 2013

okulikov commented Jul 9, 2013

rhauch Jul 15, 2013

rhauch commented Jul 15, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

rhauch commented Jul 16, 2013

rhauch Aug 2, 2013

rhauch commented Aug 2, 2013

rhauch commented Aug 6, 2013

gregjan commented Aug 28, 2013

rhauch commented Aug 28, 2013

MODE-1920: Initial access management implementation #849

MODE-1920: Initial access management implementation #849

Conversation

okulikov commented Jun 5, 2013

rhauch commented Jun 5, 2013

rhauch Jun 20, 2013

Choose a reason for hiding this comment

rhauch commented Jun 20, 2013

rhauch Jun 20, 2013

Choose a reason for hiding this comment

okulikov commented Jun 20, 2013

okulikov commented Jun 20, 2013

rhauch commented Jun 20, 2013

okulikov commented Jun 20, 2013

rhauch commented Jun 27, 2013

okulikov commented Jun 27, 2013

rhauch commented Jul 2, 2013

okulikov commented Jul 2, 2013

rhauch commented Jul 2, 2013

rhauch Jul 2, 2013

Choose a reason for hiding this comment

rhauch commented Jul 9, 2013

okulikov commented Jul 9, 2013

rhauch commented Jul 9, 2013

okulikov commented Jul 9, 2013

rhauch Jul 15, 2013

Choose a reason for hiding this comment

rhauch commented Jul 15, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

hchiorean commented Jul 16, 2013

rhauch commented Jul 16, 2013

rhauch Aug 2, 2013

Choose a reason for hiding this comment

rhauch commented Aug 2, 2013

rhauch commented Aug 6, 2013

gregjan commented Aug 28, 2013

rhauch commented Aug 28, 2013