From 165f234dd8eb550d509df72c7c12e55a5040ce2a Mon Sep 17 00:00:00 2001 From: Chris Date: Fri, 12 Dec 2025 14:34:06 -0800 Subject: [PATCH] use sso provider as anchor --- app/actions/sso/create_user_in_account.rb | 39 +++++++-- app/actions/sso/sync_user_teams.rb | 4 +- app/controllers/accounts/oidc_controller.rb | 14 ++- app/models/provider.rb | 8 +- app/models/sso_provider.rb | 1 + app/views/providers/_index.html.erb | 2 +- ...212215414_add_sso_provider_to_providers.rb | 6 ++ db/schema.rb | 6 +- lib/devise/strategies/ldap_authenticatable.rb | 12 ++- .../sso/create_user_in_account_spec.rb | 85 +++++++++++++++++-- spec/actions/sso/sync_user_teams_spec.rb | 27 +++++- spec/factories/providers.rb | 6 +- spec/factories/sso_providers.rb | 9 +- spec/models/provider_spec.rb | 6 +- 14 files changed, 195 insertions(+), 30 deletions(-) create mode 100644 db/migrate/20251212215414_add_sso_provider_to_providers.rb diff --git a/app/actions/sso/create_user_in_account.rb b/app/actions/sso/create_user_in_account.rb index 0b62fb18..c30fb570 100644 --- a/app/actions/sso/create_user_in_account.rb +++ b/app/actions/sso/create_user_in_account.rb @@ -3,20 +3,43 @@ module SSO class CreateUserInAccount extend LightService::Action - expects :email, :account + expects :email, :account, :sso_provider, :uid + expects :name, default: nil promises :user executed do |context| - user = User.find_or_initialize_by(email: context.email.downcase) + # First try to find user by SSO provider identity + provider = Provider.find_by(sso_provider: context.sso_provider, uid: context.uid) - if user.new_record? - password = SecureRandom.hex(32) - user.password = password - user.password_confirmation = password + if provider + user = provider.user + else + # Fall back to finding by email, or create new user + user = User.find_or_initialize_by(email: context.email.downcase) - unless user.save - context.fail_and_return!("Failed to create user", errors: user.errors) + if user.new_record? + password = SecureRandom.hex(32) + user.password = password + user.password_confirmation = password + + unless user.save + context.fail_and_return!("Failed to create user", errors: user.errors) + end end + + # Create provider record to link user to SSO provider + Provider.create!( + user: user, + sso_provider: context.sso_provider, + uid: context.uid, + provider: context.sso_provider.name + ) + end + + # Update name from SSO provider on every login + if context.name.present? + name_parts = context.name.split(" ", 2) + user.update(first_name: name_parts.first, last_name: name_parts.second) end AccountUser.find_or_create_by!(account: context.account, user: user) diff --git a/app/actions/sso/sync_user_teams.rb b/app/actions/sso/sync_user_teams.rb index e0e2d33e..ea66740b 100644 --- a/app/actions/sso/sync_user_teams.rb +++ b/app/actions/sso/sync_user_teams.rb @@ -1,7 +1,7 @@ class SSO::SyncUserTeams extend LightService::Organizer - def self.call(email, team_names, account) - with(email:, team_names:, account:).reduce( + def self.call(email:, team_names:, account:, sso_provider:, uid:, name: nil) + with(email:, team_names:, account:, sso_provider:, uid:, name:).reduce( SSO::CreateTeamsInAccount, SSO::CreateUserInAccount, SSO::SyncTeams, diff --git a/app/controllers/accounts/oidc_controller.rb b/app/controllers/accounts/oidc_controller.rb index 9cd2501d..2f7337a9 100644 --- a/app/controllers/accounts/oidc_controller.rb +++ b/app/controllers/accounts/oidc_controller.rb @@ -53,12 +53,22 @@ module Accounts sso_provider = @account.sso_provider if sso_provider.just_in_time_team_provisioning_mode? ar_result = ActiveRecord::Base.transaction do - SSO::SyncUserTeams.call(result.email, result.groups || [], @account) + SSO::SyncUserTeams.call( + email: result.email, + team_names: result.groups || [], + account: @account, + sso_provider: sso_provider, + uid: result.uid, + name: result.name + ) end else ar_result = SSO::CreateUserInAccount.execute( email: result.email, - account: @account + account: @account, + sso_provider: sso_provider, + uid: result.uid, + name: result.name ) end diff --git a/app/models/provider.rb b/app/models/provider.rb index 8cb7fa23..16fd3f8c 100644 --- a/app/models/provider.rb +++ b/app/models/provider.rb @@ -15,14 +15,18 @@ # created_at :datetime not null # updated_at :datetime not null # external_id :string +# sso_provider_id :bigint # user_id :bigint not null # # Indexes # -# index_providers_on_user_id (user_id) +# index_providers_on_sso_provider_id (sso_provider_id) +# index_providers_on_sso_provider_id_and_uid (sso_provider_id,uid) UNIQUE WHERE (sso_provider_id IS NOT NULL) +# index_providers_on_user_id (user_id) # # Foreign Keys # +# fk_rails_... (sso_provider_id => sso_providers.id) # fk_rails_... (user_id => users.id) # class Provider < ApplicationRecord @@ -44,8 +48,10 @@ class Provider < ApplicationRecord AVAILABLE_PROVIDERS = [ GITHUB_PROVIDER, GITLAB_PROVIDER, CUSTOM_REGISTRY_PROVIDER ].freeze validates :registry_url, presence: true, if: :container_registry? scope :has_container_registry, -> { where(provider: [ GITHUB_PROVIDER, GITLAB_PROVIDER, CUSTOM_REGISTRY_PROVIDER ]) } + scope :non_sso, -> { where(sso_provider_id: nil) } belongs_to :user + belongs_to :sso_provider, class_name: "SSOProvider", optional: true Devise.omniauth_configs.keys.each do |provider| scope provider, -> { where(provider: provider) } diff --git a/app/models/sso_provider.rb b/app/models/sso_provider.rb index ed1cbfac..afd4486f 100644 --- a/app/models/sso_provider.rb +++ b/app/models/sso_provider.rb @@ -24,6 +24,7 @@ class SSOProvider < ApplicationRecord belongs_to :account belongs_to :configuration, polymorphic: true, dependent: :destroy + has_many :providers, dependent: :nullify validates :account_id, uniqueness: true diff --git a/app/views/providers/_index.html.erb b/app/views/providers/_index.html.erb index 10a6d753..139d4667 100644 --- a/app/views/providers/_index.html.erb +++ b/app/views/providers/_index.html.erb @@ -12,7 +12,7 @@ - <% current_user.providers.each do |provider| %> + <% current_user.providers.non_sso.each do |provider| %> <% project_credential_providers = ProjectCredentialProvider.where(provider:).all %> diff --git a/db/migrate/20251212215414_add_sso_provider_to_providers.rb b/db/migrate/20251212215414_add_sso_provider_to_providers.rb new file mode 100644 index 00000000..a7e4eb6b --- /dev/null +++ b/db/migrate/20251212215414_add_sso_provider_to_providers.rb @@ -0,0 +1,6 @@ +class AddSSOProviderToProviders < ActiveRecord::Migration[7.2] + def change + add_reference :providers, :sso_provider, null: true, foreign_key: true + add_index :providers, [ :sso_provider_id, :uid ], unique: true, where: "sso_provider_id IS NOT NULL" + end +end diff --git a/db/schema.rb b/db/schema.rb index 7b187afa..71628ec1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_12_11_151950) do +ActiveRecord::Schema[7.2].define(version: 2025_12_12_215414) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -480,6 +480,9 @@ ActiveRecord::Schema[7.2].define(version: 2025_12_11_151950) do t.datetime "last_used_at" t.string "registry_url" t.string "external_id" + t.bigint "sso_provider_id" + t.index ["sso_provider_id", "uid"], name: "index_providers_on_sso_provider_id_and_uid", unique: true, where: "(sso_provider_id IS NOT NULL)" + t.index ["sso_provider_id"], name: "index_providers_on_sso_provider_id" t.index ["user_id"], name: "index_providers_on_user_id" end @@ -623,6 +626,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_12_11_151950) do add_foreign_key "project_forks", "projects", column: "parent_project_id" add_foreign_key "projects", "clusters" add_foreign_key "projects", "clusters", column: "project_fork_cluster_id" + add_foreign_key "providers", "sso_providers" add_foreign_key "providers", "users" add_foreign_key "services", "projects" add_foreign_key "sso_providers", "accounts" diff --git a/lib/devise/strategies/ldap_authenticatable.rb b/lib/devise/strategies/ldap_authenticatable.rb index 838a426c..3139f383 100644 --- a/lib/devise/strategies/ldap_authenticatable.rb +++ b/lib/devise/strategies/ldap_authenticatable.rb @@ -54,12 +54,22 @@ module Devise if sso_provider.just_in_time_team_provisioning_mode? groups = result.groups ar_result = ActiveRecord::Base.transaction do - SSO::SyncUserTeams.call(email, groups, ldap_configuration.account) + SSO::SyncUserTeams.call( + email: email, + team_names: groups, + account: ldap_configuration.account, + sso_provider: sso_provider, + uid: result.user_dn, + name: result.name + ) end else ar_result = SSO::CreateUserInAccount.execute( email: email, account: ldap_configuration.account, + sso_provider: sso_provider, + uid: result.user_dn, + name: result.name ) end diff --git a/spec/actions/sso/create_user_in_account_spec.rb b/spec/actions/sso/create_user_in_account_spec.rb index 5be8c16d..871cff74 100644 --- a/spec/actions/sso/create_user_in_account_spec.rb +++ b/spec/actions/sso/create_user_in_account_spec.rb @@ -2,26 +2,101 @@ require 'rails_helper' RSpec.describe SSO::CreateUserInAccount do let(:account) { create(:account) } + let(:sso_provider) { create(:sso_provider, account: account) } describe '.execute' do - it 'creates a new user and associates them with the account when user does not exist' do - result = described_class.execute(email: 'new@example.com', account: account) + it 'creates a new user, provider record, and associates them with the account' do + result = described_class.execute( + email: 'new@example.com', + account: account, + sso_provider: sso_provider, + uid: 'external-uid-123' + ) expect(result).to be_success expect(result.user).to be_persisted expect(result.user.email).to eq('new@example.com') expect(account.users).to include(result.user) + + provider = Provider.find_by(sso_provider: sso_provider, uid: 'external-uid-123') + expect(provider).to be_present + expect(provider.user).to eq(result.user) end - it 'associates an existing user with the account without creating a duplicate' do + it 'sets first and last name when name is provided' do + result = described_class.execute( + email: 'new@example.com', + account: account, + sso_provider: sso_provider, + uid: 'external-uid-123', + name: 'John Doe' + ) + + expect(result).to be_success + expect(result.user.first_name).to eq('John') + expect(result.user.last_name).to eq('Doe') + end + + it 'finds existing user by SSO provider uid on subsequent logins' do + first_result = described_class.execute( + email: 'user@example.com', + account: account, + sso_provider: sso_provider, + uid: 'same-uid' + ) + + second_result = described_class.execute( + email: 'different@example.com', + account: account, + sso_provider: sso_provider, + uid: 'same-uid' + ) + + expect(second_result).to be_success + expect(second_result.user).to eq(first_result.user) + expect(Provider.where(sso_provider: sso_provider, uid: 'same-uid').count).to eq(1) + end + + it 'updates name on subsequent logins' do + first_result = described_class.execute( + email: 'user@example.com', + account: account, + sso_provider: sso_provider, + uid: 'same-uid', + name: 'Old Name' + ) + + expect(first_result.user.first_name).to eq('Old') + expect(first_result.user.last_name).to eq('Name') + + second_result = described_class.execute( + email: 'user@example.com', + account: account, + sso_provider: sso_provider, + uid: 'same-uid', + name: 'New Name' + ) + + expect(second_result.user.reload.first_name).to eq('New') + expect(second_result.user.last_name).to eq('Name') + end + + it 'links existing user by email if no provider record exists' do existing_user = create(:user, email: 'existing@example.com') - result = described_class.execute(email: 'EXISTING@example.com', account: account) + result = described_class.execute( + email: 'EXISTING@example.com', + account: account, + sso_provider: sso_provider, + uid: 'new-uid' + ) expect(result).to be_success expect(result.user).to eq(existing_user) expect(account.users).to include(existing_user) - expect(User.where(email: 'existing@example.com').count).to eq(1) + + provider = Provider.find_by(sso_provider: sso_provider, uid: 'new-uid') + expect(provider.user).to eq(existing_user) end end end diff --git a/spec/actions/sso/sync_user_teams_spec.rb b/spec/actions/sso/sync_user_teams_spec.rb index 0779bb0c..3f6732b2 100644 --- a/spec/actions/sso/sync_user_teams_spec.rb +++ b/spec/actions/sso/sync_user_teams_spec.rb @@ -2,12 +2,19 @@ require 'rails_helper' RSpec.describe SSO::SyncUserTeams do let(:account) { create(:account) } + let(:sso_provider) { create(:sso_provider, account: account) } describe '.call' do it 'creates user, teams, and team memberships' do create(:team, account: account, name: 'Existing') - result = described_class.call('new@example.com', [ { name: 'Existing' }, { name: 'NewTeam' } ], account) + result = described_class.call( + email: 'new@example.com', + team_names: [ { name: 'Existing' }, { name: 'NewTeam' } ], + account: account, + sso_provider: sso_provider, + uid: 'user-uid-123' + ) expect(result).to be_success expect(result.user.email).to eq('new@example.com') @@ -22,8 +29,15 @@ RSpec.describe SSO::SyncUserTeams do create(:team_membership, user: user, team: team_to_keep) create(:team_membership, user: user, team: team_to_remove) create(:account_user, account: account, user: user) + create(:provider, user: user, sso_provider: sso_provider, uid: 'existing-uid', provider: sso_provider.name) - result = described_class.call('existing@example.com', [ { name: 'KeepTeam' } ], account) + result = described_class.call( + email: 'existing@example.com', + team_names: [ { name: 'KeepTeam' } ], + account: account, + sso_provider: sso_provider, + uid: 'existing-uid' + ) expect(result).to be_success expect(result.user.teams.reload).to contain_exactly(team_to_keep) @@ -37,8 +51,15 @@ RSpec.describe SSO::SyncUserTeams do create(:team_membership, user: user, team: team_in_account) create(:team_membership, user: user, team: team_in_other) create(:account_user, account: account, user: user) + create(:provider, user: user, sso_provider: sso_provider, uid: 'existing-uid', provider: sso_provider.name) - result = described_class.call('existing@example.com', [], account) + result = described_class.call( + email: 'existing@example.com', + team_names: [], + account: account, + sso_provider: sso_provider, + uid: 'existing-uid' + ) expect(result).to be_success expect(user.teams.reload).to contain_exactly(team_in_other) diff --git a/spec/factories/providers.rb b/spec/factories/providers.rb index 1d956e83..c3e2c524 100644 --- a/spec/factories/providers.rb +++ b/spec/factories/providers.rb @@ -15,14 +15,18 @@ # created_at :datetime not null # updated_at :datetime not null # external_id :string +# sso_provider_id :bigint # user_id :bigint not null # # Indexes # -# index_providers_on_user_id (user_id) +# index_providers_on_sso_provider_id (sso_provider_id) +# index_providers_on_sso_provider_id_and_uid (sso_provider_id,uid) UNIQUE WHERE (sso_provider_id IS NOT NULL) +# index_providers_on_user_id (user_id) # # Foreign Keys # +# fk_rails_... (sso_provider_id => sso_providers.id) # fk_rails_... (user_id => users.id) # FactoryBot.define do diff --git a/spec/factories/sso_providers.rb b/spec/factories/sso_providers.rb index 6b6f3c44..e3f02cb7 100644 --- a/spec/factories/sso_providers.rb +++ b/spec/factories/sso_providers.rb @@ -23,9 +23,10 @@ # FactoryBot.define do factory :sso_provider do - account { nil } - configuration { nil } - name { "MyString" } - enabled { false } + account + configuration { association :oidc_configuration } + name { "Test SSO Provider" } + enabled { true } + team_provisioning_mode { :disabled } end end diff --git a/spec/models/provider_spec.rb b/spec/models/provider_spec.rb index e9c6d97c..6d07a9c1 100644 --- a/spec/models/provider_spec.rb +++ b/spec/models/provider_spec.rb @@ -15,14 +15,18 @@ # created_at :datetime not null # updated_at :datetime not null # external_id :string +# sso_provider_id :bigint # user_id :bigint not null # # Indexes # -# index_providers_on_user_id (user_id) +# index_providers_on_sso_provider_id (sso_provider_id) +# index_providers_on_sso_provider_id_and_uid (sso_provider_id,uid) UNIQUE WHERE (sso_provider_id IS NOT NULL) +# index_providers_on_user_id (user_id) # # Foreign Keys # +# fk_rails_... (sso_provider_id => sso_providers.id) # fk_rails_... (user_id => users.id) # require 'rails_helper'