[Bugfix:System] Enforce password rules#12324
Conversation
465c7c4 to
84a487d
Compare
| $password_requirements = $this->core->getConfig()->getUserIdRequirements(); | ||
| $min = $password_requirements['min_length']; | ||
| $max = $password_requirements['max_length']; |
There was a problem hiding this comment.
These are not password requirements, these are user id requirements for the self account creation function. We currently don't have configurable password requirements. The isValidPassword function requires greater than 12 characters, so the default 'min' and 'max' of the username (min 6 max 12) won't work.
| return MultiResponse::RedirectOnlyResponse( | ||
| new RedirectResponse($this->core->buildUrl(['home'])) | ||
| ); | ||
| #[Route("/user_profile/change_password", methods: ["POST"])] |
github-project-automation
Bot
moved this from Seeking Reviewer
to Work in Progress
in Submitty Development
|
Thank you for the review, @IDzyre ! I mistakenly pulled the user ID requirements instead of password standards. I will update the logic to correctly reflect the isValidPassword constraints and clean up that potential duplication in the controller. I'll push the fix shortly! |
84a487d to
90b968d
Compare
| elseif (strlen($new_password) < 12) { | ||
| $this->core->addErrorMessage("New password is too short. It must be at least 12 characters long."); | ||
| } |
There was a problem hiding this comment.
We might need some other opinions, but I think passing the password through isValidPassword its self, and then adding an error message based on if it is true or false would be better than adding the logic here too. If the isValidPassword ever gets changed, and we forget to change it here, that would cause inconsistencies.
However, if an institution doesn't want to have the same password restrictions, or more strict ones, they might complain with having it passed through. @williamjallen Do you have an opinion on this?
There was a problem hiding this comment.
I agree with you @IDzyre, the logic for checking password validity should appear in only one place. One solution would be to have a validatePassword() method which returns a list of problems with the password (if any), and then have isValidPassword() call validatePassword() and return a bool.
There was a problem hiding this comment.
I agree with the above comment that we should have a universal validatePassword() method. To add to this, for security purposes, wouldn't we want more password requirements then just checking if its greater than 12 characters. From what I see all we are currently checking is if the new password and current password match and if both are 12 characters or more. Ideally, we would also want to make use of good password practices, like ensuring the user as an uppercase character, lowercase character, number, and symbol in their password as well as the length requirement. We could add all of these checks to the validatePassword() method.
There was a problem hiding this comment.
there is a function currently, Utils::isValidPassword() that checks length, uppercase, lowercase, number, and symbol and returns a bool.
There was a problem hiding this comment.
If we want to avoid the problem of missing validation in the future, which is pretty important, we should just put validation in the setPassword method on User. Have it raise a ValidationException when validations fails
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12324 +/- ##
============================================
- Coverage 21.69% 21.65% -0.05%
- Complexity 9617 9642 +25
============================================
Files 268 268
Lines 36115 36244 +129
Branches 478 486 +8
============================================
+ Hits 7836 7847 +11
- Misses 27805 27915 +110
- Partials 474 482 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
| elseif (strlen($new_password) < 12) { | ||
| $this->core->addErrorMessage("New password is too short. It must be at least 12 characters long."); | ||
| } |
There was a problem hiding this comment.
I agree with the above comment that we should have a universal validatePassword() method. To add to this, for security purposes, wouldn't we want more password requirements then just checking if its greater than 12 characters. From what I see all we are currently checking is if the new password and current password match and if both are 12 characters or more. Ideally, we would also want to make use of good password practices, like ensuring the user as an uppercase character, lowercase character, number, and symbol in their password as well as the length requirement. We could add all of these checks to the validatePassword() method.
github-project-automation
Bot
moved this from In Review
to Work in Progress
in Submitty Development
|
hi @Christian2147 and @IDzyre , |
Aaditya-2407
changed the title
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
There already is a function in site/libraries/Utils.php that has a function called isValidPassword that does exactly what I described. I would recommend using this one instead. However, for security reasons, I would also recommend that this function gets updated. I notice that it has no upper limit on password length. This could lead to several issues. For one, depending on the encryption algorithm used, a super long password may not be fully encrypted when stored. Also, someone could theoretically create a super long password, which when passed to an encryption algorithm could end up using a lot of resources to encrypt and crash the server, essentially DOSing Submitty. |
Christian2147
left a comment
Christian2147
left a comment
There was a problem hiding this comment.
See earlier comment
github-project-automation
Bot
moved this from In Review
to Work in Progress
in Submitty Development
|
@Aaditya-2407 Please see previous review feedback. Abandoned PRs may be closed if review feedback is not addressed. |
41ea047 to
ed7561d
Compare
4bdfbd0 to
7e0de2a
Compare
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
@Aaditya-2407 Please revert the unrelated changes to the PDF annotation code, fix the formatting, and see failing CI. Once the PR meets the minimum style requirements, we can begin reviewing the functionality.
|
Hi @JManion32 and @williamjallen , I have identified that the current CI failures in docker_ui.spec.js are due to the new password complexity enforcement logic. The Cypress tests were using simple passwords (e.g., 'password') which the new validator now correctly rejects, causing the UI to stay in a 'Pending' state and eventually time out. I have updated the local spec files to use stronger test credentials (e.g., P@ssw0rd123#) that satisfy the new rules. Locally, PHPUnit is passing (88/88) and ESLint/PHPCS styling is now clean. |
7e0de2a to
0a35579
Compare
There was a problem hiding this comment.
Changes to this file should be reverted, since they have nothing to do with the purpose of this PR.
github-actions
Bot
added
the
Abandoned PR - Needs New Owner
|
closed due to inactivity |
6 participants
Why is this Change Important & Necessary?
Currently, the "Change Password" form in the User Profile only checks if the new password and confirmation match, but it ignores the system-wide length requirements defined in submitty.json. This allows users to set weak passwords (e.g., "1234") that would be rejected during initial account creation, creating a security inconsistency across the platform.
What is the New Behavior?
The changePassword function has been updated to utilize Utils::isValidPassword(). This ensures that any password change must adhere to the configured min_length and max_length. If validation fails, the user is presented with a dynamic error message detailing the requirements.
What steps should a reviewer take to reproduce or test the bug or new feature?
Automated Testing & Documentation
Testing Metadata
Authentication Method: Verified using DatabaseAuthentication.
Negative Test: Confirmed that a 4-character password triggers the red error banner: "Password must be between 6 and 25 characters".
Positive Test: Confirmed that valid passwords between 6-25 characters result in a successful update.
Other information