[
https://issues.apache.org/jira/browse/OAK-6144?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16006010#comment-16006010
]
angela commented on OAK-6144:
-----------------------------
I looked at the patch and have a few comments as follows:
h4. Delegatee#syncExternalUsers
the external user will only be sync if it exists _and_ is active.... if either
condition it not met the {{SyncResult.Status}} will be
{{SyncResult.Status.NO_SUCH_IDENTITY}}. This is IMO not properly reflecting
what is happening and is confusing. Wouldn't it be better to introduce a
dedicated status {{SyncResult.Status.NOT_ACTIVE}} and the adjust
{{syncExternalUsers}} to something like:
{code}
if (id == null) -> return SyncResult.Status.NO_SUCH_IDENTITY
else if (!id.isActive()) -> return SyncResult.Status.NOT_ACTIVE
else -> sync
{code}
h4. DelegateeTest
- the ID_PASSIVE_USER is only reflected in one single test case => I would
suggest to add it with other tests as well
- I don't understand how the change to {{testSyncExternalUsersSaveError}} is
related to the modifications proposed in this issue. why do you pass an empty
string ID?
h4. ExternalIdentity
The javadoc basically just repeats what the method states. I would like to see
that the javadoc explains what is meant with an external identity being
_active_. IMO it's not self explaining what that exactly means and how it is
intended to be handled by the external authentication... please specify... if
there is room for interpretation for a given implementer of the interface, this
should also be stated in the contract. Maybe you also want to link to the
SynchronizationMBean methods that are expected to make use of this.
h4. SyncMBeanImplTest
- please add more tests illustrating what how synchronization and purge of
non-active identities work
- in addition I would like to see test covering the situation where a given
external user or group is synced into the repository and then becomes inactive
=> verifying resync and purge operations. for that you may need to adjust
{{TestIdentityProvider}} such that the active-flag can be changed.
Further comments:
# I am wondering why the {{ExternalIdentity.isActive}} is only affects the
{{SynchronizationMBean}} implementation and not by the default {{SyncHandler}}.
To me that looks a bit odd as it will lead to the situation that the sync
triggered through the JXM console works differently than the regular sync as
performed by the {{SyncHandler}} and I would like to clarify the following
scenarios:
## What happens if a new external identity performs the repo-login and the
external identity is marked as non-active?
## What happens if an external identity that has already been synchronized is
being re-synced during the repo-login and the IDP upon resync identifies this
user as no longer active?
# The patch is missing documentation for that new feature in the oak-doc for
synchronizing external identities.
In the current format and in particular with the questions wrt handling the
active-flag during regular synchronization, I don't think this patch is ready
for being committed.
//cc [~tripod], [~alex.parvulescu]
> ExternalIdentity should have a method indicating if an identity is actually
> active
> ----------------------------------------------------------------------------------
>
> Key: OAK-6144
> URL: https://issues.apache.org/jira/browse/OAK-6144
> Project: Jackrabbit Oak
> Issue Type: New Feature
> Components: auth-external
> Reporter: Manfred Baedke
> Assignee: Manfred Baedke
> Attachments: oak-6144-1.patch
>
>
> The interface ExternalIdentityProvider currently offers the method
> getIdentity(ExternalIdentityRef) to resolve a reference to an external
> Identity, but there is no way to tell if the external identity is considered
> active by the identity provider. The ability to resolve the reference doesn't
> mean that the resulting identity may actually be used for authentication or
> authorization.
> If ExternaIIdentity isn't able to express this difference, it's hard to come
> up with a sensible implemenation of e.g.
> SynchronizationMBean#purgeOrphanedUsers(), because the ability to resolve a
> reference to an external identity doesn't mean that the corresponding Oak
> user is still valid.
> A new method ExternalIdentiy#isActive() would allow us to clearly define the
> notion of an "orphaned user".
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
