[
https://issues.apache.org/jira/browse/OAK-4932?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15611267#comment-15611267
]
angela commented on OAK-4932:
-----------------------------
[~alexander.klimetschek], the proposed (minor) changes it exactly what i was
pointing to in OAK-4301; doing so would result in allow in the missing
protection resulting a critical vulnerability. we were just lucky that OAK-4301
could not be exploited. so, I don't thing that we should make that change given
the fact that some customers might need to disable the protected added with
OAK-4301 for other reasons.
for the record here the translation of a private conversation I had with
[~tripod]
{quote}
fortunately the external id reference isn't used to retrieve the
ExternalIdentity, which currently is read from the id (see ExternalUser
external = idp.getUser(id);). However, If i would change that particular line
to ExternalUser external = (ExternalUser) idp.getIdentity(ref) which IMHO was
equally legitimate and maybe even favorable, the worst case scenario would come
true and we had a really dangerous exploit.
{quote}
> DefaultSyncContext.sync(String) could use idp.getIdentity(ExternalIdentityRef)
> ------------------------------------------------------------------------------
>
> Key: OAK-4932
> URL: https://issues.apache.org/jira/browse/OAK-4932
> Project: Jackrabbit Oak
> Issue Type: Improvement
> Components: auth-external
> Reporter: Alexander Klimetschek
>
> Instead of
> [idp.getGroup(id)|https://github.com/apache/jackrabbit-oak/blob/08aadf19a5e6b1bb4ca6687623e06140fb1ec5bc/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L291]
> and
> [idp.getUser(id)|https://github.com/apache/jackrabbit-oak/blob/08aadf19a5e6b1bb4ca6687623e06140fb1ec5bc/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java#L300],
> the implementation of the DefaultSyncHandler could use
> {{ExternalIdentityProvider.getIdentity(ExternalIdentityRef)}}, as it looks up
> the reference right before (based on the {{rep:externalId}}) and fails if not
> present.
> h4. Reasoning
> Implementing {{getUser/Group(id)}} in an ExternalIdentityProvider can be
> difficult, because you need a way to search the external identity system
> efficiently by the local user id, which might not always be the case, if the
> external system uses another id and is only optimized for that.
> h4. Consequences
> # The only other place using {{ExternalIdentityProvider.getUser(String)}} is
> the
> [ExternalLoginModule|https://github.com/apache/jackrabbit-oak/blob/52f1e9a84324135e6a79678bbf209d03c0d2d77d/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java#L217],
> in case the user is pre-authenticated and does not exist locally yet.
> However, this is a specific use case that might not apply to all identity
> providers, in which case they could happily skip implementing this method. A
> note in the javadoc could clarify this for implementors.
> # {{ExternalIdentityProvider.getGroup(String)}} would then be used in no
> other place (in the sync code) and could even be deprecated, as I can't
> imagine another application specific use case for it.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
