[
https://issues.apache.org/jira/browse/OAK-6144?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16046357#comment-16046357
]
angela commented on OAK-6144:
-----------------------------
[~baedke], here my review findings. overall the patch looks better than the
first version.
h4. General
* documentation update is missing in the patch... i think the following pages
deserve an update explaining the new bevahior wrt active external identities
** _usersync.md_
** _identitymanagement.md_
** _ldap.md_
** _defaultusersync.md_
** _dynamic.md_
** _faq.md_
h4. ExternalIdentity
* Changed Javadoc of {{getDeclaredGroups}}: Instead of just changing the
description (and thus basically change the contract), I would suggest to leave
the original sentence and add an explanation wrt to the active flag and link to
the JIRA ticket. Something like (_If {@link #isActive}} is supported by a given
implementation this method is expected only return _active_ declared groups
(see OAK-6144)_).
* New method {{isActive}}:
** why does this method need to throw {{ExternalIdentityException}}? and what
is an API consumer expected to do if it's not possible to determine if the
identity is active? that looks strange to me... shouldn't _cannot identify if
it's active_ be the same thing like the default (i.e. true)?
** I would like the javadoc to state that the default is _true_
** javadoc: please add the since tag with the right Oak version to indicate
that this method has been added later on
** please link to the JIRA ticket such that it's easy from the code to find the
corresponding discussion.
h4. SynchronizationMBean
* changes to javadoc: as with {{ExternalIdentity}} I would prefer if the
behavior related to the new active-flag was added as an amendment instead of
changing the original javadoc. This would a) allow to link to the JIRA ticket
and b) hightlight the fact that the active-flag has been added later on and c)
clarify that the active-flag is optional and may not be respected by all
implementations of {{ExternalIdentity}} (which according to the API contract in
this case always return {{true}}).
h4. Delegatee
* {{syncExternalUsers}}: the {{SyncResult}} in the else statement always sets
{{NO_SUCH_IDENTITY}} status. that's not correct given the fact that you added a
new status {{INACTIVE}} for the case the identity exists but is not active.
h4. DelegateeTest
* see above: wrong status when external identity is not active
h4. DynamicSyncContext
* as far as i can see there are no test covering the changes wrt active-flag
with users and groups, please add
h4. SyncResult
* javadoc of the {{INACTIVE}} status says: {{nothing changed. identity is
inactive}}... i don't agree that stating {{nothing changed}} is useful/correct
here. how an inactive identity is handled may vary between different
sync-handler implementations, which may or may not change something.
h4. TestIdentityProvider
* Unused import => please cleanup
* {{listUsers}}: returns {{false}} in case of exception... see also above...
given the fact that the default is {{true}}, i think that should also be return
here.
* {{listGroups}}: same here.
> 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, oak-6144-2.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.4.14#64029)
