Hi,

Here are my thoughts about the current draft of the spec. Some of them are very subjective and some are about rewording phrases that I may have misunderstood due to my poor English.

• Page 4 - Contributor list has not been updated.
• General: links to external resources should have footnotes with the whole human readable url. Currently, printing the spec will make the links just disappear. For the same reason, links to sections of the spec should list the section they refer.
• P5 - Terminology: JASPIC Java Authentication SPI for Containers <---- Suggestion: add a note that it is a JCP spec. Not needed, but complimentary.
• p6 - application servers MUST provide a default mapping from group names to roles. [...] This default mapping MAY be overridden by proprietary configuration <----- I'd clarify that "may be overriden by EXPLICIT propierary configuration". As it currently reads out, GlassFish achieves this requirement by having a (disabled by default) option to do that mapping. Maybe that's the goal though and I'm just misunderstanding this. If that's the case, just ignore this point.
• p6 - an application’s callers <--- Typo: an application's caller.
• p8 - An HttpAuthenticationMechanism is a CDI bean <---- Rewording: what do you think about "HttpAuthenticationMechanisms are CDI beans". Just personal taste. Talking about it in singular just reads strange to me, like if it could be only one.
• p8 - Only the validateRequest() method must be implemented; default behaviors are specified for the other two methods. <--- Rewording: I think "Only the validateRequest() method is required to be implemented" better expresses the idea.
• p10 - If neither the application nor the container makes an HttpAuthenticationMechanism available, then HttpAuthenticationMechanism is not used.  <--- The container is required to provide implementations of a variety of authentication mechanisms. The fact is that they are disabled by default, but they are always there. That phrase makes me think that "the container MAY NOT always provide an implementation", but what it's trying to say is that they may not be enabled. I'm not saying the sentence is not well written. It's probably me, but I see it a bit confusing.
• p11 - or each of BASIC and BASIC <-- I think that was already commented somewhere. I guess that was meant to be BASIC and FORM
• p13 - getRequest(facesContext),getResponse(facesContext) <-- The code example calls to an ommited getRequest method, and also calls an statically imported (but with ommited imports) "withParams" method. Users may think both are statically imported or both ommited. I'd just inject the request and response objects. To gain some lines, I'd put the annotation and the property inline, e.g.:

                @Inject private HttpServletRequest;
                @Inject private HttpServletResponse;

• p14 - Annotations are shown that use the LoginToContinue class, but that artifact is not shown nor the package is given. Users will be forced to go to the JavaDoc of the annotation to check what that class does.
• p16 - and information sufficient to allow <-- Rewording: enough information?
• p16 - perform only perform context- and environment-independent <-- Typo
• p18 - that handle can handle <-- Typo
• p20 - Methods of the IdentityStore interface are shown, but the public interface declaration is ommited. I'd add this declaration to be consistent with the rest of the code fragments.
• p22 - A Java EE container MUST support built-in beans for the followingIdentityStoretypes, to beconfigured and made available via corresponding annotations <-- It may seem obvious, but where are these annotations expected to be used? I know they have to be used on classes discovered by the CDI runtime, but I would explicitly say that "implementations MUST support the use of these annotations on any CDI discovered type".
• p25 - boolean hasAccessToWebResource(String resource) <-- This is a concern about the API. I'm afraid people may misuse this forgetting that it only tests the GET method. When you configure the security constraints of a servlet or url pattern and you don't set the affected methods, all methods are protected. Users may think that a value of "true" returned from this method means that the user is granted access to every method. I know this is about educating developers, but I'd personally drop this method and keep just the other one, that requires methods to be passed.

Regards,

Guillermo González de Agüero

Hi again,

Some more thoughts (sorry for the inconvenience of sending through multiple emails):

• It is unspecified what happens when the @DatabaseIdentityStoreDefinition uses an unsupported hash/encoding algorithm (btw, the RI is ignoring those attributes now [1]). I'd personally expect the deployment to fail at least in the case when *only one* identity store is enabled, but its requirements cannot be satisfied.
  
