Merge pull request #916 from amatsuda/identities

User has_many OAuth Identities

Author: amatsuda

app/controllers/profiles_controller.rb

@@ -22,7 +22,7 @@ def update
       redirect_to (session.delete(:target) || root_url)
     else
       flash.now[:danger] = "Unable to save profile. Please correct the following: #{current_user.errors.full_messages.join(', ')}."
-      render :edit
+      render :edit, status: :unprocessable_entity
     end
   end
 

app/controllers/users/omniauth_callbacks_controller.rb

@@ -1,5 +1,5 @@
 class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController
-  before_action :check_current_user, only: [:twitter, :github, :developer]
+  before_action :link_identity_to_current_user, only: [:twitter, :github, :developer], if: -> { current_user }
   skip_forgery_protection only: :developer
 
   def twitter
@@ -21,26 +21,34 @@ def failure
 
   private
 
-  def check_current_user
-    if current_user.present?
-      flash[:info] = I18n.t("devise.failure.already_authenticated")
-      redirect_to(events_url)
+  # If an already logged in user tries to connect with another provider, integrate with the current_user's identity
+  def link_identity_to_current_user
+    if (identity = Identity.find_by(provider: auth_hash.provider, uid: auth_hash.uid))
+      if identity.user_id == current_user.id
+        flash[:info] = "#{provider_name} is already connected to your account."
+      else
+        flash[:danger] = "This #{provider_name} account is already connected to another user."
+      end
+    else
+      current_user.identities.create!(provider: auth_hash.provider, uid: auth_hash.uid)
+      flash[:info] = "Successfully connected #{provider_name} to your account."
     end
+
+    redirect_to edit_profile_path
   end
 
   def authenticate_with_hash(user = nil)
     logger.info "Authenticating user credentials: #{auth_hash.inspect}"
     @user = user || User.from_omniauth(auth_hash, session[:pending_invite_email])
 
     if @user.persisted?
-      flash.now[:info] = "You have signed in with #{auth_hash['provider'].capitalize}."
+      flash.now[:info] = "You have signed in with #{provider_name}."
       logger.info "Signing in user #{@user.inspect}"
 
       @user.skip_confirmation!
       sign_in @user
 
       redirect_to after_sign_in_path_for(@user)
-
     else
       redirect_to new_user_session_url, danger: "There was an error authenticating via Auth provider: #{params[:provider]}."
     end
@@ -50,4 +58,7 @@ def auth_hash
     request.env['omniauth.auth']
   end
 
+  def provider_name
+    {'github' => 'GitHub', 'twitter' => 'Twitter'}[auth_hash.provider]
+  end
 end

app/models/identity.rb

@@ -0,0 +1,23 @@
+class Identity < ApplicationRecord
+  belongs_to :user
+
+  validates :provider, presence: true
+  validates :uid, presence: true, uniqueness: {scope: :provider}
+end
+
+# == Schema Information
+#
+# Table name: identities
+#
+#  id         :integer          not null, primary key
+#  user_id    :integer          not null
+#  provider   :string           not null
+#  uid        :string           not null
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#
+# Indexes
+#
+#  index_identities_on_provider_and_uid  (provider,uid) UNIQUE
+#  index_identities_on_user_id           (user_id)
+#

app/models/user.rb

@@ -5,6 +5,7 @@ class User < ApplicationRecord
          :recoverable, :rememberable, :trackable, :confirmable, #:validatable,
          :omniauthable, omniauth_providers: [:twitter, :github, :developer]
 
+  has_many :identities,   dependent: :destroy
   has_many :invitations,  dependent: :destroy
   has_many :teammates, dependent: :destroy
   has_many :reviewer_teammates, -> { where(role: ['reviewer', 'program team', 'organizer']) }, class_name: 'Teammate'
@@ -22,7 +23,7 @@ class User < ApplicationRecord
   validates :name, presence: true, allow_nil: true
   validates_uniqueness_of :email, allow_blank: true
   validates_format_of :email, with: Devise.email_regexp, allow_blank: true, if: :email_changed?
-  validates_presence_of :email, on: :create, if: -> { provider.blank? }
+  validates_presence_of :email, on: :create, if: -> { provider.blank? && identities.blank? }
   validates_presence_of :email, on: :update, if: -> { provider.blank? || unconfirmed_email.blank? }
   validates_presence_of :password, on: :create
   validates_confirmation_of :password, on: :create
@@ -34,17 +35,33 @@ class User < ApplicationRecord
 
   attr_accessor :pending_invite_email
 
