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
>