refactor: improve SAML/SMTP error handling and secure SSO user provisioning

app/controllers/email_smtp_settings_controller.rb

@@ -8,19 +8,37 @@ class EmailSmtpSettingsController < ApplicationController
   def index; end
 
   def create
-    if @encrypted_config.update(email_configs)
+    # Store the original values in case of error
-      unless Docuseal.multitenant?
+    original_value = @encrypted_config.value
-        SettingsMailer.smtp_successful_setup(@encrypted_config.value['from_email'] || current_user.email).deliver_now!
+    Rails.logger.info "SMTP Update: Original config: #{original_value.inspect}"
+    
+    begin
+      if @encrypted_config.update(email_configs)
+        unless Docuseal.multitenant?
+          # Only attempt to send test email if SMTP settings are provided
+          if @encrypted_config.value.present? && @encrypted_config.value['address'].present?
+            SettingsMailer.smtp_successful_setup(@encrypted_config.value['from_email'] || current_user.email).deliver_now!
+          end
+        end
+
+        redirect_to settings_email_index_path, notice: I18n.t('changes_have_been_saved')
+        return
+      else
+        Rails.logger.error "SMTP Update: Update failed with errors: #{@encrypted_config.errors.full_messages}"
+        render :index, status: :unprocessable_entity
+        return
       end
-
+    rescue StandardError => e
-      redirect_to settings_email_index_path, notice: I18n.t('changes_have_been_saved')
+      Rails.logger.error "SMTP Update: Error occurred: #{e.message}"
-    else
+      Rails.logger.error e.backtrace.join("\n")
+      
+      # Restore the original values to prevent data loss
+      @encrypted_config.value = original_value
+      @encrypted_config.save if @encrypted_config.changed?
+      
+      flash[:alert] = "Error updating SMTP settings: #{e.message}"
       render :index, status: :unprocessable_entity
     end
-  rescue StandardError => e
-    flash[:alert] = e.message
-
-    render :index, status: :unprocessable_entity
   end
 
   private

app/models/user.rb

@@ -122,53 +122,37 @@ class User < ApplicationRecord
 
   # Omniauth callback method
   def self.from_omniauth(auth)
-    # Find existing user by provider and uid
+    # Find an existing user by provider and uid
     user = User.find_by(provider: auth.provider, uid: auth.uid)
-    
+    return user if user
-    if user
+
-      # Update user info from provider if needed
+    # IMPORTANT: The following section has been removed to prevent account takeover vulnerabilities.
-      user.update(
+    # Automatic linking of accounts based on email is not secure.
+    # A user should link their SSO provider only when they are already logged in.
+
+    # If the user does not exist, create a new one.
+    # NOTE: This account provisioning logic may need to be customized based on your application's needs.
+    begin
+      account = if Docuseal.multitenant?
+                  # This logic is potentially insecure and should be reviewed.
+                  # It is recommended to have a more robust way of assigning accounts.
+                  Account.find_or_create_by(name: auth.info.email&.split('@')&.last || 'SSO Account')
+                else
+                  Account.first_or_create!(name: 'Default Account')
+                end
+
+      User.create!(
         email: auth.info.email,
         first_name: auth.info.first_name || auth.info.name&.split&.first,
-        last_name: auth.info.last_name || auth.info.name&.split&.last
+        last_name: auth.info.last_name || auth.info.name&.split&.last,
-      ) if auth.info.email.present?
-      return user
-    end
-
-    # Try to find user by email if no provider/uid match
-    existing_user = User.find_by(email: auth.info.email) if auth.info.email.present?
-    
-    if existing_user
-      # Link existing account with omniauth provider
-      existing_user.update(
         provider: auth.provider,
-        uid: auth.uid
+        uid: auth.uid,
+        account: account,
+        password: Devise.friendly_token[0, 20]
       )
-      return existing_user
+    rescue ActiveRecord::RecordInvalid => e
+      Rails.logger.error "Failed to create user from omniauth: #{e.message}"
+      nil
     end
-
-    # Create new user from omniauth data
-    # For multitenant setups, we need to determine the account
-    account = if Docuseal.multitenant?
-                # In multitenant mode, create account from domain or use default logic
-                Account.find_or_create_by(name: auth.info.email&.split('@')&.last || 'SSO Account')
-              else
-                # In single-tenant mode, use the first account or create one
-                Account.first || Account.create!(name: 'Default Account')
-              end
-
-    User.create!(
-      email: auth.info.email,
-      first_name: auth.info.first_name || auth.info.name&.split&.first,
-      last_name: auth.info.last_name || auth.info.name&.split&.last,
-      provider: auth.provider,
-      uid: auth.uid,
-      account: account,
-      # Skip password validation for omniauth users
-      password: Devise.friendly_token[0, 20]
-    )
-  rescue ActiveRecord::RecordInvalid => e
-    Rails.logger.error "Failed to create user from omniauth: #{e.message}"
-    nil
   end
 end

app/views/devise/sessions/new.html.erb

@@ -37,18 +37,30 @@
       <% 
         # Check if SAML is configured (either in ENV or database)
         saml_configured = false
-        if ENV['SAML_IDP_SSO_SERVICE_URL'].present? && ENV['SAML_IDP_CERT_FINGERPRINT'].present?
+        begin
-          saml_configured = true
+          if ENV['SAML_IDP_SSO_SERVICE_URL'].present? && ENV['SAML_IDP_CERT_FINGERPRINT'].present?
-        else
+            saml_configured = true
-          saml_config_record = EncryptedConfig.find_by(key: 'saml_configs')
+            Rails.logger.info "SAML Login Page: Using ENV configuration"
-          if saml_config_record&.value.present?
+          else
-            begin
+            # Try to find SAML config in any account (not just current_account)
-              config = JSON.parse(saml_config_record.value)
+            saml_config_record = EncryptedConfig.where(key: 'saml_configs').first
-              saml_configured = config['idp_sso_service_url'].present? && config['idp_cert_fingerprint'].present?
+            Rails.logger.info "SAML Login Page: Config record found: #{saml_config_record.present?}"
-            rescue JSON::ParserError
+            
-              # Invalid JSON, treat as not configured
+            if saml_config_record&.value.present?
+              begin
+                config = JSON.parse(saml_config_record.value)
+                saml_configured = config['idp_sso_service_url'].present? && config['idp_cert_fingerprint'].present?
+                Rails.logger.info "SAML Login Page: Valid JSON: Yes, SSO URL: #{config['idp_sso_service_url'].present?}, Cert: #{config['idp_cert_fingerprint'].present?}"
+              rescue JSON::ParserError => e
+                Rails.logger.error "SAML Login Page: JSON parse error: #{e.message}"
+                # Invalid JSON, treat as not configured
+              end
+            else
+              Rails.logger.info "SAML Login Page: Config value is empty or nil"
             end
           end
+        rescue => e
+          Rails.logger.error "SAML Login Page: Error checking config: #{e.message}"
         end
       %>
       <% if User.omniauth_providers.include?(:saml) && saml_configured %>