From c00fefe8ce2049df19bddecd78af28518cf3de6c Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Mon, 4 Aug 2025 10:57:33 +0200 Subject: [PATCH] Fix creation of plugin auth providers The previous implementation would have failed when starting with an empty database and trying to migrate it. We now ensure that the database exists, by running the creation of plugin auth providers in the seeders. Since there is also a migration that's supposed to convert old identity_urls to auth provider links and that migration needs the auth providers already, it does the same as the seeder. --- app/models/plugin_auth_provider.rb | 24 +++- .../basic_data/plugin_auth_provider_seeder.rb | 37 ++++++ app/seeders/standard/basic_data_seeder.rb | 1 + ...dentity_url_to_user_auth_provider_links.rb | 30 +---- ...133700_migrate_auth_provider_urls_again.rb | 65 +++++++++++ .../lib/open_project/plugins/auth_plugin.rb | 2 +- .../spec/requests/auth_plugins_spec.rb | 4 +- .../migrate_auth_provider_urls_again_spec.rb | 110 ++++++++++++++++++ 8 files changed, 236 insertions(+), 37 deletions(-) create mode 100644 app/seeders/basic_data/plugin_auth_provider_seeder.rb create mode 100644 db/migrate/20250804133700_migrate_auth_provider_urls_again.rb create mode 100644 spec/migrations/migrate_auth_provider_urls_again_spec.rb diff --git a/app/models/plugin_auth_provider.rb b/app/models/plugin_auth_provider.rb index 603a45d3d0a..e6c1db1367b 100644 --- a/app/models/plugin_auth_provider.rb +++ b/app/models/plugin_auth_provider.rb @@ -30,16 +30,28 @@ class PluginAuthProvider < AuthProvider class << self - def create_for_plugin(config) + def register_for_plugin(config) slug = config[:name] - display_name = config[:display_name] || slug + display_name = (config[:display_name] || slug).to_s - find_or_create_by!(slug:) do |provider| - provider.available = false - provider.display_name = display_name.to_s - provider.creator = User.system + registry[slug] = display_name + end + + def create_all_registered + registry.each do |slug, display_name| + find_or_create_by!(slug:) do |provider| + provider.available = false + provider.display_name = display_name + provider.creator = User.system + end end end + + private + + def registry + @registry ||= {} + end end def human_type diff --git a/app/seeders/basic_data/plugin_auth_provider_seeder.rb b/app/seeders/basic_data/plugin_auth_provider_seeder.rb new file mode 100644 index 00000000000..846c5c7667d --- /dev/null +++ b/app/seeders/basic_data/plugin_auth_provider_seeder.rb @@ -0,0 +1,37 @@ +# 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. +#++ + +module BasicData + class PluginAuthProviderSeeder < Seeder + def seed_data! + PluginAuthProvider.create_all_registered + end + end +end diff --git a/app/seeders/standard/basic_data_seeder.rb b/app/seeders/standard/basic_data_seeder.rb index a0f02771f4b..08dc6cff6a5 100644 --- a/app/seeders/standard/basic_data_seeder.rb +++ b/app/seeders/standard/basic_data_seeder.rb @@ -37,6 +37,7 @@ module Standard ::BasicData::TimeEntryActivitySeeder, ::BasicData::ColorSeeder, ::BasicData::ColorSchemeSeeder, + ::BasicData::PluginAuthProviderSeeder, ::BasicData::ProjectPhaseColorSeeder, ::BasicData::ProjectPhaseDefinitionSeeder, ::BasicData::StatusSeeder, 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 67a50695465..a5c094277d3 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 @@ -39,34 +39,6 @@ class MoveUsersIdentityUrlToUserAuthProviderLinks < ActiveRecord::Migration[8.0] 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 + # Code to migrate data into the new table was moved into 20250804133700 after adding a fix end end diff --git a/db/migrate/20250804133700_migrate_auth_provider_urls_again.rb b/db/migrate/20250804133700_migrate_auth_provider_urls_again.rb new file mode 100644 index 00000000000..55ad828aeba --- /dev/null +++ b/db/migrate/20250804133700_migrate_auth_provider_urls_again.rb @@ -0,0 +1,65 @@ +# 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 MigrateAuthProviderUrlsAgain < ActiveRecord::Migration[8.0] + def change + reversible do |direction| + direction.up do + PluginAuthProvider.create_all_registered + + 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 } + 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 diff --git a/modules/auth_plugins/lib/open_project/plugins/auth_plugin.rb b/modules/auth_plugins/lib/open_project/plugins/auth_plugin.rb index a852fd642c9..75446d0c4a0 100644 --- a/modules/auth_plugins/lib/open_project/plugins/auth_plugin.rb +++ b/modules/auth_plugins/lib/open_project/plugins/auth_plugin.rb @@ -44,7 +44,7 @@ module OpenProject::Plugins if persist builder.provider_callbacks.each do |callback| callback.call.each do |config| - PluginAuthProvider.create_for_plugin(config) + PluginAuthProvider.register_for_plugin(config) end end end diff --git a/modules/auth_plugins/spec/requests/auth_plugins_spec.rb b/modules/auth_plugins/spec/requests/auth_plugins_spec.rb index dc9819230cc..c8792ff640f 100644 --- a/modules/auth_plugins/spec/requests/auth_plugins_spec.rb +++ b/modules/auth_plugins/spec/requests/auth_plugins_spec.rb @@ -100,7 +100,9 @@ RSpec.describe OpenProject::Plugins::AuthPlugin, with_ee: %i[sso_auth_providers] expect(strategies.keys.to_a).to eq %i[strategy_a strategy_b] end - it "persists all strategies in the database" do + it "marks all strategies for persisting in the database" do + expect(PluginAuthProvider.pluck(:slug)).to eq([]) + PluginAuthProvider.create_all_registered expect(PluginAuthProvider.pluck(:slug)).to contain_exactly("a1", "a2", "b1", "c1") end diff --git a/spec/migrations/migrate_auth_provider_urls_again_spec.rb b/spec/migrations/migrate_auth_provider_urls_again_spec.rb new file mode 100644 index 00000000000..660a0f4d93e --- /dev/null +++ b/spec/migrations/migrate_auth_provider_urls_again_spec.rb @@ -0,0 +1,110 @@ +# 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. +#++ + +require "spec_helper" +require Rails.root.join("db/migrate/20250804133700_migrate_auth_provider_urls_again.rb") + +RSpec.describe MigrateAuthProviderUrlsAgain, type: :model do + subject(:execute_migration) { ActiveRecord::Migration.suppress_messages { described_class.new.change } } + + let!(:auth_provider) { create(:oidc_provider, slug: "oidc-my-slug") } + let!(:user) { create(:user) } + let(:identity_url) { "oidc-my-slug:abc1337" } + + before do + # side-stepping ActiveRecord here, because we let AR ignore the identity_url column already + User.connection.execute("UPDATE users SET identity_url = '#{identity_url}' WHERE users.id = #{user.id}") + end + + it "succeeds" do + expect { execute_migration }.not_to raise_error + end + + it "creates a user auth provider link" do + expect { execute_migration }.to change(UserAuthProviderLink, :count).from(0).to(1) + expect(UserAuthProviderLink.first.attributes.symbolize_keys).to include( + external_id: "abc1337", auth_provider_id: auth_provider.id, user_id: user.id + ) + end + + context "when no auth provider matches the slug" do + let!(:auth_provider) { create(:oidc_provider, slug: "oidc-other-slug") } + + it "succeeds" do + expect { execute_migration }.not_to raise_error + end + + it "creates no user auth provider link" do + expect { execute_migration }.not_to change(UserAuthProviderLink, :count) + end + end + + context "when the user auth provider link already exists" do + before do + UserAuthProviderLink.create!(principal: user, auth_provider:, external_id: "abc1337") + end + + it "succeeds" do + expect { execute_migration }.not_to raise_error + end + + it "creates no additional user auth provider link" do + expect { execute_migration }.not_to change(UserAuthProviderLink, :count) + end + end + + context "when there are PluginAuthProviders to be persisted" do + let(:pending_providers) { { my_slug: "A display name" } } + let(:identity_url) { "my_slug:1337-7331" } + + before do + # Hooking into implementation of PluginAuthProvider.create_all_registered to cover that as well in this test case + allow(PluginAuthProvider).to receive(:registry).and_return(pending_providers) + end + + it "succeeds" do + expect { execute_migration }.not_to raise_error + end + + it "persists them" do + expect { execute_migration }.to change(PluginAuthProvider, :count).from(0).to(1) + expect(PluginAuthProvider.first.attributes.symbolize_keys).to include( + slug: "my_slug", display_name: "A display name", available: false + ) + end + + it "is able to create user auth provider links for them" do + expect { execute_migration }.to change(UserAuthProviderLink, :count).from(0).to(1) + expect(UserAuthProviderLink.first.attributes.symbolize_keys).to include( + external_id: "1337-7331", auth_provider_id: PluginAuthProvider.first.id, user_id: user.id + ) + end + end +end