Hi Arjan,
Thanks for your feedback.
A quick reply to your comments.
I wonder, would it be possible to omit the CallerPrincipal argument here?
> There's already a caller in the Credential, and I'm afraid this would
> confuse people who have to implement the interface.
I don't like the caller in the Credential interface
- Credentials are user supplied and should be separated from the "System
determined values". The caller(Principal) is something which is determined
based on the credentials and thus I should not mix these 2 sources.
- The current implementation of UsernamePasswordCredential uses the
user-supplied username as value. So there is no way to determine if the
'Caller' is already identified or not. And this is a key principal when the
Authorization and Authentication is separated in different identityStores.
- The unique caller identification doesn't need to be the same as
user-supplied username (you can decide to use the internal id/sequence,
email address, LDAP node cn value, ...)
callerPrincipal.getName() would be null here
This should never return null. callerPrincipal being not null means a
successful Authorization is carried out and thus the user should be
identifiable (and null is thus not an option)
maybe it's better to have ValidationType directly on the
> EmbeddedIdentityStoreDefinition (and other definitions)?
That is indeed an option.
Best regards
Rudy
On 4 September 2016 at 21:28, arjan tijms <arjan.tijms_at_gmail.com> wrote:
> Hi Rudy,
>
> Looks much improved, thanks!
>
> I have a few comments, but some are just cosmetic and could be looked at
> later.
>
> The most important one concerns the IdentityStore interface, it now has
> this method:
>
> public CredentialValidationResult validate(Credential credential,
> CallerPrincipal callerPrincipal) {
>
> I wonder, would it be possible to omit the CallerPrincipal argument here?
> There's already a caller in the Credential, and I'm afraid this would
> confuse people who have to implement the interface. I may also give the
> impression to implementors that they have to take the history of what
> happened before with other stores somehow into account. I think it would be
> best if the store itself is totally unaware of the concept of multiple
> stores, since that's what the handler should do. Since the handler is
> replaceable, a store implementation can't assume anything about it.
>
> In the implementations of the provided stores (e.g. the
> EmbeddedIdentityStore), there's this comment with some code below it.
>
> // We check also if caller != null to be sure the Authentication by
> another IdentityStore succeeded.
>
> If you look at the code flow that applies to it's:
>
> String caller = null;
> ...
> // We are Authorize Only mode, so get the caller determined
> previously.
> if (callerPrincipal != null) {
> caller = callerPrincipal.getName();
> }
>
> if (authenticated && caller != null) {
> ...
> }
>
> return INVALID_RESULT;
>
> So it callerPrincipal or callerPrincipal.getName() would be null here, the
> store would return INVALID_RESULT. But in that case, wouldn't it be better
> if the handler did not call the store at all? That would safe the store
> from having to check this case or even having to think of what other stores
> before it did. In other words, define in the contract and spec that if the
> store is used for authorization, the passed in caller is not null. Would
> that make sense?
>
> The idea here would be that a store is kept as simple as it can possibly
> be, as an often heard complaint is that all proprietary stores are quite
> complex. The multi-store algorithm should then 100% be with the handler.
>
> A somewhat cosmetic and implementation thing, but maybe still worth it to
> look at is this pattern in the stores:
>
> private EmbeddedIdentityStoreDefinition embeddedIdentityStoreDefinition;
>
> private ValidationType validationType;
>
> private void determineValidationType() {
> validationType = ValidationType.BOTH;
> if (embeddedIdentityStoreDefinition.authenticateOnly()) {
> validationType = ValidationType.AUTHENTICATION;
> } else {
> if (embeddedIdentityStoreDefinition.authorizeOnly()) {
> validationType = ValidationType.AUTHORIZATION;
> }
> }
> }
>
> public ValidationType validationType() {
> return validationType;
> }
>
> Seeing this code repeating, maybe it's better to have ValidationType
> directly on the EmbeddedIdentityStoreDefinition (and other definitions)?
>
> Then it could just be:
>
> public ValidationType validationType() {
> return embeddedIdentityStoreDefinition.validationType();
> }
>
> Another small thing, also cosmetic mostly, but there's now this kind of
> check often:
>
> if (validationType == ValidationType.AUTHENTICATION || validationType ==
> ValidationType.BOTH) {
> ...
> }
>
> What about making two helper methods on the ValidationType enum like e.g.
>
> public boolean isAuthentication() {
> return isOneOf(this, AUTHENTICATION, BOTH);
> }
>
> then the code using it could just do:
>
> if (validationType.isAuthentication()) {
> ...
> }
>
> Hope this feedback is useful ;)
>
> Kind regards,
> Arjan Tijms
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> On Sun, Sep 4, 2016 at 8:07 PM, Rudy De Busscher <rdebusscher_at_gmail.com>
> wrote:
>
>> Hi all,
>>
>> This is the updated multi IdentityStore based on earlier feedback on this
>> mailing list.
>>
>>
>> *Why multiple stores?*
>> There are various use cases where multiple identity stores can be useful.
>> And it allows us to decouple the authentication and authorization parts
>> of the current mechanism. So that groups can be retrieved by another store
>> then the credential checking.
>>
>> *Examples*
>> * When your application should allow users which are defined in multiple
>> stores, like an LDAP for your internal users and a Database store for the
>> external ones.
>> * Users are authenticated by an OAuth2 provider and the groups are
>> retrieved from a database table.
>> * The first IdentityStore based on OAuth2 allows any Google account, but
>> the second store limits the access to a certain google domain.
>>
>> *IdentityStore interface*
>>
>> * CredentialValidationResult validate(Credential credential,
>> CallerPrincipal callerPrincipal);*
>>
>> * default int priority() {*
>> * return 100;*
>> * }*
>>
>> * default ValidationType validationType() {*
>> * return ValidationType.BOTH;*
>> * }*
>>
>> *validate method*
>>
>> the CallerPrincipal parameter contains previous validated Caller
>> information (authentication) if any. Value can be null.
>>
>> *ValidationType method*
>>
>> ValidationType contains meta information for the IdentityStore handler.
>> It defines if the store does authentication only
>> (ValidationType.AUTHENTICATION), authorization only
>> (ValidationType.AUTHORIZATION) or both; the default.
>> This information is used by the handler to define if the store should be
>> called. For example, the default IdentityStore handler does not call a
>> store who does Authorization only when no successful authentication took
>> place.
>>
>> *priority method *
>>
>> determines the order in the case when there are multiple ones. They are
>> ordered by ascending values, meaning that the IdentityStore with the lowest
>> value is processed first.
>>
>> *Status rules*
>>
>> Stores should return the following Status (contained in the
>> CredentialValidationResult result parameter)
>> *NOT_VALIDATED*: Type of Credential isn't compatible with the ones
>> expected by the store.
>> *INVALID* : In case the store does Authentication, the validation of the
>> credential failed.
>> *AUTHENTICATED* : In case the store does Authentication only and the
>> validation succeeded.
>> *VALID* : In case the store does Authorization and groups are supplied
>> for the Caller (can be an empty list indicating no groups assigned to
>> caller)
>>
>> So When Store does handle both (Authentication and Authorization) it
>> should return NOT_VALIDATED, INVALID or VALID
>> In case store does Authentication only; it is NOT_VALIDATED, INVALID or
>> AUTHENTICATED
>> In case store does Authorization only; it is NOT_VALIDATED or VALID (the
>> assumption is that when store can handle the type of Credentials, the
>>
>> *IdentityStore Handler *
>>
>> Default IdentityStore Handler uses the following algorithm
>> 1) Order all identityStores based on the priority value
>> 2) Loop over all IdentityStores unless one of the stores return the
>> status INVALID
>> 3) Call a certain IdentityStore when
>> * The Store does Authentication, regardless of the result of the calls
>> to previous IdentityStores.
>> * The Store does Authorization only and another Store already
>> successful validated the Credentials (Result is AUTHENTICATED or VALID)
>> 4) The result of the call to the IdentityStore is combined with
>> 'previous' result of the there calls to the IdentityStores.
>> * Groups are added to the already defined groups of the caller.
>>
>> *Issues*
>>
>> The current default IdentityStore Handler handler can't handle this use
>> case
>> - Caller can be authenticated by one IdentityStore (for example LDAP or
>> Database)
>>
>> This is because the loop stops when an INVALID result is returned by a
>> IdentityStore. This is required to veto another successful authentication
>> like in this use case
>> - The first IdentityStore based on OAuth2 allows any Google account, but
>> the second store limits the access to a certain google domain.
>>
>> So probably we need some configuration to be able to combine both use
>> cases in one algorithm.
>>
>> *Code status*
>>
>> The current code needs probably some more tests and verification. I try
>> to write them in the following weeks.
>>
>> Changes for this updated proposal can be viewed in this commit
>> https://github.com/rdebusscher/soteria/commit/afa4cecf626266
>> d0ae400f806b0358ed7b07bc61
>> And are added to the first pull request to the Soteria repository.
>>
>> Best regards
>> Rudy
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Java EE Security API - JSR 375 - Experts" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to jsr375-experts+unsubscribe_at_googlegroups.com.
>> To post to this group, send email to jsr375-experts_at_googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/ms
>> gid/jsr375-experts/CAL%2Bwt-5QmUqZiZ9V2uTYXAE-A%2B%2BFrzNwYK
>> kYxmZBxaHyYLq15g%40mail.gmail.com
>> <https://groups.google.com/d/msgid/jsr375-experts/CAL%2Bwt-5QmUqZiZ9V2uTYXAE-A%2B%2BFrzNwYKkYxmZBxaHyYLq15g%40mail.gmail.com?utm_medium=email&utm_source=footer>
>> .
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>