From 7cd0863487498a84a87b33a58c096538497f397d Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Tue, 2 Sep 2025 15:13:23 +0530 Subject: [PATCH 1/4] feat: update model --- db/migrate/20250825070005_create_account_saml_settings.rb | 1 + db/schema.rb | 1 + enterprise/app/models/account_saml_settings.rb | 1 + 3 files changed, 3 insertions(+) diff --git a/db/migrate/20250825070005_create_account_saml_settings.rb b/db/migrate/20250825070005_create_account_saml_settings.rb index 03ec4c5ef..7a0937fbf 100644 --- a/db/migrate/20250825070005_create_account_saml_settings.rb +++ b/db/migrate/20250825070005_create_account_saml_settings.rb @@ -5,6 +5,7 @@ class CreateAccountSamlSettings < ActiveRecord::Migration[7.1] t.string :sso_url t.text :certificate t.string :sp_entity_id + t.string :idp_entity_id t.json :role_mappings, default: {} t.timestamps diff --git a/db/schema.rb b/db/schema.rb index 298558e1f..e320cf2b6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -33,6 +33,7 @@ ActiveRecord::Schema[7.1].define(version: 2025_08_25_070005) do t.string "sso_url" t.text "certificate" t.string "sp_entity_id" + t.string "idp_entity_id" t.json "role_mappings", default: {} t.datetime "created_at", null: false t.datetime "updated_at", null: false diff --git a/enterprise/app/models/account_saml_settings.rb b/enterprise/app/models/account_saml_settings.rb index 6cc8314be..3f1842acc 100644 --- a/enterprise/app/models/account_saml_settings.rb +++ b/enterprise/app/models/account_saml_settings.rb @@ -9,6 +9,7 @@ # created_at :datetime not null # updated_at :datetime not null # account_id :bigint not null +# idp_entity_id :string # sp_entity_id :string # # Indexes From 8b93241ed6f5aee838a9b3544fe02748f5641a05 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Tue, 2 Sep 2025 15:20:08 +0530 Subject: [PATCH 2/4] feat: include idp_entity_id and ceriticate details --- .../v1/accounts/saml_settings_controller.rb | 1 + .../app/models/account_saml_settings.rb | 25 ++++++++++++++++--- .../_account_saml_settings.json.jbuilder | 3 +++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/enterprise/app/controllers/api/v1/accounts/saml_settings_controller.rb b/enterprise/app/controllers/api/v1/accounts/saml_settings_controller.rb index 76e2911f1..3c9ec6ef3 100644 --- a/enterprise/app/controllers/api/v1/accounts/saml_settings_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/saml_settings_controller.rb @@ -30,6 +30,7 @@ class Api::V1::Accounts::SamlSettingsController < Api::V1::Accounts::BaseControl params.require(:saml_settings).permit( :sso_url, :certificate, + :idp_entity_id, :sp_entity_id, role_mappings: {} ) diff --git a/enterprise/app/models/account_saml_settings.rb b/enterprise/app/models/account_saml_settings.rb index 3f1842acc..13fde7db1 100644 --- a/enterprise/app/models/account_saml_settings.rb +++ b/enterprise/app/models/account_saml_settings.rb @@ -22,18 +22,37 @@ class AccountSamlSettings < ApplicationRecord validates :account_id, presence: true validates :sso_url, presence: true validates :certificate, presence: true - validates :sp_entity_id, presence: true + validates :idp_entity_id, presence: true + + before_validation :set_sp_entity_id, if: :sp_entity_id_needs_generation? def saml_enabled? sso_url.present? && certificate.present? end - def sp_entity_id_or_default - sp_entity_id.presence || "#{installation_name}-#{account_id}".downcase + def certificate_fingerprint + return nil if certificate.blank? + + begin + cert = OpenSSL::X509::Certificate.new(certificate) + OpenSSL::Digest::SHA1.new(cert.to_der).to_sExpand commentResolvedCode has comments.Press enter to view + .fingerprint.upcase.gsub(/(.{2})(?=.)/, '\1:') + rescue OpenSSL::X509::CertificateError + nil + end end private + def set_sp_entity_id + base_url = GlobalConfigService.load('FRONTEND_URL', 'http://localhost:3000') + self.sp_entity_id = "#{base_url}/saml/sp/#{account_id}" + end + + def sp_entity_id_needs_generation? + sp_entity_id.blank? + end + def installation_name GlobalConfigService.load('INSTALLATION_NAME', 'Chatwoot') end diff --git a/enterprise/app/views/api/v1/models/_account_saml_settings.json.jbuilder b/enterprise/app/views/api/v1/models/_account_saml_settings.json.jbuilder index 8c053f706..b17944605 100644 --- a/enterprise/app/views/api/v1/models/_account_saml_settings.json.jbuilder +++ b/enterprise/app/views/api/v1/models/_account_saml_settings.json.jbuilder @@ -1,6 +1,9 @@ json.id account_saml_settings.id json.account_id account_saml_settings.account_id json.sso_url account_saml_settings.sso_url +json.certificate account_saml_settings.certificate +json.fingerprint account_saml_settings.certificate_fingerprint +json.idp_entity_id account_saml_settings.idp_entity_id json.sp_entity_id account_saml_settings.sp_entity_id json.role_mappings account_saml_settings.role_mappings || {} json.created_at account_saml_settings.created_at From ba94ba3bf42c46987a22d72e51bcae147b09bcc0 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Tue, 2 Sep 2025 15:27:07 +0530 Subject: [PATCH 3/4] feat: add certificate fingerprint --- .../app/models/account_saml_settings.rb | 4 +-- .../accounts/saml_settings_controller_spec.rb | 2 +- .../models/account_saml_settings_spec.rb | 25 +++++++++++-------- spec/factories/account_saml_settings.rb | 2 +- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/enterprise/app/models/account_saml_settings.rb b/enterprise/app/models/account_saml_settings.rb index 13fde7db1..25e8aeffe 100644 --- a/enterprise/app/models/account_saml_settings.rb +++ b/enterprise/app/models/account_saml_settings.rb @@ -35,8 +35,8 @@ class AccountSamlSettings < ApplicationRecord begin cert = OpenSSL::X509::Certificate.new(certificate) - OpenSSL::Digest::SHA1.new(cert.to_der).to_sExpand commentResolvedCode has comments.Press enter to view - .fingerprint.upcase.gsub(/(.{2})(?=.)/, '\1:') + OpenSSL::Digest::SHA1.new(cert.to_der).hexdigest + .upcase.gsub(/(.{2})(?=.)/, '\1:') rescue OpenSSL::X509::CertificateError nil end diff --git a/spec/enterprise/controllers/api/v1/accounts/saml_settings_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/saml_settings_controller_spec.rb index 39e756086..646a11fae 100644 --- a/spec/enterprise/controllers/api/v1/accounts/saml_settings_controller_spec.rb +++ b/spec/enterprise/controllers/api/v1/accounts/saml_settings_controller_spec.rb @@ -102,7 +102,7 @@ RSpec.describe 'Api::V1::Accounts::SamlSettings', type: :request do saml_settings: { sso_url: 'https://idp.example.com/saml/sso', certificate: cert.to_pem, - sp_entity_id: 'chatwoot-production', + idp_entity_id: 'https://idp.example.com/saml/metadata', role_mappings: { 'Admins' => { 'role' => 1 }, 'Users' => { 'role' => 0 } } } } diff --git a/spec/enterprise/models/account_saml_settings_spec.rb b/spec/enterprise/models/account_saml_settings_spec.rb index 0e931478c..b65d0348f 100644 --- a/spec/enterprise/models/account_saml_settings_spec.rb +++ b/spec/enterprise/models/account_saml_settings_spec.rb @@ -23,10 +23,10 @@ RSpec.describe AccountSamlSettings, type: :model do expect(settings.errors[:certificate]).to include("can't be blank") end - it 'requires sp_entity_id' do - settings = build(:account_saml_settings, account: account, sp_entity_id: nil) + it 'requires idp_entity_id' do + settings = build(:account_saml_settings, account: account, idp_entity_id: nil) expect(settings).not_to be_valid - expect(settings.errors[:sp_entity_id]).to include("can't be blank") + expect(settings.errors[:idp_entity_id]).to include("can't be blank") end end @@ -56,16 +56,19 @@ RSpec.describe AccountSamlSettings, type: :model do end end - describe '#sp_entity_id_or_default' do - it 'returns sp_entity_id when present' do - settings = build(:account_saml_settings, account: account, sp_entity_id: 'custom-entity-id') - expect(settings.sp_entity_id_or_default).to eq('custom-entity-id') + describe 'sp_entity_id auto-generation' do + it 'automatically generates sp_entity_id when creating' do + settings = build(:account_saml_settings, account: account, sp_entity_id: nil) + expect(settings).to be_valid + settings.save! + expect(settings.sp_entity_id).to eq("http://localhost:3000/saml/sp/#{account.id}") end - it 'returns default entity id when sp_entity_id is blank' do - settings = build(:account_saml_settings, account: account, sp_entity_id: '') - expected = "chatwoot-#{account.id}" - expect(settings.sp_entity_id_or_default).to eq(expected) + it 'does not override existing sp_entity_id' do + custom_id = 'https://custom.example.com/saml/sp/123' + settings = build(:account_saml_settings, account: account, sp_entity_id: custom_id) + settings.save! + expect(settings.sp_entity_id).to eq(custom_id) end end end diff --git a/spec/factories/account_saml_settings.rb b/spec/factories/account_saml_settings.rb index c4b7c9ef1..4262daf7b 100644 --- a/spec/factories/account_saml_settings.rb +++ b/spec/factories/account_saml_settings.rb @@ -15,7 +15,7 @@ FactoryBot.define do cert.sign(key, OpenSSL::Digest.new('SHA256')) cert.to_pem end - sp_entity_id { 'chatwoot-test' } + idp_entity_id { 'https://idp.example.com/saml/metadata' } role_mappings { {} } trait :with_role_mappings do From 66abb424e4b849d0b342b08c1fe6bfcfe8b067ba Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Tue, 2 Sep 2025 15:27:45 +0530 Subject: [PATCH 4/4] test: certificate fingerprint --- .../models/account_saml_settings_spec.rb | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/spec/enterprise/models/account_saml_settings_spec.rb b/spec/enterprise/models/account_saml_settings_spec.rb index b65d0348f..65d5119d7 100644 --- a/spec/enterprise/models/account_saml_settings_spec.rb +++ b/spec/enterprise/models/account_saml_settings_spec.rb @@ -71,4 +71,47 @@ RSpec.describe AccountSamlSettings, type: :model do expect(settings.sp_entity_id).to eq(custom_id) end end + + describe '#certificate_fingerprint' do + let(:valid_cert_pem) do + key = OpenSSL::PKey::RSA.new(2048) + cert = OpenSSL::X509::Certificate.new + cert.version = 2 + cert.serial = 1 + cert.subject = OpenSSL::X509::Name.parse('/C=US/ST=Test/L=Test/O=Test/CN=test.example.com') + cert.issuer = cert.subject + cert.public_key = key.public_key + cert.not_before = Time.zone.now + cert.not_after = cert.not_before + (365 * 24 * 60 * 60) + cert.sign(key, OpenSSL::Digest.new('SHA256')) + cert.to_pem + end + + it 'returns fingerprint for valid certificate' do + settings = build(:account_saml_settings, account: account, certificate: valid_cert_pem) + fingerprint = settings.certificate_fingerprint + + expect(fingerprint).to be_present + expect(fingerprint).to match(/^[A-F0-9]{2}(:[A-F0-9]{2}){19}$/) # SHA1 fingerprint format + end + + it 'returns nil for blank certificate' do + settings = build(:account_saml_settings, account: account, certificate: '') + expect(settings.certificate_fingerprint).to be_nil + end + + it 'returns nil for invalid certificate' do + settings = build(:account_saml_settings, account: account, certificate: 'invalid-cert-data') + expect(settings.certificate_fingerprint).to be_nil + end + + it 'formats fingerprint correctly' do + settings = build(:account_saml_settings, account: account, certificate: valid_cert_pem) + fingerprint = settings.certificate_fingerprint + + # Should be uppercase with colons separating each byte + expect(fingerprint).to match(/^[A-F0-9:]+$/) + expect(fingerprint.count(':')).to eq(19) # 20 bytes = 19 colons + end + end end