TWCable / grabbit

Grabbit - Fast Content Sync tool for AEM/CQ
Apache License 2.0
125 stars 64 forks source link

Issue 65 user and group support #148

Closed jbornemann closed 7 years ago

jbornemann commented 8 years ago

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

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

review-ninja commented 8 years ago

ReviewNinja

jbornemann commented 8 years ago

@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:

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

viveksachdeva commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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?

jbornemann commented 8 years ago

Moving most of our classes to Java should be pretty straightforward, and even automatable; however, specifically what I'm thinking of is our unit testing situtation. We utilize Groovy meta-class in several places, along with GroovyMocks for ease-of-testing, which we obviously would have to find another approach if we move to Java.

jbornemann commented 8 years ago

Thanks for the review @viveksachdeva !

jbornemann commented 8 years ago

Hey @sagarsane , mind having a look when you have a few moments?

sagarsane commented 8 years ago

Sorry :-). Yeah I will look at it soon!

jbornemann commented 8 years ago

Thanks :) It's a big change, so wanted to get plenty of eyes

jbornemann commented 8 years ago

Hey @sagarsane @viveksachdeva could I get a ninja star? If we are done looking at this, I'd like to get it in

viveksachdeva commented 8 years ago

I would try and test this PR this weekend..

jbornemann commented 8 years ago

Thanks @viveksachdeva

jbornemann commented 8 years ago

Hey @viveksachdeva, just a reminder when you test to exclude your source instance's admin user using a excludePath.

I added a README update about this

viveksachdeva commented 8 years ago

After installing this branch and trying to sync any content, I ma getting following error: 06.11.2016 19:47:59.943 ERROR [127.0.0.1 [1478441879795] GET /grabbit/content HTTP/1.1] org.apache.sling.engine.impl.SlingRequestProcessorImpl service: Uncaught Throwable java.lang.UnsupportedOperationException: Deserialization not allowed for class org.springframework.batch.core.JobExecution (on Sun Nov 06 19:47:59 IST 2016) at org.kantega.notsoserial.DefaultNotSoSerial.preventDeserialization(DefaultNotSoSerial.java:256) at org.kantega.notsoserial.DefaultNotSoSerial.onBeforeResolveClass(DefaultNotSoSerial.java:248) at org.kantega.notsoserial.ObjectInputStreamClassVisitor.onBeforeResolveClass(ObjectInputStreamClassVisitor.java:48) at java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:1620) at java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1521) at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1781) at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1353) at java.io.ObjectInputStream.readObject(ObjectInputStream.java:373) at org.springframework.batch.support.SerializationUtils.deserialize(SerializationUtils.java:68)

sagarsane commented 8 years ago

@viveksachdeva what did you config look like? And does your Grabbit src and Grabbit dest both have this branch installed?

viveksachdeva commented 8 years ago

@sagarsane

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

and both have same branch installed...

sagarsane commented 8 years ago

Just curious .. batchSize: -10? :)

viveksachdeva commented 8 years ago

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

sagarsane commented 8 years ago

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

sagarsane commented 8 years ago

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 commented 8 years ago

@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 :

@viveksachdeva 👻 what did you do to it? 😛

sagarsane commented 8 years ago

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

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

viveksachdeva commented 8 years ago

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

viveksachdeva commented 8 years ago

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 james.devore@spambob.com with effective type [sling:OrderedFolder] at org.apache.jackrabbit.oak.api.CommitFailedException.asRepositoryException(CommitFailedException.java:225)

jbornemann commented 7 years ago

@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 commented 7 years ago

Oh interesting :)

jbornemann commented 7 years ago

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

viveksachdeva commented 7 years ago

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

viveksachdeva commented 7 years ago

@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 commented 7 years ago

Thanks guys!