adamsaghy commented on PR #6670: URL: https://github.com/apache/fineract/pull/6670#issuecomment-6213320645
@cmandib Please see my minor nits: - A unit test only works by luck. changeUserPasswordThrowsWhenRawPasswordMatchesCurrentPassword doesn’t stub user.getId(). It passes only because the method throws before reaching the history lookup. That’s fine, but a verify(appUserPreviewPasswordRepository, never()).findByUserId(...) would make it obvious that the current-password check fires first. - What the history count covers has changed. With the current-password check, “history count = 3” now blocks 4 distinct passwords: the current one plus the last 3. That’s arguably right, but it’s a behaviour change. The description already offers to drop it, so it’s worth a line in the JIRA or release notes either way. - “Check all history” now costs more. With value 0, each history row now costs one bcrypt matches() call, roughly tens of ms each, inside the transaction. That’s fine in practice; I’m only flagging it since the old equals() was free. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
