-
Notifications
You must be signed in to change notification settings - Fork 71
THREESCALE-8916: Make strong passwords mandatory #4195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
THREESCALE-8916: Make strong passwords mandatory #4195
Conversation
Implemented 9 years ago, transparently migrated on login. At this point I think the migration is already completed.
da18ea7 to
5b00c7f
Compare
| const RE_STRONG_PASSWORD = new RegExp( | ||
| '^' + | ||
| '(?=.*\\d)' + // at least one digit | ||
| '(?=.*[a-z])' + // at least one lowercase | ||
| '(?=.*[A-Z])' + // at least one uppercase | ||
| `(?=.*[${STRONG_PASSWORD_SPECIAL_CHARS.replace(/[[\]\\]/g, '\\$&')}])` + // at least one special char | ||
| '(?!.*\\s)' + // no whitespace | ||
| `.{${STRONG_PASSWORD_MIN_SIZE},}` + // minimum length | ||
| '$' | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to NIST guidelines, passwords have to be long, not contain funny characters. I think we should have a default password setting of maybe 12 characters and allow customers to set their own REGEX. This will also allow us to keep short passwords in dev mode so we don't have to type funny stuff to login local server when developing 😎
We better read about current best practices though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So currently NIST seems to care about password length and expiration is not recommended. I think standard was 14 chars at the moment without character enforcement. Maybe we should have a setting to allow customers setting minimal password length. idk if we should just keep what we had before and have standard 14 chars, and when strong password chosen, then go for 16 chars. Adding a new field for a custom length is probably an overkill. Lets discuss further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should follow NIST SP 800-63B-4 Date Published: July 2025
https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-63B-4.pdf
See section 3.1.1.2. Password Verifiers
Maybe we don't even need the strong password setting.
| self.password = unencrypted_password | ||
| ThreeScale::Analytics.user_tracking(self).track('Migrated to BCrypt') | ||
| update_columns(password_digest: password_digest, salt: nil, crypted_password: nil) | ||
| authenticate(unencrypted_password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel uneasy about this whole file. What is it doing? Why is it wrapping? What was super?
|
|
||
| def password_required? | ||
| signup.by_user? && super | ||
| (signup.by_user? || using_password?) && super |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this method? What is super? I wonder if we should separate usage of password on signup and for validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked AI whether the method can be split for checking whether password fields should be shown and whether password field has to be validated. Idk if the analysis makes sense but here it is:
● You're absolutely right - this is a confusing design! The same password_required? method is being used for two different purposes:
1. Model validation - "Should we validate password presence when saving?"
2. UI rendering - "Should we show password fields in forms?"
And worse, it's got that triple-level super chain that's hard to follow.
Better Design
This could be much clearer with separate methods:
For Validation:
# In User model
def validate_password_presence?
signup.by_user? && password_digest_requires_validation? && legacy_password_requires_validation?
end
validates_presence_of :password, if: :validate_password_presence?
For UI:
# In User model
def needs_password_field?
signup.by_user? # Or even more specific: !using_external_auth?
end
# In templates
{% if user.needs_password_field? %}
<input name="account[user][password]" type="password">
{% endif %}
Why the current design exists
Looking at the code, this seems to be a result of:
1. Legacy migration - Moving from crypted_password to password_digest (bcrypt)
2. Monkey-patching Rails - Overriding has_secure_password's password_required?
3. No refactoring - Just kept adding super chains instead of cleaning it up
The method does too many things and the name doesn't clearly indicate which concern it addresses.
| public_search account_plans_ui_visible change_account_plan_permission service_plans_ui_visible | ||
| change_service_plan_permission enforce_sso | ||
| useraccountarea_enabled hide_service signups_enabled account_approval_required public_search | ||
| account_plans_ui_visible change_account_plan_permission service_plans_ui_visiblechange_service_plan_permission |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| account_plans_ui_visible change_account_plan_permission service_plans_ui_visiblechange_service_plan_permission | |
| account_plans_ui_visible change_account_plan_permission service_plans_ui_visible change_service_plan_permission |
For both provider and buyer users
Co-authored-by: Claude <noreply@anthropic.com>
It was actually showing "\n" in flash errors Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
It's fine to leave the password nil when there's an open_id set
33fe198 to
25f4a95
Compare

Note
This PR includes all changes from #4194. To make it easier to review, jlledom#3 includes only the actual diff to make strong passwords mandatory.
What this PR does / why we need it:
Currently, a provider can enforce strong password for the developer portal, and weak passwords are accepted by default. About admin portal, there's no option to enable strong passwords, weak passwords are always accepted.
I think both situations make no sense. I don't think it's acceptable to allow users decide whether they enforce strong passwords or not, as long as strong passwords are possible, that should be the default. And same thing about admin portal.
In this PR, I remove any option to accept weak passwords in both admin or developer portals. Existing passwords will continue to work, but new passwords will be enforced to be strong. Also, I increased the minimal number of characters to 16.
This affects multiple screens, but also API endpoints, this is the complete list:
Besides, while doing this, I found a few UI errors that I told Claude to fix. This are the affected screens:
Another thing I noticed is strong passwords, even when enabled, were not being enforced for users not created by a human. For instance, the default admin user created when a buyer is created was always accepting weak passwords no matter the setting. I also fixed this.
Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-8916
Verification steps
You can go through any (ideally all) screens above an try to set a weak password. Also tests should pass.