[#2328] bugfix: renamed bcrypt algo name so it's usable, refactored algo name… #2327
Conversation
lprimak
changed the title
lprimak
force-pushed
the
rename-bcrypt-algo-name
branch
from
549253b to
f9eb089
Compare
bmarwell
left a comment
bmarwell
left a comment
There was a problem hiding this comment.
Veto. Don't do this.
It's common format, and this would make Shiro the odd man out.
https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
Also in /etc/passwd.
… handling, changed default non-password hashing algo to SHA-256 as per the help text
lprimak
force-pushed
the
rename-bcrypt-algo-name
branch
from
f9eb089 to
258336c
Compare
|
I believe the changed code should satisfy your concerns. |
bmarwell
left a comment
bmarwell
left a comment
There was a problem hiding this comment.
Approved, with a question left.
|
|
||
| return new StringJoiner("$", "$", "") | ||
| .add(this.getAlgorithmName()) | ||
| .add(getBcryptVersion(this.getAlgorithmName())) |
There was a problem hiding this comment.
You renamed version to algorithmName in line 64, but here you use version again. While "version" is probably the technically correct term, I like "algorithmName" a bit better. Can we (should we) align this? Or am I confusing something?
There was a problem hiding this comment.
Since the "version" is the "official" algorithm name, i.e. the name stored in the password file,
and the longer one is an alias (or the other way around?) the naming convention seems correct.
I don't see how this can really be aligned further without encroaching on the "veto" :)
However, I am open to ideas.
|
|
||
| private static final String HEX_PREFIX = "0x"; | ||
| private static final String DEFAULT_ALGORITHM_NAME = "MD5"; | ||
| private static final String DEFAULT_ALGORITHM_NAME = "SHA-256"; |
There was a problem hiding this comment.
We need to document this on shiro-site.
2 participants
… handling
fixes #2328
Following this checklist to help us incorporate your contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a GitHub issue. Your pull request should address just this issue, without pulling in other changes.
[#XXX] - Fixes bug in SessionManager,where you replace
#XXXwith the appropriate GitHub issue. Best practiceis to use the GitHub issue title in the pull request title and in the first line of the commit message.
fixes #XXXif merging the PR should close a related issue.mvn verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i.Trivial changes like typos do not require a GitHub issue (javadoc, comments...).
In this case, just format the pull request title like
[DOC] - Add javadoc in SessionManager.If this is your first contribution, you have to read the Contribution Guidelines
If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.
To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.