On 9/29/15 8:21 AM, arjan tijms wrote:
> Ps.
>
> Regarding this interface:
>
> public interface JaasSubjectPrincipalResolver {
> String getCaller(Subject subject);
> List<String> getCallerGroups(Subject subject, String callerName);
> List<String> getCallerRoles(Subject subject, String callerName);
> }
>
> See https://github.com/javaee-security-spec/javaee-security-proposals/blob/master/identity-store-readonly/src/main/java/javax/security/identitystore/persistence/JaasSubjectPrincipalResolver.java
>
> May be worthwhile to remark that in a way this seems to solve a part
> of the following issue:
> https://java.net/jira/browse/JAVAEE_SECURITY_SPEC-11
>
> Namely, the part where it asks for:
>
> "Alternatively or perhaps additionally two new callback handlers could
> be introduced that write resp. read the proprietary principals from a
> given Subject. These handlers could work with the existing callback
> types CallerPrincipalCallback and GroupPrincipalCallback."
[Alex] Agreed, I will note that
> I do wonder why the callerName is needed as second parameter in the
> getCallerGroups method. As far as I know, a Subject normally would not
> carry groups for multiple (different) callers. It's always one subject
> per caller, although that single caller may be identified by different
> IDs (principals).
[Alex] Agreed, I guess at the time I was thinking it would be useful to
know the caller to provide some context, if even for logging. But
perhaps not.
>
> Kind regards,
> Arjan Tijms
>
>
>
>
>
> On Tue, Sep 29, 2015 at 1:57 PM, arjan tijms<arjan.tijms_at_gmail.com> wrote:
>> Hi,
>>
>> On Tue, Sep 29, 2015 at 7:42 AM, Alex Kosowski<alex.kosowski_at_oracle.com> wrote:
>>
>>> Features:
>>> 1. Follows the simple model: Credentials in -> Caller, groups, roles out
>>> 2. The IdentityStore is read only, no create/update/delete for callers,
>>> groups, roles or credentials
>>> 3. Caller, Groups and Roles are just Strings, and are searchable by regular
>>> expression
>>> 4. Credential support is extendable by using CDI Qualifiers to annotate a
>>> CredentialValidator implementation
>>> 5. Standardized support for credentials, including Username/Password, Basic
>>> Authentication, Token
>>> 6. Standardized support for persistence mechanisms, including file, LDAP,
>>> database, and a JAAS adapter
>> This looks very good. In fact, it's the best proposal I've seen till date.
>>
>> I do have a few comments (as usual :P):
>>
>> The IdentityStore's interface is now this:
>>
>> public interface IdentityStore {
>> List<String> getCallerGroups(String callerName);
>> List<String> getCallerRoles(String callerName);
>> public CredentialValidationResult validate(Credential credential);
>> }
>>
>> A tiny typo here; the "public" modifier is not needed of course on the
>> validate() method.
>>
>> The validate() method itself is really nice. Just passing in a
>> Credential is much better than the often seen {username, credential}
>> pair, as not every mechanism has an explicit username being passed in
>> (like e.g. certificates).
>>
>> The CredentialValidationResult is conceptually really nice as well. It
>> packs both the result status, as the result data. This is quite a bit
>> better as many existing mechanisms which depend on either an exception
>> being thrown (e.g. LoginException) or a null, and/or depend on
>> followup calls to the identity store for the groups and/or depend on
>> grabbing this data from some (TLS based) context.
>>
>> I do wonder if the getCallerGroups() and getCallerRoles() methods are
>> needed here. An authentication mechanism would not likely use these,
>> right? I do like to have the ability to ask what groups a user has
>> according to the identity store, but maybe these should be on a
>> different (optional?) interface, along with a third method
>> List<String> getCallers()? (which has to be carefully considered, as
>> the result may be too big for some stores).
>>
>> As for the implementation of CredentialValidationResult, it now
>> contains this constructor:
>>
>> public CredentialValidationResult(
>> Status status,
>> String callerName,
>> IdentityStore identityStore) {
>>
>> this(status, callerName,
>> getCallerGroups(status,callerName,identityStore),
>> getCallerRoles(status, callerName, identityStore));
>> }
>>
>> see: https://github.com/javaee-security-spec/javaee-security-proposals/blob/master/identity-store-readonly/src/main/java/javax/security/identitystore/CredentialValidationResult.java#L92
>>
>> This calls out to the following static method:
>>
>> private static List<String> getCallerGroups(Status status, String
>> callerName, IdentityStore identityStore) {
>> if (null == identityStore)
>> throw new NullPointerException("identityStore");
>>
>> if (VALID == status) {
>> return identityStore.getCallerGroups(callerName);
>> } else {
>> return null;
>> }
>> }
>>
>> I wonder if this was the main reason for having
>> getCallerGroups()/getCallerRoles() on the IdentityStore interface?
>>
>> For the sake of simplicity, maybe it's better to omit this constructor
>> and the associated private static methods?
>>
>>
>>> Here is a usage case example:
>>>
>>> @Inject
>>> IdentityStore idStore;
>> Perhaps it's better to have the credential type as a qualifier of the
>> IdentityStore?
>>
>> After all, the authentication mechanism already knows what the
>> credential type is (it has to interact with the user to obtain it and
>> instantiate a suitable type). With a qualified IdentityStore, a user
>> can have multiple stores available, one for each credential type.
>>
>> E.g.
>>
>> @Inject @CredentialCapable(UsernamePasswordCredential.class)
>> IdentityStore idStore;
>>
>> Then what an IdentityStore instance does inside its validate() method
>> is largely a concern of that implementation. The API can provide some
>> guidelines perhaps, but I would really hold back on putting any hard
>> requirements here (it's different of course for the default
>> implementations like mentioned in point 6 above).
>>
>> So you could have e.g. the following implementation:
>>
>> @RequestScoped
>> @CredentialCapable(UsernamePasswordCredential.class)
>> public QuickTestIdentityStore implements IdentityStore {
>>
>> public CredentialValidationResult validate(Credential credential) {
>> UsernamePasswordCredential usernamePasswordCredential =
>> (UsernamePasswordCredential) credential;
>>
>> if ("john".equals(usernamePasswordCredential.getCaller()) {
>> return new CredentialValidationResult(
>> VALID,
>> "john.harrison",
>> new ArrayList(asList("admin", "sysop", "dev")),
>> new ArrayList(asList("can_update_accounts", "can_see_updates"))
>> }
>>
>> return INVALID_RESULT;
>> }
>> }
>>
>> Regarding the name "UsernamePasswordCredential", shouldn't it be
>> "CallerNamePasswordCredential"?
>>
>> The constructor already hints at this, as "callerName" is passed in
>> for the "username":
>>
>> public UsernamePasswordCredential(String callerName, Password password)
>>
>>
>> Regarding the roles that are returned now; how does this interact with
>> group to role mapping? Can the identity store leave this empty, and
>> would the authorization system (e.g. JACC or proprietary) be
>> responsible for the mapping then (as happens today), or should the
>> IdentityStore always be injected with the GroupToRole mapper and do
>> this mapping?
>>
>> All existing authentication mechanisms and specifically JASPIC only
>> need groups. They'd have to ignore the roles anyway.
>>
>>
>> We may want to specify which things should never be null. E.g. should
>> both groups and roles not better always be the empty list instead of
>> null?
>>
>> Same thing for the CredentialValidationResult returned from validate.
>> Maybe this should also never be null?
>>
>> /**
>> * Validates the given credential.
>> *
>> * @param credential The credential
>> * @return The validation result, including associated caller
>> roles and groups. Never null.
>> */
>> public CredentialValidationResult validate(Credential credential);
>>
>>
>> Hope this feedback is useful.
>>
>> Kind regards,
>> Arjan Tijms
>>
>>
>>
>>
>>
>>
>>
>>> // For example, in a JASPIC SAM...
>>> String caller = null;
>>> List<String> groups = null;
>>> List<String> roles = null;
>>> CredentialValidationResult result;
>>> Credential cred;
>>>
>>> cred = new UsernamePasswordCredential("john",new Password("secret"));
>>> result = idStore.validate(cred);
>>> if (Status.VALID == result.getStatus()) {
>>> // authentication was successful
>>> caller = result.getCallerName();
>>> groups = result.getCallerGroups();
>>> roles = result.getCallerRoles();
>>>
>>> // Callback JASPIC
>>> } else {
>>> // Invalid or not validated
>>> }
>>>
>>> I completely rewrote the Proof of Concept (POC) from the previous iteration
>>> and updated the proposal design doc. Let's discuss any issues you may have
>>> with this design.
>>>
>>> Please comment on this proposal Google doc:
>>> https://docs.google.com/document/d/1xMa32W73gPYYo53wRX60WasDDTuC7YFlI0XBm3dRym8/edit?usp=sharing
>>>
>>> The proposal Google doc should be open for comments by anyone on the
>>> jsr375-experts_at_googlegroups.com Google group. If you are having trouble
>>> commenting, please let me know. To comment, click the Comments button on the
>>> top right of the document.
>>>
>>> Here is the POC in the GitHub Proposal Repo:
>>> https://github.com/javaee-security-spec/javaee-security-proposals/tree/master/identity-store-readonly
>>>
>>> Note that the DatabaseIdentityStore in the POC is without an implementation
>>> because I ran out of time.
>>>
>>> Here is the generated JavaDoc:
>>> https://javaee-security-spec.java.net/
>>>
>>> What do you think? Once we come to a consensus for an acceptable Identity
>>> Store API design, the proposal will become the basis for the Identity Store
>>> section of the spec.
>>>
>>> With regards,
>>> Alex
>>>