fix: ensure new passwords are distinct #468
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -176,12 +176,20 @@ def index() -> str | Response: # noqa: PLR0911, PLR0912 | |||
|
|
|||
| # Handle Change Password Form Submission | |||
There was a problem hiding this comment.
Is this section needed if the change_password route function exists? I tried removing this section to see if the tests would pass, and they do. I'm not sure if that's because this isn't needed, or just because it isn't tested.
Probably better to refactor these areas so that there's only one implementation of each flow.
There was a problem hiding this comment.
Agreed, it looks like only changes to display name, username, directory visibility, and bio are handled via the /settings route, while 2fa, password changes, smtp settings, pgp settings, account deletion, verified user and admin user settings are handled by dedicated routes.
| @@ -176,12 +176,20 @@ def index() -> str | Response: # noqa: PLR0911, PLR0912 | |||
|
|
|||
| # Handle Change Password Form Submission | |||
There was a problem hiding this comment.
Agreed, it looks like only changes to display name, username, directory visibility, and bio are handled via the /settings route, while 2fa, password changes, smtp settings, pgp settings, account deletion, verified user and admin user settings are handled by dedicated routes.
| @@ -25,5 +31,15 @@ def __call__(self, form: Form, field: Field) -> None: | |||
| raise ValidationError(self.message) | |||
|
|
|||
|
|
|||
| def is_valid_password_swap( | |||
| *, check_password: Callable[[str], bool], old_password: str, new_password: str | |||
There was a problem hiding this comment.
I don't love passing a callable here, is the goal to keep all password validation logic in one part of the code?
| # the correctness test passes, since timing differences leak length information | ||
| if check_password(old_password): | ||
| return not bytes_are_equal(old_password.encode(), new_password.encode()) | ||
| raise WrongPassword |
There was a problem hiding this comment.
Raising an error isn't really consistent with the existing code style. I am open to refactoring the flow control to be error-based, but I'd prefer that as a separate PR.
This PR ensures that the change password flow is consistent, safely & consistently flashes relevant error messages, and doesn't allow for a change to an identical password.
Passing Workflows: