From 1118c437099bd9b4ae3e039891870685f1561df9 Mon Sep 17 00:00:00 2001
From: hoshinotsuyoshi <guitarpopnot330@gmail.com>
Date: Sun, 27 Oct 2024 05:54:26 +0900
Subject: [PATCH] backend: Refactor authentication and email models

- Introduced `UserDatabaseAuthentication` model with secure password management.
- Created `UserEmail` model to manage email addresses independently.
- Updated login, signup, and set password mutations to use the new models.
- Modified invitation mailer to work with `UserEmail`.
- Adjusted tests to reflect the separation of user authentication and email logic.
- Removed `pgcrypto` extension as UUIDs are now handled without it.
---
 backend/app/graphql/mutations/login.rb        | 12 +++--
 backend/app/graphql/mutations/set_password.rb |  4 +-
 backend/app/graphql/mutations/signup.rb       | 13 ++----
 .../graphql/mutations/verify_email_address.rb | 14 +++---
 backend/app/mailers/invitation_mailer.rb      |  8 ++--
 backend/app/models/user.rb                    | 19 ++++----
 .../models/user_database_authentication.rb    |  5 +++
 backend/app/models/user_email.rb              |  8 ++++
 backend/config/application.rb                 |  2 +
 .../db/migrate/00000000000001_create_users.rb |  7 +++
 ...0000000000001_enable_pgcrypto_extension.rb |  5 ---
 ...02_create_user_database_authentications.rb | 10 +++++
 .../db/migrate/00000000000002_create_users.rb | 12 -----
 .../00000000000003_create_user_emails.rb      | 13 ++++++
 ...s.rb => 00000000000004_create_sessions.rb} |  0
 backend/db/schema.rb                          | 27 ++++++++---
 .../user_database_authentications.rb          |  6 +++
 backend/spec/factories/user_emails.rb         |  7 +++
 backend/spec/factories/users.rb               |  9 ++--
 backend/spec/graphql/mutations/login_spec.rb  |  6 ++-
 backend/spec/graphql/mutations/signup_spec.rb | 45 +++++++------------
 .../graphql/resolvers/me_resolver_spec.rb     |  5 ++-
 backend/spec/models/user_spec.rb              |  4 +-
 .../spec/system/scenarios/login_flow_spec.rb  |  8 ++--
 24 files changed, 149 insertions(+), 100 deletions(-)
 create mode 100644 backend/app/models/user_database_authentication.rb
 create mode 100644 backend/app/models/user_email.rb
 create mode 100644 backend/db/migrate/00000000000001_create_users.rb
 delete mode 100644 backend/db/migrate/00000000000001_enable_pgcrypto_extension.rb
 create mode 100644 backend/db/migrate/00000000000002_create_user_database_authentications.rb
 delete mode 100644 backend/db/migrate/00000000000002_create_users.rb
 create mode 100644 backend/db/migrate/00000000000003_create_user_emails.rb
 rename backend/db/migrate/{00000000000003_create_sessions.rb => 00000000000004_create_sessions.rb} (100%)
 create mode 100644 backend/spec/factories/user_database_authentications.rb
 create mode 100644 backend/spec/factories/user_emails.rb

diff --git a/backend/app/graphql/mutations/login.rb b/backend/app/graphql/mutations/login.rb
index 774ca12..0ddff54 100644
--- a/backend/app/graphql/mutations/login.rb
+++ b/backend/app/graphql/mutations/login.rb
@@ -11,12 +11,16 @@ class Login < BaseMutation
 
     def resolve(email_address:, password:)
       errors = []
-      user = User.authenticate_by(email_address:, password:)
+      user = UserEmail.verified.find_by(email_address:)&.user
       if user
-        start_new_session_for(user)
+        auth = UserDatabaseAuthentication.authenticate_by(user:, password:)
+        if auth
+          start_new_session_for(auth.user)
+        else
+          errors << :something_wrong
+        end
       else
-        error = :something_wrong
-        errors << error
+        errors << :something_wrong
       end
 
       { success: !!user, errors: }
diff --git a/backend/app/graphql/mutations/set_password.rb b/backend/app/graphql/mutations/set_password.rb
index 500a32a..6e31e53 100644
--- a/backend/app/graphql/mutations/set_password.rb
+++ b/backend/app/graphql/mutations/set_password.rb
@@ -10,8 +10,8 @@ class SetPassword < BaseMutation
     def resolve(password:)
       user = User.transaction do
         current_user.lock!
