JDBC QueryAndEncodeDatabaseAuthenticationHandler #665
Conversation
| /** | ||
| * Instantiates a new Prefix suffix principal name transformer. | ||
| */ | ||
| public PrefixSuffixPrincipalNameTransformer() { |
There was a problem hiding this comment.
I'm not sure to see the benefit of having this constructor. Isn't it already the default behaviour?
There was a problem hiding this comment.
If you overload the constructor, the default "goes away". So we are just keeping this to make sure existing default behavior stays where it was.
There was a problem hiding this comment.
Although it's good to keep backward compatibility, we have huge breaking changes in 4.1 (I'm thinking about the services definition).
There was a problem hiding this comment.
The idea is tha 8000 t both prefix and suffix are optional. It doesn't make sense to create a class with both value as null, but it isn't possible to provide options through ctors to just set one and not the other. This is how we can keep both optional.
There was a problem hiding this comment.
Just to be sure of my understanding: you mean using the default constructor and then only one of the setters, don't you?
There was a problem hiding this comment.
Yes, exactly.
I can do:
var trans = new PrefixSuffixPrincipalNameTransformer()
trans.setPrefix(...)Or:
var trans = new PrefixSuffixPrincipalNameTransformer()
trans.setSuffix(...)Or:
var trans = new PrefixSuffixPrincipalNameTransformer("prefix", "suffix")But this would be ugly, if I didnt have the default ctor:
var trans = new PrefixSuffixPrincipalNameTransformer(null, "suffix")|
Moved the digest object down to be local. This should be ready so far as it goes. |
| * @return the digested password | ||
| */ | ||
| private byte[] digestEncodedPassword(final String encodedPassword, final Map<String, Object> values) { | ||
| final MessageDigest messageDigest = DigestUtils.getDigest(this.algorithmName); |
There was a problem hiding this comment.
MessageDigest seems to be a low-level class: wasn't there some more elaborated solution (based on Shiro for example)?
There was a problem hiding this comment.
Shiro provides default impls of MessageDigest. At the end, you'd still be working with a message digest, but the impl is provided by shiro based on the algorithm provided. We do have commons codec that does this well, and I am not sure pulling another library in just to do the hashing bit for something we already have a component for is all too helpful....unless shiro does something monumentally better than codec to justify the leap.
There was a problem hiding this comment.
Isn't it exactly what we are looking for: http://shiro.apache.org/static/1.2.1/apidocs/org/apache/shiro/crypto/hash/SimpleHash.html#SimpleHash(java.lang.String,java.lang.Object,java.lang.Object,int)?
There was a problem hiding this comment.
Ah excellent. That's what I was looking for. Thanks. I'll see if I can tweak the PR. (Pulling in Shiro just for hashing seems a bit overkill, but I think it will be useful down the road).
|
This PR is now based on shiro. |
|
+1 |
|
bump. |
|
Bump. Proceeding to merge, if there are no other comments. |
|
Assuming things are ok with this PR, I'll get this merged by tomorrow. |
Conflicts: pom.xml
JDBC QueryAndEncodeDatabaseAuthenticationHandler
This is a JDBC authn handler that has support for dynamic salts, put together by @chasegawa ; I mostly just added support for field names and made it more generic.
This is what it can do:
The handler will:
Unit tests accompany the PR.