-  def self.from_omniauth(auth, invitation_email=nil)
-    where(provider: auth.provider, uid: auth.uid).first_or_create do |user|
-      password = Devise.friendly_token[0,20]
-      user.name = auth['info']['name'] if user.name.blank?
-      user.email = invitation_email || auth['info']['email'] || '' if user.email.blank?
-      user.password = password
-      user.password_confirmation = password
-      if !user.confirmed? && invitation_email.present? && user.email == invitation_email
-        user.skip_confirmation!
-      end
+  def self.from_omniauth(auth, invitation_email = nil)
+    # First, lookup in identities table
+    identity = Identity.find_by(provider: auth.provider, uid: auth.uid)
+    return identity.user if identity
+
+    # Fall back to legacy lookup in users table
+    user = find_by(provider: auth.provider, uid: auth.uid)
+    return user if user
+
+    # Create new user with identity
+    create_from_omniauth!(auth, invitation_email)
+  end
+
+  def self.create_from_omniauth!(auth, invitation_email = nil)
+    user = new(
+      name: auth['info']['name'],
+      email: invitation_email || auth['info']['email'] || '',
+      password: (password = Devise.friendly_token[0, 20]),
+      password_confirmation: password
+    )
+    user.identities.build(provider: auth.provider, uid: auth.uid)
+
+    if invitation_email.present? && (user.email == invitation_email)
+      user.skip_confirmation!
     end
+
+    user.tap(&:save!)
   end
 
   def check_pending_invite_email
@@ -66,7 +83,7 @@ def gravatar_hash
   end
 
   def connected?(provider)
-    self.provider == provider
+    identities.exists?(provider: provider) || (self.provider == provider)
   end
 
   def complete?

app/views/profiles/edit.html.haml

@@ -31,8 +31,6 @@
       .widget
         .widget-header
           %i.bi.bi-lock
-          - if current_user.provider.present?
-            %i.bi{class: "bi-#{current_user.provider.downcase}"}
           %h3 Identity Services
         .widget-content
           %p
@@ -48,6 +46,20 @@
           .form-group
             = f.label :password_confirmation
             = f.password_field :password_confirmation, class: 'form-control', placeholder: 'Confirm password'
+          %hr
+          %p Connect your social accounts to enable sign-in via these providers.
+          .connected-accounts
+            - %w[github twitter].each do |provider|
+              - if current_user.connected?(provider)
+                .service.mb-2
+                  %button.btn.btn-secondary.disabled{class: "btn-#{provider}-alt"}
+                    %i{class: "bi bi-#{provider}"}
+                    = "Connected via #{provider.capitalize}"
+              - else
+                .service.mb-2
+                  = button_to send("user_#{provider}_omniauth_authorize_path"), data: {turbo: false}, class: "btn btn-#{provider}", form: {style: 'display: inline-block'} do
+                    %i{class: "bi bi-#{provider}"}
+                    = "Connect #{provider.capitalize}"
 
     %fieldset.col-md-6
       .widget
@@ -59,13 +71,6 @@
             Add or change your profile icon image by visiting
             = link_to('Gravatar', 'https://en.gravatar.com/connect/', target: "_blank")
 
-          - if current_user.provider.present?
-            .service
-              %button.btn.btn-secondary.disabled{class: "btn-#{current_user.provider.downcase}-alt"}
-                %i{class: "bi bi-#{current_user.provider.downcase}"}
-                | Connected via
-                = current_user.provider
-
       - if current_user.teammates.present?
         .widget
           .widget-header

db/migrate/20251219061613_create_identities.rb

@@ -0,0 +1,13 @@
+class CreateIdentities < ActiveRecord::Migration[8.1]
+  def change
+    create_table :identities do |t|
+      t.references :user, null: false, foreign_key: true
+      t.string :provider, null: false
+      t.string :uid, null: false
+
+      t.timestamps
+    end
+
+    add_index :identities, [:provider, :uid], unique: true
+  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[8.1].define(version: 2025_02_15_111232) do
+ActiveRecord::Schema[8.1].define(version: 2025_12_19_061613) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_catalog.plpgsql"
 
@@ -77,6 +77,16 @@
     t.index ["slug"], name: "index_events_on_slug"
   end
 
+  create_table "identities", force: :cascade do |t|
+    t.datetime "created_at", null: false
+    t.string "provider", null: false
+    t.string "uid", null: false
+    t.datetime "updated_at", null: false
+    t.bigint "user_id", null: false
+    t.index ["provider", "uid"], name: "index_identities_on_provider_and_uid", unique: true
+    t.index ["user_id"], name: "index_identities_on_user_id"
+  end
+
   create_table "invitations", force: :cascade do |t|
     t.datetime "created_at", precision: nil
     t.string "email"
@@ -392,6 +402,7 @@
 
   add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id"
   add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id"
+  add_foreign_key "identities", "users"
   add_foreign_key "pages", "websites"
   add_foreign_key "session_format_configs", "session_formats"
   add_foreign_key "session_format_configs", "websites"

spec/controllers/users/omniauth_callbacks_controller_spec.rb

@@ -5,12 +5,12 @@
     Rails.application.reload_routes_unless_loaded
   end
 
+  let(:twitter_auth_hash) { OmniAuth.config.mock_auth[:twitter] }
+  let(:github_auth_hash) { OmniAuth.config.mock_auth[:github] }
+
   describe '#twitter' do
