From 2db9ef305c427fe0c5d2f5aa7c006067ce959006 Mon Sep 17 00:00:00 2001 From: Pavel Balashou Date: Wed, 21 May 2025 08:53:20 +0200 Subject: [PATCH 1/3] [#63912] Support multiple authentication provider user links https://community.openproject.org/work_packages/63912 --- app/contracts/users/base_contract.rb | 15 ++- app/controllers/account_controller.rb | 4 +- app/controllers/concerns/user_invitation.rb | 2 +- app/models/auth_provider.rb | 8 +- app/models/user.rb | 20 ++- app/models/user_auth_provider_link.rb | 41 ++++++ .../authentication/omniauth_service.rb | 52 +++++--- app/services/users/register_user_service.rb | 2 +- app/services/users/set_attributes_service.rb | 20 +++ ...dentity_url_to_user_auth_provider_links.rb | 72 +++++++++++ .../apiv3/components/schemas/user_model.yml | 2 + .../strategies/warden/jwt_oidc.rb | 2 +- .../authentication_validator_spec.rb | 15 ++- .../users/shared_contract_examples.rb | 5 +- spec/controllers/account_controller_spec.rb | 6 +- .../omni_auth_login_controller_spec.rb | 67 +++++----- spec/factories/user_factory.rb | 14 +- spec/features/auth/omniauth_spec.rb | 56 ++++---- spec/models/user_spec.rb | 37 ++---- .../api/v3/user/create_user_resource_spec.rb | 9 +- .../authentication/omniauth_service_spec.rb | 40 +++--- .../users/register_user_service_spec.rb | 121 +++++++++--------- spec/views/users/edit.html.erb_spec.rb | 7 +- 23 files changed, 401 insertions(+), 216 deletions(-) create mode 100644 app/models/user_auth_provider_link.rb create mode 100644 db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb diff --git a/app/contracts/users/base_contract.rb b/app/contracts/users/base_contract.rb index f699978b022..2a19ecc3deb 100644 --- a/app/contracts/users/base_contract.rb +++ b/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,6 +55,7 @@ module Users end validate :validate_password_writable + validate :validate_identity_url_writable validate :existing_auth_source delegate :available_custom_fields, to: :model @@ -65,6 +63,7 @@ module Users def reduce_writable_attributes(attributes) super.tap do |writable| writable << "password" if password_writable? + writable << "identity_url" if identity_url_writable? end end @@ -77,6 +76,10 @@ module Users 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? diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index 15d2c0e1e1a..368fc8b41a2 100644 --- a/app/controllers/account_controller.rb +++ b/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) diff --git a/app/controllers/concerns/user_invitation.rb b/app/controllers/concerns/user_invitation.rb index 98055acdf23..f2e8076f010 100644 --- a/app/controllers/concerns/user_invitation.rb +++ b/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:).destroy_all UserPassword.where(user_id:).destroy_all end diff --git a/app/models/auth_provider.rb b/app/models/auth_provider.rb index 8a38a4894b0..1070da838fd 100644 --- a/app/models/auth_provider.rb +++ b/app/models/auth_provider.rb @@ -29,6 +29,8 @@ class AuthProvider < ApplicationRecord belongs_to :creator, class_name: "User" + has_many :user_auth_provider_links, dependent: :destroy + validates :display_name, presence: true validates :display_name, uniqueness: true @@ -39,7 +41,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 @@ -55,6 +57,10 @@ class AuthProvider < ApplicationRecord URI.join(auth_url, "callback").to_s end + def to_s + slug + end + protected def unset_direct_provider diff --git a/app/models/user.rb b/app/models/user.rb index b74fd4cdec5..0bdae4df2cd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -96,6 +96,7 @@ 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 # Users blocked via brute force prevention # use lambda here, so time is evaluated on each query @@ -308,11 +309,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 +381,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 +556,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 diff --git a/app/models/user_auth_provider_link.rb b/app/models/user_auth_provider_link.rb new file mode 100644 index 00000000000..5d13653b217 --- /dev/null +++ b/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 diff --git a/app/services/authentication/omniauth_service.rb b/app/services/authentication/omniauth_service.rb index bade473d323..afd703eac6b 100644 --- a/app/services/authentication/omniauth_service.rb +++ b/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 @@ -53,6 +55,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 +69,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 @@ -134,7 +141,7 @@ module Authentication find_invited_user || find_existing_user || remap_existing_user || - initialize_new_user + User.new end ## @@ -142,12 +149,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 identity_url.present? + slug, external_id = 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 +167,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(identity_url).first&.user + end end ## @@ -166,13 +182,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 +284,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 no be set i + # 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 diff --git a/app/services/users/register_user_service.rb b/app/services/users/register_user_service.rb index b498044d593..9dc53f2923f 100644 --- a/app/services/users/register_user_service.rb +++ b/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) diff --git a/app/services/users/set_attributes_service.rb b/app/services/users/set_attributes_service.rb index aa9d9e30a11..99aeb5e0fd7 100644 --- a/app/services/users/set_attributes_service.rb +++ b/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]) diff --git a/db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb b/db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb new file mode 100644 index 00000000000..64705083537 --- /dev/null +++ b/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 + 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 + remove_column :users, :identity_url, :string + end +end diff --git a/docs/api/apiv3/components/schemas/user_model.yml b/docs/api/apiv3/components/schemas/user_model.yml index 97a850cb8a4..1ebbe6ebf5f 100644 --- a/docs/api/apiv3/components/schemas/user_model.yml +++ b/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. Use `UserAuthProviderLink` API instead. # Conditions - User is self, or `create_user` or `manage_user` permission globally + deprecated: true createdAt: type: string format: date-time diff --git a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb b/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb index aa067d434bf..fb17558f8e0 100644 --- a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb +++ b/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 = UserAuthProviderLink.where(auth_provider: provider, external_id: payload["sub"]).first&.user authentication_result(user) end, ->(error) { fail_with_header!(error: "invalid_token", error_description: error) } diff --git a/modules/storages/spec/common/storages/peripherals/connection_validators/nextcloud/authentication_validator_spec.rb b/modules/storages/spec/common/storages/peripherals/connection_validators/nextcloud/authentication_validator_spec.rb index 1caef612c63..5bde4f6e5c0 100644 --- a/modules/storages/spec/common/storages/peripherals/connection_validators/nextcloud/authentication_validator_spec.rb +++ b/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 diff --git a/spec/contracts/users/shared_contract_examples.rb b/spec/contracts/users/shared_contract_examples.rb index 5ca579f69a3..b80a2cefd6a 100644 --- a/spec/contracts/users/shared_contract_examples.rb +++ b/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 diff --git a/spec/controllers/account_controller_spec.rb b/spec/controllers/account_controller_spec.rb index 1f23ca26925..638c254b0d0 100644 --- a/spec/controllers/account_controller_spec.rb +++ b/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", diff --git a/spec/controllers/omni_auth_login_controller_spec.rb b/spec/controllers/omni_auth_login_controller_spec.rb index 5cb53d822ca..5da888deb82 100644 --- a/spec/controllers/omni_auth_login_controller_spec.rb +++ b/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 diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb index 57ac688aa75..57bf0644b6f 100644 --- a/spec/factories/user_factory.rb +++ b/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,17 @@ 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: external_id) + end end diff --git a/spec/features/auth/omniauth_spec.rb b/spec/features/auth/omniauth_spec.rb index ffea3396e07..8b904610ad1 100644 --- a/spec/features/auth/omniauth_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 1b548c44a69..1b9bbf176f5 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/requests/api/v3/user/create_user_resource_spec.rb b/spec/requests/api/v3/user/create_user_resource_spec.rb index d87bb8d0b7f..f2424ccbdc2 100644 --- a/spec/requests/api/v3/user/create_user_resource_spec.rb +++ b/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 diff --git a/spec/services/authentication/omniauth_service_spec.rb b/spec/services/authentication/omniauth_service_spec.rb index 0d0eb8951f0..d4018b0abcb 100644 --- a/spec/services/authentication/omniauth_service_spec.rb +++ b/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 diff --git a/spec/services/users/register_user_service_spec.rb b/spec/services/users/register_user_service_spec.rb index a23ef6182de..9365be63f04 100644 --- a/spec/services/users/register_user_service_spec.rb +++ b/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 diff --git a/spec/views/users/edit.html.erb_spec.rb b/spec/views/users/edit.html.erb_spec.rb index 5d5cf546ee6..893f53d2228 100644 --- a/spec/views/users/edit.html.erb_spec.rb +++ b/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}:veryuniqueid") } + let(:provider) { create(:oidc_provider, slug: "test_provider", display_name: "The Test Provider") } before do assign(:user, user) From 83794a74663b30998864ae164605d052ff4cbe7f Mon Sep 17 00:00:00 2001 From: Pavel Balashou Date: Thu, 5 Jun 2025 13:03:13 +0200 Subject: [PATCH 2/3] [#63912] React on PR comments. Important changes: 1. Use ignored_columns to try to avoid downtime. 2. Add unique constraint for auth_provider_id+external_id. Co-authored-by: Jan Sandbrink <453584+username@users.noreply.github.com> --- app/contracts/users/base_contract.rb | 6 +++--- app/controllers/concerns/user_invitation.rb | 2 +- app/models/auth_provider.rb | 5 +---- app/models/user.rb | 3 +++ app/services/authentication/omniauth_service.rb | 10 ++++------ ...e_users_identity_url_to_user_auth_provider_links.rb | 2 +- docs/api/apiv3/components/schemas/user_model.yml | 2 +- .../authentication/strategies/warden/jwt_oidc.rb | 2 +- spec/factories/user_factory.rb | 4 +--- 9 files changed, 16 insertions(+), 20 deletions(-) diff --git a/app/contracts/users/base_contract.rb b/app/contracts/users/base_contract.rb index 2a19ecc3deb..70d38278d5e 100644 --- a/app/contracts/users/base_contract.rb +++ b/app/contracts/users/base_contract.rb @@ -62,6 +62,9 @@ module Users 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 @@ -69,9 +72,6 @@ module Users private - ## - # Password is not a regular attribute so it bypasses - # attribute writable checks def password_writable? user.admin? || user.id == model.id end diff --git a/app/controllers/concerns/user_invitation.rb b/app/controllers/concerns/user_invitation.rb index f2e8076f010..b11137b4038 100644 --- a/app/controllers/concerns/user_invitation.rb +++ b/app/controllers/concerns/user_invitation.rb @@ -103,7 +103,7 @@ module UserInvitation end def reset_login(user_id) - UserAuthProviderLink.where(user_id:).destroy_all + UserAuthProviderLink.where(user_id:).delete_all UserPassword.where(user_id:).destroy_all end diff --git a/app/models/auth_provider.rb b/app/models/auth_provider.rb index 1070da838fd..6b93ece9382 100644 --- a/app/models/auth_provider.rb +++ b/app/models/auth_provider.rb @@ -30,6 +30,7 @@ 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 @@ -57,10 +58,6 @@ class AuthProvider < ApplicationRecord URI.join(auth_url, "callback").to_s end - def to_s - slug - end - protected def unset_direct_provider diff --git a/app/models/user.rb b/app/models/user.rb index 0bdae4df2cd..dc09e8cd937 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -97,6 +97,7 @@ class User < Principal 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 @@ -110,6 +111,8 @@ class User < Principal :with_time_zone, :having_reminder_mail_to_send + self.ignored_columns += [:identity_url] + def self.create_blocked_scope(scope, blocked) scope.where(blocked_condition(blocked)) end diff --git a/app/services/authentication/omniauth_service.rb b/app/services/authentication/omniauth_service.rb index afd703eac6b..5faf0209595 100644 --- a/app/services/authentication/omniauth_service.rb +++ b/app/services/authentication/omniauth_service.rb @@ -37,7 +37,6 @@ module Authentication :controller, :contract, :user_attributes, - :identity_url, :user delegate :session, to: :controller @@ -122,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 @@ -153,8 +151,8 @@ module Authentication return unless token token.user.tap do |user| - if identity_url.present? - slug, external_id = identity_url.split(":", 2) + 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! @@ -170,7 +168,7 @@ module Authentication if developer_provider? User.find_by(mail: auth_hash[:uid]) else - UserAuthProviderLink.with_identity_url(identity_url).first&.user + UserAuthProviderLink.with_identity_url(user_attributes[:identity_url]).first&.user end end @@ -294,7 +292,7 @@ module Authentication # 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 no be set i + # 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" diff --git a/db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb b/db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb index 64705083537..1e492080c24 100644 --- a/db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb +++ b/db/migrate/20250512114003_move_users_identity_url_to_user_auth_provider_links.rb @@ -36,6 +36,7 @@ class MoveUsersIdentityUrlToUserAuthProviderLinks < ActiveRecord::Migration[8.0] 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| @@ -67,6 +68,5 @@ class MoveUsersIdentityUrlToUserAuthProviderLinks < ActiveRecord::Migration[8.0] end end end - remove_column :users, :identity_url, :string end end diff --git a/docs/api/apiv3/components/schemas/user_model.yml b/docs/api/apiv3/components/schemas/user_model.yml index 1ebbe6ebf5f..b5a8286ca84 100644 --- a/docs/api/apiv3/components/schemas/user_model.yml +++ b/docs/api/apiv3/components/schemas/user_model.yml @@ -83,7 +83,7 @@ allOf: - 'null' description: |- User's identity_url for OmniAuth authentication. - **Deprecated:** It will be removed in the near future. Use `UserAuthProviderLink` API instead. + **Deprecated:** It will be removed in the near future. # Conditions diff --git a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb b/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb index fb17558f8e0..1088aa1c0c6 100644 --- a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb +++ b/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 = UserAuthProviderLink.where(auth_provider: provider, external_id: payload["sub"]).first&.user + 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) } diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb index 57bf0644b6f..cab39ac30b1 100644 --- a/spec/factories/user_factory.rb +++ b/spec/factories/user_factory.rb @@ -73,9 +73,7 @@ FactoryBot.define do 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: external_id) - + user.user_auth_provider_links.create!(auth_provider:, external_id:) end end From 4d35299d67f4f5b62f09c4ba60e21e25f5917e5e Mon Sep 17 00:00:00 2001 From: Pavel Balashou Date: Fri, 6 Jun 2025 11:27:51 +0200 Subject: [PATCH 3/3] [#63912] Try to fix migrations CI run. --- app/models/principal.rb | 2 ++ app/models/user.rb | 2 -- ...30731153909_add_file_link_journals_to_existing_containers.rb | 1 + db/migrate/20240131130134_fix_inherited_group_memberships.rb | 1 + .../queries/placeholder_users/placeholder_user_query_spec.rb | 2 +- spec/views/users/edit.html.erb_spec.rb | 2 +- 6 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/principal.rb b/app/models/principal.rb index 4706cde6a2f..c031896c8cb 100644 --- a/app/models/principal.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index dc09e8cd937..15a23b29511 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -111,8 +111,6 @@ class User < Principal :with_time_zone, :having_reminder_mail_to_send - self.ignored_columns += [:identity_url] - def self.create_blocked_scope(scope, blocked) scope.where(blocked_condition(blocked)) end diff --git a/db/migrate/20230731153909_add_file_link_journals_to_existing_containers.rb b/db/migrate/20230731153909_add_file_link_journals_to_existing_containers.rb index 558664edd23..d4482548176 100644 --- a/db/migrate/20230731153909_add_file_link_journals_to_existing_containers.rb +++ b/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 diff --git a/db/migrate/20240131130134_fix_inherited_group_memberships.rb b/db/migrate/20240131130134_fix_inherited_group_memberships.rb index c1698c5a00e..5740d8e2e2f 100644 --- a/db/migrate/20240131130134_fix_inherited_group_memberships.rb +++ b/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| diff --git a/spec/models/queries/placeholder_users/placeholder_user_query_spec.rb b/spec/models/queries/placeholder_users/placeholder_user_query_spec.rb index dc762df5909..d697bb2bb80 100644 --- a/spec/models/queries/placeholder_users/placeholder_user_query_spec.rb +++ b/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 diff --git a/spec/views/users/edit.html.erb_spec.rb b/spec/views/users/edit.html.erb_spec.rb index 893f53d2228..43b84ef74cf 100644 --- a/spec/views/users/edit.html.erb_spec.rb +++ b/spec/views/users/edit.html.erb_spec.rb @@ -41,7 +41,7 @@ RSpec.describe "users/edit" do end context "authentication provider" do - let(:user) { create(:user, identity_url: "#{provider}:veryuniqueid") } + let(:user) { create(:user, identity_url: "#{provider.slug}:veryuniqueid") } let(:provider) { create(:oidc_provider, slug: "test_provider", display_name: "The Test Provider") } before do