-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SHIRO-552] Support base64 encoded salt in JdbcRealm #138
[SHIRO-552] Support base64 encoded salt in JdbcRealm #138
Conversation
@steinarb can you rebase your branch against master? |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
55e6e93
to
7c8baad
Compare
@bdemers I have rebased the branch against master and force-pushed (but I'm not really sure what the force push did to the pull request...?). |
Refer to this link for build results (access rights to CI server needed): |
Thanks @steinarb! I just retargeted the PR against |
@@ -247,7 +261,11 @@ protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token) | |||
info = new SimpleAuthenticationInfo(username, password.toCharArray(), getName()); | |||
|
|||
if (salt != null) { | |||
info.setCredentialsSalt(ByteSource.Util.bytes(salt)); | |||
if (saltStyle == SaltStyle.COLUMN && saltIsBase64Encoded) { | |||
info.setCredentialsSalt(ByteSource.Util.bytes(Base64.decode(salt))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably decode this once in setSaltIsBase64Encoded()
and reduce the branch here
scratch that, i was miss reading this whole thing... (still need some caffeine)
@steinarb Looks good to me! can you sign an Apache CLA? https://www.apache.org/licenses/#contributor-license-agreements |
Refer to this link for build results (access rights to CI server needed): |
@bdemers I have filled out the ICLA PDF and printed it to another PDF and signed the result with gpg and sent both the PDF and the asc to secretary@apache.org (I wasn't able to fill out anything in the signature field in the PDF but I assume the .asc takes care of that...?) |
@steinarb yup, you should be all set! |
Hi @steinarb , any update about the ICLA? |
Hi @fpapon I understood from the comment by @bdemers above that it was OK...? In my INBOX there's received an automated response from secretary@apache.org, received 8 weeks ago, saying that "If you have been invited as a committer, please advise the (P)PMC that your ICLA has been filed." |
@steinarb ok, thanks! |
@steinarb yes, thanks! |
7c8baad
to
2b4594e
Compare
@fpapon rebased, conflict resolved, built with "mvn clean install" with no errors, and branch force-pushed |
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@steinarb thanks!
This fixes SPAP-552
The change has been tested in https://github.com/steinarb/authservice/ and worked as a drop in replacement for the existing AuthserviceDbRealm
Note that the change makes JdbcRealm expect salt to be base64 encoded by default. It is possible to get the original behaviour back, by setting the JdbcRealm saltIsBase64Encoded property to false.