Add Java EE Security 1.0 API examples #1
Conversation
|
@bbuerkle, this is the first example demonstrating usage of built-in |
|
Vinay - I can only comment on the grammar. Is that what you would like me to do? Also- I think you should add Will Hopkins to this review. |
wmhopkins
left a comment
There was a problem hiding this comment.
Overall, this looks great. I do have a few comments on ways we can better illustrate the API, and I think we need to eliminate the PlaintextPasswordHash. In general, we should be illustrating secure ways of doing things, not insecure ways. Is an LDAP example feasible? I think LDAP is more common than Database.
| @@ -0,0 +1,251 @@ | ||
| # Built-in Database Identity Store | ||
| JSR 375 mandates that a Java EE container MUST support built-in `IdentityStore` backed by a database. |
There was a problem hiding this comment.
We might want to use the LdapIdentityStore provider instead of database, as this will be a more common case.
There was a problem hiding this comment.
Agree, may be I will try to include LdapIdentityStore one as a seperate example and retain this one as well.
|
|
||
| This example demonstrates how a `DatabaseIdentityStore` can be configured to point to a backend database and how it can then be used as an `IdentityStore`. | ||
|
|
||
| In this example, following users are defined along with the roles they are in. |
There was a problem hiding this comment.
I think "groups" is more accurate than "roles" in this context.
| dataSourceLookup="${'jdbc/__default'}", // points to database bundled with Glassfish | ||
| callerQuery="#{'select password from caller where name = ?'}", | ||
| groupsQuery="select group_name from caller_groups where caller_name = ?", | ||
| hashAlgorithm = PlaintextPasswordHash.class, |
There was a problem hiding this comment.
We should not use PlaintextPasswordHash -- we don't want to even suggest that someone could do this. If we put it out there as an example, it will inevitably be included in at least some applications, and it's a huge security hole. The issue is moot if we use LdapIdentityStore. If we stay with this, we should make at least some minimal attempt to do hashing, but a "simple" hash might also be the wrong thing to do, because it may also be insecure, but will fool people into thinking they're doing secure password hashing. Password hashing is hard to get right on your own, which is the entire reason for having the Pbkdf2PasswordHash -- better to use that, unless we're going to put the effort into implementing some other secure password hash algorithm.
There was a problem hiding this comment.
I will replace PlaintextPasswordHash with Pbkdf2PasswordHash. I will try to include LdapIdentityStore as a separate example.
There was a problem hiding this comment.
Database example has been modified to use Pbkdf2PasswordHash. If time permits I will try to add an example with LdapIdentityStore.
| callerQuery="#{'select password from caller where name = ?'}", | ||
| groupsQuery="select group_name from caller_groups where caller_name = ?", | ||
| hashAlgorithm = PlaintextPasswordHash.class, | ||
| hashAlgorithmParameters = { |
There was a problem hiding this comment.
These parameters are meaningless, and therefore may be more likely to confuse than educate. If you want to have parameters, you should supply a PasswordHash impl that will actually use them. If you write your own, you should include an initialize() method that processes any parameters that you show.
There was a problem hiding this comment.
I will modify accordingly.
There was a problem hiding this comment.
Pbkdf2PasswordHash is being used, these parameters have been replace now:
public String[] getDyna() {
return new String[]{"Pbkdf2PasswordHash.Algorithm=PBKDF2WithHmacSHA512", "Pbkdf2PasswordHash.SaltSizeBytes=64"};
}| // and the container will actually handle the login after we return from | ||
| // this method. | ||
| return httpMessageContext.notifyContainerAboutLogin( | ||
| result.getCallerPrincipal(), result.getCallerGroups()); |
There was a problem hiding this comment.
Perhaps use result.getCallerPrincipal().getName(), which will demonstrate that the caller name can be passed as a string. I hope that passing CallerPrincipal as an argument does not become a pattern, because CallerPrincipal, ultimately, is not the platform caller principal in GF, and likely won't be on other platforms either. It is also not an "interesting" application caller principal, because it adds no value for an application. It carries nothing but a name. So it ends up as an extra principal carried around by the subject, but adding no value, and is passed here only because it happens to be returned by IdentityStoreHandler.
This is an area where I think the spec could have used more work to clarify the purpose and use of CallerPrincipal; unfortunately we ran out of time.
There was a problem hiding this comment.
Switched to BasicAuthenticationMechanism, got addressed as part of that.
| private IdentityStoreHandler identityStoreHandler; | ||
|
|
||
| @Override | ||
| public AuthenticationStatus validateRequest(HttpServletRequest request, HttpServletResponse response, HttpMessageContext httpMessageContext) throws AuthenticationException { |
There was a problem hiding this comment.
Maybe use the BasicAuthenticationMechanism instead, for this example? It will invoke any configured identity store.
There was a problem hiding this comment.
Agree. Will modify accordingly.
|
|
||
| ```java | ||
| return httpMessageContext.notifyContainerAboutLogin( | ||
| result.getCallerPrincipal(), result.getCallerGroups()); |
There was a problem hiding this comment.
Similar comment to above -- use result.getCallerPrincipal().getName()? It's probably reasonable to just use result.getCallerPrincipal() sometimes; we should show both methods in the various examples.
There was a problem hiding this comment.
Right. I will keep that in mind while adding examples in glassfish-samples. for tutorial-examples, I will change it to getName().
There was a problem hiding this comment.
As you suggested, I will be using BasicAuthenticationMechanism for this example, hence this too will get addressed as part of that.
| <dependency> | ||
| <groupId>javax.security.enterprise</groupId> | ||
| <artifactId>javax.security.enterprise-api</artifactId> | ||
| <version>1.0-b11</version> |
There was a problem hiding this comment.
Will update it to 1.0.
| * bundled with the application. | ||
| */ | ||
| @RequestScoped | ||
| public class TestAuthenticationMechanism implements HttpAuthenticationMechanism { |
There was a problem hiding this comment.
Use BasicAuthenticationMechanism instead, for this example?
There was a problem hiding this comment.
Yes, I will change it accordingly.
|
|
||
| public CredentialValidationResult validate(UsernamePasswordCredential usernamePasswordCredential) { | ||
|
|
||
| if (usernamePasswordCredential.compareTo("reza", "secret1")) { |
There was a problem hiding this comment.
Maybe add a comment that this is for illustrative purposes only, and that a real IdentityStore should include secure storage and credential validation capabilities? This example is so trivial that it's obviously not usable, but we don't want to encourage the use of hard-coded/in-memory stores, or user databases trivially provided as unencrypted files, etc.
There was a problem hiding this comment.
Right. I will add the comment as suggested.
bbuerkle
left a comment
There was a problem hiding this comment.
Hi Vinay - I edited the readme's for both examples.
|
|
||
| To support this mandatory requirement, `DatabaseIdentityStore` comes bundled with Glassfish-RI. | ||
|
|
||
| This example demonstrates how a `DatabaseIdentityStore` can be configured to point to a backend database and how it can then be used as an `IdentityStore`. |
There was a problem hiding this comment.
Change to: This example demonstrates how you can configure a "DatabaseIdentityStore" to point to a backend database and then use it as an "IdentityStore."
|
|
||
| This example demonstrates how a `DatabaseIdentityStore` can be configured to point to a backend database and how it can then be used as an `IdentityStore`. | ||
|
|
||
| In this example, following users are defined along with the roles they are in. |
There was a problem hiding this comment.
In this example, "the" following users are defined, along with the roles they are in. (add the and comma)
| |alex|secret2|foo,bar| | ||
| |arjan|secret2|foo| | ||
| |werner|secret2|foo| | ||
|
|
There was a problem hiding this comment.
You should not use real names here. It is best to use something more generic like Joe, Sam, Tom, Sue.
| |arjan|secret2|foo| | ||
| |werner|secret2|foo| | ||
|
|
||
| When a request is made to the application with certain credentials, the authentication mechanism bundled with this application comes into affect and an authentication is done against the `DatabaseIdentityStore` as defined in the application. |
There was a problem hiding this comment.
...the authentication mechanism bundled with the application comes into effect and authentication is performed against the ...
|
|
||
| When a request is made to the application with certain credentials, the authentication mechanism bundled with this application comes into affect and an authentication is done against the `DatabaseIdentityStore` as defined in the application. | ||
|
|
||
| Post authentication, the application will also verify the roles the caller is in and will send the details as part of the response. |
There was a problem hiding this comment.
Post authentication, the application also verifies the roles the caller is in and sends the details as part of the response.
| ```java | ||
| @DeclareRoles({ "foo", "bar", "kaz" }) | ||
| ``` | ||
| In Glassfish 5.0, role to group mapping is enabled by default. Hence there is no need to bundle `web.xml` with the application to provide mapping between roles and the groups in Glassfish. |
There was a problem hiding this comment.
In GlassFish 5.0, role to group mapping is enabled by default. Therefore, you do not need to bundle web.xml with the application to provide mapping between roles and groups.
| return httpMessageContext.responseUnauthorized(); | ||
| ``` | ||
|
|
||
| When a request is made to the application, the roles the user is in, gets returned as part of the repsonse. |
There was a problem hiding this comment.
...the roles the user is in get returned as part of the response.
There was a problem hiding this comment.
This class is going to be replaced completely with BasicAuthenticationMechanism as noted above.
|
|
||
| } | ||
| ``` | ||
| Notice that, the container itself needs to be made aware of the supported roles and it is done with the help of `@DeclareRoles` annotation as shown above. |
There was a problem hiding this comment.
Note that the container needs to be made aware of the supported roles, which is achieved with the help of the '@DeclareRoles' annotation as shown above.
| ``` | ||
| In Glassfish 5.0, role to group mapping is enabled by default. Hence there is no need to bundle `web.xml` with the application to provide mapping between roles and the groups in Glassfish. | ||
|
|
||
| In this example, we would be using credentials of user `reza` to make a request and see if response is according to credentails as specified in `TestIdentityStore` above. |
There was a problem hiding this comment.
In this example, we are using the credentials of user 'reza' to make a request and to validate the response according to the credentials specified in 'TestIdentityStore' above.
or
In this example, we are using the credentials of user 'reza' to make a request and to determine if the response is consistent with the credentials specified in 'TestIdentityStore' above.
Not sure which one is technically correct.
There was a problem hiding this comment.
The former one, have replaced accordingly.
|
|
||
| `asadmin deploy <project>/target/custom-identity-store.war` | ||
|
|
||
| Post which, a request can be made to the application using the URL shown below: |
There was a problem hiding this comment.
After the application is deployed, we can make a request to the application using the URL shown below:"
vinayvishal
left a comment
There was a problem hiding this comment.
@bbuerkle , I have incorporated most of the comments in README, please feel free to modify the contents if you still find any grammatical errors.
@wmhopkins , I will try to include LdapIdentityStore as a separate example. I will update the examples to incorporate your comments and will push the changes to PR.
| # Built-in Database Identity Store | ||
| JSR 375 mandates that a Java EE container MUST support built-in `IdentityStore` backed by a database. | ||
|
|
||
| To support this mandatory requirement, `DatabaseIdentityStore` comes bundled with Glassfish-RI. |
|
|
||
| To support this mandatory requirement, `DatabaseIdentityStore` comes bundled with Glassfish-RI. | ||
|
|
||
| This example demonstrates how a `DatabaseIdentityStore` can be configured to point to a backend database and how it can then be used as an `IdentityStore`. |
|
|
||
| This example demonstrates how a `DatabaseIdentityStore` can be configured to point to a backend database and how it can then be used as an `IdentityStore`. | ||
|
|
||
| In this example, following users are defined along with the roles they are in. |
|
|
||
| This example demonstrates how a `DatabaseIdentityStore` can be configured to point to a backend database and how it can then be used as an `IdentityStore`. | ||
|
|
||
| In this example, following users are defined along with the roles they are in. |
| |alex|secret2|foo,bar| | ||
| |arjan|secret2|foo| | ||
| |werner|secret2|foo| | ||
|
|
| ``` | ||
| In Glassfish 5.0, role to group mapping is enabled by default. Hence there is no need to bundle `web.xml` with the application to provide mapping between roles and the groups in Glassfish. | ||
|
|
||
| In this example, we would be using credentials of user `reza` to make a request and see if response is according to credentails as specified in `TestIdentityStore` above. |
There was a problem hiding this comment.
The former one, have replaced accordingly.
|
|
||
| `asadmin deploy <project>/target/custom-identity-store.war` | ||
|
|
||
| Post which, a request can be made to the application using the URL shown below: |
| <dependency> | ||
| <groupId>javax.security.enterprise</groupId> | ||
| <artifactId>javax.security.enterprise-api</artifactId> | ||
| <version>1.0-b11</version> |
There was a problem hiding this comment.
Will update it to 1.0.
| * bundled with the application. | ||
| */ | ||
| @RequestScoped | ||
| public class TestAuthenticationMechanism implements HttpAuthenticationMechanism { |
There was a problem hiding this comment.
Yes, I will change it accordingly.
|
|
||
| public CredentialValidationResult validate(UsernamePasswordCredential usernamePasswordCredential) { | ||
|
|
||
| if (usernamePasswordCredential.compareTo("reza", "secret1")) { |
There was a problem hiding this comment.
Right. I will add the comment as suggested.
30bf5ce to
a993cf1
Compare
a993cf1 to
760ba08
Compare
|
Being replaced by #3 . |
Demonstrates built-in DatabaseIdentityStore usage as specified in JSR 375. Details can be found in the README of this example.