• The @DatabaseIdentityStoreDefinition JavaDoc talks about making the supported options an enum. I don't think that's a good idea as new algorithms can always emerge and different platforms may support a wider range than others. I propose to create some String constants for commonly used hash/encoding algorithms (SHA1, SHA256 - BASE64, HEX, etc) and clarify that the platform is not required to support more than the ones bundled with Java SE.

Regards,

Guillermo González de Agüero.

[1] https://github.com/javaee-security-spec/soteria/blob/c5357aece090abad95c764a7cd8138b79ab26729/impl/src/main/java/org/glassfish/soteria/identitystores/DataBaseIdentityStore.java#L95

Guillermo González de Agüero

On Fri, May 26, 2017 at 7:29 PM, Guillermo González de Agüero <z06.guillermo@...> wrote:

Hi,

Here are my thoughts about the current draft of the spec. Some of them are very subjective and some are about rewording phrases that I may have misunderstood due to my poor English.

• Page 4 - Contributor list has not been updated.
• General: links to external resources should have footnotes with the whole human readable url. Currently, printing the spec will make the links just disappear. For the same reason, links to sections of the spec should list the section they refer.
• P5 - Terminology: JASPIC Java Authentication SPI for Containers <---- Suggestion: add a note that it is a JCP spec. Not needed, but complimentary.
• p6 - application servers MUST provide a default mapping from group names to roles. [...] This default mapping MAY be overridden by proprietary configuration <----- I'd clarify that "may be overriden by EXPLICIT propierary configuration". As it currently reads out, GlassFish achieves this requirement by having a (disabled by default) option to do that mapping. Maybe that's the goal though and I'm just misunderstanding this. If that's the case, just ignore this point.
• p6 - an application’s callers <--- Typo: an application's caller.
• p8 - An HttpAuthenticationMechanism is a CDI bean <---- Rewording: what do you think about "HttpAuthenticationMechanisms are CDI beans". Just personal taste. Talking about it in singular just reads strange to me, like if it could be only one.
• p8 - Only the validateRequest() method must be implemented; default behaviors are specified for the other two methods. <--- Rewording: I think "Only the validateRequest() method is required to be implemented" better expresses the idea.
• p10 - If neither the application nor the container makes an HttpAuthenticationMechanism available, then HttpAuthenticationMechanism is not used.  <--- The container is required to provide implementations of a variety of authentication mechanisms. The fact is that they are disabled by default, but they are always there. That phrase makes me think that "the container MAY NOT always provide an implementation", but what it's trying to say is that they may not be enabled. I'm not saying the sentence is not well written. It's probably me, but I see it a bit confusing.
• p11 - or each of BASIC and BASIC <-- I think that was already commented somewhere. I guess that was meant to be BASIC and FORM
• p13 - getRequest(facesContext),getResponse(facesContext) <-- The code example calls to an ommited getRequest method, and also calls an statically imported (but with ommited imports) "withParams" method. Users may think both are statically imported or both ommited. I'd just inject the request and response objects. To gain some lines, I'd put the annotation and the property inline, e.g.:

                @Inject private HttpServletRequest;
                @Inject private HttpServletResponse;

