Merge pull request #18971 from opf/feature/63912-support-multiple-authentication-provider-user-links

[#63912] Support multiple authentication provider user links

app/contracts/users/base_contract.rb

@@ -47,9 +47,6 @@ module Users
     attribute :status,
               writable: ->(*) { can_create_or_manage_users? }
 
-    attribute :identity_url,
-              writable: ->(*) { user.admin? }
-
     attribute :force_password_change,
               writable: ->(*) { user.admin? }
 
@@ -58,25 +55,31 @@ module Users
     end
 
     validate :validate_password_writable
+    validate :validate_identity_url_writable
     validate :existing_auth_source
 
     delegate :available_custom_fields, to: :model
 
     def reduce_writable_attributes(attributes)
       super.tap do |writable|
+        # `password` and `identity_url` are not regular attributes so they bypass
+        # attribute writable checks. therewore they must be added to the list
+        # of writable attributes under certain conditions.
         writable << "password" if password_writable?
+        writable << "identity_url" if identity_url_writable?
       end
     end
 
     private
 
-    ##
-    # Password is not a regular attribute so it bypasses
-    # attribute writable checks
     def password_writable?
       user.admin? || user.id == model.id
     end
 
+    def identity_url_writable?
+      user.admin?
+    end
+
     ##
     # User#password is not an ActiveModel property,
     # but just an accessor, so we need to identify it being written there.
@@ -88,6 +91,12 @@ module Users
       errors.add :password, :error_readonly if model.password.present?
     end
 
+    def validate_identity_url_writable
+      return if identity_url_writable?
+
+      errors.add(:identity_url, :error_readonly) if model.user_auth_provider_links.any?(&:changed?)
+    end
+
     # rubocop:disable Rails/DynamicFindBy
     def existing_auth_source
       if ldap_auth_source_id && LdapAuthSource.find_by_unique(ldap_auth_source_id).nil?

app/controllers/account_controller.rb

@@ -289,7 +289,7 @@ class AccountController < ApplicationController
 
     invited = session[:invitation_token].present?
     get = request.get? && allow
-    post = (request.post? || request.patch?) && (session[:auth_source_registration] || allow)
+    post = (request.post? || request.patch?) && (session[:auth_source_registration].present? || allow)
 
     invited || get || post
   end
@@ -333,7 +333,7 @@ class AccountController < ApplicationController
       @user.consented_at = DateTime.now
     end
 
-    if session[:auth_source_registration]
+    if session[:auth_source_registration].present?
       register_with_auth_source(@user)
     else
       register_plain_user(@user)

app/controllers/concerns/user_invitation.rb

@@ -103,7 +103,7 @@ module UserInvitation
   end
 
   def reset_login(user_id)
-    User.where(id: user_id).update_all identity_url: nil
+    UserAuthProviderLink.where(user_id:).delete_all
     UserPassword.where(user_id:).destroy_all
   end
 

app/models/auth_provider.rb

@@ -29,6 +29,9 @@
 class AuthProvider < ApplicationRecord
   belongs_to :creator, class_name: "User"
 
+  has_many :user_auth_provider_links, dependent: :destroy
+  has_many :users, through: :user_auth_provider_links
+
   validates :display_name, presence: true
   validates :display_name, uniqueness: true
 
@@ -39,7 +42,7 @@ class AuthProvider < ApplicationRecord
   end
 
   def user_count
-    @user_count ||= User.where("identity_url LIKE ?", "#{slug}%").count
+    @user_count ||= user_auth_provider_links.count
   end
 
   def human_type

app/models/principal.rb

@@ -122,6 +122,8 @@ class Principal < ApplicationRecord
 
   before_create :set_default_empty_values
 
+  self.ignored_columns += [:identity_url]
+
   # Columns required for formatting the principal's name.
   def self.columns_for_name(formatter = nil)
     raise NotImplementedError, "Redefine in subclass" unless self == Principal

app/models/user.rb

@@ -96,6 +96,8 @@ class User < Principal
   has_many :emoji_reactions, dependent: :destroy
   has_many :reminders, foreign_key: "creator_id", dependent: :destroy, inverse_of: :creator
   has_many :remote_identities, dependent: :destroy
+  has_many :user_auth_provider_links, dependent: :destroy
+  has_many :auth_providers, through: :user_auth_provider_links
 
   # Users blocked via brute force prevention
   # use lambda here, so time is evaluated on each query
@@ -308,11 +310,18 @@ class User < Principal
     end
   end
 
-  def authentication_provider
-    return nil if identity_url.blank?
+  def active_user_auth_provider_link
+    # note: order("updated_at") is not used, because it returns nil if relation is not persisted
+    user_auth_provider_links.max_by(&:updated_at)
+  end
 
-    slug = identity_url.split(":", 2).first
-    AuthProvider.find_by(slug:)
+  def identity_url
+    link = active_user_auth_provider_link
+    "#{link.auth_provider.slug}:#{link.external_id}" if link.present?
+  end
+
+  def authentication_provider
+    active_user_auth_provider_link&.auth_provider
   end
 
   # Return user's authentication provider for display
@@ -373,7 +382,7 @@ class User < Principal
 
   # Is the user authenticated via an external authentication source via OmniAuth?
   def uses_external_authentication?
-    identity_url.present?
+    user_auth_provider_links.exists?
   end
 
   #
@@ -548,7 +557,7 @@ class User < Principal
   #   - OmniAuth
   #   - LDAP
   def missing_authentication_method?
-    identity_url.nil? && passwords.empty? && ldap_auth_source_id.nil?
+    !uses_external_authentication? && passwords.empty? && ldap_auth_source_id.nil?
   end
 
   # Returns the anonymous user.  If the anonymous user does not exist, it is created.  There can be only

app/models/user_auth_provider_link.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+class UserAuthProviderLink < ApplicationRecord
+  belongs_to :user
+  belongs_to :auth_provider
+
+  scope :with_identity_url, ->(identity_url) do
+    return nil if identity_url.blank?
+
+    slug, external_id = identity_url.split(":", 2)
+    where(auth_provider: AuthProvider.find_by(slug:), external_id:)
+  end
+end

app/services/authentication/omniauth_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH
@@ -35,7 +37,6 @@ module Authentication
                   :controller,
                   :contract,
                   :user_attributes,
-                  :identity_url,
                   :user
 
     delegate :session, to: :controller
@@ -53,6 +54,11 @@ module Authentication
       unless contract.validate
         result = ServiceResult.failure(errors: contract.errors)
         Rails.logger.error do
+          # Try to provide some context of the auth_hash in case of errors
+          auth_uid = begin
+            hash = auth_hash || {}
+            hash.dig(:info, :uid) || hash[:uid] || "unknown"
+          end
           "[OmniAuth strategy #{strategy.name}] Failed to process omniauth response for #{auth_uid}: #{result.message}"
         end
         inspect_response(Logger::ERROR)
@@ -62,7 +68,7 @@ module Authentication
 
       # Create or update the user from omniauth
       # and assign non-nil parameters from the registration form - if any
-      assignable_params = (additional_user_params || {}).reject { |_, v| v.nil? }
+      assignable_params = (additional_user_params || {}).compact
       update_user_from_omniauth!(assignable_params)
 
       # If we have a new or invited user, we still need to register them
@@ -115,7 +121,6 @@ module Authentication
     def update_user_from_omniauth!(additional_user_params)
       # Find or create the user from the auth hash
       self.user_attributes = build_omniauth_hash_to_user_attributes.merge(additional_user_params)
-      self.identity_url = user_attributes[:identity_url]
       self.user = lookup_or_initialize_user
 
       # Assign or update the user with the omniauth attributes
@@ -134,7 +139,7 @@ module Authentication
       find_invited_user ||
         find_existing_user ||
         remap_existing_user ||
-        initialize_new_user
+        User.new
     end
 
     ##
@@ -142,12 +147,17 @@ module Authentication
     def find_invited_user
       return unless session.include?(:invitation_token)
 
-      tok = Token::Invitation.find_by value: session[:invitation_token]
-      return unless tok
+      token = Token::Invitation.find_by value: session[:invitation_token]
+      return unless token
 
-      tok.user.tap do |user|
-        user.identity_url = user_attributes[:identity_url]
-        tok.destroy
+      token.user.tap do |user|
+        if user_attributes[:identity_url].present?
+          slug, external_id = user_attributes[:identity_url].split(":", 2)
+          link = user.user_auth_provider_links.find_or_initialize_by(auth_provider: AuthProvider.find_by!(slug:))
+          link.external_id = external_id
+          link.save!
+        end
+        token.destroy
         session.delete :invitation_token
       end
     end
@@ -155,7 +165,11 @@ module Authentication
     ##
     # Find an existing user by the identity url
     def find_existing_user
-      User.find_by(identity_url:)
+      if developer_provider?
+        User.find_by(mail: auth_hash[:uid])
+      else
+        UserAuthProviderLink.with_identity_url(user_attributes[:identity_url]).first&.user
+      end
     end
 
     ##
@@ -166,13 +180,6 @@ module Authentication
       User.not_builtin.find_by_login(user_attributes[:login])
     end
 
-    ##
-    # Create the new user and try to activate it
-    # according to settings and system limits
-    def initialize_new_user
-      User.new(identity_url: user_attributes[:identity_url])
-    end
-
     ##
     # Update or assign the user attributes
     def update_attributes
@@ -275,15 +282,20 @@ module Authentication
     # For SAML, the global UID may change with every session
     # (in case of transient nameIds)
     def identity_url_from_omniauth
+      return if developer_provider?
+
       identifier = auth_hash[:info][:uid] || auth_hash[:uid]
       "#{auth_hash[:provider]}:#{identifier}"
     end
 
     ##
-    # Try to provide some context of the auth_hash in case of errors
-    def auth_uid
-      hash = auth_hash || {}
-      hash.dig(:info, :uid) || hash[:uid] || "unknown"
+    # Indicates whether OmniAuth::Strategies::Delevoper strategy is used.
+    # https://github.com/omniauth/omniauth/blob/0bcfd5b25bf946422cd4d9c40c4f514121ac04d6/lib/omniauth/strategies/developer.rb
+    # if true:
+    #   identity_url should not be set and
+    #   user should be found by mail, because mail plays uid role in developer strategy
+    def developer_provider?
+      Rails.env.local? && auth_hash[:provider] == "developer"
     end
   end
 end

app/services/users/register_user_service.rb

@@ -126,7 +126,7 @@ module Users
     end
 
     def skip_omniauth_user?
-      user.identity_url.blank?
+      user.user_auth_provider_links.blank?
     end
 
     def limited_provider?(user)

app/services/users/set_attributes_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH
@@ -36,6 +38,7 @@ module Users
 
     def set_attributes(params)
       self.pref = params.delete(:pref)
+      set_user_auth_provider_links(params.delete(:identity_url))
 
       super
     end
@@ -60,6 +63,23 @@ module Users
         .call(pref)
     end
 
+    def set_user_auth_provider_links(identity_url)
+      if identity_url.present?
+        slug, external_id = identity_url.split(":", 2)
+        if slug.present? && external_id.present?
+          auth_provider_id = AuthProvider.where(slug:).pick(:id)
+          if auth_provider_id.present?
+            model
+              .user_auth_provider_links
+              .find_or_initialize_by(auth_provider_id:)
+              .assign_attributes(external_id:)
+          else
+            raise ActiveRecord::RecordNotFound, "AuthProvider with slug: \"#{slug}\" has been not found"
+          end
+        end
+      end
+    end
+
     # rubocop:disable Metrics/AbcSize
     def assign_name_attributes_from_mail(params)
       placeholder = placeholder_name(params[:mail])

db/migrate/20230731153909_add_file_link_journals_to_existing_containers.rb

@@ -30,6 +30,7 @@
 
 class AddFileLinkJournalsToExistingContainers < ActiveRecord::Migration[7.0]
   def up
+    SystemUser.reset_column_information
     system_user = SystemUser.first
     containers = Storages::FileLink.includes(:container).map(&:container).uniq.compact
 

db/migrate/20240131130134_fix_inherited_group_memberships.rb

@@ -2,6 +2,7 @@ class FixInheritedGroupMemberships < ActiveRecord::Migration[7.0]
   def up
     # Recreate member_roles for all group members
     # due to regression https://community.openproject.org/work_packages/52528
+    Group.reset_column_information
     Group
       .where(id: Member.where(project_id: nil, user_id: Group.select(:id)).select(:user_id))
       .find_each do |group|

db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb

@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+class MoveUsersIdentityUrlToUserAuthProviderLinks < ActiveRecord::Migration[8.0]
+  def change
+    create_table :user_auth_provider_links do |t|
+      t.references :user, null: false, foreign_key: { on_delete: :cascade, on_update: :cascade }
+      t.references :auth_provider, null: false, foreign_key: { on_delete: :cascade, on_update: :cascade }
+      t.string :external_id, null: false
+      t.timestamps null: false
+      t.index %i[user_id auth_provider_id], unique: true
+      t.index %i[auth_provider_id external_id], unique: true
+    end
+
+    reversible do |direction|
+      direction.up do
+        users_data = execute(<<-SQL.squish).values
+          SELECT id, identity_url FROM users WHERE type = 'User' AND NOT (identity_url = '' OR identity_url IS NULL);
+        SQL
+        auth_providers_data = execute(<<-SQL.squish).values
+          SELECT id, slug FROM auth_providers
+        SQL
+
+        insert_data = users_data.filter_map do |user_id, identity_url|
+          slug, external_id = identity_url.split(":", 2)
+          next if slug.blank? || external_id.blank?
+
+          auth_provider_id = auth_providers_data.find { |_, auth_provider_slug| auth_provider_slug == slug }.first
+          next if auth_provider_id.blank?
+
+          "(#{user_id}, #{auth_provider_id}, '#{external_id}', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"
+        end.join(",\n")
+
+        if insert_data.present?
+          execute(<<-SQL.squish).values
+            INSERT INTO user_auth_provider_links (user_id, auth_provider_id, external_id, created_at, updated_at)
+              VALUES #{insert_data}
+              ON CONFLICT DO NOTHING
+              RETURNING id
+          SQL
+        end
+      end
+    end
+  end
+end

docs/api/apiv3/components/schemas/user_model.yml

@@ -83,10 +83,12 @@ allOf:
           - 'null'
         description: |-
           User's identity_url for OmniAuth authentication.
+          **Deprecated:** It will be removed in the near future.
     
           # Conditions
     
           - User is self, or `create_user` or `manage_user` permission globally
+        deprecated: true
       createdAt:
         type: string
         format: date-time

lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb

@@ -30,7 +30,7 @@ module OpenProject
                                            error_description: "Requires scope #{scope} to access this resource."
                 end
 
-                user = User.find_by(identity_url: "#{provider.slug}:#{payload['sub']}")
+                user = provider.user_auth_provider_links.find_by(external_id: payload["sub"])&.user
                 authentication_result(user)
               end,
               ->(error) { fail_with_header!(error: "invalid_token", error_description: error) }

modules/storages/spec/common/storages/peripherals/connection_validators/nextcloud/authentication_validator_spec.rb

@@ -41,8 +41,10 @@ module Storages
           context "when using OAuth2" do
             let(:user) { create(:user) }
             let(:storage) do
-              create(:nextcloud_storage_with_local_connection, :as_not_automatically_managed,
-                     oauth_client_token_user: user, origin_user_id: "m.jade@death.star")
+              create(:nextcloud_storage_with_local_connection,
+                     :as_not_automatically_managed,
+                     oauth_client_token_user: user,
+                     origin_user_id: "m.jade@death.star")
             end
 
             before { User.current = user }
@@ -72,8 +74,9 @@ module Storages
           context "when using OpenID Connect" do
             let(:storage) { create(:nextcloud_storage_configured, :oidc_sso_enabled) }
 
-            let(:user) { create(:user, authentication_provider: oidc_provider) }
+            let(:user) { create(:user, identity_url: "#{oidc_provider.slug}:123123123123") }
             let!(:oidc_provider) { create(:oidc_provider, scope:) }
+            let!(:saml_provider) { create(:saml_provider) }
             let(:scope) { "openid email profile offline_access" }
 
             before do
@@ -91,7 +94,7 @@ module Storages
 
             describe "error and warning handling" do
               it "returns a warning if the current user isn't provisioned" do
-                user.update!(identity_url: nil)
+                user.user_auth_provider_links.destroy_all
                 result = validator.call
 
                 expect(result[:non_provisioned_user]).to be_warning
@@ -102,7 +105,9 @@ module Storages
               end
 
               it "returns a warning if the user is not provisioned by an oidc provider" do
-                user.update!(identity_url: "ldap-provider:this-will-trigger-a-warning")
+                link = user.user_auth_provider_links.first
+                link.update!(auth_provider_id: saml_provider.id)
+
                 result = validator.call
 
                 expect(result[:provisioned_user_provider]).to be_warning

spec/contracts/users/shared_contract_examples.rb

@@ -84,7 +84,10 @@ RSpec.shared_examples_for "user contract" do
 
     describe "cannot set the identity url" do
       before do
-        user.identity_url = "saml:123412foo"
+        user.user_auth_provider_links.build(
+          auth_provider: create(:oidc_provider),
+          external_id: "123123123"
+        )
       end
 
       it_behaves_like "contract is invalid", identity_url: :error_readonly

spec/controllers/account_controller_spec.rb

@@ -1049,10 +1049,12 @@ RSpec.describe AccountController, :skip_2fa_stage do
 
   describe "registering through auth source" do
     context "when not providing all required fields" do
-      let(:omniauth_strategy) { double("Google Strategy", name: "google") } # rubocop:disable RSpec/VerifiedDoubles
+      let(:slug) { "google" }
+      let(:omniauth_strategy) { double("Google Strategy", name: slug) } # rubocop:disable RSpec/VerifiedDoubles
+      let!(:oidc_google) { create(:oidc_provider_google, slug:) }
       let(:omniauth_hash) do
         OmniAuth::AuthHash.new(
-          provider: "google",
+          provider: slug,
           strategy: omniauth_strategy,
           uid: "123545",
           info: { name: "foo",

spec/controllers/omni_auth_login_controller_spec.rb

@@ -32,6 +32,7 @@ require "spec_helper"
 
 # Concern is included into AccountController and depends on methods available there
 RSpec.describe OmniAuthLoginController, :skip_2fa_stage do
+  let!(:auth_provider) { create(:oidc_provider_google, slug: "google") }
   let(:omniauth_strategy) { double("Google Strategy", name: "google") } # rubocop:disable RSpec/VerifiedDoubles
   let(:omniauth_hash) do
     OmniAuth::AuthHash.new(
@@ -192,7 +193,7 @@ RSpec.describe OmniAuthLoginController, :skip_2fa_stage do
 
         it "shows a notice about the activated account", :aggregate_failures do
           expect(flash[:notice]).to eq(I18n.t("notice_account_registered_and_logged_in"))
-          user = User.last
+          user = User.find_by!(firstname: "foo")
 
           expect(user.firstname).to eq "foo"
           expect(user.lastname).to eq "bar"
@@ -213,16 +214,11 @@ RSpec.describe OmniAuthLoginController, :skip_2fa_stage do
         )
       end
 
-      let(:user) do
-        build(:user, force_password_change: false,
-                     identity_url: "google:123545")
+      let!(:user) do
+        create(:user, force_password_change: false, identity_url: "google:123545")
       end
 
       context "with an active account" do
-        before do
-          user.save!
-        end
-
         it "signs in the user after successful external authentication" do
           post :callback, params: { provider: :google }
 
@@ -374,31 +370,39 @@ RSpec.describe OmniAuthLoginController, :skip_2fa_stage do
               expect(OpenProject::OmniAuth::Authorization).not_to have_received(:after_login!).with(user, any_args)
             end
 
-            it "is approved against any other provider" do
-              omniauth_hash.provider = "some other"
+            context "with other provider availabe" do
+              let(:other_provider_slug) { "some other" }
 
-              post :callback, params: { provider: :google }
+              before do
+                create(:oidc_provider, slug: other_provider_slug)
+              end
 
-              expect(response).to redirect_to home_url(first_time_user: true)
+              it "is approved against other provider" do
+                omniauth_hash.provider = other_provider_slug
 
-              # The authorization is successful which results in the registration
-              # of a new user in this case because we changed the provider
-              # and there isn't a user with that identity URL yet.
-              new_user = User.find_by identity_url: "some other:123545"
-              expect(OpenProject::OmniAuth::Authorization)
-                .to have_received(:after_login!).with(new_user, any_args)
-            end
+                post :callback, params: { provider: :google }
 
-            # ... and to confirm that, here's what happens when the authorization fails
-            it "is rejected against any other provider with the wrong email" do
-              omniauth_hash.provider = "yet another"
-              config.global_email = "yarrrr@joro.es"
+                expect(response).to redirect_to home_url(first_time_user: true)
 
-              post :callback, params: { provider: :google }
+                # The authorization is successful which results in the registration
+                # of a new user in this case because we changed the provider
+                # and there isn't a user with that identity URL yet.
+                new_user = UserAuthProviderLink.with_identity_url("some other:123545").first.user
+                expect(OpenProject::OmniAuth::Authorization)
+                  .to have_received(:after_login!).with(new_user, any_args)
+              end
 
-              expect(response).to redirect_to signin_path
-              expect(flash[:error]).to eq "I only want to see yarrrr@joro.es here."
-              expect(OpenProject::OmniAuth::Authorization).not_to have_received(:after_login!).with(user, any_args)
+              # ... and to confirm that, here's what happens when the authorization fails
+              it "is rejected against any other provider with the wrong email" do
+                omniauth_hash.provider = "yet another"
+                config.global_email = "yarrrr@joro.es"
+
+                post :callback, params: { provider: :google }
+
+                expect(response).to redirect_to signin_path
+                expect(flash[:error]).to eq "I only want to see yarrrr@joro.es here."
+                expect(OpenProject::OmniAuth::Authorization).not_to have_received(:after_login!).with(user, any_args)
+              end
             end
           end
         end
@@ -407,8 +411,7 @@ RSpec.describe OmniAuthLoginController, :skip_2fa_stage do
       context "with a registered and not activated accout",
               with_settings: { self_registration: Setting::SelfRegistration.by_email } do
         before do
-          user.register
-          user.save!
+          user.registered!
 
           post :callback, params: { provider: :google }
         end
@@ -422,8 +425,7 @@ RSpec.describe OmniAuthLoginController, :skip_2fa_stage do
       context "with an invited user and self registration disabled",
               with_settings: { self_registration: Setting::SelfRegistration.disabled } do
         before do
-          user.invite
-          user.save!
+          user.invited!
 
           post :callback, params: { provider: :google }
         end
@@ -437,8 +439,7 @@ RSpec.describe OmniAuthLoginController, :skip_2fa_stage do
       context "with a locked account",
               with_settings: { brute_force_block_after_failed_logins: 0 } do
         before do
-          user.lock
-          user.save!
+          user.locked!
 
           post :callback, params: { provider: :google }
         end

spec/factories/user_factory.rb

@@ -36,12 +36,12 @@ FactoryBot.define do
     sequence(:mail) { |n| "bobmail#{n}.bobbit@bob.com" }
     password { "adminADMIN!" }
     password_confirmation { "adminADMIN!" }
-    identity_url { nil }
 
     transient do
       preferences { {} }
       authentication_provider { nil }
       external_id { SecureRandom.uuid }
+      identity_url { nil }
     end
 
     language { "en" }
@@ -65,7 +65,15 @@ FactoryBot.define do
       end
 
       if factory.authentication_provider.present?
-        user.update!(identity_url: "#{factory.authentication_provider.slug}:#{factory.external_id}")
+        user.user_auth_provider_links.create!(auth_provider: factory.authentication_provider,
+                                              external_id: factory.external_id)
+      end
+      if factory.identity_url.present?
+        slug, external_id = factory.identity_url.split(":", 2)
+        raise "slug or external_id is blank" if slug.blank? || external_id.blank?
+
+        auth_provider = AuthProvider.find_by(slug:) || create(:oidc_provider, slug:)
+        user.user_auth_provider_links.create!(auth_provider:, external_id:)
       end
     end
 

spec/features/auth/omniauth_spec.rb

@@ -34,14 +34,17 @@ RSpec.describe "Omniauth authentication" do
   # Load ViewAccountLoginAuthProvider to have this spec passing
   OpenProject::Hooks::ViewAccountLoginAuthProvider
 
+  let(:mail) { "omnibob@example.com" }
+  let(:login) { "omnibob" }
+  let(:firstname) { "omni" }
+  let(:lastname) { "bob" }
   let(:user) do
     create(:user,
            force_password_change: false,
-           identity_url: "developer:omnibob@example.com",
-           login: "omnibob",
-           mail: "omnibob@example.com",
-           firstname: "omni",
-           lastname: "bob")
+           login:,
+           mail:,
+           firstname:,
+           lastname:)
   end
 
   before do
@@ -64,7 +67,7 @@ RSpec.describe "Omniauth authentication" do
     translation.scan(/(^.*) %\{/).first.first
   end
 
-  context "sign in existing user" do
+  describe "existing user sign in" do
     it "redirects to back url" do
       visit account_lost_password_path
       click_link("Omniauth Developer", match: :first, visible: :all)
@@ -141,16 +144,16 @@ RSpec.describe "Omniauth authentication" do
 
       SeleniumHubWaiter.wait
       # login form developer strategy
-      fill_in("first_name", with: user.firstname)
+      fill_in("first_name", with: firstname)
       # intentionally do not supply last_name
-      fill_in("email", with: user.mail)
+      fill_in("email", with: mail)
       click_link_or_button "Sign In"
 
       expect(page).to have_content "Last name can't be blank"
       # on register form, we are prompted for a last name
       within("#content") do
         SeleniumHubWaiter.wait
-        fill_in("user_lastname", with: user.lastname)
+        fill_in("user_lastname", with: lastname)
         click_link_or_button "Create"
       end
 
@@ -159,20 +162,11 @@ RSpec.describe "Omniauth authentication" do
     end
   end
 
-  context "register on the fly",
-          with_settings: {
-            self_registration?: true,
-            self_registration: Setting::SelfRegistration.automatic
-          } do
-    let(:user) do
-      User.new(force_password_change: false,
-               identity_url: "developer:omnibob@example.com",
-               login: "omnibob",
-               mail: "omnibob@example.com",
-               firstname: "omni",
-               lastname: "bob")
-    end
-
+  describe "on-the-fly registration",
+           with_settings: {
+             self_registration?: true,
+             self_registration: Setting::SelfRegistration.automatic
+           } do
     it_behaves_like "omniauth user registration"
 
     it "redirects to homescreen" do
@@ -181,15 +175,15 @@ RSpec.describe "Omniauth authentication" do
 
       SeleniumHubWaiter.wait
       # login form developer strategy
-      fill_in("first_name", with: user.firstname)
+      fill_in("first_name", with: firstname)
       # intentionally do not supply last_name
-      fill_in("email", with: user.mail)
+      fill_in("email", with: mail)
       click_link_or_button "Sign In"
 
       # on register form, we are prompted for a last name
       within("#content") do
         SeleniumHubWaiter.wait
-        fill_in("user_lastname", with: user.lastname)
+        fill_in("user_lastname", with: lastname)
         click_link_or_button "Create"
       end
 
@@ -202,10 +196,10 @@ RSpec.describe "Omniauth authentication" do
     end
   end
 
-  context "registration by email",
-          with_settings: {
-            self_registration: Setting::SelfRegistration.by_email
-          } do
+  describe "email registration",
+           with_settings: {
+             self_registration: Setting::SelfRegistration.by_email
+           } do
     shared_examples "registration with registration by email" do
       it "still automatically activates the omniauth account" do
         visit login_path
@@ -236,7 +230,7 @@ RSpec.describe "Omniauth authentication" do
     end
   end
 
-  context "error occurs" do
+  describe "error handling" do
     shared_examples "omniauth signin error" do
       it "fails with generic error message" do
         # set omniauth to test mode will redirect all calls to omniauth

spec/models/queries/placeholder_users/placeholder_user_query_spec.rb

@@ -154,7 +154,7 @@ RSpec.describe Queries::PlaceholderUsers::PlaceholderUserQuery do
 
     describe "#results" do
       it "is the same as handwriting the query" do
-        expected = "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"type\" = 'PlaceholderUser' ORDER BY \"users\".\"lastname\" DESC, \"users\".\"id\" DESC"
+        expected = "SELECT \"users\".\"id\", \"users\".\"login\", \"users\".\"firstname\", \"users\".\"lastname\", \"users\".\"mail\", \"users\".\"admin\", \"users\".\"status\", \"users\".\"last_login_on\", \"users\".\"language\", \"users\".\"ldap_auth_source_id\", \"users\".\"created_at\", \"users\".\"updated_at\", \"users\".\"type\", \"users\".\"first_login\", \"users\".\"force_password_change\", \"users\".\"failed_login_count\", \"users\".\"last_failed_login_on\", \"users\".\"consented_at\", \"users\".\"webauthn_id\" FROM \"users\" WHERE \"users\".\"type\" = 'PlaceholderUser' ORDER BY \"users\".\"lastname\" DESC, \"users\".\"id\" DESC"
 
         expect(instance.results.to_sql).to eql expected
       end

spec/models/user_spec.rb

@@ -374,20 +374,16 @@ RSpec.describe User do
   end
 
   describe "#authentication_provider" do
-    let!(:provider) { create(:oidc_provider, slug: "test_provider") }
+    context "when there is a link between user and auth provider" do
+      let!(:user) { create(:user, identity_url: "provider:123123213") }
 
-    before do
-      user.identity_url = "test_provider:veryuniqueid"
-      user.save!
+      it "returns the provider when there is a link" do
+        provider = AuthProvider.find_by!(slug: "provider")
+        expect(user.authentication_provider).to eql(provider)
+      end
     end
 
-    it "returns the provider" do
-      expect(user.authentication_provider).to eql(provider)
-    end
-
-    context "when no matching provider exists" do
-      let!(:provider) { nil }
-
+    context "when there is no link" do
       it "returns nil" do
         expect(user.authentication_provider).to be_nil
       end
@@ -395,20 +391,15 @@ RSpec.describe User do
   end
 
   describe "#human_authentication_provider" do
-    let!(:provider) { create(:oidc_provider, slug: "test_provider", display_name: "Karl") }
+    context "when there is a link between user and auth provider" do
+      let(:user) { create(:user, identity_url: "provider:123123213") }
 
-    before do
-      user.identity_url = "test_provider:veryuniqueid"
-      user.save!
+      it "returns a human readable name" do
+        expect(user.human_authentication_provider).to eql("Foobar")
+      end
     end
 
-    it "returns a human readable name" do
-      expect(user.human_authentication_provider).to eql("Karl")
-    end
-
-    context "when no matching provider exists" do
-      let!(:provider) { nil }
-
+    context "when no provider exists" do
       it "returns nil" do
         expect(user.authentication_provider).to be_nil
       end
@@ -503,7 +494,7 @@ RSpec.describe User do
 
   describe "#uses_external_authentication?" do
     context "with identity_url" do
-      let(:user) { build(:user, identity_url: "test_provider:veryuniqueid") }
+      let(:user) { create(:user, identity_url: "test_provider:veryuniqueid") }
 
       it "returns true" do
         expect(user).to be_uses_external_authentication

spec/requests/api/v3/user/create_user_resource_spec.rb

@@ -45,6 +45,7 @@ RSpec.describe API::V3::Users::UsersAPI do
       language: "de"
     }
   end
+  let(:auth_provider) { create(:oidc_provider_google) }
 
   before do
     login_as(current_user)
@@ -142,11 +143,9 @@ RSpec.describe API::V3::Users::UsersAPI do
       end
 
       context "with identity_url" do
-        let(:identity_url) { "google:3289272389298" }
+        let(:identity_url) { "#{auth_provider.slug}:3289272389298" }
 
-        before do
-          parameters[:identityUrl] = identity_url
-        end
+        before { parameters[:identityUrl] = identity_url }
 
         it "creates the user with the given identity_url" do
           send_request
@@ -201,7 +200,7 @@ RSpec.describe API::V3::Users::UsersAPI do
 
     describe "active status" do
       context "with identity_url" do
-        let(:identity_url) { "google:3289272389298" }
+        let(:identity_url) { "#{auth_provider.slug}:3289272389298" }
 
         before do
           parameters[:identityUrl] = identity_url

spec/services/authentication/omniauth_service_spec.rb

@@ -30,13 +30,14 @@
 require "spec_helper"
 
 RSpec.describe Authentication::OmniauthService do
-  let(:strategy) { double("Omniauth Strategy", name: "saml") }
+  shared_let(:auth_provider) { create(:oidc_provider_google) }
   let(:additional_info) do
     {}
   end
+  let(:strategy) { instance_double(OmniAuth::Strategy, name: auth_provider.slug) }
   let(:auth_hash) do
     OmniAuth::AuthHash.new(
-      provider: "google",
+      provider: auth_provider.slug,
       uid: "123545",
       info: {
         name: "foo",
@@ -95,7 +96,7 @@ RSpec.describe Authentication::OmniauthService do
     let(:call) { instance.call }
 
     context "with an active found user" do
-      shared_let(:user) { create(:user, login: "foo@bar.com", identity_url: "google:123545") }
+      shared_let(:user) { create(:user, login: "foo@bar.com", identity_url: "#{auth_provider.slug}:123545") }
 
       it "does not call register user service and logs in the user" do
         allow(Users::RegisterUserService).to receive(:new)
@@ -187,7 +188,7 @@ RSpec.describe Authentication::OmniauthService do
 
     context "with an active user remapped",
             with_settings: { oauth_allow_remapping_of_existing_users?: true } do
-      let!(:user) { create(:user, identity_url: "foo", login: auth_email.downcase) }
+      let!(:user) { create(:user, login: auth_email.downcase) }
 
       shared_examples_for "a successful remapping of foo" do
         before do
@@ -212,7 +213,7 @@ RSpec.describe Authentication::OmniauthService do
           aggregate_failures "User attributes" do
             expect(user.firstname).to eq "foo"
             expect(user.lastname).to eq "bar"
-            expect(user.identity_url).to eq "google:123545"
+            expect(user.identity_url).to eq "#{auth_provider.slug}:123545"
           end
 
           aggregate_failures "Message expectations" do
@@ -257,27 +258,34 @@ RSpec.describe Authentication::OmniauthService do
         end
       end
     end
-
-    describe "assuming registration/activation failed" do
-      let(:register_success) { false }
-      let(:register_message) { "Oh noes :(" }
-    end
   end
 
   describe "#identity_url_from_omniauth" do
-    let(:auth_hash) { { provider: "developer", uid: "veryuniqueid", info: {} } }
+    let(:auth_hash) { { provider:, uid: "veryuniqueid", info: {} } }
 
     subject { instance.send(:identity_url_from_omniauth) }
 
-    it "returns the correct identity_url" do
-      expect(subject).to eql("developer:veryuniqueid")
+    context "when provider is developer" do
+      let(:provider) { "developer" }
+
+      it "returns nil" do
+        expect(subject).to be_nil
+      end
     end
 
-    context "with uid mapped from info" do
-      let(:auth_hash) { { provider: "developer", uid: "veryuniqueid", info: { uid: "internal" } } }
+    context "when provider is not developer" do
+      let(:provider) { "another_provider_slug" }
 
       it "returns the correct identity_url" do
-        expect(subject).to eql("developer:internal")
+        expect(subject).to eql("another_provider_slug:veryuniqueid")
+      end
+
+      context "with uid mapped from info" do
+        let(:auth_hash) { { provider:, uid: "veryuniqueid", info: { uid: "internal" } } }
+
+        it "returns the correct identity_url" do
+          expect(subject).to eql("#{provider}:internal")
+        end
       end
     end
   end

spec/services/users/register_user_service_spec.rb

@@ -79,7 +79,19 @@ RSpec.describe Users::RegisterUserService, with_ee: %i[sso_auth_providers] do
   end
 
   describe "#register_omniauth_user" do
-    let(:user) { User.new(status: Principal.statuses[:registered], identity_url: "azure:1234") }
+    let(:auth_provider) { create(:oidc_provider_google, limit_self_registration:) }
+    let(:user) do
+      user = User.new(status: Principal.statuses[:registered])
+      user.user_auth_provider_links = [
+        UserAuthProviderLink.new(
+          auth_provider:,
+          external_id: "1234",
+          created_at: Time.zone.now,
+          updated_at: Time.zone.now
+        )
+      ]
+      user
+    end
     let(:instance) { described_class.new(user) }
 
     before do
@@ -87,70 +99,64 @@ RSpec.describe Users::RegisterUserService, with_ee: %i[sso_auth_providers] do
       allow(user).to receive(:save).and_return true
     end
 
-    it "tries to activate that user regardless of settings" do
-      with_all_registration_options do |_type|
-        call = instance.call
-        expect(call).to be_success
-        expect(call.result).to eq user
-        expect(call.message).to eq I18n.t(:notice_account_registered_and_logged_in)
+    context "with limit_self_registration disabled" do
+      let(:limit_self_registration) { false }
+
+      it "tries to activate that user regardless of settings" do
+        with_all_registration_options do |_type|
+          call = instance.call
+          expect(call).to be_success
+          expect(call.result).to eq user
+          expect(call.message).to eq I18n.t(:notice_account_registered_and_logged_in)
+        end
       end
     end
 
-    context "with limit_self_registration enabled and self_registration disabled",
-            with_settings: {
-              self_registration: 0,
-            } do
-      it "fails to activate due to disabled self registration" do
-        create(:oidc_provider, slug: "azure")
-        call = instance.call
-        expect(call).not_to be_success
-        expect(call.result).to eq user
-        expect(call.message).to eq I18n.t("account.error_self_registration_limited_provider", name: "azure")
+    context "with limit_self_registration enabled" do
+      let(:limit_self_registration) { true }
+
+      context "with self_registration disabled", with_settings: { self_registration: 0 } do
+        it "fails to activate due to disabled self registration" do
+          call = instance.call
+          expect(call).not_to be_success
+          expect(call.result).to eq user
+          expect(call.message).to eq I18n.t("account.error_self_registration_limited_provider", name: auth_provider.slug)
+        end
       end
-    end
 
-    context "with limit_self_registration enabled and self_registration manual",
-            with_settings: {
-              self_registration: 2,
-            } do
-      it "registers the user, but does not activate it" do
-        create(:oidc_provider, slug: "azure")
-        call = instance.call
-        expect(call).to be_success
-        expect(call.result).to eq user
-        expect(user).to be_registered
-        expect(user).not_to have_received(:activate)
-        expect(call.message).to eq I18n.t(:notice_account_pending)
+      context "with self_registration manual", with_settings: { self_registration: 2 } do
+        it "registers the user, but does not activate it" do
+          call = instance.call
+          expect(call).to be_success
+          expect(call.result).to eq user
+          expect(user).to be_registered
+          expect(user).not_to have_received(:activate)
+          expect(call.message).to eq I18n.t(:notice_account_pending)
+        end
       end
-    end
 
-    context "with limit_self_registration enabled and self_registration email",
-            with_settings: {
-              self_registration: 1,
-            } do
-      it "registers the user, but does not activate it" do
-        create(:oidc_provider, slug: "azure")
-
-        call = instance.call
-        expect(call).to be_success
-        expect(call.result).to eq user
-        expect(user).to be_registered
-        expect(user).not_to have_received(:activate)
-        expect(call.message).to eq I18n.t(:notice_account_register_done)
+      context "with self_registration email", with_settings: { self_registration: 1 } do
+        it "registers the user, but does not activate it" do
+          call = instance.call
+          expect(call).to be_success
+          expect(call.result).to eq user
+          expect(user).to be_registered
+          expect(user).not_to have_received(:activate)
+          expect(call.message).to eq I18n.t(:notice_account_register_done)
+        end
       end
-    end
 
-    context "with limit_self_registration enabled and self_registration automatic",
-            with_settings: {
-              self_registration: 3,
-            } do
-      it "activates the user" do
-        create(:oidc_provider, slug: "azure")
-        call = instance.call
-        expect(call).to be_success
-        expect(call.result).to eq user
-        expect(user).to have_received(:activate)
-        expect(call.message).to eq I18n.t(:notice_account_activated)
+      context "with self_registration automatic",
+              with_settings: {
+                self_registration: 3
+              } do
+        it "activates the user" do
+          call = instance.call
+          expect(call).to be_success
+          expect(call.result).to eq user
+          expect(user).to have_received(:activate)
+          expect(call.message).to eq I18n.t(:notice_account_activated)
+        end
       end
     end
   end
@@ -339,7 +345,4 @@ RSpec.describe Users::RegisterUserService, with_ee: %i[sso_auth_providers] do
       expect(call.message).to eq I18n.t(:notice_activation_failed)
     end
   end
-
-  describe "#with_saved_user_result" do
-  end
 end

spec/views/users/edit.html.erb_spec.rb

@@ -41,11 +41,8 @@ RSpec.describe "users/edit" do
   end
 
   context "authentication provider" do
-    let(:user) do
-      build(:user, id: 1, # id is required to create route to edit
-                   identity_url: "test_provider:veryuniqueid")
-    end
-    let!(:provider) { create(:oidc_provider, slug: "test_provider", display_name: "The Test Provider") }
+    let(:user) { create(:user, identity_url: "#{provider.slug}:veryuniqueid") }
+    let(:provider) { create(:oidc_provider, slug: "test_provider", display_name: "The Test Provider") }
 
     before do
       assign(:user, user)