Skip to content

FINERACT-2005: Prohibit password re-use with configurable global setting#5367

Open
airajena wants to merge 1 commit intoapache:developfrom
airajena:FINERACT-2005/prohibit-password-reuse
Open

FINERACT-2005: Prohibit password re-use with configurable global setting#5367
airajena wants to merge 1 commit intoapache:developfrom
airajena:FINERACT-2005/prohibit-password-reuse

Conversation

@airajena
Copy link
Contributor

@airajena airajena commented Jan 22, 2026

Description

Implements FINERACT-2005: Prohibit password re-use with a configurable global setting.

Changes

  • Added new global configuration restrict-reuse-of-password (enabled by default, value=3)
  • Added getPasswordReuseRestrictionCount() method to ConfigurationDomainService
  • Updated AppUserWritePlatformServiceJpaRepositoryImpl to use the configurable value instead of hardcoded constant
  • If the configuration is disabled OR value is 0, password reuse check is skipped entirely

How It Works

  • Enabled (default): Checks last N passwords (configurable, default=3). Throws PasswordPreviouslyUsedException if match found.
  • Disabled: Password reuse check is completely skipped.

Related Issue

Resolves FINERACT-2005

IOhacker
IOhacker approved these changes Jan 22, 2026
View reviewed changes
Copy link
Contributor

@IOhacker IOhacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@airajena airajena force-pushed the FINERACT-2005/prohibit-password-reuse branch from 5ddd32b to 814bb25 Compare January 23, 2026 04:40
adamsaghy
adamsaghy reviewed Jan 23, 2026
View reviewed changes
.../java/org/apache/fineract/infrastructure/configuration/api/GlobalConfigurationConstants.java Outdated
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";
Copy link
Contributor

@adamsaghy adamsaghy Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@airajena airajena Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

adamsaghy
adamsaghy reviewed Jan 23, 2026
View reviewed changes
...a/org/apache/fineract/infrastructure/configuration/domain/ConfigurationDomainServiceJpa.java Outdated
return null;
}
Long value = property.getValue();
return value != null && value > 0 ? value.intValue() : null;
Copy link
Contributor

@adamsaghy adamsaghy Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go with 0 if no value was provided, no?

Copy link
Contributor Author

@airajena airajena Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! Now it returns 0 when enabled but no value is provided, instead of null

adamsaghy
adamsaghy reviewed Jan 23, 2026
View reviewed changes
...apache/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java
if (aPreviewPassword.getPassword().equals(passWordEncodedValue)) {
throw new PasswordPreviouslyUsedException();
final Integer passwordReuseRestrictionCount = this.configurationDomainService.getPasswordReuseRestrictionCount();
if (passwordReuseRestrictionCount != null) {
Copy link
Contributor

@adamsaghy adamsaghy Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if its enabled, but no value was provided, it is doing nothing which contradicts with the purpose of this field!

Copy link
Contributor Author

@airajena airajena Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

adamsaghy
adamsaghy reviewed Jan 23, 2026
View reviewed changes
...in/resources/db/changelog/tenant/parts/0208_add_configuration_restrict_reuse_of_password.xml Outdated
<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."/>
Copy link
Contributor

@adamsaghy adamsaghy Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@airajena airajena Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
adamsaghy requested changes Jan 23, 2026
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly see my concerns!

@airajena airajena force-pushed the FINERACT-2005/prohibit-password-reuse branch from 814bb25 to 552c5f4 Compare January 24, 2026 10:57
@airajena
Copy link
Contributor Author

airajena commented Jan 24, 2026

Kindly see my concerns!

Addressed all the concerns. Also following pre commits(./gradlew spotlessApply spotbugsMain spotbugsTest checkstyleMain checkstyleTest)

adamsaghy
adamsaghy requested changes Jan 26, 2026
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add proper testing to ensure correct behaviour!

@adamsaghy
Copy link
Contributor

adamsaghy commented Jan 27, 2026 

 org.apache.fineract.integrationtests.SchedulerJobsTestResults
  
    Test businessDateIsCorrectForCronJob() FAILED (17.9s)
  
    java.lang.IndexOutOfBoundsException: Index 59 out of bounds for length 59
        at org.apache.fineract.integrationtests.SchedulerJobsTestResults.tearDown(SchedulerJobsTestResults.java:158)

@airajena airajena force-pushed the FINERACT-2005/prohibit-password-reuse branch from 552c5f4 to 1f7f0f2 Compare February 4, 2026 05:39
@airajena airajena force-pushed the FINERACT-2005/prohibit-password-reuse branch from 1f7f0f2 to 8ab89ed Compare February 5, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamsaghy adamsaghy adamsaghy requested changes

@IOhacker IOhacker IOhacker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@airajena @adamsaghy @IOhacker