• p14 - Annotations are shown that use the LoginToContinue class, but that artifact is not shown nor the package is given. Users will be forced to go to the JavaDoc of the annotation to check what that class does.
• p16 - and information sufficient to allow <-- Rewording: enough information?
• p16 - perform only perform context- and environment-independent <-- Typo
• p18 - that handle can handle <-- Typo
• p20 - Methods of the IdentityStore interface are shown, but the public interface declaration is ommited. I'd add this declaration to be consistent with the rest of the code fragments.
• p22 - A Java EE container MUST support built-in beans for the followingIdentityStoretypes, to beconfigured and made available via corresponding annotations <-- It may seem obvious, but where are these annotations expected to be used? I know they have to be used on classes discovered by the CDI runtime, but I would explicitly say that "implementations MUST support the use of these annotations on any CDI discovered type".
• p25 - boolean hasAccessToWebResource(String resource) <-- This is a concern about the API. I'm afraid people may misuse this forgetting that it only tests the GET method. When you configure the security constraints of a servlet or url pattern and you don't set the affected methods, all methods are protected. Users may think that a value of "true" returned from this method means that the user is granted access to every method. I know this is about educating developers, but I'd personally drop this method and keep just the other one, that requires methods to be passed.

Regards,

Guillermo González de Agüero

Arjan,

Do you have any comment on Guillermo's points below? In particular, I'm interested in your thoughts on the risk posed by the single-arg hasAccessToWebResource() method, and the suggestion to remove it and always require that methods be specified. That seems like a good idea to me. The other question is a minor point about the example JSF code.

Thanks,

Will

-------- Forwarded Message --------

Subject:	Re: [javaee-security-spec] Public Review Draft Comments Guillermo González de Agüero
Date:	Mon, 10 Jul 2017 01:59:05 -0400
From:	Will Hopkins <will.hopkins@...>
To:	javaee-security-spec@javaee.groups.io

On 05/26/2017 01:29 PM, Guillermo González de Agüero wrote:

Hi,

Here are my thoughts about the current draft of the spec. Some of them are very subjective and some are about rewording phrases that I may have misunderstood due to my poor English.

• Page 4 - Contributor list has not been updated.

Updated (the full list was not available on the JSR page when the PDR was published).

• General: links to external resources should have footnotes with the whole human readable url. Currently, printing the spec will make the links just disappear. For the same reason, links to sections of the spec should list the section they refer.

TODO

• P5 - Terminology: JASPIC Java Authentication SPI for Containers <---- Suggestion: add a note that it is a JCP spec. Not needed, but complimentary.

Added.

• p6 - application servers MUST provide a default mapping from group names to roles. [...] This default mapping MAY be overridden by proprietary configuration <----- I'd clarify that "may be overriden by EXPLICIT propierary configuration". As it currently reads out, GlassFish achieves this requirement by having a (disabled by default) option to do that mapping. Maybe that's the goal though and I'm just misunderstanding this. If that's the case, just ignore this point.

Added the word "explicit"

• p6 - an application’s callers <--- Typo: an application's caller.

Plural seems correct to me -- does the application have just one caller?

• p8 - An HttpAuthenticationMechanism is a CDI bean <---- Rewording: what do you think about "HttpAuthenticationMechanisms are CDI beans". Just personal taste. Talking about it in singular just reads strange to me, like if it could be only one.

I do think it reads better the way you suggest, but I've been trying to avoid pluralizing any type names that appear in the spec, since the type names themselves aren't plural and the plural form therefore doesn't refer any actual type. I do it in a few places, but trying to keep it to a minimum.

• p8 - Only the validateRequest() method must be implemented; default behaviors are specified for the other two methods. <--- Rewording: I think "Only the validateRequest() method is required to be implemented" better expresses the idea.

Not sure I see much difference between "must be implemented" and "is required to be implemented", particularly given the explanatory text about default behavior ...

• p10 - If neither the application nor the container makes an HttpAuthenticationMechanism available, then HttpAuthenticationMechanism is not used.  <--- The container is required to provide implementations of a variety of authentication mechanisms. The fact is that they are disabled by default, but they are always there. That phrase makes me think that "the container MAY NOT always provide an implementation", but what it's trying to say is that they may not be enabled. I'm not saying the sentence is not well written. It's probably me, but I see it a bit confusing.

See your point. I think it should be clear in context that the container is required to provide several implementations, and that this is about what's enabled, but I've attempted to clarify the language.

