diff --git a/.eslintrc b/.eslintrc index 6a472bfd8b0..9a047b4141e 100644 --- a/.eslintrc +++ b/.eslintrc @@ -25,6 +25,7 @@ "indent": "off", "max-classes-per-file": "off", "newline-per-chained-call": "off", + "no-empty": ["error", { "allowEmptyCatch": true }], "no-param-reassign": ["off", "never"], "no-confusing-arrow": "off", "no-plusplus": "off", diff --git a/app/controllers/event_disavowal_controller.rb b/app/controllers/event_disavowal_controller.rb index 9cb4432ba43..4b1d494d27d 100644 --- a/app/controllers/event_disavowal_controller.rb +++ b/app/controllers/event_disavowal_controller.rb @@ -12,6 +12,7 @@ def new extra: EventDisavowal::BuildDisavowedEventAnalyticsAttributes.call(disavowed_event), ).to_h, ) + @forbidden_passwords = forbidden_passwords end def create @@ -20,12 +21,19 @@ def create if result.success? handle_successful_password_reset else + @forbidden_passwords = forbidden_passwords render :new end end private + def forbidden_passwords + disavowed_event.user.email_addresses.flat_map do |email_address| + ForbiddenPasswords.new(email_address.email).call + end + end + def password_reset_from_disavowal_form @password_reset_from_disavowal_form ||= EventDisavowal::PasswordResetFromDisavowalForm.new( disavowed_event, diff --git a/app/controllers/users/reset_passwords_controller.rb b/app/controllers/users/reset_passwords_controller.rb index c92e8421f19..ea0b920e147 100644 --- a/app/controllers/users/reset_passwords_controller.rb +++ b/app/controllers/users/reset_passwords_controller.rb @@ -121,6 +121,7 @@ def handle_unsuccessful_password_reset(result) return end + @forbidden_passwords = forbidden_passwords(resource.email_addresses) render :edit end diff --git a/app/javascript/packs/pw-strength.js b/app/javascript/packs/pw-strength.js index b4a1ee5f99c..61e53b406a0 100644 --- a/app/javascript/packs/pw-strength.js +++ b/app/javascript/packs/pw-strength.js @@ -96,17 +96,35 @@ function disableSubmit(submitEl, length = 0, score = 0) { } } +/** + * @param {HTMLElement?} element + * + * @return {string[]} + */ +export function getForbiddenPasswords(element) { + try { + return JSON.parse(element.dataset.forbidden); + } catch { + return []; + } +} + function analyzePw() { const { userAgent } = window.navigator; const input = document.querySelector( - '#password_form_password, #reset_password_form_password, #update_user_password_form_password', + [ + '#password_form_password', + '#event_disavowal_password_reset_from_disavowal_form_password', + '#reset_password_form_password', + '#update_user_password_form_password', + ].join(','), ); const pwCntnr = document.getElementById('pw-strength-cntnr'); const pwStrength = document.getElementById('pw-strength-txt'); const pwFeedback = document.getElementById('pw-strength-feedback'); const submit = document.querySelector('input[type="submit"]'); - const forbiddenPasswordsElement = document.querySelector('[data-forbidden-passwords]'); - const { forbiddenPasswords } = forbiddenPasswordsElement.dataset; + const forbiddenPasswordsElement = document.querySelector('[data-forbidden]'); + const forbiddenPasswords = getForbiddenPasswords(forbiddenPasswordsElement); disableSubmit(submit); @@ -116,7 +134,7 @@ function analyzePw() { pwCntnr.className = ''; function checkPasswordStrength(e) { - const z = zxcvbn(e.target.value, JSON.parse(forbiddenPasswords)); + const z = zxcvbn(e.target.value, forbiddenPasswords); const [cls, strength] = getStrength(z); const feedback = getFeedback(z); pwCntnr.className = cls; diff --git a/app/views/devise/passwords/edit.html.erb b/app/views/devise/passwords/edit.html.erb index 8512503efb2..a2819ddf810 100644 --- a/app/views/devise/passwords/edit.html.erb +++ b/app/views/devise/passwords/edit.html.erb @@ -13,7 +13,7 @@ <%= f.full_error :reset_password_token %> <%= f.input :password, label: t('forms.passwords.edit.labels.password'), required: true, input_html: { class: 'password-toggle' } %> - <%= render 'devise/shared/password_strength' %> + <%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %> <%= f.button :submit, t('forms.passwords.edit.buttons.submit'), class: 'mb3' %> <% end %> diff --git a/app/views/devise/shared/_password_strength.html.erb b/app/views/devise/shared/_password_strength.html.erb index 495bf668a96..5ab53260b4e 100644 --- a/app/views/devise/shared/_password_strength.html.erb +++ b/app/views/devise/shared/_password_strength.html.erb @@ -10,9 +10,9 @@ <%= t('instructions.password.strength.intro') %> - - ... - + <%= tag.span '...', id: 'pw-strength-txt', class: 'bold', data: { + forbidden: local_assigns[:forbidden_passwords], + } %>
diff --git a/app/views/event_disavowal/new.html.erb b/app/views/event_disavowal/new.html.erb index b8093c0c69c..da332e90875 100644 --- a/app/views/event_disavowal/new.html.erb +++ b/app/views/event_disavowal/new.html.erb @@ -13,7 +13,7 @@ input_html: { value: @disavowal_token, name: :disavowal_token } %> <%= f.input :password, label: t('forms.passwords.edit.labels.password'), required: true, input_html: { aria: { invalid: false }, class: 'password-toggle' } %> - <%= render 'devise/shared/password_strength' %> + <%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %> <%= f.button :submit, t('forms.passwords.edit.buttons.submit'), class: 'mb3' %> <% end %> diff --git a/app/views/sign_up/passwords/new.html.erb b/app/views/sign_up/passwords/new.html.erb index 7dc69bdc39f..cfab89bbe84 100644 --- a/app/views/sign_up/passwords/new.html.erb +++ b/app/views/sign_up/passwords/new.html.erb @@ -14,7 +14,7 @@ <%= f.input :password, required: true, input_html: { aria: { invalid: false, describedby: 'password-description' }, class: 'password-toggle' } %> - <%= render 'devise/shared/password_strength' %> + <%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %> <%= hidden_field_tag :confirmation_token, @confirmation_token, id: 'confirmation_token' %> <%= f.input :request_id, as: :hidden, input_html: { value: params[:request_id] || request_id } %>
diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index 098def4e1ae..860a4884748 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -13,7 +13,7 @@ <%= f.error_notification %> <%= f.input :password, label: t('forms.passwords.edit.labels.password'), required: true, input_html: { aria: { invalid: false, describedby: 'password-description' }, class: 'password-toggle' } %> - <%= render 'devise/shared/password_strength' %> + <%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %> <%= f.button :submit, t('forms.buttons.submit.update'), class: 'mt2 mb3' %> <% end %> diff --git a/spec/controllers/event_disavowal_controller_spec.rb b/spec/controllers/event_disavowal_controller_spec.rb index 822a885b5bb..1c4ef2bd8e7 100644 --- a/spec/controllers/event_disavowal_controller_spec.rb +++ b/spec/controllers/event_disavowal_controller_spec.rb @@ -23,6 +23,17 @@ get :new, params: { disavowal_token: disavowal_token } end + + it 'assigns forbidden passwords' do + expect(@analytics).to receive(:track_event).with( + Analytics::EVENT_DISAVOWAL, + build_analytics_hash, + ) + + get :new, params: { disavowal_token: disavowal_token } + + expect(assigns(:forbidden_passwords)).to all(be_a(String)) + end end context 'with an invalid disavowal_token' do @@ -39,6 +50,22 @@ get :new, params: { disavowal_token: disavowal_token } end + + it 'does not assign forbidden passwords' do + event.update!(disavowed_at: Time.zone.now) + + expect(@analytics).to receive(:track_event).with( + Analytics::EVENT_DISAVOWAL_TOKEN_INVALID, + build_analytics_hash( + success: false, + errors: { event: [t('event_disavowals.errors.event_already_disavowed')] }, + ), + ) + + get :new, params: { disavowal_token: disavowal_token } + + expect(assigns(:forbidden_passwords)).to be_nil + end end end @@ -74,6 +101,25 @@ post :create, params: params end + + it 'assigns forbidden passwords' do + expect(@analytics).to receive(:track_event).with( + Analytics::EVENT_DISAVOWAL_PASSWORD_RESET, + build_analytics_hash( + success: false, + errors: { password: ['is too short (minimum is 12 characters)'] }, + ), + ) + + params = { + disavowal_token: disavowal_token, + event_disavowal_password_reset_from_disavowal_form: { password: 'too short' }, + } + + post :create, params: params + + expect(assigns(:forbidden_passwords)).to all(be_a(String)) + end end context 'with an invalid disavowal_token' do diff --git a/spec/controllers/users/reset_passwords_controller_spec.rb b/spec/controllers/users/reset_passwords_controller_spec.rb index 2d106b51452..a0e0cf7b2ca 100644 --- a/spec/controllers/users/reset_passwords_controller_spec.rb +++ b/spec/controllers/users/reset_passwords_controller_spec.rb @@ -139,6 +139,7 @@ put :update, params: { reset_password_form: form_params } + expect(assigns(:forbidden_passwords)).to all(be_a(String)) expect(response).to render_template(:edit) end end diff --git a/spec/javascripts/packs/pw-strength-spec.js b/spec/javascripts/packs/pw-strength-spec.js new file mode 100644 index 00000000000..db7f7960698 --- /dev/null +++ b/spec/javascripts/packs/pw-strength-spec.js @@ -0,0 +1,43 @@ +describe('pw-strength', () => { + let getForbiddenPasswords; + before(async () => { + window.LoginGov = { I18n: { t() {} } }; + ({ getForbiddenPasswords } = await import('../../../app/javascript/packs/pw-strength')); + }); + + after(() => { + delete window.LoginGov; + }); + + describe('getForbiddenPasswords', () => { + it('returns empty array if given argument is null', () => { + const element = null; + const result = getForbiddenPasswords(element); + + expect(result).to.deep.equal([]); + }); + + it('returns empty array if element has absent dataset value', () => { + const element = document.createElement('span'); + const result = getForbiddenPasswords(element); + + expect(result).to.deep.equal([]); + }); + + it('returns empty array if element has invalid dataset value', () => { + const element = document.createElement('span'); + element.setAttribute('data-forbidden', 'nil'); + const result = getForbiddenPasswords(element); + + expect(result).to.deep.equal([]); + }); + + it('parsed array of forbidden passwords', () => { + const element = document.createElement('span'); + element.setAttribute('data-forbidden', '["foo","bar","baz"]'); + const result = getForbiddenPasswords(element); + + expect(result).to.be.deep.equal(['foo', 'bar', 'baz']); + }); + }); +}); diff --git a/spec/views/devise/shared/_password_strength.html.erb_spec.rb b/spec/views/devise/shared/_password_strength.html.erb_spec.rb new file mode 100644 index 00000000000..0116d269dff --- /dev/null +++ b/spec/views/devise/shared/_password_strength.html.erb_spec.rb @@ -0,0 +1,35 @@ +require 'rails_helper' + +describe 'devise/shared/_password_strength.html.erb' do + describe 'forbidden attributes' do + context 'when local is unassigned' do + before do + render + end + + it 'omits data-forbidden attribute from strength text tag' do + expect(rendered).to have_selector('#pw-strength-txt:not([data-forbidden])') + end + end + + context 'when local is nil' do + before do + render 'devise/shared/password_strength', forbidden_passwords: nil + end + + it 'omits data-forbidden attribute from strength text tag' do + expect(rendered).to have_selector('#pw-strength-txt:not([data-forbidden])') + end + end + + context 'when local is assigned' do + before do + render 'devise/shared/password_strength', forbidden_passwords: ['a', 'b', 'c'] + end + + it 'adds JSON-encoded data-forbidden to strength text tag' do + expect(rendered).to have_selector('#pw-strength-txt[data-forbidden="[\"a\",\"b\",\"c\"]"]') + end + end + end +end