Skip to content

Conversation

@jbornemann
Copy link
Member

@jbornemann jbornemann commented Aug 26, 2016

This PR provides support for syncing authorizable nodes (Users, Groups), in addition to a few minor commits:

  • batch-server.log logs everything under com.twcable.grabbit.server namespace
  • batch-client.log logs everything under com.twcable.grabbit.client namespace
  • JSR-283 spec added to docs for contributor reference
  • Minor refactoring

To make this change wholesome, and useful; a few other changes will need to be tackled as part of

@review-ninja
Copy link

ReviewNinja

@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch 5 times, most recently from 62e51dc to af1a2ed Compare August 30, 2016 18:14
@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch from 8f3c1a4 to 739efa6 Compare September 6, 2016 14:46
@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch 11 times, most recently from b01b3ce to 73efeca Compare October 10, 2016 03:17
@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch 3 times, most recently from 32d932d to f010b6d Compare October 11, 2016 01:49
@jbornemann jbornemann added this to the v8.0 milestone Oct 11, 2016
@jbornemann
Copy link
Member Author

jbornemann commented Oct 11, 2016

@sagarsane @viveksachdeva Would you mind taking a look at this? I was able to find the time to revisit this after awhile.

The only thing I need to add to this is a README change.

With this change, you should be able to sync users, and groups under the /home path. One thing to note (that I will include in the README), is that you will need to add the admin user as an exclude path within the config - system won't let you modify admin user.

I created some test users, and test groups and synced between instances - some things to check:

  • password still works between servers
  • preferences, and profile are retained

Some things that won't work (but will work with future work mentioned above) :

  • Group membership. Groups synced will show up as having no members. (We need to support referential integrity. "Weak Reference", "Reference" property types)
  • ACLs.

@viveksachdeva
Copy link

@jbornemann : Just a thought..
As we want to move from Groovy to Java 8, would it be better if new files we create be in Java instead of Groovy?

@jbornemann
Copy link
Member Author

@viveksachdeva it's a good thought. I think it would be better for us to move to be consistently Java, or consistently Groovy across our codebase. There are also some bigger questions and challenges we will need to tackle when we move to Java that are out of scope for this enhancement.

@viveksachdeva
Copy link

Agreed that Moving everything to Java is out of scope of this.. Will there be a challenge to have new classes in Java instead of Groovy?

@viveksachdeva
Copy link

@sagarsane

{
    "serverUsername" : "admin",
    "serverPassword" : "admin",
    "serverHost" : "localhost",
    "serverPort" : "8080",
    "batchSize"  : -10,
    "pathConfigurations" :  [
{
            "path" : "/content/geometrixx"
        }
    ]
}

and both have same branch installed...

@sagarsane
Copy link
Contributor

Just curious .. batchSize: -10? :)

@viveksachdeva
Copy link

viveksachdeva commented Nov 6, 2016

@sagarsane : Yeah.. I added that when I was testing an old branch.. Any negative value defaults to 100.. :)

@sagarsane
Copy link
Contributor

kk! I'll test it out soon too after I'm done with initial review ..

@sagarsane
Copy link
Contributor

So, here's my observation --

The Users AND Groups are getting created correctly .. BUT, the node names created on the destination are NOT the same as those from the source (seems to be randomized by Jackrabbit in some way). However, when I install the user / group using package manager, it is able to keep the node name the same. Because of that, using Grabbit, the USER to GROUP association is getting lost while it is NOT getting lost (and being retained) when using Content Packages.

For example:

Source has a Grabbit USER and GROUP as follows:
USER:
screen shot 2016-11-06 at 11 48 16 am

GROUP:
screen shot 2016-11-06 at 11 48 27 am

Whereas on destination, the Grabbit USER and GROUP is as follows (note the different node name and the UUID):

USER:
screen shot 2016-11-06 at 11 49 43 am

GROUP:
screen shot 2016-11-06 at 11 50 45 am

