Merge pull request #914 from alphagov/lowercase-admin-user-emails

Lowercase admin user email addresses

app/models/admin_user.rb

@@ -26,7 +26,6 @@ class MustBeAtLeastOneAdminUser < RuntimeError; end
   # = Validations =
   validates :first_name, :last_name, presence: true
   validates :email, presence: true, email: true
-  validates :email, uniqueness: { case_sensitive: false }
   validates :role, presence: true, inclusion: { in: ROLES }
 
   # = Callbacks =
@@ -42,7 +41,9 @@ def self.timeout_in
   end
 
   def self.find_or_create_from!(provider, auth_data)
-    find_or_create_by!(email: auth_data.fetch(:uid)) do |user|
+    email = auth_data.fetch(:uid).downcase
+
+    find_or_create_by!(email: email) do |user|
       user.first_name = auth_data.info.fetch(:first_name)
       user.last_name = auth_data.info.fetch(:last_name)
       groups = Array.wrap(auth_data.info.fetch(:groups))
@@ -56,7 +57,7 @@ def self.find_or_create_from!(provider, auth_data)
       end
     end
   rescue ActiveRecord::RecordNotUnique => e
-    find_by!(email: auth_data.fetch(:uid))
+    find_by!(email: email)
   rescue ActiveRecord::RecordInvalid => e
     Appsignal.send_exception(e) and return nil
   end
@@ -83,6 +84,10 @@ def destroy(current_user: nil)
     end
   end
 
+  def email=(value)
+    super(value.to_s.downcase)
+  end
+
   def name
     "#{last_name}, #{first_name}"
   end

db/migrate/20240227155037_lowercase_admin_user_emails.rb

@@ -0,0 +1,11 @@
+class LowercaseAdminUserEmails < ActiveRecord::Migration[6.1]
+  class AdminUser < ActiveRecord::Base; end
+
+  def change
+    up_only do
+      AdminUser.find_each do |user|
+        user.email = user.email.downcase
+      end
+    end
+  end
+end

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: 2024_01_29_152626) do
+ActiveRecord::Schema.define(version: 2024_02_27_155037) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "intarray"

spec/models/admin_user_spec.rb

@@ -33,11 +33,6 @@
     it { is_expected.to allow_value("[email protected]").for(:email)}
     it { is_expected.not_to allow_value("jimbo").for(:email) }
 
-    it "should validate uniqueness of email" do
-      FactoryBot.create(:moderator_user)
-      is_expected.to validate_uniqueness_of(:email).case_insensitive
-    end
-
     it "should allow sysadmin role" do
       u = FactoryBot.build(:admin_user, :role => 'sysadmin')
       expect(u).to be_valid
@@ -73,6 +68,72 @@
     end
   end
 
+  describe "class methods" do
+    describe ".find_or_create_from!" do
+      let(:provider) { IdentityProvider.providers.first }
+
+      context "when a user doesn't exist" do
+        let(:auth_data1) do
+          OmniAuth::AuthHash.new(
+            uid: "[email protected]",
+            provider: "example",
+            info: {
+              first_name: "Anne",
+              last_name: "Admin",
+              groups: ["sysadmins"]
+            }
+          )
+        end
+
+        let(:auth_data2) do
+          OmniAuth::AuthHash.new(
+            uid: "[email protected]",
+            provider: "example",
+            info: {
+              first_name: "Anne",
+              last_name: "Admin",
+              groups: ["sysadmins"]
+            }
+          )
+        end
+
+        it "creates only one new user" do
+          expect {
+            AdminUser.find_or_create_from!(provider, auth_data1)
+          }.to change(AdminUser, :count).by(1)
+
+          expect {
+            AdminUser.find_or_create_from!(provider, auth_data2)
+          }.not_to change(AdminUser, :count)
+        end
+      end
+
+      context "when a user exists" do
+        let(:auth_data) do
+          OmniAuth::AuthHash.new(
+            uid: "[email protected]",
+            provider: "example",
+            info: {
+              first_name: "Anne",
+              last_name: "Admin",
+              groups: ["sysadmins"]
+            }
+          )
+        end
+
+        before do
+          FactoryBot.create(:sysadmin_user, email: "[email protected]", first_name: "Anne", last_name: "Admin")
+        end
+
+        it "doesn't create a new user" do
+          expect {
+            AdminUser.find_or_create_from!(provider, auth_data)
+          }.not_to change(AdminUser, :count)
+        end
+      end
+    end
+  end
+
   describe "instance methods" do
     describe "#destroy" do
       context "when there is no current user and there is more than one" do
@@ -114,6 +175,13 @@
       end
     end
 
+    describe "#email=" do
+      it "should downcase the email address" do
+        user = FactoryBot.create(:moderator_user, :email => '[email protected]')
+        expect(user.email).to eq('[email protected]')
+      end
+    end
+
     describe "#name" do
       it "should return a user's name" do
         user = FactoryBot.create(:moderator_user, :first_name => 'Jo', :last_name => 'Public')