Enforce password validation rules on system admins (#9207)

decidim-system/app/forms/decidim/system/admin_form.rb

@@ -15,6 +15,8 @@ class AdminForm < Form
       validates :password, presence: true, unless: :admin_exists?
       validates :password_confirmation, presence: true, unless: :admin_exists?
 
+      validates :password, password: { email: :email }
+
       validate :email, :email_uniqueness
 
       private

decidim-system/app/models/decidim/system/admin.rb

@@ -8,6 +8,8 @@ class Admin < ApplicationRecord
 
       validates :email, uniqueness: true
 
+      validates :password, password: { email: :email }
+
       private
 
       # Changes default Devise behaviour to use ActiveJob to send async emails.

decidim-system/lib/decidim/system/test/factories.rb

@@ -5,7 +5,7 @@
 FactoryBot.define do
   factory :admin, class: "Decidim::System::Admin" do
     sequence(:email) { |n| "admin#{n}@citizen.corp" }
-    password { "password1234" }
-    password_confirmation { "password1234" }
+    password { "decidim123456" }
+    password_confirmation { "decidim123456" }
   end
 end

decidim-system/spec/commands/decidim/system/create_admin_spec.rb

@@ -25,6 +25,23 @@ module System
           end
         end
 
+        describe "when the password is too common" do
+          before do
+            create(:admin, email: "email@foo.bar")
+          end
+
+          let(:params) do
+            {
+              password: "password1234",
+              password_confirmation: "password1234"
+            }
+          end
+
+          it "broadcasts invalid" do
+            expect { command.call }.to broadcast(:invalid)
+          end
+        end
+
         describe "when the admin doesn't exist" do
           before do
             create(:admin, email: "email@foo.bar")
@@ -33,8 +50,8 @@ module System
           let(:params) do
             {
               email: "different_email@foo.bar",
-              password: "fake123",
-              password_confirmation: "fake123"
+              password: "fake123456",
+              password_confirmation: "fake123456"
             }
           end
 

decidim-system/spec/lib/tasks/decidim_system_create_admin_spec.rb

@@ -38,15 +38,29 @@
     end
 
     context "when provided data is invalid" do
-      let(:email) { "invalid" }
-      let(:password_confirmation) { "invalid" }
+      context "when passwords don't match" do
+        let(:email) { "invalid" }
+        let(:password_confirmation) { "invalid" }
 
-      it "prevents creation of admin and displays validation errors" do
-        expect { task.execute }.not_to(change { Decidim::System::Admin.count })
+        it "prevents creation of admin and displays validation errors" do
+          expect { task.execute }.not_to(change { Decidim::System::Admin.count })
 
-        expect($stdout.string).to include("Some errors prevented creation of admin")
-        expect($stdout.string).to include("Email is invalid")
-        expect($stdout.string).to include("Password confirmation doesn't match Password")
+          expect($stdout.string).to include("Some errors prevented creation of admin")
+          expect($stdout.string).to include("Email is invalid")
+          expect($stdout.string).to include("Password confirmation doesn't match Password")
+        end
+      end
+
+      context "when password is too common" do
+        let(:password) { "password1234" }
+        let(:password_confirmation) { "password1234" }
+
+        it "prevents creation of admin and displays validation errors" do
+          expect { task.execute }.not_to(change { Decidim::System::Admin.count })
+
+          expect($stdout.string).to include("Some errors prevented creation of admin")
+          expect($stdout.string).to include("Password is too common")
+        end
       end
     end
   end

decidim-system/spec/system/manage_admins_spec.rb

@@ -11,43 +11,84 @@
     visit decidim_system.admins_path
   end
 
-  it "creates a new admin" do
-    find(".actions .new").click
+  describe "when creating a new admin" do
+    context "with a valid password" do
+      it "is created" do
+        find(".actions .new").click
+
+        within ".new_admin" do
+          fill_in :admin_email, with: "admin@foo.bar"
+          fill_in :admin_password, with: "decidim123456"
+          fill_in :admin_password_confirmation, with: "decidim123456"
+
+          find("*[type=submit]").click
+        end
+
+        within ".success.flash" do
+          expect(page).to have_content("successfully")
+        end
+
+        within "table" do
+          expect(page).to have_content("admin@foo.bar")
+        end
+      end
+    end
 
-    within ".new_admin" do
-      fill_in :admin_email, with: "admin@foo.bar"
-      fill_in :admin_password, with: "fake123"
-      fill_in :admin_password_confirmation, with: "fake123"
+    context "with an invalid password" do
+      it "gives an error" do
+        find(".actions .new").click
 
-      find("*[type=submit]").click
-    end
+        within ".new_admin" do
+          fill_in :admin_email, with: "admin@foo.bar"
+          fill_in :admin_password, with: "password1234"
+          fill_in :admin_password_confirmation, with: "password1234"
 
-    within ".success.flash" do
-      expect(page).to have_content("successfully")
-    end
+          find("*[type=submit]").click
+        end
 
-    within "table" do
-      expect(page).to have_content("admin@foo.bar")
+        expect(page).to have_css(".form-error.is-visible", text: "is too common")
+      end
     end
   end
 
-  it "updates an admin" do
-    within find("tr", text: admin.email) do
-      click_link "Edit"
-    end
+  describe "when updating an admin" do
+    context "with a valid password" do
+      it "is updated" do
+        within find("tr", text: admin.email) do
+          click_link "Edit"
+        end
 
-    within ".edit_admin" do
-      fill_in :admin_email, with: "admin@another.domain"
+        within ".edit_admin" do
+          fill_in :admin_email, with: "admin@another.domain"
 
-      find("*[type=submit]").click
-    end
+          find("*[type=submit]").click
+        end
 
-    within ".success.flash" do
-      expect(page).to have_content("successfully")
+        within ".success.flash" do
+          expect(page).to have_content("successfully")
+        end
+
+        within "table" do
+          expect(page).to have_content("admin@another.domain")
+        end
+      end
     end
 
-    within "table" do
-      expect(page).to have_content("admin@another.domain")
+    context "with an invalid password" do
+      it "gives an error" do
+        within find("tr", text: admin.email) do
+          click_link "Edit"
+        end
+
+        within ".edit_admin" do
+          fill_in :admin_password, with: "password1234"
+          fill_in :admin_password_confirmation, with: "password1234"
+
+          find("*[type=submit]").click
+        end
+
+        expect(page).to have_css(".form-error.is-visible", text: "is too common")
+      end
     end
   end
 

decidim-system/spec/system/sessions_spec.rb

@@ -5,8 +5,8 @@
 describe "Sessions", type: :system do
   let!(:admin) do
     create(:admin, email: "admin@example.org",
-                   password: "123456",
-                   password_confirmation: "123456")
+                   password: "decidim123456",
+                   password_confirmation: "decidim123456")
   end
 
   before do
@@ -17,7 +17,7 @@
     it "lets you into the system panel" do
       within ".new_admin" do
         fill_in :admin_email, with: "admin@example.org"
-        fill_in :admin_password, with: "123456"
+        fill_in :admin_password, with: "decidim123456"
         find("*[type=submit]").click
       end