Date
1 - 2 of 2
JetBrains' concerns with JSR-375
|
Will Hopkins
EG:
Forwarding this mail from JetBrains, with their permission. They are happier with the PFD spec, but did provide a list of concerns they have. They understand that it will be difficult to make any non-trivial changes to the spec at this point, but I think it's worth us reviewing the list to see if there's anything we want to do differently, and to respond to the concerns. Will -------- Forwarded Message --------
Thanks Trisha (and Sergey). This is very helpful feeback. Indeed, many (not all) of these questions have been discussed in the EG. At this point, I think it makes sense to include the entire EG in the conversation. Can I forward this to the EG mailing list? Alternatively, if you (or Sergey) would like to send your concerns directly to the list, that might facilitate a more direct conversation. Please note that, while we value the feedback, and may still be able to make small changes to the language of the spec, we are now just days away from our date to submit FAB materials. We would need a very compelling reason to make any non-trivial change to the API, and doing so would almost certainly cause us to miss our dates -- which would have a follow-on effect for the entire EE 8 release. Regards, Will On 07/27/2017 12:12 PM, Trisha Gee
wrote:
-- 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
|
||||||||||
|
|
||||||||||
|
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:
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.
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.
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.
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.
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.
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
|
||||||||||
|
|