See inline comments
On 18 August 2016 at 11:58, arjan tijms <arjan.tijms_at_gmail.com> wrote:
> Hi,
>
> On Thu, Aug 18, 2016 at 11:10 AM, Rudy De Busscher <rdebusscher_at_gmail.com>
> wrote:
>
>> 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)
>>
>
> I'm not sure if it really needs to know if authentication was already
> successful. This is likely something the handler can better coordinate, but
> maybe I'm not seeing something. At least in the current implementations of
> the stores you don't see the implementations of the stores actually doing
> something with the status. The embedded, database and ldap stores all only
> pass it through.
>
>
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.
*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
> The store most definitely needs the CallerPrincipal indeed.
>
>
>> - 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?
>>
>
> What about using a special credential type? There already is the
> CallerOnlyCredential
>
> See https://github.com/javaee-security-spec/soteria/blob/
> master/api/src/main/java/javax/security/identitystore/credential/
> CallerOnlyCredential.java
>
> If the status is really needed by the store, we could introduce one or
> more credential types for that, e.g.
>
> class AuthorizeOnlyCredential implement Credential {
> String callerName;
> Status status;
> // ctor + getters/setters
> }
>
> 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)
- 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.
>>
>
>