-    let(:twitter_auth_hash) { OmniAuth.config.mock_auth[:twitter] }
-    let(:github_auth_hash) { OmniAuth.config.mock_auth[:github] }
     let(:user) { build(:user) }
-    let(:invitation) { build(:invitation, slug: "abc123", email: '[email protected]') }
-    let(:other_invitation) { build(:invitation, slug: "def456", email: '[email protected]') }
+    let(:invitation) { build(:invitation, slug: 'abc123', email: '[email protected]') }
 
     before :each do
       session[:invitation_slug] = invitation.slug
@@ -19,27 +19,80 @@
       allow(controller).to receive(:current_user).and_return(User.last)
     end
 
-    it "allows twitter login" do
+    it 'allows twitter login for new user' do
       get :twitter
       expect(response).to redirect_to(edit_profile_url)
+      expect(User.last.identities.find_by(provider: 'twitter')).to be_present
     end
   end
 
-  describe "GET #new" do
-    let(:github_auth_hash) { OmniAuth.config.mock_auth[:github] }
+  describe 'signing in' do
+    before :each do
+      request.env['omniauth.auth'] = github_auth_hash
+      request.env['devise.mapping'] = Devise.mappings[:user]
+    end
+
+    context 'when user exists with identity' do
+      let!(:user) { create(:user) }
+      let!(:identity) { user.identities.create!(provider: 'github', uid: github_auth_hash.uid) }
+
+      it 'finds user via identity and signs in' do
+        expect { get :github }.not_to change { User.count }
+        expect(controller.current_user).to eq(user)
+      end
+    end
 
+    context 'when user exists with legacy provider/uid on User' do
+      let!(:user) { create(:user, provider: 'github', uid: github_auth_hash.uid) }
+
+      it 'finds user via legacy lookup and signs in' do
+        expect { get :github }.not_to change { User.count }
+        expect(controller.current_user).to eq(user)
+      end
+    end
+  end
+
+  describe 'linking identity to logged-in user' do
     before :each do
       request.env['omniauth.auth'] = github_auth_hash
       request.env['devise.mapping'] = Devise.mappings[:user]
     end
 
-    it "redirects a user if they are currently logged in" do
-      organizer = create(:organizer)
-      sign_in(organizer)
-      get :github
+    context 'when connecting a new provider' do
+      it 'creates identity and redirects to profile' do
+        user = create(:user)
+        sign_in(user)
+
+        expect { get :github }.to change { user.identities.count }.by(1)
+        expect(response).to redirect_to(edit_profile_path)
+        expect(flash[:info]).to eq('Successfully connected GitHub to your account.')
+      end
+    end
+
+    context 'when provider is already connected to current user' do
+      it 'shows already connected message' do
+        user = create(:user)
+        user.identities.create!(provider: 'github', uid: github_auth_hash.uid)
+        sign_in(user)
+
+        expect { get :github }.not_to change { Identity.count }
+        expect(response).to redirect_to(edit_profile_path)
+        expect(flash[:info]).to eq('GitHub is already connected to your account.')
+      end
+    end
+
+    context 'when provider is already connected to another user' do
+      it 'shows error message' do
+        other_user = create(:user)
+        other_user.identities.create!(provider: 'github', uid: github_auth_hash.uid)
+
+        user = create(:user)
+        sign_in(user)
 
-      expect(response).to redirect_to(events_url)
-      expect(controller.current_user).to eq(organizer)
+        expect { get :github }.not_to change { Identity.count }
+        expect(response).to redirect_to(edit_profile_path)
+        expect(flash[:danger]).to eq('This GitHub account is already connected to another user.')
+      end
     end
   end
 end

spec/models/user_spec.rb

@@ -48,13 +48,14 @@
         }.to_not raise_error
       end
 
-      it "creates a new user and sets provider for user" do
+      it "creates a new user with identity" do
         user = nil
         expect {
           user = User.from_omniauth(twitter_auth_hash)
         }.to change { User.count }.by(1)
-        expect(user.provider).to eq("twitter")
-        expect(user.uid).to eq(twitter_auth_hash.uid)
+        expect(user.identities.count).to eq(1)
+        expect(user.identities.first.provider).to eq('twitter')
+        expect(user.identities.first.uid).to eq(twitter_auth_hash.uid)
       end
     end
 

spec/system/profile_spec.rb

@@ -29,8 +29,9 @@
 
   scenario "A user attempts to save their bio without email", js: true do
     visit(edit_profile_path)
-    fill_in('Email', with: '')
-    click_button 'Save'
+    fill_in('Email', with: '', fill_options: {clear: :backspace})
+    # Submit form via JavaScript to bypass any browser validation
+    page.execute_script("document.querySelector('form').submit()")
     expect(page).to have_content("Unable to save profile. Please correct the following: Email can't be blank")
   end