• p11 - or each of BASIC and BASIC <-- I think that was already commented somewhere. I guess that was meant to be BASIC and FORM

Yup, already fixed.

• p13 - getRequest(facesContext),getResponse(facesContext) <-- The code example calls to an ommited getRequest method, and also calls an statically imported (but with ommited imports) "withParams" method. Users may think both are statically imported or both ommited. I'd just inject the request and response objects. To gain some lines, I'd put the annotation and the property inline, e.g.:

                @Inject private HttpServletRequest;
                @Inject private HttpServletResponse;

This is Arjan's code -- I'd like him to comment before changing it.

• p14 - Annotations are shown that use the LoginToContinue class, but that artifact is not shown nor the package is given. Users will be forced to go to the JavaDoc of the annotation to check what that class does.

Added some explanatory text.

• p16 - and information sufficient to allow <-- Rewording: enough information?

Left as is.

• p16 - perform only perform context- and environment-independent <-- Typo

Fixed.

• p18 - that handle can handle <-- Typo

Fixed.

• p20 - Methods of the IdentityStore interface are shown, but the public interface declaration is ommited. I'd add this declaration to be consistent with the rest of the code fragments.

Added declaration for IdentityStore, CredentialValidationResult, and IdentityStoreHandler.

• p22 - A Java EE container MUST support built-in beans for the followingIdentityStoretypes, to beconfigured and made available via corresponding annotations <-- It may seem obvious, but where are these annotations expected to be used? I know they have to be used on classes discovered by the CDI runtime, but I would explicitly say that "implementations MUST support the use of these annotations on any CDI discovered type".

Chose not to add this because it is implicit and if I added it there I'd need to comb through the spec for any other place a similar statement would need to be made.

• p25 - boolean hasAccessToWebResource(String resource) <-- This is a concern about the API. I'm afraid people may misuse this forgetting that it only tests the GET method. When you configure the security constraints of a servlet or url pattern and you don't set the affected methods, all methods are protected. Users may think that a value of "true" returned from this method means that the user is granted access to every method. I know this is about educating developers, but I'd personally drop this method and keep just the other one, that requires methods to be passed.

That's a good point; the other option would be to treat it as if all methods had been specified, rather than just get. That would eliminate the possibility of accidentally granting access to a method other than GET based on a true result from the call, because true would only be returned if access was granted to all methods. But it might be better to make the caller specify.

Unfortunately didn't see this earlier, would like to run it by Arjan.

Will

Regards,

Guillermo González de Agüero

-- 
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

Thanks, Arjan. I'll remove the single-arg method.

I think I'll leave the code snippet as is, only because time is short and there are still other changes to make.

On 07/10/2017 10:13 AM, arjan tijms wrote:

Hi,

That one method can be deleted if needed, it's just a convenience method really. 

About the example, it's about finding the balance between brevity and clarity. The intend was more to give an impression of the context the feature is being used in, not to be a fully working example with all imports, dependencies etc fully written out (for that the RI and specifically its test folder is more suited I think).

I wouldn't mind it being a bit more complete, but the intend at least was just to give an impression.

Kind regards,

Arjan Tijms

On Mon, Jul 10, 2017 at 3:16 PM, Will Hopkins <will.hopkins@...> wrote:

Arjan,

Do you have any comment on Guillermo's points below? In particular, I'm interested in your thoughts on the risk posed by the single-arg hasAccessToWebResource() method, and the suggestion to remove it and always require that methods be specified. That seems like a good idea to me. The other question is a minor point about the example JSF code.

Thanks,

Will

-------- Forwarded Message --------

Subject:	Re: [javaee-security-spec] Public Review Draft Comments Guillermo González de Agüero
Date:	Mon, 10 Jul 2017 01:59:05 -0400
From:	Will Hopkins <will.hopkins@...>
To:	javaee-security-spec@javaee.groups.io

On 05/26/2017 01:29 PM, Guillermo González de Agüero wrote:

