Re: JetBrains' concerns with JSR-375


Arjan Tijms
 

Hi,

Thanks a bunch for providing the comments. Good feedback, but as Will mentioned it's indeed very late in the process.

Let me review the list as requested:

  • Core IdentityStore interface: in the IntelliJ IDEA API we don’t accept commits with interfaces like: there are 2 methods, you could implement the first, the second or both and provide the third method that returns what was really implemented. Are there other alternatives? E.g. splitting into multiple interfaces?

This has indeed been a concern and a personal concern for me as well. For the longest time theIdentityStore interface indeed only had a single method; validate(). 

But then we started with the multi-store epic and it turned out to be quite challenging. The key here is that an identity store is not inherently bound to one of those 2 mentioned methods, with the third one describing what was implemented. That would be awkward indeed.

The point is that an identity store can natively implement both of these methods and can then be configured (externally, by for instance the application developer) to be used for either one or both of these purposes. So the third method does not return what was implemented, it returns where the identity store is to be used for.

The current design evolved from a lot of discussion with multiple proposals being implemented and rejected again. At some point we spend so much time on this that we could not really afford to go on with it. Given more time we perhaps could have arrived at a better solution, but we finally reached consensus about this solution, and it worked, so we stuck with it. I personally was hoping to be able to revise some aspects if there was time left at some point, but that time was never there. Somewhat planned was moving the configuration (priority() and validationTypes()) to an external structure of some kind.

