[ https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15514871#comment-15514871 ]
Alexander Klimetschek edited comment on OAK-4845 at 9/26/16 4:46 PM: --------------------------------------------------------------------- Attached a patch which fixes the issue by allowing local groups without a {{rep:externalId}} (and includes above test). It still checks and prevents a group added by another identity provider. * [^OAK-4845.patch] (alternatively --[on github|https://github.com/alexkli/jackrabbit-oak/commit/42c855cb583a85ef19ab839bf10d398967923d8c] (outdated)-- [on github|https://github.com/alexkli/jackrabbit-oak/commit/398ae15d5d23c5416087260ac8a00b1fefdbbfb0]) I could imagine there can be scenarios with 2 IDPs where folks might want to add users from one IDP to be added to groups from another IDP. In that case, reverting OAK-4397 would probably be the answer. Don't have a clear opinion on that, though. I also noticed that some of the unit tests, especially {{testMembershipForExistingForeignGroup}}, of which I based the new test on, do not make sure {{root.commit()}} is called after the sync, which at least makes group membership changes not yet visible to {{user.declaredMemberOf()}} checked at the end. * fixed that in [^missing-commits-in-tests.patch] (alternatively [on github|https://github.com/alexkli/jackrabbit-oak/commit/38689f79843b11a1f81b56f74d6e9810d93f34c9]) was (Author: alexander.klimetschek): Attached a patch which fixes the issue by allowing local groups without a {{rep:externalId}} (and includes above test). It still checks and prevents a group added by another identity provider. * [^OAK-4845.patch] (alternatively [on github|https://github.com/alexkli/jackrabbit-oak/commit/42c855cb583a85ef19ab839bf10d398967923d8c]) I could imagine there can be scenarios with 2 IDPs where folks might want to add users from one IDP to be added to groups from another IDP. In that case, reverting OAK-4397 would probably be the answer. Don't have a clear opinion on that, though. I also noticed that some of the unit tests, especially {{testMembershipForExistingForeignGroup}}, of which I based the new test on, do not make sure {{root.commit()}} is called after the sync, which at least makes group membership changes not yet visible to {{user.declaredMemberOf()}} checked at the end. * fixed that in [^missing-commits-in-tests.patch] (alternatively [on github|https://github.com/alexkli/jackrabbit-oak/commit/38689f79843b11a1f81b56f74d6e9810d93f34c9]) > Regression: DefaultSyncContext does not sync membership to a local group > ------------------------------------------------------------------------ > > Key: OAK-4845 > URL: https://issues.apache.org/jira/browse/OAK-4845 > Project: Jackrabbit Oak > Issue Type: Bug > Components: auth-external > Affects Versions: 1.5.3, 1.4.7 > Reporter: Alexander Klimetschek > Priority: Critical > Attachments: OAK-4845.patch, missing-commits-in-tests.patch > > > OAK-4397 introduced a regression: it does not allow syncing to a locally > existing group anymore (that does not belong to another IDP). > Updating to 1.4.7 now gives "Existing authorizable 'X' is not a group from > this IDP 'foo'.", and the group is not synced and most importantly, > memberships for external users are not updated anymore. Looking at the group > in JCR, it does not have a {{rep:externalId}} at all, only a > {{rep:lastSynced}}. > Code wise, this is because the {{rep:externalId}} is only ever set in > {{createGroup()}}, i.e. when the external sync creates that group initially. > If the group is already present locally (but not owned by another IDP!), then > it won't use it due to the new {{isSameIDP()}} check, which will also fail if > there is no {{rep:externalId}} property set. > The use case is that we are defining the group locally as part of our > application already (including ACs etc.) and we want certain external users > be added to it, based on some some of their settings on the external IDP > side. FWIW, we don't care for the syncing of properties for the group itself, > just for the memberships. -- This message was sent by Atlassian JIRA (v6.3.4#6332)