Hi,

Here are my thoughts about the current draft of the spec. Some of them are very subjective and some are about rewording phrases that I may have misunderstood due to my poor English.

• Page 4 - Contributor list has not been updated.

Updated (the full list was not available on the JSR page when the PDR was published).

• General: links to external resources should have footnotes with the whole human readable url. Currently, printing the spec will make the links just disappear. For the same reason, links to sections of the spec should list the section they refer.

TODO

• P5 - Terminology: JASPIC Java Authentication SPI for Containers <---- Suggestion: add a note that it is a JCP spec. Not needed, but complimentary.

Added.

• p6 - application servers MUST provide a default mapping from group names to roles. [...] This default mapping MAY be overridden by proprietary configuration <----- I'd clarify that "may be overriden by EXPLICIT propierary configuration". As it currently reads out, GlassFish achieves this requirement by having a (disabled by default) option to do that mapping. Maybe that's the goal though and I'm just misunderstanding this. If that's the case, just ignore this point.

Added the word "explicit"

• p6 - an application’s callers <--- Typo: an application's caller.

Plural seems correct to me -- does the application have just one caller?

• p8 - An HttpAuthenticationMechanism is a CDI bean <---- Rewording: what do you think about "HttpAuthenticationMechanisms are CDI beans". Just personal taste. Talking about it in singular just reads strange to me, like if it could be only one.

I do think it reads better the way you suggest, but I've been trying to avoid pluralizing any type names that appear in the spec, since the type names themselves aren't plural and the plural form therefore doesn't refer any actual type. I do it in a few places, but trying to keep it to a minimum.

• p8 - Only the validateRequest() method must be implemented; default behaviors are specified for the other two methods. <--- Rewording: I think "Only the validateRequest() method is required to be implemented" better expresses the idea.

Not sure I see much difference between "must be implemented" and "is required to be implemented", particularly given the explanatory text about default behavior ...

• p10 - If neither the application nor the container makes an HttpAuthenticationMechanism available, then HttpAuthenticationMechanism is not used.  <--- The container is required to provide implementations of a variety of authentication mechanisms. The fact is that they are disabled by default, but they are always there. That phrase makes me think that "the container MAY NOT always provide an implementation", but what it's trying to say is that they may not be enabled. I'm not saying the sentence is not well written. It's probably me, but I see it a bit confusing.

See your point. I think it should be clear in context that the container is required to provide several implementations, and that this is about what's enabled, but I've attempted to clarify the language.

• p11 - or each of BASIC and BASIC <-- I think that was already commented somewhere. I guess that was meant to be BASIC and FORM

Yup, already fixed.

• p13 - getRequest(facesContext),getResponse(facesContext) <-- The code example calls to an ommited getRequest method, and also calls an statically imported (but with ommited imports) "withParams" method. Users may think both are statically imported or both ommited. I'd just inject the request and response objects. To gain some lines, I'd put the annotation and the property inline, e.g.:

                @Inject private HttpServletRequest;
                @Inject private HttpServletResponse;

This is Arjan's code -- I'd like him to comment before changing it.

• p14 - Annotations are shown that use the LoginToContinue class, but that artifact is not shown nor the package is given. Users will be forced to go to the JavaDoc of the annotation to check what that class does.

Added some explanatory text.

• p16 - and information sufficient to allow <-- Rewording: enough information?

Left as is.

• p16 - perform only perform context- and environment-independent <-- Typo

Fixed.

• p18 - that handle can handle <-- Typo

Fixed.

• p20 - Methods of the IdentityStore interface are shown, but the public interface declaration is ommited. I'd add this declaration to be consistent with the rest of the code fragments.

Added declaration for IdentityStore, CredentialValidationResult, and IdentityStoreHandler.