-        if current_user.before_set_own_password_status?
-          current_user.update!(onboarding_status: :onboarded, password:)
+        if current_user.database_authentication.password.nil?
+          current_user.database_authentication.update!(password:)
           current_user
         else
           nil
diff --git a/backend/app/graphql/mutations/signup.rb b/backend/app/graphql/mutations/signup.rb
index f0dfa62..0456168 100644
--- a/backend/app/graphql/mutations/signup.rb
+++ b/backend/app/graphql/mutations/signup.rb
@@ -12,18 +12,13 @@ def resolve(email_address:)
       user = nil
       errors = []
       ApplicationRecord.transaction do
-        user = User.find_by(
-          email_address:, onboarding_status: :before_verify_email_address
-        ) ||
-        User.create(
-          email_address:, password: SecureRandom.uuid, onboarding_status: :before_verify_email_address
-        )
+        user = User.create_with!(email_address:) unless UserEmail.find_by(email_address:)
       end
-      if user.valid?
-        InvitationMailer.invite(user.id).deliver_later
+      if user&.valid?
+        InvitationMailer.invite(user.email.id).deliver_later
         success = true
       else
-        errors += user.errors.errors
+        errors << :taken
         success = false
       end
       { success:, errors: }
diff --git a/backend/app/graphql/mutations/verify_email_address.rb b/backend/app/graphql/mutations/verify_email_address.rb
index 8b845aa..5ed926e 100644
--- a/backend/app/graphql/mutations/verify_email_address.rb
+++ b/backend/app/graphql/mutations/verify_email_address.rb
@@ -8,16 +8,16 @@ class VerifyEmailAddress < BaseMutation
     field :success, GraphQL::Types::Boolean, null: false
 
     def resolve(signed_id:)
-      user = nil
-      User.transaction do
-        user = User
+      user_email = nil
+      UserEmail.transaction do
+        user_email = UserEmail
           .lock
           .find_signed(signed_id, purpose: :invite)
-        next unless user
-        user.update!(onboarding_status: :before_set_own_password) if user.before_verify_email_address_status?
-        start_new_session_for(user)
+        next unless user_email
+        user_email.touch(:confirmed_at) # rubocop:disable Rails/SkipsModelValidations
+        start_new_session_for(user_email.user)
       end
-      { success: !!user }
+      { success: !!user_email }
     end
   end
 end
diff --git a/backend/app/mailers/invitation_mailer.rb b/backend/app/mailers/invitation_mailer.rb
index 75be013..ae1c926 100644
--- a/backend/app/mailers/invitation_mailer.rb
+++ b/backend/app/mailers/invitation_mailer.rb
@@ -1,7 +1,7 @@
 class InvitationMailer < ApplicationMailer
-  def invite(user_id)
-    @user = User.find(user_id)
-    @signed_id = @user.signed_id(purpose: :invite, expires_in: 1.hour)
-    mail to: @user.email_address
+  def invite(user_email_id)
+    @user_email = UserEmail.find(user_email_id)
+    @signed_id = @user_email.signed_id(purpose: :invite, expires_in: 1.hour)
+    mail to: @user_email.email
   end
 end
diff --git a/backend/app/models/user.rb b/backend/app/models/user.rb
index 35b040c..0e852c6 100644
--- a/backend/app/models/user.rb
+++ b/backend/app/models/user.rb
@@ -1,14 +1,15 @@
 class User < ApplicationRecord
-  has_secure_password
   has_many :sessions, dependent: :destroy
+  has_one :database_authentication, dependent: :destroy, class_name: :UserDatabaseAuthentication, inverse_of: :user
+  has_one :email, dependent: :destroy, class_name: :UserEmail, inverse_of: :user
 
-  normalizes :email_address, with: -> e { e.strip.downcase }
+  delegate :email_address, to: :email
 
-  validates :email_address, uniqueness: true
-
-  enum :onboarding_status, %w[
-    before_verify_email_address
-    before_set_own_password
-    onboarded
-  ].index_by(&:itself), suffix: 'status'
+  class << self
+    def create_with!(email_address:)
+      create!
+        .tap { _1.create_email!(email_address:) }
+        .tap { _1.create_database_authentication!(password: SecureRandom.uuid) }
+    end
+  end
 end
