Arjan/Rudy,
Thanks a lot for the suggestion. Arjan's reply for some reason was
considered "suspicious" and possible fishing or malware by Gmail. Not sure,
why?
I CC my reply to the new Google Group, because whatever Oracle may do about
JSR 375 after JavaOne, java.net is sure to go away and as of now there's no
sign JSRs, Glassfish or other projects (except OpenJDK which only uses the
mere URL but otherwise was said to have its own cluster for a long time
now) get seamless migration support of e.g. this mailing list or JIRA.
There is no harm sending future discussions to both lists. In the unlikely
event there's going to be some "java.oracle.com" or so hosting a new
mailing list and importing everything, we have a backup-copy. Otherwise all
that's left of the java.net list will be a huge tarball (if Alex has the
time to request it soon enouth, AFAIK only Spec Leads and project owners on
java.net are eligable to request that)
Kind Regards,
Werner Keil | JCP Executive Committee Member, JSR 363 Co Spec Lead |
Eclipse UOMo Lead, Babel Language Champion | Apache Committer
Twitter @wernerkeil | @UnitAPI | @JSR354 | @AgoravaProj | @DeviceMap
| #DevOps | #EclipseUOMo
Skype werner.keil | Google+ gplus.to/wernerkeil
On Wed, Aug 17, 2016 at 4:52 PM, arjan tijms <arjan.tijms_at_gmail.com> wrote:
> 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
>>
>>
>