• p22 - A Java EE container MUST support built-in beans for the followingIdentityStoretypes, to beconfigured and made available via corresponding annotations <-- It may seem obvious, but where are these annotations expected to be used? I know they have to be used on classes discovered by the CDI runtime, but I would explicitly say that "implementations MUST support the use of these annotations on any CDI discovered type".

Chose not to add this because it is implicit and if I added it there I'd need to comb through the spec for any other place a similar statement would need to be made.

• p25 - boolean hasAccessToWebResource(String resource) <-- This is a concern about the API. I'm afraid people may misuse this forgetting that it only tests the GET method. When you configure the security constraints of a servlet or url pattern and you don't set the affected methods, all methods are protected. Users may think that a value of "true" returned from this method means that the user is granted access to every method. I know this is about educating developers, but I'd personally drop this method and keep just the other one, that requires methods to be passed.

That's a good point; the other option would be to treat it as if all methods had been specified, rather than just get. That would eliminate the possibility of accidentally granting access to a method other than GET based on a true result from the call, because true would only be returned if access was granted to all methods. But it might be better to make the caller specify.

Unfortunately didn't see this earlier, would like to run it by Arjan.

Will

Regards,

Guillermo González de Agüero

-- 
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

It probably has already gone by now but +1 to removal - I have seen bug reports in the past where code has performed checks like this and not realised it is only checking GET whilst allowing requests in for all other methods.

On Mon, 10 Jul 2017 at 15:15 Will Hopkins <will.hopkins@...> wrote:

Thanks, Arjan. I'll remove the single-arg method.

I think I'll leave the code snippet as is, only because time is short and there are still other changes to make.

On 07/10/2017 10:13 AM, arjan tijms wrote:

Hi,

That one method can be deleted if needed, it's just a convenience method really. 

About the example, it's about finding the balance between brevity and clarity. The intend was more to give an impression of the context the feature is being used in, not to be a fully working example with all imports, dependencies etc fully written out (for that the RI and specifically its test folder is more suited I think).

I wouldn't mind it being a bit more complete, but the intend at least was just to give an impression.

Kind regards,

Arjan Tijms

On Mon, Jul 10, 2017 at 3:16 PM, Will Hopkins <will.hopkins@...> wrote:

Arjan,

Do you have any comment on Guillermo's points below? In particular, I'm interested in your thoughts on the risk posed by the single-arg hasAccessToWebResource() method, and the suggestion to remove it and always require that methods be specified. That seems like a good idea to me. The other question is a minor point about the example JSF code.

Thanks,

Will

-------- Forwarded Message --------

Subject:	Re: [javaee-security-spec] Public Review Draft Comments Guillermo González de Agüero
Date:	Mon, 10 Jul 2017 01:59:05 -0400
From:	Will Hopkins <will.hopkins@...>
To:	javaee-security-spec@javaee.groups.io

On 05/26/2017 01:29 PM, Guillermo González de Agüero wrote:

Hi,

Here are my thoughts about the current draft of the spec. Some of them are very subjective and some are about rewording phrases that I may have misunderstood due to my poor English.

• Page 4 - Contributor list has not been updated.

Updated (the full list was not available on the JSR page when the PDR was published).

• General: links to external resources should have footnotes with the whole human readable url. Currently, printing the spec will make the links just disappear. For the same reason, links to sections of the spec should list the section they refer.

TODO

• P5 - Terminology: JASPIC Java Authentication SPI for Containers <---- Suggestion: add a note that it is a JCP spec. Not needed, but complimentary.

Added.

• p6 - application servers MUST provide a default mapping from group names to roles. [...] This default mapping MAY be overridden by proprietary configuration <----- I'd clarify that "may be overriden by EXPLICIT propierary configuration". As it currently reads out, GlassFish achieves this requirement by having a (disabled by default) option to do that mapping. Maybe that's the goal though and I'm just misunderstanding this. If that's the case, just ignore this point.

Added the word "explicit"

• p6 - an application’s callers <--- Typo: an application's caller.