diff --git a/backend/app/models/user_database_authentication.rb b/backend/app/models/user_database_authentication.rb
new file mode 100644
index 0000000..310734e
--- /dev/null
+++ b/backend/app/models/user_database_authentication.rb
@@ -0,0 +1,5 @@
+class UserDatabaseAuthentication < ApplicationRecord
+  has_secure_password
+
+  belongs_to :user, inverse_of: :database_authentication
+end
diff --git a/backend/app/models/user_email.rb b/backend/app/models/user_email.rb
new file mode 100644
index 0000000..7ea5391
--- /dev/null
+++ b/backend/app/models/user_email.rb
@@ -0,0 +1,8 @@
+class UserEmail < ApplicationRecord
+  belongs_to :user, inverse_of: :email
+  alias_attribute :email, :email_address
+
+  normalizes :email_address, with: -> e { e.strip.downcase }
+  validates :email_address, uniqueness: true
+  scope :verified, -> { where.not(confirmed_at: nil) }
+end
diff --git a/backend/config/application.rb b/backend/config/application.rb
index 7e49e30..ced0eb3 100644
--- a/backend/config/application.rb
+++ b/backend/config/application.rb
@@ -51,6 +51,8 @@ class Application < Rails::Application
         files = files.reject { _1.end_with?(".erb") }
         system("bin/rubocop --autocorrect-all " + files.join(" "), exception: true)
       end
+
+      g.orm :active_record, primary_key_type: :uuid
     end
     # Initialize configuration defaults for originally generated Rails version.
     config.load_defaults 8.0
diff --git a/backend/db/migrate/00000000000001_create_users.rb b/backend/db/migrate/00000000000001_create_users.rb
new file mode 100644
index 0000000..e846c5e
--- /dev/null
+++ b/backend/db/migrate/00000000000001_create_users.rb
@@ -0,0 +1,7 @@
+class CreateUsers < ActiveRecord::Migration[8.0]
+  def change
+    create_table :users, id: :uuid do |t|
+      t.timestamps
+    end
+  end
+end
diff --git a/backend/db/migrate/00000000000001_enable_pgcrypto_extension.rb b/backend/db/migrate/00000000000001_enable_pgcrypto_extension.rb
deleted file mode 100644
index cb85bdd..0000000
--- a/backend/db/migrate/00000000000001_enable_pgcrypto_extension.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-class EnablePgcryptoExtension < ActiveRecord::Migration[8.0]
-  def change
-    enable_extension 'pgcrypto'
-  end
-end
diff --git a/backend/db/migrate/00000000000002_create_user_database_authentications.rb b/backend/db/migrate/00000000000002_create_user_database_authentications.rb
new file mode 100644
index 0000000..d0d6939
--- /dev/null
+++ b/backend/db/migrate/00000000000002_create_user_database_authentications.rb
@@ -0,0 +1,10 @@
+class CreateUserDatabaseAuthentications < ActiveRecord::Migration[8.0]
+  def change
+    create_table :user_database_authentications, id: :uuid do |t|
+      t.references :user, null: false, foreign_key: true, type: :uuid, index: { unique: true }
+      t.string :password_digest, null: false
+
+      t.timestamps
+    end
+  end
+end
diff --git a/backend/db/migrate/00000000000002_create_users.rb b/backend/db/migrate/00000000000002_create_users.rb
deleted file mode 100644
index 8a20c40..0000000
--- a/backend/db/migrate/00000000000002_create_users.rb
+++ /dev/null
@@ -1,12 +0,0 @@
-class CreateUsers < ActiveRecord::Migration[8.0]
-  def change
-    create_table :users, id: :uuid do |t|
-      t.string :email_address, null: false
-      t.string :password_digest, null: false
-      t.string :onboarding_status, null: false
-
-      t.timestamps
-    end
-    add_index :users, :email_address, unique: true
-  end
-end
diff --git a/backend/db/migrate/00000000000003_create_user_emails.rb b/backend/db/migrate/00000000000003_create_user_emails.rb
new file mode 100644
index 0000000..17945ba
--- /dev/null
+++ b/backend/db/migrate/00000000000003_create_user_emails.rb
@@ -0,0 +1,13 @@
+class CreateUserEmails < ActiveRecord::Migration[8.0]
+  def change
+    create_table :user_emails, id: :uuid do |t|
+      t.references :user, null: false, foreign_key: true, type: :uuid, index: { unique: true }
+      t.string :email_address, null: false
+      t.datetime :confirmed_at
+
+      t.timestamps
+    end
+
+    add_index :user_emails, :email_address, unique: true
+  end
+end
diff --git a/backend/db/migrate/00000000000003_create_sessions.rb b/backend/db/migrate/00000000000004_create_sessions.rb
similarity index 100%
rename from backend/db/migrate/00000000000003_create_sessions.rb
rename to backend/db/migrate/00000000000004_create_sessions.rb
diff --git a/backend/db/schema.rb b/backend/db/schema.rb
index a2f989a..f5130da 100644
--- a/backend/db/schema.rb
+++ b/backend/db/schema.rb
@@ -10,10 +10,9 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 3) do
+ActiveRecord::Schema[8.0].define(version: 4) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_catalog.plpgsql"
-  enable_extension "pgcrypto"
 
   create_table "sessions", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
     t.uuid "user_id", null: false
@@ -24,14 +23,30 @@
     t.index ["user_id"], name: "index_sessions_on_user_id"
   end
 
-  create_table "users", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
-    t.string "email_address", null: false
+  create_table "user_database_authentications", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
+    t.uuid "user_id", null: false
     t.string "password_digest", null: false
-    t.string "onboarding_status", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.index ["email_address"], name: "index_users_on_email_address", unique: true
+    t.index ["user_id"], name: "index_user_database_authentications_on_user_id", unique: true
+  end
+
+  create_table "user_emails", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
+    t.uuid "user_id", null: false
+    t.string "email_address", null: false
+    t.datetime "confirmed_at"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["email_address"], name: "index_user_emails_on_email_address", unique: true
+    t.index ["user_id"], name: "index_user_emails_on_user_id", unique: true
+  end
+
+  create_table "users", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
   end
 
   add_foreign_key "sessions", "users"
+  add_foreign_key "user_database_authentications", "users"
+  add_foreign_key "user_emails", "users"
 end
diff --git a/backend/spec/factories/user_database_authentications.rb b/backend/spec/factories/user_database_authentications.rb
new file mode 100644
index 0000000..383b919
--- /dev/null
+++ b/backend/spec/factories/user_database_authentications.rb
@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :user_database_authentication do
+    association(:user)
+    password { SecureRandom.alphanumeric }
+  end
+end
diff --git a/backend/spec/factories/user_emails.rb b/backend/spec/factories/user_emails.rb
new file mode 100644
index 0000000..e98b0e3
--- /dev/null
+++ b/backend/spec/factories/user_emails.rb
@@ -0,0 +1,7 @@
+FactoryBot.define do
+  factory :user_email do
+    association(:user)
+    email_address { "#{SecureRandom.alphanumeric}@example.com" }
+    confirmed_at { Time.current }
+  end
+end
diff --git a/backend/spec/factories/users.rb b/backend/spec/factories/users.rb
index 34b915c..45eb4e7 100644
--- a/backend/spec/factories/users.rb
+++ b/backend/spec/factories/users.rb
@@ -1,7 +1,10 @@
 FactoryBot.define do
   factory :user do
-    email_address { "#{SecureRandom.alphanumeric}@example.com" }
-    password { SecureRandom.alphanumeric }
-    onboarding_status { :onboarded }
+    trait :onboarded do
+      after(:create) do |instance|
+        create(:user_database_authentication, user: instance)
+        create(:user_email, user: instance)
+      end
+    end
   end
 end
diff --git a/backend/spec/graphql/mutations/login_spec.rb b/backend/spec/graphql/mutations/login_spec.rb
index 9281db1..f30153e 100644
--- a/backend/spec/graphql/mutations/login_spec.rb
+++ b/backend/spec/graphql/mutations/login_spec.rb
@@ -37,7 +37,11 @@
     end
 
     context "when password is correct" do
