Hi All,
I do wonder if the getCallerGroups() and getCallerRoles() methods are
> needed here. An authentication mechanism would not likely use these,
> right?
Is indeed a good remark. What if some of those application don't need any
authorization, but just authentication.
And I know there are a few issues around CDI and generics, but I love to
see the interface based (or the authentication version) on a generic type.
So that my custom identityStore could return objects instead of Strings
public interface IdentityStore<T> {
List<T> getCallerGroups(String callerName);
List<T> getCallerRoles(String callerName);
public CredentialValidationResult validate(Credential credential);
}
Didn't had the time to check all the material (project deadline by the end
of the week and still quite some things to do :( ) but will the CRUD
version of the IdentityStore be an Extends of this readonly version?
Regards
Rudy
On 29 September 2015 at 13:57, 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
> >
>