Additionally it must be noted that the IdentityStore interface is a little less developer facing than what it may seem to be. The main consumer of it (the authentication mechanism), uses the IdentityStoreHandler (https://github.com/javaee/security-api/blob/master/src/main/java/javax/security/enterprise/identitystore/IdentityStoreHandler.java), which is an interface that only has a single method.

 
  • It looks strange to have in IdentityStore "Set<String> getCallerGroups(CredentialValidationResult validationResult)” and "Set<String> getCallerGroups()" in CredentialValidationResult. It has been explained, it will work, but implementers have to remember all details, it looks like a place for potential bugs. 
To quickly go over this; the validate() method would normally do authentication and the return of the groups associated with the authenticated user in one, potentially optimised, operation. "Set<String> getCallerGroups(CredentialValidationResult validationResult)” splits the latter part of that operation off to an individual method, for those cases where you only want to retrieve the groups.

This is indeed the duality that's being seen: CredentialValidationResult is the result of the operation that does both authentication and the returning of those groups, which by itself appeared to be very common after studying existing implementations.

Conceptually, it would have been cleaner to have a separate validate() method doing only validation, and then always having to call getCallerGroups() for the groups, but that would have made the very common optimisation of doing both of these things in one operation impossible or at least quite hard (pushing extra complexity to the implementation and thus extra complexity to the TCK and other tests).

Using CredentialValidationResult as parameter of the getCallerGroups call is perhaps another small additional source of confusion. Basically the signature was  "Set<String> getCallerGroups(CallerPrincipal callerPrincipal)”, but there were concerns raised about potentially retrieving groups for callers in different stores with non-unique names (for instance two different callers both with the name "john" existing in a DB store and an LDAP store).

These concerns were countered with it being the application or its configurator's responsibility to make sure names are unique among the stores that are configured, but no consensus was reached over this and hence the extra certainty of passing in the CredentialValidationResult was used.
 

  • What is the rule for naming caller groups? Potential issues we can foresee include duplicates, conflicts, non-related group invocations, etc.  

This version of the specification puts no constraints on the names of caller groups. Like in most existing Java EE proprietary implementations and in fact the Java EE specification regarding the closely related concept of roles, the names are completely free form and it's up to the application code how these are used. For instance, a name can represent a simple well understood real-life group name such as "administrator", or it could be a fine grained business rule like name such as "is_able_to_view_margins".

If there's any ambiguity, then it's up to the deployer and/or whatever application level mechanism can be used to map the group names to application logical local role names. The latter are by definition not conflicting or duplicated within a given single application. We had hoped to standardise this group to role mapping as well, but unfortunately didn't have the resources to also include this in the 1.0 version of the spec.

In the meantime at least the container specific mapping facilities can be used for this, something which is quite commonly available in Java EE implementations. Hopefully this can be standardises in the next revision of the spec.
 

  • Annotations: @LdapIdentityStoreDefinition contains 20+ attributes.  Is it likely some bean will need all these non default attribute values? It could lead to some difficult-to-read code
This concern was not raised before, but I can't say I entirely disagree with this. LDAP though is a notoriously flexible "format", by which I mean that the information can be stored in a very large number of different ways. An alternative could have been to have multiple @LdapIdentityStoreDefinition annotations, each with a specific kind of approach to retrieving the data we need from LDAP. I'm not sure if that would have made things simpler or not.

 
  • “Chapter 2: Authentication mechanism” describes the HttpAuthenticationMechanism interface and "is specified only for use in the Servlet container". Isn't this more an extension of Servlet 3.x specification?
Yes, in a way it certainly is. Like the JASPIC Container Servlet Profile on which it builds, this could potentially have been put in the Servlet specification as well.

However, the way the JCP and specs are currently organised makes it non-trivial to have the kind of inter-spec coordination needed to realise this. Several attempts were made though; the Servlet EG was asked to take ownership of the so-called HttpServletRequest CDI build-in bean (a kind of producer that makes the injection of HttpServletRequest possible). This however was not entirely understood by the Servlet EG, in part because not all members are fully up to date with what CDI exactly entails. With this quite simple CDI artefact being rejected, the chances of the Servlet EG incorporating the larger HttpAuthenticationMechanism, which is heavily dependent on CDI were deemed slim.

Additionally, the mentioned JASPIC Container Servlet Profile, on which the HttpAuthenticationMechanism directly builds, did not make it into the Servlet spec either. At some point the Servlet EG seemed willing to adopt it (in fact, the Servlet spec itself already encourages this, but does not demand it), but ultimately this did not make it in.

That all said, being an extension of the Servlet spec or building on it, is not unprecedented. JSF for instance builds on Servlet and is its own spec as well.
 

  • IdentityStore priority: “Lower numbers represent higher priority”. But “getPriority()” methods in the JDK (Thread, AWT) have “higher numbers for higher priority”.  The IntelliJ API, Spring, Hibernate and other frameworks use the same concept.  Is “lower numbers for higher priority” is used in OS threads? Or what's the reasoning behind this decision?

The reasoning behind it is more about ordering. Lower priority stores are consulted first, higher priority later. This is basically how the Interceptor Spec works as well; lower priority interceptors are put first in the chain, higher priority ones later, and then the chain is executed from the beginning. An interceptor has the ability to not continue the chain, so in a certain way an earlier (lower level priority) interceptor is more powerful than a later level. 

The default traversal algorithm for stores in JSR 375 uses a "first one to validate successfully wins" strategy, but this is not necessarily the inherent way that the identity store priority has to be seen. The traversal algorithm (handler, really) is replaceable, and could use a strategy such that later (higher priority) stores essentially "win".

See also the documentation on the @Priority annotation, which is what this decision was directly based on:

/**
 * The Priority annotation can be applied to classes to indicate
 * in what order the classes should be used.  The effect of using
 * the Priority annotation in any particular instance is defined
 * by other specifications that define the use of a specific class.
 * <p>
 * For example, the Interceptors specification defines the use of
 * priorities on interceptors to control the order in which
 * interceptors are called.
 * <p>
 * Priority values should generally be non-negative, with negative values
 * reserved for special meanings such as "undefined" or "not specified".
 * A specification that defines use of the Priority annotation may define
 * the range of allowed priorities and any priority values with special
 * meaning.
 *
 * @since Common Annotations 1.2
 */
@Target({TYPE})
@Retention(RUNTIME)
@Documented
public @interface Priority

Hope this clears it up at least a little. Given the point we are in the process I'm not sure how much if any action we can still take, other than perhaps some clarification in the spec text and/or javadoc.

Kind regards,
Arjan Tijms


 
If you have further questions about the feedback, please feel free to contact Sergey directly, or myself. And we'll be more than happy to either receive email clarification on the questions, or links to relevant discussions in the mailing list archives - we acknowledge we're late to the process and that some (or all) of these points may have been discussed previously.  Of course, any confusion we have may also be shared by end users!

Thanks once again for reaching out and trying to understand our issues.

Trisha

On Tue, Jul 25, 2017 at 7:15 PM Will Hopkins <will.hopkins@...> wrote:
Great, thanks for letting me know. We actually have a couple more tweaks planned for an updated PFD that should be available shortly, I'll forward that along when it's ready.

Let us know if anything further comes up.


Will


On 07/25/2017 06:33 AM, Trisha Gee wrote:
Those who had concerns with the spec are much happier with the updated version.  I *still* haven't had more detailed feedback from those who had concerns (sorry), I will give them a poke.

On 23 July 2017 at 23:08, Will Hopkins <will.hopkins@...> wrote:
Hi Trisha,

Just wanted to touch base and see if there was any further feedback ...

Regards,

Will


On 07/11/2017 01:30 PM, Will Hopkins wrote:
Thanks, Trisha.

On 07/11/2017 01:12 PM, Trisha Gee wrote:

Thank you so much for reaching out and for providing the updated spec. I appreciate you have a difficult job balancing delivering something in a reasonable time frame that also provides a good set of standards for the future, and I also appreciate that our feedback is rather late in the process (but we are new to the EC!).

I will pass on the updated spec to our (JetBrains) EC working group, specifically to those who had concerns around this JSR, and will also be chasing them again for the more detailed feedback I have promised you. I'll most likely be putting you directly in touch with those who had specific concerns so I'm not getting in the way as a middle man.

Trisha


On Tue, 11 Jul 2017, 19:02 Will Hopkins, <will.hopkins@...> wrote:
Hi Trisha,

I'm the spec lead for JSR-375, and wanted to reach out to you about the concerns expressed with your "no" vote on the JSR-375 Public Review ballot.

I'm obviously interested to hear the detailed feedback you mentioned, but in advance of that I want to provide you with an updated version of the spec (attached), and the accompanying javadoc (https://www.dropbox.com/s/1bq7n8sz25eyfcb/jsr375-javadoc-1.0-b07-pfd.zip?dl=0), which we have just submitted as a Proposed Final Draft. I think it should address at least some of your concerns. In particular, it provides a solution for the issue of static attribute values for the LDAP identity store and other annotations (Issue #76) -- all annotation attributes can now be specified as EL expressions.

I'd also like to address the concern about lack of support for OpenID (I assume this means OpenID Connect), and potentially other technologies. We made an explicit decision not to include OpenID Connect, because we didn't think it would fit in the time we had available, but we do agree that OpenID Connect, and OAuth2, are important technologies. The idea was to provide a base on which features like OpenID Connect could be built, and we do think that JSR-375 is useful, both on its own merits, and as an enabling technology for future additions.

We look forward to your detailed feedback.

Best Regards,

Will Hopkins
Spec Lead
JSR-375

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803

-- 
Will Hopkins | WebLogic Security Architect | +1.781.442.0310
Oracle Application Development
35 Network Drive, Burlington, MA 01803


Join javaee-security-spec@javaee.groups.io to automatically receive all group messages.