As you can see, because of this, the Group <-> User association is getting lost... :(

@jbornemann
Copy link
Member Author

jbornemann commented Nov 7, 2016

@sagarsane Yes, the user/group association will be lost until #157 is implemented. This is an issue with reference associations in general. We don't have any mechanism for preserving references yet since we create nodes with the node API, etc; jcr:uuid's are different between server to server. Then, groups will have little use until #158 is implemented ;-) I wasn't going to advertise the release of Users/Groups until the v8.0 milestone is met (and the solution is complete).

Yes, since we aren't using workspace importXML (like I believe the Package Manager uses), rather the UserManager, our behavior is different. We don't import an XML snapshot of a repository; We stream and save nodes. Each server has a unique authorizable path for users/groups. I'm not sure of their exact intentions, but a couple of guesses :

  • avoid many-to-one user/group import collisions
  • reduce cross-server attack vector space for user/group compromise

@viveksachdeva 👻 what did you do to it? 😛

@sagarsane
Copy link
Contributor

Talked to @jbornemann on my findings .. and realized that those findings are expected.

Apart from the review comments, +1 testing from me.

@viveksachdeva
Copy link

Giving this one more try.. :) this time 6.1 -> 6.1

@viveksachdeva
Copy link

Getting a different error now: :(

My config file:

{
    "serverUsername" : "admin",
    "serverPassword" : "admin",
    "serverHost" : "localhost",
    "serverPort" : "4502",
    "batchSize"  : -10,
    "pathConfigurations" :  [
{
            "path" : "/home/groups",
        },
{
"path":"/home/users",
"excludePaths" : [
"r/rK06m522ibBJYwww-hF2"
]
}
    ]

For groups:

07.11.2016 20:22:13.212 ERROR [clientJobLauncherTaskExecutor-1] org.springframework.batch.core.step.AbstractStep Encountered an error executing step clientJcrNodes in job clientJob
javax.jcr.nodetype.ConstraintViolationException: OakConstraint0001: /home/groups/projects/admin/outdoors[[rep:AuthorizableFolder, rep:AccessControllable, mix:lockable]]: No matching definition found for child node ID02YXfXlqOHbGY-dVzN with effective type [nt:unstructured]
at org.apache.jackrabbit.oak.api.CommitFailedException.asRepositoryException(CommitFailedException.java:225)
at org.apache.jackrabbit.oak.api.CommitFailedException.asRepositoryException(CommitFailedException.java:212)

For users:
07.11.2016 20:22:13.307 ERROR [clientJobLauncherTaskExecutor-2] org.springframework.batch.core.step.AbstractStep Encountered an error executing step clientJcrNodes in job clientJob
javax.jcr.nodetype.ConstraintViolationException: OakConstraint0001: /home/users/geometrixx-outdoors[[rep:AuthorizableFolder, mix:lockable, rep:AccessControllable]]: No matching definition found for child node [email protected] with effective type [sling:OrderedFolder]
at org.apache.jackrabbit.oak.api.CommitFailedException.asRepositoryException(CommitFailedException.java:225)

@jbornemann
Copy link
Member Author

@viveksachdeva your testing pointed out some good issues. As in the Geometrixx example content, we need to support all sorts of arbitrary data under authorizables; not just profile/preferences.

I started the day by researching more on how authorizable node names are generated to maybe potentially avoid this problem all together(good reference material in our case is AuthorizableNodeName, RandomizedAuthorizableNodeName), and examining some ways we could approach things a bit differently. I thought that maybe we could provide a customized UserManager, and inject our own AuthorizableNodeName implementation, or really just use AuthorizableNodeName.DEFAULT; but UserManager is bound to JackrabbitSession, so I don't think this is feasible in isolation. Furthermore, AuthorizableNodeName is only passed in an authorizableId, so it would be tricky to generate authorizable nodes only based on the node name coming in.

I'm currently modifying the approach to send all nodes under authorizables with authorizables. It's almost there, but there are still issues. I'm going to pick it up again tomorrow.

@sagarsane
Copy link
Contributor

Oh interesting :)

@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch 4 times, most recently from d811f6d to dc59cbd Compare November 14, 2016 06:26
@jbornemann
Copy link
Member Author

Thanks for the find @viveksachdeva. Could you retest this? These scenarios should be covered now.

@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch from dc59cbd to 4093436 Compare November 14, 2016 08:31
@viveksachdeva
Copy link

awesome :D .... i will test this one out in some time ..

@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch 2 times, most recently from 17b4824 to 011af90 Compare November 16, 2016 03:40
@viveksachdeva
Copy link

@jbornemann : I am now able to sync users and groups but relationship is lost(which is expected at this point in time)... So +1 Testing :)

@jbornemann jbornemann force-pushed the issue-65-user-and-group-support branch from 011af90 to c33b6c0 Compare November 17, 2016 03:59
@jbornemann jbornemann merged commit c33b6c0 into master Nov 17, 2016
@jbornemann jbornemann deleted the issue-65-user-and-group-support branch November 17, 2016 04:04
@jbornemann
Copy link
Member Author

Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants