From 0868affb9d5bf5db352a8c5d378e0612181d561d Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Tue, 19 Jun 2018 11:32:57 -0500 Subject: [PATCH 1/3] LG-319 Record password strength stats **Why**: So that we can measure how our password policies are affecting the stength of passwords people are creating **How**: We do not want the password metrics to be associated with the user records, so we create a new record, named `PasswordMetric` which captures the name of the metric we are calculating, value (e.g. the length of the password), and count (e.g. how many users have a password of a given length). The code then does an atomic insert any time a user sets, resets, or changes their password. --- .../sign_up/passwords_controller.rb | 4 +- .../users/reset_passwords_controller.rb | 5 ++ app/forms/update_user_password_form.rb | 5 ++ app/models/password_metric.rb | 26 ++++++++++ app/services/password_metrics_incrementer.rb | 19 ++++++++ .../20180619145839_create_password_metrics.rb | 12 +++++ db/schema.rb | 11 ++++- .../sign_up/passwords_controller_spec.rb | 10 ++++ .../users/reset_passwords_controller_spec.rb | 23 +++++++++ spec/forms/update_user_password_form_spec.rb | 10 ++++ spec/models/password_metric_spec.rb | 48 +++++++++++++++++++ .../password_metrics_incrementer_spec.rb | 17 +++++++ 12 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 app/models/password_metric.rb create mode 100644 app/services/password_metrics_incrementer.rb create mode 100644 db/migrate/20180619145839_create_password_metrics.rb create mode 100644 spec/models/password_metric_spec.rb create mode 100644 spec/services/password_metrics_incrementer_spec.rb diff --git a/app/controllers/sign_up/passwords_controller.rb b/app/controllers/sign_up/passwords_controller.rb index 74e1554f16c..1e3eef67306 100644 --- a/app/controllers/sign_up/passwords_controller.rb +++ b/app/controllers/sign_up/passwords_controller.rb @@ -46,10 +46,12 @@ def permitted_params def process_successful_password_creation @user.confirm + password = permitted_params[:password] UpdateUser.new( user: @user, - attributes: { reset_requested_at: nil, password: permitted_params[:password] } + attributes: { reset_requested_at: nil, password: password } ).call + PasswordMetricsIncrementer.new(password).increment_password_metrics sign_in_and_redirect_user end diff --git a/app/controllers/users/reset_passwords_controller.rb b/app/controllers/users/reset_passwords_controller.rb index 77777f833b1..a73f775d188 100644 --- a/app/controllers/users/reset_passwords_controller.rb +++ b/app/controllers/users/reset_passwords_controller.rb @@ -137,10 +137,15 @@ def update_user attributes = { password: user_params[:password] } attributes[:confirmed_at] = Time.zone.now unless resource.confirmed? UpdateUser.new(user: resource, attributes: attributes).call + increment_password_metrics mark_profile_inactive end + def increment_password_metrics + PasswordMetricsIncrementer.new(user_params[:password]).increment_password_metrics + end + def mark_profile_inactive resource.active_profile&.deactivate(:password_reset) end diff --git a/app/forms/update_user_password_form.rb b/app/forms/update_user_password_form.rb index 5d648ff4440..416b7663c72 100644 --- a/app/forms/update_user_password_form.rb +++ b/app/forms/update_user_password_form.rb @@ -24,6 +24,7 @@ def process_valid_submission update_user_password email_user_about_password_change encrypt_user_profile_if_active + increment_password_metrics end def update_user_password @@ -42,6 +43,10 @@ def encrypt_user_profile_if_active encryptor.call end + def increment_password_metrics + PasswordMetricsIncrementer.new(password).increment_password_metrics + end + def encryptor @_encryptor ||= ActiveProfileEncryptor.new(user, user_session, password) end diff --git a/app/models/password_metric.rb b/app/models/password_metric.rb new file mode 100644 index 00000000000..a0e92a5b89b --- /dev/null +++ b/app/models/password_metric.rb @@ -0,0 +1,26 @@ +class PasswordMetric < ApplicationRecord + enum metric: %i[length guesses_log10] + + def self.increment(metric, value) + create_row_for_metric_category(metric, value) + sql = <<-SQL + UPDATE password_metrics + SET count = count + 1 + WHERE metric = #{metrics[metric.to_s]} AND value = #{value} + SQL + connection.execute(sql) + end + + private_class_method def self.create_row_for_metric_category(metric, value) + metric_key = metrics[metric.to_s] + # Insert a row with the count equal to 0 if a row does not already exist + sql = <<-SQL + INSERT INTO password_metrics (metric, value, count) + SELECT #{metric_key}, #{value}, 0 + WHERE NOT EXISTS ( + SELECT id FROM password_metrics WHERE metric = #{metric_key} AND value = #{value} + ) + SQL + connection.execute(sql) + end +end diff --git a/app/services/password_metrics_incrementer.rb b/app/services/password_metrics_incrementer.rb new file mode 100644 index 00000000000..e95fdfcfac4 --- /dev/null +++ b/app/services/password_metrics_incrementer.rb @@ -0,0 +1,19 @@ +class PasswordMetricsIncrementer + def initialize(password) + @password = password + end + + def increment_password_metrics + PasswordMetric.increment(:length, password.length) + PasswordMetric.increment(:guesses_log10, guesses_log10) + end + + private + + attr_reader :password + + # Disable :reek:UncommunicativeMethodName b/c this method name ends with a number + def guesses_log10 + Zxcvbn::Tester.new.test(password).guesses_log10.round(1) + end +end diff --git a/db/migrate/20180619145839_create_password_metrics.rb b/db/migrate/20180619145839_create_password_metrics.rb new file mode 100644 index 00000000000..a8492d952fe --- /dev/null +++ b/db/migrate/20180619145839_create_password_metrics.rb @@ -0,0 +1,12 @@ +class CreatePasswordMetrics < ActiveRecord::Migration[5.1] + def change + create_table :password_metrics do |t| + t.integer :metric, null: false + t.float :value, null: false + t.integer :count, null: false + t.index :metric + t.index :value + t.index [:metric, :value], unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 62ed4fc141c..72d33d863db 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180607144007) do +ActiveRecord::Schema.define(version: 20180619145839) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -80,6 +80,15 @@ t.index ["updated_at"], name: "index_otp_requests_trackers_on_updated_at" end + create_table "password_metrics", force: :cascade do |t| + t.integer "metric", null: false + t.float "value", null: false + t.integer "count", null: false + t.index ["metric", "value"], name: "index_password_metrics_on_metric_and_value", unique: true + t.index ["metric"], name: "index_password_metrics_on_metric" + t.index ["value"], name: "index_password_metrics_on_value" + end + create_table "profiles", force: :cascade do |t| t.integer "user_id", null: false t.boolean "active", default: false, null: false diff --git a/spec/controllers/sign_up/passwords_controller_spec.rb b/spec/controllers/sign_up/passwords_controller_spec.rb index f36d41c7944..c0cb9dd7b7e 100644 --- a/spec/controllers/sign_up/passwords_controller_spec.rb +++ b/spec/controllers/sign_up/passwords_controller_spec.rb @@ -47,6 +47,16 @@ post :create, params: { password_form: { password: 'NewVal' }, confirmation_token: token } end + + it 'saves password metrics' do + token = 'new token' + create(:user, confirmation_token: token, confirmation_sent_at: Time.zone.now) + + post :create, params: { password_form: { password: 'saltypickles' }, confirmation_token: token } + + expect(PasswordMetric.where(metric: 'length', value: 12, count: 1).count).to eq(1) + expect(PasswordMetric.where(metric: 'guesses_log10', value: 7.1, count: 1).count).to eq(1) + end end describe '#new' do diff --git a/spec/controllers/users/reset_passwords_controller_spec.rb b/spec/controllers/users/reset_passwords_controller_spec.rb index 4cbcbda5b0a..db8d5220f1e 100644 --- a/spec/controllers/users/reset_passwords_controller_spec.rb +++ b/spec/controllers/users/reset_passwords_controller_spec.rb @@ -270,6 +270,29 @@ expect(response).to redirect_to new_user_session_path end end + + it 'increments password metrics' do + raw_reset_token, db_confirmation_token = + Devise.token_generator.generate(User, :reset_password_token) + + user = create( + :user, + :unconfirmed, + reset_password_token: db_confirmation_token, + reset_password_sent_at: Time.zone.now + ) + + stub_email_notifier(user) + + password = 'saltypickles' + params = { password: password } + + get :edit, params: { reset_password_token: raw_reset_token } + put :update, params: { reset_password_form: params } + + expect(PasswordMetric.where(metric: 'length', value: 12, count: 1).count).to eq(1) + expect(PasswordMetric.where(metric: 'guesses_log10', value: 7.1, count: 1).count).to eq(1) + end end describe '#create' do diff --git a/spec/forms/update_user_password_form_spec.rb b/spec/forms/update_user_password_form_spec.rb index 5ab385548f6..d49045b5632 100644 --- a/spec/forms/update_user_password_form_spec.rb +++ b/spec/forms/update_user_password_form_spec.rb @@ -65,6 +65,16 @@ expect(email_notifier).to have_received(:send_password_changed_email) end + + it 'increments password metrics for the password' do + params[:password] = 'saltypickles' + stub_email_delivery + + subject.submit(params) + + expect(PasswordMetric.where(metric: 'length', value: 12, count: 1).count).to eq(1) + expect(PasswordMetric.where(metric: 'guesses_log10', value: 7.1, count: 1).count).to eq(1) + end end context 'when the user has an active profile' do diff --git a/spec/models/password_metric_spec.rb b/spec/models/password_metric_spec.rb new file mode 100644 index 00000000000..50a04f565de --- /dev/null +++ b/spec/models/password_metric_spec.rb @@ -0,0 +1,48 @@ +require 'rails_helper' + +describe PasswordMetric do + describe '.increment' do + context 'when a metric with the given metric and value does not exists' do + it 'creates the metric with a count of 1' do + PasswordMetric.increment(:length, 10) + + expect(PasswordMetric.count).to eq(1) + expect(PasswordMetric.find_by(metric: :length, value: 10).count).to eq(1) + end + end + + context 'when a metric with the same value exists' do + before do + PasswordMetric.create( + metric: 'length', + value: 9.0, + count: 2 + ) + end + + it 'creates the metric with a value of 1' do + PasswordMetric.increment(:length, 10) + + expect(PasswordMetric.count).to eq(2) + expect(PasswordMetric.find_by(metric: :length, value: 10).count).to eq(1) + end + end + + context 'when a metric with the given category and value does exist' do + before do + PasswordMetric.create( + metric: 'length', + value: 10.0, + count: 1 + ) + end + + it 'increments the value' do + PasswordMetric.increment(:length, 10) + + expect(PasswordMetric.count).to eq(1) + expect(PasswordMetric.find_by(metric: :length, value: 10).count).to eq(2) + end + end + end +end diff --git a/spec/services/password_metrics_incrementer_spec.rb b/spec/services/password_metrics_incrementer_spec.rb new file mode 100644 index 00000000000..bfbd18758c1 --- /dev/null +++ b/spec/services/password_metrics_incrementer_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +describe PasswordMetricsIncrementer do + let(:password) { 'saltypickles' } + let(:guesses_log10) { 7.1 } + + subject { described_class.new(password) } + + describe '#increment_password_metrics' do + it 'increments password metrics for the length and guesses' do + subject.increment_password_metrics + + expect(PasswordMetric.where(metric: 'length', value: 12, count: 1).count).to eq(1) + expect(PasswordMetric.where(metric: 'guesses_log10', value: 7.1, count: 1).count).to eq(1) + end + end +end From 1057c7762ba8fd2b08f8b3d1ab791f1ad08753ee Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 21 Jun 2018 14:02:41 -0500 Subject: [PATCH 2/3] some sql sanitizing --- app/models/password_metric.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/app/models/password_metric.rb b/app/models/password_metric.rb index a0e92a5b89b..50d42d1ad32 100644 --- a/app/models/password_metric.rb +++ b/app/models/password_metric.rb @@ -3,24 +3,24 @@ class PasswordMetric < ApplicationRecord def self.increment(metric, value) create_row_for_metric_category(metric, value) - sql = <<-SQL - UPDATE password_metrics - SET count = count + 1 - WHERE metric = #{metrics[metric.to_s]} AND value = #{value} - SQL - connection.execute(sql) + query = sanitize_sql_array [ + 'UPDATE password_metrics', + 'SET count = count + 1', + "WHERE metric = #{metrics[metric.to_s]} AND value = #{value}", + ] + connection.execute(query) end private_class_method def self.create_row_for_metric_category(metric, value) metric_key = metrics[metric.to_s] # Insert a row with the count equal to 0 if a row does not already exist - sql = <<-SQL - INSERT INTO password_metrics (metric, value, count) - SELECT #{metric_key}, #{value}, 0 - WHERE NOT EXISTS ( - SELECT id FROM password_metrics WHERE metric = #{metric_key} AND value = #{value} - ) - SQL - connection.execute(sql) + query = sanitize_sql_array [ + 'INSERT INTO password_metrics (metric, value, count)', + "SELECT #{metric_key}, #{value}, 0", + 'WHERE NOT EXISTS (', + "SELECT id FROM password_metrics WHERE metric = #{metric_key} AND value = #{value}", + ')', + ] + connection.execute(query) end end From 497626ce1a49332490c5929630dba51c662c4ec2 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 21 Jun 2018 14:10:08 -0500 Subject: [PATCH 3/3] sanitize, but better this time --- app/models/password_metric.rb | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/app/models/password_metric.rb b/app/models/password_metric.rb index 50d42d1ad32..2b2b496b3b7 100644 --- a/app/models/password_metric.rb +++ b/app/models/password_metric.rb @@ -3,24 +3,26 @@ class PasswordMetric < ApplicationRecord def self.increment(metric, value) create_row_for_metric_category(metric, value) - query = sanitize_sql_array [ - 'UPDATE password_metrics', - 'SET count = count + 1', - "WHERE metric = #{metrics[metric.to_s]} AND value = #{value}", - ] - connection.execute(query) + query = <<-SQL + UPDATE password_metrics + SET count = count + 1 + WHERE metric = ? AND value = ? + SQL + sanitized_query = sanitize_sql_array([query, metrics[metric.to_s], value]) + connection.execute(sanitized_query) end private_class_method def self.create_row_for_metric_category(metric, value) metric_key = metrics[metric.to_s] # Insert a row with the count equal to 0 if a row does not already exist - query = sanitize_sql_array [ - 'INSERT INTO password_metrics (metric, value, count)', - "SELECT #{metric_key}, #{value}, 0", - 'WHERE NOT EXISTS (', - "SELECT id FROM password_metrics WHERE metric = #{metric_key} AND value = #{value}", - ')', - ] - connection.execute(query) + query = <<-SQL + INSERT INTO password_metrics (metric, value, count) + SELECT ?, ?, 0 + WHERE NOT EXISTS ( + SELECT id FROM password_metrics WHERE metric = ? AND value = ? + ) + SQL + sanitized_query = sanitize_sql_array([query, metric_key, value, metric_key, value]) + connection.execute(sanitized_query) end end