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:08 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
>
>
>
>