All,
*partialValidationResult* -> I was also not completely happy with the
solution, but I believe we need feedback of the current status within the
IdentityStore.
For example
- When a Store does only authorization, it needs to know if the
authentication was already successful and what the callerPrincipal is
(because it will be used to determine the authorization info)
- When multiple stores can do the authentication, the second store should
not set the result to INVALID when the first already verified the
credentials successfully.
- etc ...
So IdentityStore needs status and CallerPrincipal, which is almost all the
things within CredentialValidationResult.
The use of the constructor was to keep the CredentialValidationResult as an
unmodifiable entity as it was before. (and should be so that the info can't
be modified easily afterward.)
Only pass Status and CallerPrincipal as parameters, keep this solution or
someone another idea?
*Regarding Annotated / All discovery*
When in annotated mode, only beans with a scope are considered. So when we
introduce @IdentityStore, it needs to be a scope or it needs to be
recognized by the CDI extension and registered 'manually'
And we need to veto the normal procedure because otherwise the bean gets
defined as CDI (because fulfills all requirements) and will be added by CDI
extension.
When we don't define @IdentityStore, the developer can always annotate his
own implementations of the IdentityStore with @RequestScoped or
@ApplicationScoped when he is using the annotated mode.
Any other thoughts about this?
Best regards
Rudy
On 17 August 2016 at 17:07, Werner Keil <werner.keil_at_gmail.com> wrote:
> 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/f93aece6a04e
>> 57f073157b30ec5d50248bd50837/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/f93aece6a04e
>> 57f073157b30ec5d50248bd50837/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/f93aece6a04e57f0
>> 73157b30ec5d50248bd50837/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.NON
>> E_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/f93aece6a04e
>> 57f073157b30ec5d50248bd50837/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.getCallerPrinci
>> pal());
>> } 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
>>>
>>>
>>
> --
> 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/
> msgid/jsr375-experts/CAAGawe0esYx-FJiGWCDnyHLYUMi6Aw0AjMJ%
> 3DSyAzN5E17B%2BP9Q%40mail.gmail.com
> <https://groups.google.com/d/msgid/jsr375-experts/CAAGawe0esYx-FJiGWCDnyHLYUMi6Aw0AjMJ%3DSyAzN5E17B%2BP9Q%40mail.gmail.com?utm_medium=email&utm_source=footer>
> .
> For more options, visit https://groups.google.com/d/optout.
>