From 8905e4e639cf03b758da558568a86c9816253b2d Mon Sep 17 00:00:00 2001 From: Matijs van Zuijlen Date: Fri, 19 Aug 2022 23:41:41 +0200 Subject: [PATCH] Require user passwords to be strong This adds validation of passwords using the zxcvbn password strength estimation gem. --- publify_core/app/models/user.rb | 2 +- publify_core/lib/publify_core.rb | 1 + .../lib/publify_core/testing_support/factories.rb | 2 +- publify_core/publify_core.gemspec | 1 + .../controllers/admin/users_controller_spec.rb | 10 ++++++---- .../spec/controllers/setup_controller_spec.rb | 14 +++++++++++--- publify_core/spec/features/login_spec.rb | 6 ++++-- publify_core/spec/features/reset_password_spec.rb | 6 +++--- publify_core/spec/features/setup_spec.rb | 6 ++++-- publify_core/spec/features/signup_spec.rb | 10 ++++++---- publify_core/spec/models/user_spec.rb | 5 +++++ 11 files changed, 43 insertions(+), 20 deletions(-) diff --git a/publify_core/app/models/user.rb b/publify_core/app/models/user.rb index 226183e3c9..5b938466a2 100644 --- a/publify_core/app/models/user.rb +++ b/publify_core/app/models/user.rb @@ -12,7 +12,7 @@ class User < ApplicationRecord # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable and :omniauthable devise :database_authenticatable, :registerable, - :recoverable, :rememberable, :trackable, :validatable + :recoverable, :rememberable, :trackable, :validatable, :zxcvbnable include ConfigManager include StringLengthLimit diff --git a/publify_core/lib/publify_core.rb b/publify_core/lib/publify_core.rb index b1071e352e..e482c29e7e 100644 --- a/publify_core/lib/publify_core.rb +++ b/publify_core/lib/publify_core.rb @@ -2,6 +2,7 @@ require "devise" require "devise-i18n" +require "devise_zxcvbn" require "publify_core/version" require "publify_core/engine" diff --git a/publify_core/lib/publify_core/testing_support/factories.rb b/publify_core/lib/publify_core/testing_support/factories.rb index a44df09815..8eacf71372 100644 --- a/publify_core/lib/publify_core/testing_support/factories.rb +++ b/publify_core/lib/publify_core/testing_support/factories.rb @@ -21,7 +21,7 @@ notify_via_email { false } notify_on_new_articles { false } notify_on_comments { false } - password { "top-secret" } + password { "top-Secret12!$#" } state { "active" } profile { User::CONTRIBUTOR } diff --git a/publify_core/publify_core.gemspec b/publify_core/publify_core.gemspec index b4c3234665..13cbb40b67 100644 --- a/publify_core/publify_core.gemspec +++ b/publify_core/publify_core.gemspec @@ -28,6 +28,7 @@ Gem::Specification.new do |s| s.add_dependency "carrierwave", "~> 2.0" s.add_dependency "devise", "~> 4.7.1" s.add_dependency "devise-i18n", "~> 1.2" + s.add_dependency "devise_zxcvbn", "~> 6.0" s.add_dependency "dynamic_form", "~> 1.1.4" s.add_dependency "fog-aws", "~> 3.2" s.add_dependency "fog-core", "~> 2.2" diff --git a/publify_core/spec/controllers/admin/users_controller_spec.rb b/publify_core/spec/controllers/admin/users_controller_spec.rb index 5a947850d2..b14ab92290 100644 --- a/publify_core/spec/controllers/admin/users_controller_spec.rb +++ b/publify_core/spec/controllers/admin/users_controller_spec.rb @@ -7,6 +7,7 @@ let(:admin) { create(:user, :as_admin) } let(:publisher) { create(:user, :as_publisher) } let(:contributor) { create(:user, :as_contributor) } + let(:strong_password) { "fhnehnhfiiuh" } render_views @@ -48,8 +49,8 @@ before do sign_in admin post :create, params: { user: { login: "errand", email: "corey@test.com", - password: "testpass", - password_confirmation: "testpass", + password: strong_password, + password_confirmation: strong_password, profile: User::CONTRIBUTOR, text_filter_name: "markdown", nickname: "fooo", firstname: "bar" } } @@ -70,8 +71,9 @@ it "redirects to index" do post :update, params: { id: contributor.id, user: { login: "errand", - email: "corey@test.com", password: "testpass", - password_confirmation: "testpass" } } + email: "corey@test.com", + password: strong_password, + password_confirmation: strong_password } } expect(response).to redirect_to(action: "index") end diff --git a/publify_core/spec/controllers/setup_controller_spec.rb b/publify_core/spec/controllers/setup_controller_spec.rb index d2bad1964a..6713707ab0 100644 --- a/publify_core/spec/controllers/setup_controller_spec.rb +++ b/publify_core/spec/controllers/setup_controller_spec.rb @@ -3,6 +3,8 @@ require "rails_helper" describe SetupController, type: :controller do + let(:strong_password) { "fhnehnhfiiuh" } + describe "#index" do describe "when no blog is configured" do before do @@ -35,7 +37,7 @@ before do ActionMailer::Base.deliveries.clear post :create, params: { setting: { blog_name: "Foo", email: "foo@bar.net", - password: "foo123bar" } } + password: strong_password } } end it "correctly initializes blog and users" do @@ -64,13 +66,13 @@ describe "when passing incorrect parameters" do it "empty blog name should raise an error" do post :create, params: { setting: { blog_name: "", email: "foo@bar.net", - password: "foobar123" } } + password: strong_password } } expect(response).to redirect_to(action: "index") end it "empty email should raise an error" do post :create, params: { setting: { blog_name: "Foo", email: "", - password: "foobar123" } } + password: strong_password } } expect(response).to redirect_to(action: "index") end @@ -79,6 +81,12 @@ password: "" } } expect(response).to redirect_to(action: "index") end + + it "weak password should raise an error" do + post :create, params: { setting: { blog_name: "Foo", email: "foo@bar.net", + password: "foo123bar" } } + expect(response).to redirect_to(action: "index") + end end end diff --git a/publify_core/spec/features/login_spec.rb b/publify_core/spec/features/login_spec.rb index e0947b34ae..238433c09f 100644 --- a/publify_core/spec/features/login_spec.rb +++ b/publify_core/spec/features/login_spec.rb @@ -3,19 +3,21 @@ require "rails_helper" RSpec.feature "Logging in", type: :feature do + let(:strong_password) { "fhnehnhfiiuh" } + before do stub_request(:get, "http://www.google.com/search?output=rss&q=link:www.example.com&tbm=blg"). to_return(status: 200, body: "", headers: {}) load Rails.root.join("db/seeds.rb") Blog.first.update blog_name: "Awesome!", base_url: "http://www.example.com/" - create :user, :as_admin, login: "admin", password: "a-secret" + create :user, :as_admin, login: "admin", password: strong_password end scenario "Admin logs in" do visit "/admin" fill_in :user_login, with: "admin" - fill_in :user_password, with: "a-secret" + fill_in :user_password, with: strong_password click_button I18n.t("devise.sessions.new.sign_in") diff --git a/publify_core/spec/features/reset_password_spec.rb b/publify_core/spec/features/reset_password_spec.rb index b97f746cac..23c09ceea3 100644 --- a/publify_core/spec/features/reset_password_spec.rb +++ b/publify_core/spec/features/reset_password_spec.rb @@ -6,7 +6,7 @@ before do load Rails.root.join("db/seeds.rb") Blog.first.update blog_name: "Awesome!", base_url: "http://www.example.com/" - create :user, :as_admin, login: "admin", password: "forget-me" + create :user, :as_admin, login: "admin", password: "Fo_rgEt-1m5e2" end scenario "Admin resets password" do @@ -23,8 +23,8 @@ visit url - fill_in :user_password, with: "a-secret" - fill_in :user_password_confirmation, with: "a-secret" + fill_in :user_password, with: "a5-SeCre1T4" + fill_in :user_password_confirmation, with: "a5-SeCre1T4" click_button I18n.t("devise.passwords.edit.change_my_password") expect(page).to have_text I18n.t("devise.passwords.updated") diff --git a/publify_core/spec/features/setup_spec.rb b/publify_core/spec/features/setup_spec.rb index 1232e9ff7f..9a3c414fb7 100644 --- a/publify_core/spec/features/setup_spec.rb +++ b/publify_core/spec/features/setup_spec.rb @@ -3,6 +3,8 @@ require "rails_helper" RSpec.feature "Blog setup", type: :feature do + let(:strong_password) { "fhnehnhfiiuh" } + before do stub_request(:get, "http://www.google.com/search?output=rss&q=link:www.example.com&tbm=blg"). @@ -18,7 +20,7 @@ # Set up the blog fill_in :setting_blog_name, with: "Awesome blog" fill_in :setting_email, with: "foo@bar.com" - fill_in :setting_password, with: "test1234" + fill_in :setting_password, with: strong_password click_button I18n.t!("generic.save") # Confirm set up success @@ -38,7 +40,7 @@ visit admin_dashboard_path fill_in :user_login, with: "admin" - fill_in :user_password, with: "test1234" + fill_in :user_password, with: strong_password find("input[type=submit]").click expect(page).to have_text I18n.t!("admin.dashboard.index.welcome_back", user_name: "admin") diff --git a/publify_core/spec/features/signup_spec.rb b/publify_core/spec/features/signup_spec.rb index 5265f9873f..7b576bd0e8 100644 --- a/publify_core/spec/features/signup_spec.rb +++ b/publify_core/spec/features/signup_spec.rb @@ -3,12 +3,14 @@ require "rails_helper" RSpec.feature "Signing up", type: :feature do + let(:strong_password) { "whhefwhfwefhiu" } + before do load Rails.root.join("db/seeds.rb") Blog.first.update(blog_name: "Awesome!", base_url: "http://www.example.com/", allow_signup: 1) - create :user, :as_admin, login: "admin", password: "a-secret" + create :user, :as_admin, login: "admin" end scenario "User signs up for an account" do @@ -18,8 +20,8 @@ # Create account fill_in :user_login, with: "hello" fill_in :user_email, with: "hello@hello.com" - fill_in :user_password, with: "hush-hush" - fill_in :user_password_confirmation, with: "hush-hush" + fill_in :user_password, with: strong_password + fill_in :user_password_confirmation, with: strong_password click_button I18n.t!("devise.registrations.new.sign_up") # Confirm creation success @@ -32,7 +34,7 @@ # Confirm ability to sign in visit admin_dashboard_path fill_in :user_login, with: "hello" - fill_in :user_password, with: "hush-hush" + fill_in :user_password, with: strong_password find("input[type=submit]").click expect(page).to have_text I18n.t!("devise.sessions.signed_in") diff --git a/publify_core/spec/models/user_spec.rb b/publify_core/spec/models/user_spec.rb index 54ea166556..8ff5c217b0 100644 --- a/publify_core/spec/models/user_spec.rb +++ b/publify_core/spec/models/user_spec.rb @@ -98,6 +98,11 @@ expect(bar).not_to allow_value("foo@foo.com").for(:email) end + + it "requires a strong password" do + nearly_valid_user = build(:user) + expect(nearly_valid_user).not_to allow_value("password01").for(:password) + end end describe "#initialize" do