Hi Rudy,
Looks really cool and thanks for putting the effort in!
Big +1 from me on the very idea of having multiple stores. This is
essential for a large array of use cases, specifically the one you mention
in your example.
>Proposal : *IdentityStoreHandler* which has the same signature as the
IdentityStore.
+1
> IdentityStore interface receives a new method which determines the order
of the IdentityStores when there are multiple ones.
*int priority();*
I've been thinking about this for some time, contemplating whether
@Priority would be an easier option, but Guillermo mentioned correctly that
this would clash with @Alternative on an identity store. So with int
priority() being available as a default method in the interface, users who
don't care about priority don't really have to see it in their store
implementations (which is always a big benefit of annotations, that you can
use them optionally).
One alternative, again hinted at by Guillermo, is to have an annotation for
the identity store. We now kinda assume CDI scanning mode *all*, but users
may prefer scanning mode *annotated*. If we were to introduce an annotation
for the IdentityStore, we could also put the priority there. Then either
the handler could read it from the annotation, or the default method could.
The annotation would only be a convenience for custom stores not for the
build-in ones.
E.g.
@CustomIdentityStore(priority=100)
public class MyStore implement IdentityStore {
...
}
So +1 for me on the int priority() method, but worthwhile to discuss if
"some annotation" that we may need anyway could also be used.
>* IdentityStores are *ordered by ascending values of the priority value*.
This means that the IdentityStore with the lowest value is processed first.
see next bullet.
+1
>* The default IdentityStoreHandler follows this algorithm when the
validate() method is called:
- *Loop over all stores*, in order of the priority value, until
ValidationResult.Status is INVALID or loop is terminated.
- At the end of the loop, change the status in following cases
* NONE -> INVALID
* AUTHENTICATED -> VALID
+1 for that loop.
I do have a concern about the implementation as done by the current PR.
The IdentityStore interface has been modified with a
CredentialValidationResult as input (partialValidationResult).
See
https://github.com/rdebusscher/soteria/blob/f93aece6a04e57f073157b30ec5d50248bd50837/api/src/main/java/javax/security/identitystore/IdentityStore.java
As far as I can see, this is for the implementations (like
DataBaseIdentityStore) just an opaque object that they need to pass around
and eventually put into the CredentialValidationResult that they return.
E,g,
See
https://github.com/rdebusscher/soteria/blob/f93aece6a04e57f073157b30ec5d50248bd50837/impl/src/main/java/org/glassfish/soteria/identitystores/DataBaseIdentityStore.java#L81
The CredentialValidationResult constructor then performs some logic on the
passed in partial result:
See
https://github.com/rdebusscher/soteria/blob/f93aece6a04e57f073157b30ec5d50248bd50837/api/src/main/java/javax/security/identitystore/CredentialValidationResult.java#L166
I wonder if this logic could not be moved to the IdentityStoreHandler. This
one currently has a loop where it feeds the result continuously into the
available stores:
CredentialValidationResult result = CredentialValidationResult.NONE_RESULT;
Iterator<IdentityStore> storeIterator = identityStores.iterator();
while (storeIterator.hasNext() && result.getStatus() !=
CredentialValidationResult.Status.INVALID) {
result = storeIterator.next().validate(result, credential);
}
See
https://github.com/rdebusscher/soteria/blob/f93aece6a04e57f073157b30ec5d50248bd50837/impl/src/main/java/org/glassfish/soteria/cdi/DefaultIdentityStoreHandler.java#L67
This could become something like:
CredentialValidationResult globalResult = NONE_RESULT;
Iterator<IdentityStore> storeIterator = identityStores.iterator();
while (storeIterator.hasNext() && globalResult.getStatus() != INVALID) {
CredentialValidationResult latestResult =
storeIterator.next().validate(credential);
if (!isOneOf(latestResult.getStatus(), AUTHENTICATED, VALID) {
if (result != null) {
latestResult.setGroups(globalResult.getGroups());
latestResult.setCallerPrincipal(globalResult.getCallerPrincipal());
} else {
if (latestResult.getStatus() != NONE) {
throw new IllegalArgumentException("non null global result
required when Status is not NONE");
} else {
latestResult.setGroups(null);
latestResult.setCallerPrincipal(null);
}
}
if (globalResult != null && globalResult.getStatus() == VALID &&
latestResult.getStatus() == AUTHENTICATED) {
latestResult.setStatus(VALID);
}
globalResult = latestResult;
}
We could additionally split out the merging algorithm to a reusable method
(and this method could be in CredentialValidationResult, going full circle
;)), so custom handlers can re-use it if they want. By externalizing this
logic we not only keep the IdentityStore interface simpler and unburden the
developer from passing around a partial result (which they may not care
for), but it also allows custom handlers to customize the merge algorithm.
You'd then get e.g.
CredentialValidationResult globalResult = NONE_RESULT;
Iterator<IdentityStore> storeIterator = identityStores.iterator();
while (storeIterator.hasNext() && globalResult.getStatus() != INVALID) {
globalResult = CredentialValidationResult.merge(globalResult,
storeIterator.next().validate(credential));
}
Kind regards,
Arjan Tijms
On Sat, Aug 13, 2016 at 2:24 PM, Rudy De Busscher <rdebusscher_at_gmail.com>
wrote:
> Hi all,
>
> I have put together the text around my idea of the multi IdentityStore.
>
> The code is not yet completely ready and is missing an example and test.
> I'll try to create that in the next few days and make a pull request at
> that time.
>
> *Multiple IdentityStore proposal*
>
> *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.
>
> *Technical changes*
> * When multiple stores are allowed, we need another interface which can be
> injected into the AuthenticationMechanism.
> Proposal : *IdentityStoreHandler* which has the same signature as the
> IdentityStore.
>
> * IdentityStore interface receives a new method which determines the order
> of the IdentityStores when there are multiple ones.
> *int priority();*
>
> * The current **Definition annotations are extended by a member which is
> transferred to the priority value of the IdentityStore.
>
> * The *ValidationResult.Status enum* is extended with some constants.
> Values and meanings are
>
> *NONE* : The default status, indicating that no IdentityStore has
> been consulted yet.
>
> *AUTHENTICATED* : Indicates that the credential is validated but
> that no Groups are assigned yet.
>
> *NOT_VALIDATED* : Indicates that the credential could not be
> validated
>
> *INVALID* : Indicates that the credential is not valid after a
> validation attempt.
>
> *VALID* : Indicates that the credential is valid after a
> validation attempt and Groups are assigned.
>
> * IdentityStores are *ordered by ascending values of the priority value*.
> This means that the IdentityStore with the lowest value is processed first.
> see next bullet.
>
> * The default IdentityStoreHandler follows this algorithm when the
> validate() method is called:
> - *Loop over all stores*, in order of the priority value, until
> ValidationResult.Status is INVALID or loop is terminated.
> - At the end of the loop, change the status in following cases
> * NONE -> INVALID
> * AUTHENTICATED -> VALID
>
> * The IdentityStores logic is
> - If the store decides to do *authentication logic* (a store can
> also decide to perform only the logic in the case the Status is NONE or
> NOT_VALIDATED, meaning the user is not authenticated yet) it should set the
> status to
> + *NOT_VALIDATED* : When credentials could not be validated
> (should not set this Status when the Status was already AUTHENTICATED or
> VALID)
> + *AUTHENTICATED* : When credentials are valid but the store
> didn't assign any groups (Should not set this status when the original
> Status was VALID)
> + *INVALID* : Credentials are not valid and the store is
> allowed to veto another authentication by another IdentityStore.
> + *VALID* : When credentials are valid and the store did assign
> groups (which can be the empty set)
>
> - The IdentityStore can decide that only *authorization* is
> performed, and should only do this in the case where Status has the value
> AUTHENTICATED or VALID.
> When the store adds Groups, it should change the Status to VALID.
>
> Best regards
> Rudy
>
>