[
https://issues.apache.org/jira/browse/OAK-3886?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15148951#comment-15148951
]
Tobias Bocanegra edited comment on OAK-3886 at 2/16/16 5:30 PM:
----------------------------------------------------------------
Thanks! (a git pull request would be easier to comment on..)
bq. public interface SupportedCredentials
I don't like the name of this class, as it is used as provider interface for
the IDP. how about {{CredentialsSupport}}. Or even add a 2nd interface
{{CredentialsSupportProvider}} that has a {{CredentialsSupport
getCredentialsSupport()}}. this way, the {{getUserId(Credentials)}} doesn't
look awkward on the IDP impl.
bq. * implemenation and the corresponding {@link ExternalIdentityProvider}.
typo.
bq. supportedCredentials = SupportedCredentialsImpl.getInstance();
getInstance() is a bit confusing. how about {{getDefaultSupport()}} ?
bq. Object logId = (userId != null) ? userId : credentials;
could be {{final}}
bq. ExternalLoginModuleFactory
...has an unrelated change
bq. ExternalLoginModuleFactoryTest
...has an unrelated change
other than that, it looks ok.
was (Author: tripod):
Thanks! (a git pull request would be easier to comment on..)
> public interface SupportedCredentials
I don't like the name of this class, as it is used as provider interface for
the IDP. how about {{CredentialsSupport}}. Or even add a 2nd interface
{{CredentialsSupportProvider}} that has a {{CredentialsSupport
getCredentialsSupport()}}. this way, the {{getUserId(Credentials)}} doesn't
look awkward on the IDP impl.
> * implemenation and the corresponding {@link ExternalIdentityProvider}.
typo.
> supportedCredentials = SupportedCredentialsImpl.getInstance();
getInstance() is a bit confusing. how about {{getDefaultSupport()}} ?
> Object logId = (userId != null) ? userId : credentials;
could be {{final}}
> ExternalLoginModuleFactory
...has an unrelated change
> ExternalLoginModuleFactoryTest
...has an unrelated change
other than that, it looks ok.
> Delegate supported Credentials types to ExternalIdentityProvider
> ----------------------------------------------------------------
>
> Key: OAK-3886
> URL: https://issues.apache.org/jira/browse/OAK-3886
> Project: Jackrabbit Oak
> Issue Type: Improvement
> Components: auth-external
> Reporter: Alexander Klimetschek
> Attachments: OAK-3886.patch
>
>
> Currently, the ExternalLoginModule [only supports
> SimpleCredentials|https://github.com/apache/jackrabbit-oak/blob/cc78f6fdd122d1c9f200b43fc2b9536518ea996b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java#L415-L419].
> As the TODO says, it would be good to allow the ExternalIdentityProvider
> specify the supported types, in case they have custom authentication schemes
> that don't fit the username + password pattern of the SimpleCredentials.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
