Hi,
On Thu, Aug 18, 2016 at 4:35 PM, Rudy De Busscher <rdebusscher_at_gmail.com>
wrote:
>
> The stores definitely (at least the more advanced ones where there is a
> split between authentication and authorization or where authentication can
> come from multiple 'sources') needs to know the status.
>
Okay, would be great then to have an example of that ;)
In general I'd hope to have as many of the combination / merging logic in
the handler, so as much as is reasonably possible the handlers can
implement alternative rules for combining stores. I.e. the classic
authenticate-until-one-succeeds could be the default, but an alternative
user provided handler could implement e.g. an algorithm that mandates all
stores to succeed, or it could want the majority of the stores to succeed
etc.
The less logic is within the stores themselves and the more is in the
handler, the easier that would be.
> *The embedded, database and ldap stores all only pass it through.*
>
> Correct, a mistake of me. Thay should only perform their logic when
> status != VALID and != AUTHENTICATED
>
Isn't this something the handler could better control?
I.e. if they should not do anything when the status is that, the handler
could simply not call them. If identity stores *must* check the status,
then this is an extra complexity that I'd rather not see being forced into
every store unless absolutely needed.
> Currently credential types already direct a store what to do. Every store
>> is by its contract already mandated to switch on the credential types it
>> supports, and then take appropriate action for each of them.
>>
>>
>> Yes, but I'm not fond of that idea because
> - I don't like these constructs if (credentials instanceof xxx) and favor
> if (Status.AUTHENTICATED == xxx)
>
In general I agree, having to downcast is indeed what makes an API more
difficult to use. Still, in this case the API already asks for switching on
the credential. Perhaps regardless of this story we should see if we could
solve that problem.
I too would rather not see code like this effectively mandated:
@Override
public CredentialValidationResult validate(Credential credential) {
if (credential instanceof UsernamePasswordCredential) {
return validate((UsernamePasswordCredential) credential);
}
if (credential instanceof CallerOnlyCredential) {
return validate((CallerOnlyCredential) credential);
}
return NOT_VALIDATED_RESULT;
}
One solution could be to go back to the day 1 (or so) proposals of having
different interfaces and then have some code somewhere do the instance of
checks.
Maybe a Credential could contain a reference to the interface that handles
it. E.g
public class UsernamePasswordCredential extends AbstractClearableCredential
{
Class<?> identityStoreClass() {
return UsernamePasswordIdentityStore.class;
}
// ...
}
And then either the handler or the default implementation of
validate(Credential credential) uses that to somewhat dynamically check if
the store has the right interface. One con is that this does give the
XYZIdentityStore interface and the XYZCredential a dependency on each
other.
With that in place though a store could do something like:
public class DataBaseIdentityStore implements
UsernamePasswordIdentityStore, AuthorizeOnlyIdentityStore {
@Override
public CredentialValidationResult validate(UsernamePasswordCredential
usernamePasswordCredential) {
// ...
}
@Override
public CredentialValidationResult validate(AuthorizeOnlyCredential
authorizeOnlyCredential) {
// ...
}
}
What do you think about that?
Kind regards,
Arjan Tijms
> - When the store has another order in the chain, it can receive another
> type for the Credential parameter (AuthorizeOnlyCredential instead of
> UsernamePasswordCredential)
>
> But I agree, the partialValidationresult is not the best solution. But
> didn't found anything better (yet) which fulfills all the requirements (of
> passing Status and CallerCredentials) to the IdentityStore (except adding
> those 2 parameters).
>
>
>
>>
>>
>>> *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.
>>>
>>
>> This is already the case now. The CDI extension doesn't so much add user
>> provided beans itself, but just checks if there's a normally activated bean
>> available that implements the IdentityStore interface.
>>
>> The confusion may be with the build-in beans, which are only activated
>> using the *IdentityStoreDefinition annotations. These beans reside within
>> the Soteria jar and are already excluded from being scanned.
>>
>> With or without our own annotation, user provided IdentityStore
>> implementations should always normally be activated by CDI.
>>
>>
>>> 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.
>>>
>>
>> That's true, so just asking the developer to put an existing scope on the
>> IdentityStore would solve the issue as well ;)
>>
>> In fact, at the moment such scope is actually required as I recently
>> found out. Soteria doesn't yet check if the identity store it obtains is
>> (implicitly) @Dependent. If it is, it has to release the reference,
>> otherwise proxy/reference leaks will occur :O
>>
>> Kind regards,
>> Arjan Tijms
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>>
>>>
>>> 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/securit
>>>>> y/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/glassfis
>>>>> h/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/id
>>>>> entitystore/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/glassfis
>>>>> h/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.setCallerPrincipa
>>>>> l(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
>>>>>>
>>>>>>
>>>>>
>>>> --
>>>> 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/CAAGawe0esYx-FJiGWCDnyHLYUMi6Aw0AjMJ%3DSy
>>>> AzN5E17B%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.
>>>>
>>>
>>> --
>>> 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-6jiXHCKm1RnUzqB3VNgAQoh5X6XR04oX
>>> V2V4SuU9t0Lw%40mail.gmail.com
>>> <https://groups.google.com/d/msgid/jsr375-experts/CAL%2Bwt-6jiXHCKm1RnUzqB3VNgAQoh5X6XR04oXV2V4SuU9t0Lw%40mail.gmail.com?utm_medium=email&utm_source=footer>
>>> .
>>>
>>> For more options, visit https://groups.google.com/d/optout.
>>>
>>
>>
>