[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[ https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16144319#comment-16144319 ] Alexander Klimetschek edited comment on OAK-4845 at 8/28/17 8:56 PM: - Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose (this happens after the provisioning of 1) # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their assigned external IDP organization what users would should automatically become members of "foobar-users"; this might use a standardized name for the external group, say "foobar-external", which vendor A tells customer X about # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. It is possible to add individual AC permissions to principals (i.e. the external group) that don't exist yet. But then the benefit of aggregation of ACs in role groups is lost. [Dynamic membership|http://jackrabbit.apache.org/oak/docs/security/authentication/external/dynamic.html] is a fairly new feature for the oak IDP sync, but afaics this itself does not help, as long as the Oak User Management does not support dynamic groups (proposed in OAK-2687 but at this point deferred; and probably quite extensive work) to have members that reference another group that does not exist yet and application user management UIs on top of Oak can do this (please correct if I might be missing something here). Here is another *simple solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what other groups it should be). At that point there are no members at all. Once the group is turned external, all the usual protection applies, including the "remove member" issue discussed above. was (Author: alexander.klimetschek): Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose (this happens after the provisioning of 1) # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their assigned external IDP organization what users would should automatically become members of "foobar-users"; this might use a standardized name for the external group, say "foobar-external", which vendor A tells customer X about # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. Here is another *simple possible solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what othe
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[ https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16144319#comment-16144319 ] Alexander Klimetschek edited comment on OAK-4845 at 8/28/17 8:50 PM: - Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose (this happens after the provisioning of 1) # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their assigned external IDP organization what users would should automatically become members of "foobar-users"; this might use a standardized name for the external group, say "foobar-external", which vendor A tells customer X about # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. Here is another *simple possible solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what other groups it should be). At that point there are no members at all. Once the group is turned external, all the usual protection applies, including the "remove member" issue discussed above. was (Author: alexander.klimetschek): Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose (this happens after the provisioning of 1) # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their assigned external IDP organization what users would should be part of foobar-uses; this might use a standardized name for the external group, say "foobar-external" # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. Here is another *simple possible solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what other groups it should be). At that point there are no members at all. Once the group is turned external, all the usual protection applies, including the "remove member" issue discussed above. > 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.4.7, 1.5.3 >Reporter: Alexander Klimetschek >Assignee: Dominique Jäggi >Priority: Critical > Fix For: 1.6.0 > > Attachments: missing-commi
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[ https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16144319#comment-16144319 ] Alexander Klimetschek edited comment on OAK-4845 at 8/28/17 8:49 PM: - Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose (this happens after the provisioning of 1) # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their assigned external IDP organization what users would should be part of foobar-uses; this might use a standardized name for the external group, say "foobar-external" # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. Here is another *simple possible solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what other groups it should be). At that point there are no members at all. Once the group is turned external, all the usual protection applies, including the "remove member" issue discussed above. was (Author: alexander.klimetschek): Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose (this happens after the provisioning of 1) # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their IDP what users would should be part of foobar-uses; this might use a standardized name for the external group, say "foobar-external" # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. Here is another *simple possible solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what other groups it should be). At that point there are no members at all. Once the group is turned external, all the usual protection applies, including the "remove member" issue discussed above. > 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.4.7, 1.5.3 >Reporter: Alexander Klimetschek >Assignee: Dominique Jäggi >Priority: Critical > Fix For: 1.6.0 > > Attachments: missing-commits-in-tests.patch, OAK-4845.patch, > OAK-4845-test-lostmembership.patch > > > OAK-4397 introdu
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[ https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16144319#comment-16144319 ] Alexander Klimetschek edited comment on OAK-4845 at 8/28/17 8:49 PM: - Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose (this happens after the provisioning of 1) # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their IDP what users would should be part of foobar-uses; this might use a standardized name for the external group, say "foobar-external" # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. Here is another *simple possible solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what other groups it should be). At that point there are no members at all. Once the group is turned external, all the usual protection applies, including the "remove member" issue discussed above. was (Author: alexander.klimetschek): Some folks using our product & IDP ran into this issue again. The problem can really be a *chicken and egg scenario*: # the Oak-based system & application is setup by vendor A for customer X, before customer X onboards # an organization within the external identity provider system is given to customer X, within which they can add their users and groups as they choose # this happens after the provisioning of 1. # there is a standard local role group in the Oak-based system, e.g. "foobar-users" that aggregates the permission to use a part of the application called "foobar" # customer X wants to map an external group to that role, i.e. control in their IDP what users would should be part of foobar-uses; this might use a standardized name for the external group, say "foobar-external" # thus vendor A wants to setup that mapping of "foobar-external" being a member of "foobar-users" _before_ the customer X onboards Unfortunately, vendor A cannot run an IDP sync yet, as the external group "foobar-users" doesn't really exist yet. It is worth noting that even running the sync in the first place can be difficult before the customer was fully onboarded and has done extra steps. Thus the only way is to create it locally - but then the DefaultSyncHandler will fail to update it as it's not an external group. Here is another *simple possible solution*: In the DefaultSyncHandler, sync an external group to an existing local group ONLY if that group is completely empty (= no members), and turn it into an external group on the first sync. That step would be irreversible, i.e. the group would then stay external. This should allow setup of such a group locally just as vehicle to define the roles an external group should have (i.e. member of what other groups it should be). At that point there are no members at all. Once the group is turned external, all the usual protection applies, including the "remove member" issue discussed above. > 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.4.7, 1.5.3 >Reporter: Alexander Klimetschek >Assignee: Dominique Jäggi >Priority: Critical > Fix For: 1.6.0 > > Attachments: missing-commits-in-tests.patch, OAK-4845.patch, > OAK-4845-test-lostmembership.patch > > > OAK-4397 introduced a regression: it does not al
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[
https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15524062#comment-15524062
]
Alexander Klimetschek edited comment on OAK-4845 at 9/26/16 8:18 PM:
-
I noticed that a lost membership is not applied in such a "local group" case
either. This was always true, even before OAK-4397.
First the [isSameIDP() check inside
syncMemberships()|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L480-L482]
fills the {{declaredExternalGroups}} map and then later the [removal of lost
memberships|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L535-L536]
is only done against this list/map (code links are oak 1.4.2, but current
trunk is identical). Hence any group that existed locally before, would not get
a {{rep:externalId}}, and later on membership removal it would get ignored.
I see the need that you want to make sure a membership is not removed
accidentally if it "additionally" comes in from another IDP. OAK-4397
definitely made the entire behavior more consistent, in that you can't even add
such memberships in the first place.
However, from all I can see in our use case we have no other option than have
this group exist locally before (and definitely there are production systems
with this state already). IMO it should be ok to say a "local only group" with
no {{rep:externalId}} gets upgraded to an IDP group by adding the
{{rep:externalId}} the first time it is synced - and from then on all the
checks stay in place, to prevent another IDP to mess with it. This would be a
slightly different patch.
was (Author: alexander.klimetschek):
I noticed that a lost membership is not applied in such a "local group" case
either. This was always true, even before OAK-4397.
First the [isSameIDP() check inside
syncMemberships()|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L480-L482]
fills the {{declaredExternalGroups}} map and then later the [removal of lost
memberships|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L535-L536]
is only done against this list/map (code links are oak 1.4.2, but current
trunk is identical). Hence any group that existed locally before, would not get
a {{rep:externalId}}, and hence later on membership removal it would get
ignored.
I see the need that you want to make sure a membership is not removed
accidentally if it "additionally" comes in from another IDP. OAK-4397
definitely made the entire behavior more consistent, in that you can't even add
such memberships in the first place.
However, from all I can see in our use case we have no other option than have
this group exist locally before (and definitely there are production systems
with this state already). IMO it should be ok to say a "local only group" with
no {{rep:externalId}} gets upgraded to an IDP group by adding the
{{rep:externalId}} the first time it is synced - and from then on all the
checks stay in place, to prevent another IDP to mess with it. This would be a
slightly different patch.
> 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
> i
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[
https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15524062#comment-15524062
]
Alexander Klimetschek edited comment on OAK-4845 at 9/26/16 8:18 PM:
-
I noticed that a lost membership is not applied in such a "local group" case
either. This was always true, even before OAK-4397.
First the [isSameIDP() check inside
syncMemberships()|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L480-L482]
fills the {{declaredExternalGroups}} map and then later the [removal of lost
memberships|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L535-L536]
is only done against this list/map (code links are oak 1.4.2, but current
trunk is identical). Hence any group that existed locally before, would not get
a {{rep:externalId}}, and hence later on membership removal it would get
ignored.
I see the need that you want to make sure a membership is not removed
accidentally if it "additionally" comes in from another IDP. OAK-4397
definitely made the entire behavior more consistent, in that you can't even add
such memberships in the first place.
However, from all I can see in our use case we have no other option than have
this group exist locally before (and definitely there are production systems
with this state already). IMO it should be ok to say a "local only group" with
no {{rep:externalId}} gets upgraded to an IDP group by adding the
{{rep:externalId}} the first time it is synced - and from then on all the
checks stay in place, to prevent another IDP to mess with it. This would be a
slightly different patch.
was (Author: alexander.klimetschek):
I noticed that a lost membership is not applied in such a local group
membership either. This was always true, even before OAK-4397.
First the [isSameIDP() check inside
syncMemberships()|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L480-L482]
fills the {{declaredExternalGroups}} map and then later the [removal of lost
memberships|https://github.com/apache/jackrabbit-oak/blob/8c928feafc77c40d75142f69931b6ff3b5837f9d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L535-L536]
is only done against this list/map (code links are oak 1.4.2, but current
trunk is identical). Hence any group that existed locally before, would not get
a {{rep:externalId}}, and hence later on membership removal it would get
ignored.
I see the need that you want to make sure a membership is not removed
accidentally if it "additionally" comes in from another IDP. OAK-4397
definitely made the entire behavior more consistent, in that you can't even add
such memberships in the first place.
However, from all I can see in our use case we have no other option than have
this group exist locally before (and definitely there are production systems
with this state already). IMO it should be ok to say a "local only group" with
no {{rep:externalId}} gets upgraded to an IDP group by adding the
{{rep:externalId}} the first time it is synced - and from then on all the
checks stay in place, to prevent another IDP to mess with it. This would be a
slightly different patch.
> 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!)
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[
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)
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[
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/23/16 12:08 AM:
--
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])
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]
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]
> 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)
[jira] [Comment Edited] (OAK-4845) Regression: DefaultSyncContext does not sync membership to a local group
[
https://issues.apache.org/jira/browse/OAK-4845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15514797#comment-15514797
]
Alexander Klimetschek edited comment on OAK-4845 at 9/22/16 11:51 PM:
--
Here is a unit test for DefaultSyncContextTest replicating the situation. I
deliberately verify only the user membership and not checking for
rep:lastSynced or rep:externalId being set, to test only the actual use case
that otherwise wouldn't be supported.
{code:java}
/**
* @see https://issues.apache.org/jira/browse/OAK-4845";>OAK-4845
*/
@Test
public void testMembershipForExistingLocalGroup() throws Exception {
syncConfig.user().setMembershipNestingDepth(1).setMembershipExpirationTime(-1).setExpirationTime(-1);
syncConfig.group().setExpirationTime(-1);
ExternalUser externalUser = idp.getUser(USER_ID);
ExternalIdentityRef groupRef =
externalUser.getDeclaredGroups().iterator().next();
// create the group locally (has no rep:externalId)
Group gr = userManager.createGroup(groupRef.getId());
root.commit();
sync(externalUser);
User user = userManager.getAuthorizable(externalUser.getId(),
User.class);
assertNotNull(user);
// verify membership gets added
assertTrue(gr.isDeclaredMember(user));
Iterator declared = user.declaredMemberOf();
assertTrue(declared.hasNext());
assertTrue(gr.getID().equals(declared.next().getID()));
}
{code}
was (Author: alexander.klimetschek):
Here is a unit test for DefaultSyncContextTest replicating the situation. I
deliberately verify only the user membership and not checking for
rep:lastSynced or rep:externalId being set, to test only the actual use case
that otherwise wouldn't be supported.
{code:java}
/**
* @see https://issues.apache.org/jira/browse/OAK-4845";>OAK-4845
*/
@Test
public void testMembershipForExistingLocalGroup() throws Exception {
syncConfig.user().setMembershipNestingDepth(1).setMembershipExpirationTime(-1).setExpirationTime(-1);
syncConfig.group().setExpirationTime(-1);
ExternalUser externalUser = idp.getUser(USER_ID);
ExternalIdentityRef groupRef =
externalUser.getDeclaredGroups().iterator().next();
// create the group locally (has no rep:externalId)
Group gr = userManager.createGroup(groupRef.getId());
root.commit();
SyncResult result = syncCtx.sync(externalUser);
assertSame(SyncResult.Status.ADD, result.getStatus());
User user = userManager.getAuthorizable(externalUser.getId(),
User.class);
assertNotNull(user);
// verify membership gets added
assertTrue(gr.isDeclaredMember(user));
Iterator declared = user.declaredMemberOf();
assertTrue(declared.hasNext());
assertTrue(gr.getID().equals(declared.next().getID()));
}
{code}
> 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)
