-
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?
Changes from all commits
732d05c
f1ddee2
0eb58cc
3eb95d5
c87be43
fbd2a20
ff3f13b
35acc2b
02b270b
b986bea
e69ed9e
485d40f
9f64588
59aa2f7
9ea6e69
ae3d035
f2fa458
899cff0
c6ca8d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| [allowlist] | ||
| description = "Global Allowlist" | ||
|
|
||
| # Ignore based on any subset of the file path | ||
| paths = [ | ||
| '''test/unit/authentication/by_password_test.rb''', | ||
| '''test/integration/admin/api/buyers_users_controller_test.rb''' | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,7 +56,7 @@ def account_params | |
| def user_params | ||
| { | ||
| signup_type: partner.signup_type, | ||
| password: permitted_params[:password].presence || SecureRandom.hex, | ||
| password: permitted_params[:password].presence, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user is valid without a password when it's an SSO user. So there's no need to enforce a random password. Also, this random password is not shown to the caller anywhere, so it couldn't be used anyway. After this change, The SSO users for partners don't have a password, the same as any other SSO user in the project. |
||
| email: permitted_params[:email], | ||
| first_name: permitted_params[:first_name], | ||
| last_name: permitted_params[:last_name], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,19 +22,19 @@ def destroy | |
| def create | ||
| @user = @account.users.build | ||
| @user.email = params[:email] | ||
| @user.password = SecureRandom.hex | ||
| @user.password = params[:password].presence | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same, no password required. |
||
| @user.first_name = params[:first_name].presence | ||
| @user.last_name = params[:last_name].presence | ||
| @user.open_id = params[:open_id].presence | ||
| @user.username = params[:username] | ||
| @user.signup_type = :partner | ||
| @user.role = :admin | ||
| @user.activate! | ||
| if @user.save | ||
| render json: {id: @user.id, success: true} | ||
| else | ||
| render json: {errors: @user.errors, success: false} | ||
| end | ||
| @user.save! | ||
|
|
||
| render json: {id: @user.id, success: true} | ||
| rescue StandardError | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why rescue instead of rely on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the previous call to |
||
| render json: {errors: @user.errors, success: false}, status: :unprocessable_entity | ||
| end | ||
|
|
||
| private | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,14 @@ | ||
| import validate from 'validate.js' | ||
|
|
||
| // IMPORTANT: These STRONG_PASSWORD_* constants are duplicated from the backend. | ||
| // The source of truth is app/lib/authentication/by_password.rb. If those constants change in Ruby, | ||
| // you must update them here as well. Do not modify these without updating the backend first. | ||
| // The error message is also defined in config/locales/en.yml at: | ||
| // activerecord.errors.models.user.attributes.password.weak | ||
| const STRONG_PASSWORD_MIN_SIZE = 15 | ||
| const RE_STRONG_PASSWORD = new RegExp(`^[ -~]{${STRONG_PASSWORD_MIN_SIZE},}$`) | ||
| const STRONG_PASSWORD_FAIL_MSG = `Password must be at least ${STRONG_PASSWORD_MIN_SIZE} characters long, and contain only valid characters.` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we just check the size and not a RE? |
||
|
|
||
|
Comment on lines
3
to
11
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enforce the same NIST policy in frontend that we enforce in backend |
||
| const loginConstraints = { | ||
| username: { presence: { allowEmpty: false, message: 'Email or username is mandatory' } }, | ||
| password: { presence: { allowEmpty: false, message: 'Password is mandatory' } } | ||
|
|
@@ -12,7 +21,7 @@ function validateLogin (fields: Record<keyof typeof loginConstraints, string>): | |
| const changePasswordConstraints = { | ||
| password: { | ||
| presence: { allowEmpty: false, message: 'Password is mandatory' }, | ||
| length: { minimum: 6 } | ||
| format: { pattern: RE_STRONG_PASSWORD, message: STRONG_PASSWORD_FAIL_MSG } | ||
| }, | ||
| passwordConfirmation: { | ||
| presence: { allowEmpty: false, message: 'Password confirmation is mandatory' }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,35 +4,26 @@ module ByPassword | |
| extend ActiveSupport::Concern | ||
|
|
||
| # strong passwords | ||
| SPECIAL_CHARACTERS = '-+=><_$#.:;!?@&*()~][}{|' | ||
| RE_STRONG_PASSWORD = / | ||
| \A | ||
| (?=.*\d) # number | ||
| (?=.*[a-z]) # lowercase | ||
| (?=.*[A-Z]) # uppercase | ||
| (?=.*[#{Regexp.escape(SPECIAL_CHARACTERS)}]) # special char | ||
| (?!.*\s) # does not end with space | ||
| .{8,} # at least 8 characters | ||
| \z | ||
| /x | ||
| STRONG_PASSWORD_FAIL_MSG = "Password must be at least 8 characters long, and contain both upper and lowercase letters, a digit and one special character of #{SPECIAL_CHARACTERS}.".freeze | ||
| STRONG_PASSWORD_MIN_SIZE = 15 | ||
| # All printable characters in ASCII, from 'space' (32) to ~ (126) | ||
| # at least STRONG_PASSWORD_MIN_SIZE characters | ||
| RE_STRONG_PASSWORD = /\A[ -~]{#{STRONG_PASSWORD_MIN_SIZE},}\z/ | ||
| STRONG_PASSWORD_FAIL_MSG = I18n.t('activerecord.errors.models.user.attributes.password.weak', min_size: STRONG_PASSWORD_MIN_SIZE) | ||
|
|
||
| included do | ||
| # We only need length validations as they are already set in Authentication::ByPassword | ||
| has_secure_password validations: false | ||
|
|
||
| validates_presence_of :password, if: :password_required? | ||
| validates_presence_of :password, if: :validate_password? | ||
|
|
||
| validates_confirmation_of :password, allow_blank: true | ||
|
|
||
| validates :password, format: { :with => RE_STRONG_PASSWORD, :message => STRONG_PASSWORD_FAIL_MSG, | ||
| if: -> { password_required? && provider_requires_strong_passwords? } } | ||
| validates :password, length: { minimum: 6, allow_blank: true, | ||
| if: -> { password_required? && !provider_requires_strong_passwords? } } | ||
| if: -> { validate_strong_password? } } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get it, I thought we will validate passwords according to nist requirements unconditionally which IIRC was 15 characters min. But
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think I took 16 before your comment about NIST. I can make it 15, NP.
I don't get this. What do you mean by configured by the provider? this PR is about making strong password mandatory, not when configured by the provider, we already have that in production. There only exception to strong passwords validation now are:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, maybe I misinterpret But then why do we need both - If we want, we can make strong passwords disabled by default in development mode. And not care about Or are there other situations
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah we could, but we would still need a way to enable it in development when we want, for instance, I need it enabled to actually develop the feature. Adding a exception in the code is worse than a setting IMO, basically because I would need to make changes in the code every time I need to switch it on an off, and have it lingering in the git cache, maybe even committing those changes by accident.
Sample data is basically for John Doe, and John Doe is created automatically in buyer signup in all environments, also production.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Min pass size to 15: b986bea |
||
|
|
||
| validates :lost_password_token, :password_digest, length: { maximum: 255 } | ||
|
|
||
| attr_accessible :password, :password_confirmation | ||
| attr_accessible :password, :password_confirmation, as: %i[default member admin] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to fix a mass-assignment error from
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand... if there is no |
||
|
|
||
| scope :with_valid_password_token, -> { where { lost_password_token_generated_at >= 24.hours.ago } } | ||
|
|
||
|
|
@@ -45,8 +36,15 @@ def find_with_valid_password_token(token) | |
| end | ||
| end | ||
|
|
||
| def password_required? | ||
| signup.by_user? && (password_digest.blank? || password_digest_changed?) | ||
| def validate_password? | ||
| will_save_change_to_password_digest? || (signup.by_user? && password_digest.blank?) | ||
| end | ||
|
|
||
| def validate_strong_password? | ||
| return false if Rails.configuration.three_scale.strong_passwords_disabled | ||
| return false if signup.sample_data? | ||
|
|
||
| validate_password? | ||
| end | ||
|
Comment on lines
39
to
48
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This replaces
The new methods are tested and return proper values for all known scenarios. |
||
|
|
||
| def just_changed_password? | ||
|
|
@@ -59,9 +57,10 @@ def expire_password_token | |
|
|
||
| def generate_lost_password_token | ||
| token = SecureRandom.hex(32) | ||
| return unless update_columns(lost_password_token: token, lost_password_token_generated_at: Time.current) | ||
| self.lost_password_token = token | ||
| self.lost_password_token_generated_at = Time.current | ||
|
|
||
| token | ||
| token if save(validate: false) | ||
| end | ||
|
|
||
| def generate_lost_password_token! | ||
|
|
@@ -82,7 +81,7 @@ def update_password(new_password, new_password_confirmation) | |
| end | ||
|
|
||
| def using_password? | ||
| password_digest.present? | ||
| password_digest_in_database.present? | ||
| end | ||
|
|
||
| def can_set_password? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,7 +64,7 @@ def ensure_users(count) | |
| def signup_user | ||
| email_part = email.split('@') | ||
| user_attributes = { email: "#{email_part[0]}+test@#{email_part[1]}", username: 'john', first_name: 'John', | ||
| last_name: 'Doe', password: '123456', password_confirmation: '123456', signup_type: :minimal} | ||
| last_name: 'Doe', password: '123456', password_confirmation: '123456', signup_type: :sample_data} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order to exclude "John Doe" from strong password requirement, I added a new signup type |
||
| signup_params = ::Signup::SignupParams.new(plans: [], user_attributes: user_attributes, account_attributes: { org_name: 'Developer' }) | ||
| ::Signup::DeveloperAccountManager.new(@provider).create(signup_params) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -344,14 +344,6 @@ def provider_id_for_audits | |
| account.try!(:provider_id_for_audits) || provider_account.try!(:provider_id_for_audits) | ||
| end | ||
|
|
||
| def provider_requires_strong_passwords? | ||
| # use fields definitons source (instance variable) as backup when creating new record | ||
| # and there is no provider account (its still new record and not set through association.build) | ||
| if source = fields_definitions_source_root | ||
| source.settings.strong_passwords_enabled? | ||
| end | ||
| end | ||
|
Comment on lines
-347
to
-353
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dead code AFAIK |
||
|
|
||
| protected | ||
|
|
||
| def account_for_sphinx | ||
|
|
@@ -436,6 +428,11 @@ def minimal? | |
| signup_type == :minimal | ||
| end | ||
|
|
||
| def sample_data? | ||
| # This is true only for John Doe | ||
| signup_type == :sample_data | ||
| end | ||
|
|
||
| def api? | ||
| signup_type == :api | ||
| end | ||
|
|
@@ -457,7 +454,7 @@ def cas? | |
| end | ||
|
|
||
| def machine? | ||
| minimal? || api? || created_by_provider? || open_id? || cas? || oauth2? | ||
| minimal? || sample_data? || api? || created_by_provider? || open_id? || cas? || oauth2? | ||
| end | ||
|
|
||
| def by_user? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,8 +75,8 @@ def make_activation_code | |
| self.activation_code = self.class.make_token | ||
| end | ||
|
|
||
| def activate_on_minimal_signup? | ||
| minimal_signup? && password.present? && !account.try!(:bought_account_plan).try!(:approval_required?) | ||
| def activate_on_minimal_or_sample_data? | ||
| (minimal_signup? || signup.sample_data?) && password.present? && !account.try!(:bought_account_plan).try!(:approval_required?) | ||
|
Comment on lines
+78
to
+79
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method determines whether the new user must be automatically activated or not. I added the |
||
| end | ||
|
|
||
| def generate_email_verification_token | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| </div> | ||
| <%= form.fields_for [:user, @user ] do |user| %> | ||
| <%= user.user_defined_form %> | ||
| <%= user.input :password, as: :patternfly_input, required: true %> | ||
| <%= user.input :password, as: :patternfly_input, input_html: { type: 'password' }, required: true %> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Password input was in clear text |
||
| <% end %> | ||
| </section> | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,8 +13,8 @@ | |
| <% end %> | ||
|
|
||
| <%= form.inputs :name => "Change Password" do %> | ||
| <%= form.input :password, required: true, input_html: { autocomplete: 'off' } %> | ||
| <%= form.input :password_confirmation, required: true, input_html: { autocomplete: 'off' } %> | ||
| <%= form.input :password, required: false, input_html: { autocomplete: 'off' } %> | ||
| <%= form.input :password_confirmation, required: false, input_html: { autocomplete: 'off' } %> | ||
|
Comment on lines
+16
to
+17
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was wrong, there's no scenario when it's required to change a password in this form.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact, it was a "fake" requirement, the form submit was working without provided values. |
||
| <% end -%> | ||
|
|
||
| <% if can? :update_role, @user %> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,6 @@ | |
| - content_for :javascripts do | ||
| = stylesheet_packs_chunks_tag 'pf_form' | ||
|
|
||
| - if current_user.can_set_password? && !current_account.settings.enforce_sso? | ||
| - content_for :page_header_alert do | ||
| br | ||
| = pf_inline_alert t('.set_password_html', href: new_provider_password_path), variant: :info | ||
|
|
||
| - using_password = current_user.using_password? | ||
|
|
||
| div class="pf-c-card" | ||
| div class="pf-c-card__body" | ||
| = semantic_form_for current_user, builder: Fields::PatternflyFormBuilder, | ||
|
|
@@ -20,13 +13,12 @@ div class="pf-c-card" | |
|
|
||
| = form.inputs 'User Information' do | ||
| = form.user_defined_form | ||
| - if using_password | ||
| = form.input :password, as: :patternfly_input, | ||
| label: t('.new_password_label'), | ||
| required: current_user.password_required?, | ||
| input_html: { value: '', required: current_user.password_required? } | ||
| = form.input :password, as: :patternfly_input, | ||
| label: t('.new_password_label'), | ||
| required: false, | ||
| input_html: { value: '', type: 'password', required: false } | ||
|
|
||
| - if using_password | ||
| - if current_user.using_password? | ||
|
Comment on lines
+16
to
+21
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Password change inputs are now always visible. However, the |
||
| = form.inputs "Provide your current password and update your personal details" do | ||
| = form.input :current_password, as: :patternfly_input, | ||
| required: true, | ||
|
|
||
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.
Had to add this so our password leak system allows me to commit.