FINERACT-2005: Prohibit password re-use with configurable global setting #5367
Conversation
airajena
force-pushed
the
FINERACT-2005/prohibit-password-reuse
branch
from
5ddd32b to
814bb25
Compare
| public static final String ASSET_OWNER_TRANSFER_OUTSTANDING_INTEREST_CALCULATION_STRATEGY = "outstanding-interest-calculation-strategy-for-external-asset-transfer"; | ||
| public static final String ALLOWED_LOAN_STATUSES_FOR_EXTERNAL_ASSET_TRANSFER = "allowed-loan-statuses-for-external-asset-transfer"; | ||
| public static final String ALLOWED_LOAN_STATUSES_OF_DELAYED_SETTLEMENT_FOR_EXTERNAL_ASSET_TRANSFER = "allowed-loan-statuses-of-delayed-settlement-for-external-asset-transfer"; | ||
| public static final String RESTRICT_REUSE_OF_PASSWORD = "restrict-reuse-of-password"; |
There was a problem hiding this comment.
I dont like the naming. This global configuration serves a dual purpose:
- whether we want to restrict password reusage
- For the N last used password to be checked.
Can you please use something which fit better for this?
There was a problem hiding this comment.
Good point! I've renamed it to PASSWORD_REUSE_CHECK_HISTORY_COUNT which I think better captures both purposes. Let me know if you'd prefer a different name.
| return null; | ||
| } | ||
| Long value = property.getValue(); | ||
| return value != null && value > 0 ? value.intValue() : null; |
There was a problem hiding this comment.
Go with 0 if no value was provided, no?
There was a problem hiding this comment.
Fixed! Now it returns 0 when enabled but no value is provided, instead of null
| if (aPreviewPassword.getPassword().equals(passWordEncodedValue)) { | ||
| throw new PasswordPreviouslyUsedException(); | ||
| final Integer passwordReuseRestrictionCount = this.configurationDomainService.getPasswordReuseRestrictionCount(); | ||
| if (passwordReuseRestrictionCount != null) { |
There was a problem hiding this comment.
if its enabled, but no value was provided, it is doing nothing which contradicts with the purpose of this field!
There was a problem hiding this comment.
I've updated the logic so when it's enabled with value 0 (or no value), it now checks against ALL previous passwords for that user. This ensures password reuse is actually prevented when the feature is enabled.
| <column name="string_value"/> | ||
| <column name="enabled" valueBoolean="false"/> | ||
| <column name="is_trap_door" valueBoolean="false"/> | ||
| <column name="description" value="Number of previous passwords to check when a user changes their password. Set to 0 or disable to allow password reuse."/> |
There was a problem hiding this comment.
Set to 0 or disable to allow password reuse. i dont like this. If it is enabled, it should prevent password reusage! Having it enabled and value = 0 should not "allow password reuse" but instead do not allow any reused password!
There was a problem hiding this comment.
Completely agree! I've updated the description to: "When enabled, prevents password reuse. The value specifies how many previous passwords to check (e.g., 3 = last 3 passwords). Set to 0 to check ALL previous passwords. Disable this setting to allow password reuse. This makes it clear that enabling with 0 is the strictest setting, not the most permissive.
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
Kindly see my concerns!
airajena
force-pushed
the
FINERACT-2005/prohibit-password-reuse
branch
from
814bb25 to
552c5f4
Compare
Addressed all the concerns. Also following pre commits(./gradlew spotlessApply spotbugsMain spotbugsTest checkstyleMain checkstyleTest) |
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
Please add proper testing to ensure correct behaviour!
3 participants
Description
Implements FINERACT-2005: Prohibit password re-use with a configurable global setting.
Changes
restrict-reuse-of-password(enabled by default, value=3)How It Works
PasswordPreviouslyUsedExceptionif match found.Related Issue
Resolves FINERACT-2005