-      before { User.create!(email_address: "alice@example.com", password: "password", onboarding_status: :before_verify_email_address) }
+      before do
+        user = create(:user)
+        create(:user_email, user:, email: "alice@example.com")
+        create(:user_database_authentication, user:, password: "password")
+      end
       it "no errors" do
         expect { subject }.to change(Session, :count).by(1)
         expect(subject_response_to_hash).to match(
diff --git a/backend/spec/graphql/mutations/signup_spec.rb b/backend/spec/graphql/mutations/signup_spec.rb
index 7a29264..6451330 100644
--- a/backend/spec/graphql/mutations/signup_spec.rb
+++ b/backend/spec/graphql/mutations/signup_spec.rb
@@ -65,39 +65,24 @@
       let!(:email_address) { 'test@example.com' }
       let!(:context) { {} }
 
-      context 'when onboarding_status: :before_verify_email_address' do
-        before { create(:user, email_address:, onboarding_status: :before_verify_email_address) }
-        it 'sends an invitation email' do
-          expect { subject }
-            .to have_enqueued_job(ActionMailer::MailDeliveryJob)
-            .with('InvitationMailer', 'invite', 'deliver_now', { args: [be_a(String)] })
-          expect(subject_response_to_hash).to match(
-            data: {
-              signup: {
-                success: true,
-                errors: [],
-              },
-            },
-          )
-        end
+      before do
+        user = create(:user)
+        create(:user_email, user:, email: "test@example.com")
       end
 
-      context 'when onboarding_status: :before_set_own_password' do
-        before { create(:user, email_address:, onboarding_status: :before_set_own_password) }
-        it 'does not send invitation email' do
-          expect { subject }
-            .not_to have_enqueued_job(ActionMailer::MailDeliveryJob)
-          expect(subject_response_to_hash).to match(
-            data: {
-              signup: {
-                success: false,
-                errors: [
-                  { __typename: "Taken" },
-                ],
-              },
+      it 'does not send invitation email' do
+        expect { subject }
+          .not_to have_enqueued_job(ActionMailer::MailDeliveryJob)
+        expect(subject_response_to_hash).to match(
+          data: {
+            signup: {
+              success: false,
+              errors: [
+                { __typename: "Taken" },
+              ],
             },
-          )
-        end
+          },
+        )
       end
     end
   end
diff --git a/backend/spec/graphql/resolvers/me_resolver_spec.rb b/backend/spec/graphql/resolvers/me_resolver_spec.rb
index a28a3f4..d8444c3 100644
--- a/backend/spec/graphql/resolvers/me_resolver_spec.rb
+++ b/backend/spec/graphql/resolvers/me_resolver_spec.rb
@@ -15,9 +15,10 @@
         }
       GRAPHQL
     end
-    let!(:current_user) { User.new(email_address: "alice@example.com") }
+    let!(:user) { create(:user) }
+    let!(:user_email) { create(:user_email, user:, email: 'alice@example.com') }
     let!(:variables) { {} }
-    let!(:context) { { current_user: } }
+    let!(:context) { { current_user: user } }
 
     it "returns me" do
       expect(subject_response_to_hash).to eq(
diff --git a/backend/spec/models/user_spec.rb b/backend/spec/models/user_spec.rb
index 4a63dd9..611a431 100644
--- a/backend/spec/models/user_spec.rb
+++ b/backend/spec/models/user_spec.rb
@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 RSpec.describe User, type: :model do
-  it { is_expected.to have_secure_password }
   it { is_expected.to have_many(:sessions).dependent(:destroy) }
-  it { is_expected.to normalize(:email_address).from(" Alice@Example.com ").to("alice@example.com") }
+  it { is_expected.to have_one(:database_authentication).dependent(:destroy) }
+  it { is_expected.to have_one(:email).dependent(:destroy) }
 end
diff --git a/backend/spec/system/scenarios/login_flow_spec.rb b/backend/spec/system/scenarios/login_flow_spec.rb
index c19cc48..7751e6e 100644
--- a/backend/spec/system/scenarios/login_flow_spec.rb
+++ b/backend/spec/system/scenarios/login_flow_spec.rb
@@ -1,17 +1,17 @@
 require 'rails_helper'
 
 RSpec.describe "login flow", type: :system do
-  let!(:user) { create(:user) }
+  let!(:user) { create(:user, :onboarded) }
 
   it 'me -> login -> me -> logout -> me -> login' do
     visit '/me'
     expect(page).to have_no_content("hello, It's me!")
 
-    fill_in "email", with: user.email_address
-    fill_in "password", with: user.password
+    fill_in "email", with: user.email.email
+    fill_in "password", with: user.database_authentication.password
     click_button "Login"
 
-    retry_on_error { expect(page).to have_content(user.email_address) }
+    retry_on_error { expect(page).to have_content(user.email.email) }
     expect(page).to have_content("hello, It's me!")
     accept_alert do
       click_button "Logout"