From b2ecc6bc678609a37b95be7621ca9a4873dc6619 Mon Sep 17 00:00:00 2001
From: Josh Waldrep <joshua.waldrep5+github@gmail.com>
Date: Sat, 3 Jan 2026 21:13:24 -0500
Subject: [PATCH] refactor: improve SSO provider management and logging

- Simplified `name_id_format` selection logic in SSO provider form.
- Switched raw database query to sanitized SQL in client secret tests.
- Added condition to log JIT account creation only when identity persists.
- Sanitized failure reasons in SSO login failure handling.
- Added SSO provider connection test policy tests for super admin and regular users.
---
 app/controllers/oidc_accounts_controller.rb  | 16 +++++++++-------
 app/controllers/sessions_controller.rb       |  6 +++++-
 app/views/admin/sso_providers/_form.html.erb |  3 +--
 test/models/sso_provider_test.rb             |  8 +++++---
 test/policies/sso_provider_policy_test.rb    |  8 ++++++++
 5 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/app/controllers/oidc_accounts_controller.rb b/app/controllers/oidc_accounts_controller.rb
index bfc6597ab..8e346fe79 100644
--- a/app/controllers/oidc_accounts_controller.rb
+++ b/app/controllers/oidc_accounts_controller.rb
@@ -118,17 +118,19 @@ class OidcAccountsController < ApplicationController
 
     if @user.save
       # Create the OIDC (or other SSO) identity
-      OidcIdentity.create_from_omniauth(
+      identity = OidcIdentity.create_from_omniauth(
         build_auth_hash(@pending_auth),
         @user
       )
 
-      # Log JIT account creation
-      SsoAuditLog.log_jit_account_created!(
-        user: @user,
-        provider: @pending_auth["provider"],
-        request: request
-      )
+      # Only log JIT account creation if identity was successfully created
+      if identity.persisted?
+        SsoAuditLog.log_jit_account_created!(
+          user: @user,
+          provider: @pending_auth["provider"],
+          request: request
+        )
+      end
 
       # Clear pending auth from session
       session.delete(:pending_oidc_auth)
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index cb375d752..337447ee3 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -144,11 +144,15 @@ class SessionsController < ApplicationController
   end
 
   def failure
+    # Sanitize reason to known values only
+    known_reasons = %w[sso_provider_unavailable sso_invalid_response sso_failed]
+    sanitized_reason = known_reasons.include?(params[:message]) ? params[:message] : "sso_failed"
+
     # Log failed SSO attempt
     SsoAuditLog.log_login_failed!(
       provider: params[:strategy],
       request: request,
-      reason: params[:message]
+      reason: sanitized_reason
     )
 
     message = case params[:message]
diff --git a/app/views/admin/sso_providers/_form.html.erb b/app/views/admin/sso_providers/_form.html.erb
index 88de8c6c6..61b8a6c17 100644
--- a/app/views/admin/sso_providers/_form.html.erb
+++ b/app/views/admin/sso_providers/_form.html.erb
@@ -158,8 +158,7 @@
           <label class="block text-sm font-medium text-primary mb-1"><%= t("admin.sso_providers.form.name_id_format") %></label>
           <select name="sso_provider[settings][name_id_format]"
                   class="w-full px-3 py-2 border border-primary rounded-lg text-sm">
-            <option value="" <%= "selected" if sso_provider.settings&.dig("name_id_format").blank? %>><%= t("admin.sso_providers.form.name_id_email") %></option>
-            <option value="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress" <%= "selected" if sso_provider.settings&.dig("name_id_format") == "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress" %>><%= t("admin.sso_providers.form.name_id_email") %></option>
+            <option value="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress" <%= "selected" if sso_provider.settings&.dig("name_id_format").blank? || sso_provider.settings&.dig("name_id_format") == "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress" %>><%= t("admin.sso_providers.form.name_id_email") %></option>
             <option value="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" <%= "selected" if sso_provider.settings&.dig("name_id_format") == "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" %>><%= t("admin.sso_providers.form.name_id_persistent") %></option>
             <option value="urn:oasis:names:tc:SAML:2.0:nameid-format:transient" <%= "selected" if sso_provider.settings&.dig("name_id_format") == "urn:oasis:names:tc:SAML:2.0:nameid-format:transient" %>><%= t("admin.sso_providers.form.name_id_transient") %></option>
             <option value="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" <%= "selected" if sso_provider.settings&.dig("name_id_format") == "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" %>><%= t("admin.sso_providers.form.name_id_unspecified") %></option>
diff --git a/test/models/sso_provider_test.rb b/test/models/sso_provider_test.rb
index 81ca0b6df..9613ae255 100644
--- a/test/models/sso_provider_test.rb
+++ b/test/models/sso_provider_test.rb
@@ -96,9 +96,11 @@ class SsoProviderTest < ActiveSupport::TestCase
     assert_equal "super_secret_value", provider.client_secret
 
     # Raw database value should be encrypted (not plain text)
-    raw_value = ActiveRecord::Base.connection.execute(
-      "SELECT client_secret FROM sso_providers WHERE id = '#{provider.id}'"
-    ).first["client_secret"]
+    raw_value = ActiveRecord::Base.connection.select_value(
+      ActiveRecord::Base.sanitize_sql_array(
+        [ "SELECT client_secret FROM sso_providers WHERE id = ?", provider.id ]
+      )
+    )
 
     assert_not_equal "super_secret_value", raw_value
   end
diff --git a/test/policies/sso_provider_policy_test.rb b/test/policies/sso_provider_policy_test.rb
index 8632afdd0..1aa1e706c 100644
--- a/test/policies/sso_provider_policy_test.rb
+++ b/test/policies/sso_provider_policy_test.rb
@@ -86,6 +86,14 @@ class SsoProviderPolicyTest < ActiveSupport::TestCase
     assert_not SsoProviderPolicy.new(@regular_user, @provider).toggle?
   end
 
+  test "super admin can test connection" do
+    assert SsoProviderPolicy.new(@super_admin, @provider).test_connection?
+  end
+
+  test "regular user cannot test connection" do
+    assert_not SsoProviderPolicy.new(@regular_user, @provider).test_connection?
+  end
+
   test "scope returns all providers for super admin" do
     SsoProvider.create!(
       strategy: "google_oauth2",