Plural seems correct to me -- does the application have just one caller?

• p8 - An HttpAuthenticationMechanism is a CDI bean <---- Rewording: what do you think about "HttpAuthenticationMechanisms are CDI beans". Just personal taste. Talking about it in singular just reads strange to me, like if it could be only one.

I do think it reads better the way you suggest, but I've been trying to avoid pluralizing any type names that appear in the spec, since the type names themselves aren't plural and the plural form therefore doesn't refer any actual type. I do it in a few places, but trying to keep it to a minimum.

• p8 - Only the validateRequest() method must be implemented; default behaviors are specified for the other two methods. <--- Rewording: I think "Only the validateRequest() method is required to be implemented" better expresses the idea.

Not sure I see much difference between "must be implemented" and "is required to be implemented", particularly given the explanatory text about default behavior ...

• p10 - If neither the application nor the container makes an HttpAuthenticationMechanism available, then HttpAuthenticationMechanism is not used.  <--- The container is required to provide implementations of a variety of authentication mechanisms. The fact is that they are disabled by default, but they are always there. That phrase makes me think that "the container MAY NOT always provide an implementation", but what it's trying to say is that they may not be enabled. I'm not saying the sentence is not well written. It's probably me, but I see it a bit confusing.

See your point. I think it should be clear in context that the container is required to provide several implementations, and that this is about what's enabled, but I've attempted to clarify the language.

• p11 - or each of BASIC and BASIC <-- I think that was already commented somewhere. I guess that was meant to be BASIC and FORM

Yup, already fixed.

• p13 - getRequest(facesContext),getResponse(facesContext) <-- The code example calls to an ommited getRequest method, and also calls an statically imported (but with ommited imports) "withParams" method. Users may think both are statically imported or both ommited. I'd just inject the request and response objects. To gain some lines, I'd put the annotation and the property inline, e.g.:

                @Inject private HttpServletRequest;
                @Inject private HttpServletResponse;

This is Arjan's code -- I'd like him to comment before changing it.

• p14 - Annotations are shown that use the LoginToContinue class, but that artifact is not shown nor the package is given. Users will be forced to go to the JavaDoc of the annotation to check what that class does.

Added some explanatory text.

• p16 - and information sufficient to allow <-- Rewording: enough information?

Left as is.

• p16 - perform only perform context- and environment-independent <-- Typo

Fixed.

• p18 - that handle can handle <-- Typo

Fixed.

• p20 - Methods of the IdentityStore interface are shown, but the public interface declaration is ommited. I'd add this declaration to be consistent with the rest of the code fragments.

Added declaration for IdentityStore, CredentialValidationResult, and IdentityStoreHandler.

• p22 - A Java EE container MUST support built-in beans for the followingIdentityStoretypes, to beconfigured and made available via corresponding annotations <-- It may seem obvious, but where are these annotations expected to be used? I know they have to be used on classes discovered by the CDI runtime, but I would explicitly say that "implementations MUST support the use of these annotations on any CDI discovered type".

Chose not to add this because it is implicit and if I added it there I'd need to comb through the spec for any other place a similar statement would need to be made.

• p25 - boolean hasAccessToWebResource(String resource) <-- This is a concern about the API. I'm afraid people may misuse this forgetting that it only tests the GET method. When you configure the security constraints of a servlet or url pattern and you don't set the affected methods, all methods are protected. Users may think that a value of "true" returned from this method means that the user is granted access to every method. I know this is about educating developers, but I'd personally drop this method and keep just the other one, that requires methods to be passed.

That's a good point; the other option would be to treat it as if all methods had been specified, rather than just get. That would eliminate the possibility of accidentally granting access to a method other than GET based on a true result from the call, because true would only be returned if access was granted to all methods. But it might be better to make the caller specify.

Unfortunately didn't see this earlier, would like to run it by Arjan.

Will

Regards,

Guillermo González de Agüero

-- 
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