diff --git a/app/controllers/oidc_accounts_controller.rb b/app/controllers/oidc_accounts_controller.rb index a89dc9395..c85c4dbec 100644 --- a/app/controllers/oidc_accounts_controller.rb +++ b/app/controllers/oidc_accounts_controller.rb @@ -91,14 +91,15 @@ class OidcAccountsController < ApplicationController return end - # Create user with a secure random password since they're using SSO - secure_password = SecureRandom.base58(32) + # Create SSO-only user without local password. + # Security: JIT users should NOT have password_digest set to prevent + # chained authentication attacks where SSO users gain local login access + # via password reset. @user = User.new( email: email, first_name: @pending_auth["first_name"], last_name: @pending_auth["last_name"], - password: secure_password, - password_confirmation: secure_password + skip_password_validation: true ) # Create new family for this user diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index 76d0adb1a..482654a3a 100644 --- a/app/controllers/password_resets_controller.rb +++ b/app/controllers/password_resets_controller.rb @@ -11,12 +11,17 @@ class PasswordResetsController < ApplicationController def create if (user = User.find_by(email: params[:email])) - PasswordMailer.with( - user: user, - token: user.generate_token_for(:password_reset) - ).password_reset.deliver_later + # Security: Block password reset for SSO-only users. + # These users have no local password and must authenticate via SSO. + unless user.sso_only? + PasswordMailer.with( + user: user, + token: user.generate_token_for(:password_reset) + ).password_reset.deliver_later + end end + # Always redirect to pending step to prevent email enumeration redirect_to new_password_reset_path(step: "pending") end @@ -25,6 +30,13 @@ class PasswordResetsController < ApplicationController end def update + # Security: Block password setting for SSO-only users. + # Defense-in-depth: even if they somehow get a reset token, block the update. + if @user.sso_only? + redirect_to new_session_path, alert: t("password_resets.sso_only_user") + return + end + if @user.update(password_params) redirect_to new_session_path, notice: t(".success") else diff --git a/app/models/user.rb b/app/models/user.rb index 8f1b6e2b7..569cafd2b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,5 +1,7 @@ class User < ApplicationRecord - has_secure_password + # Allow nil password for SSO-only users (JIT provisioning). + # Custom validation ensures password is present for non-SSO registration. + has_secure_password validations: false belongs_to :family belongs_to :last_viewed_chat, class_name: "Chat", optional: true @@ -17,6 +19,11 @@ class User < ApplicationRecord validate :ensure_valid_profile_image validates :default_period, inclusion: { in: Period::PERIODS.keys } validates :default_account_order, inclusion: { in: AccountOrder::ORDERS.keys } + + # Password is required on create unless the user is being created via SSO JIT. + # SSO JIT users have password_digest = nil and authenticate via OIDC only. + validates :password, presence: true, on: :create, unless: :skip_password_validation? + validates :password, length: { minimum: 8 }, allow_nil: true normalizes :email, with: ->(email) { email.strip.downcase } normalizes :unconfirmed_email, with: ->(email) { email&.strip&.downcase } @@ -104,6 +111,20 @@ class User < ApplicationRecord ai_enabled && ai_available? end + # SSO-only users have OIDC identities but no local password. + # They cannot use password reset or local login. + def sso_only? + password_digest.nil? && oidc_identities.exists? + end + + # Check if user has a local password set (can authenticate locally) + def has_local_password? + password_digest.present? + end + + # Attribute to skip password validation during SSO JIT provisioning + attr_accessor :skip_password_validation + # Deactivation validate :can_deactivate, if: -> { active_changed? && !active } after_update_commit :purge_later, if: -> { saved_change_to_active?(from: true, to: false) } @@ -258,6 +279,10 @@ class User < ApplicationRecord end private + def skip_password_validation? + skip_password_validation == true + end + def default_dashboard_section_order %w[cashflow_sankey outflows_donut net_worth_chart balance_sheet] end diff --git a/config/locales/views/password_resets/en.yml b/config/locales/views/password_resets/en.yml index 89b152b49..c491edca8 100644 --- a/config/locales/views/password_resets/en.yml +++ b/config/locales/views/password_resets/en.yml @@ -2,6 +2,7 @@ en: password_resets: disabled: Password reset via Sure is disabled. Please reset your password through your identity provider. + sso_only_user: Your account uses SSO for authentication. Please contact your administrator to manage your credentials. edit: title: Reset password new: diff --git a/test/controllers/oidc_accounts_controller_test.rb b/test/controllers/oidc_accounts_controller_test.rb index dab141d1d..d6c7c0a8c 100644 --- a/test/controllers/oidc_accounts_controller_test.rb +++ b/test/controllers/oidc_accounts_controller_test.rb @@ -191,4 +191,30 @@ class OidcAccountsControllerTest < ActionController::TestCase assert_redirected_to new_session_path assert_equal "No pending OIDC authentication found", flash[:alert] end + + # Security: JIT users should NOT have password_digest set + test "JIT user is created without password_digest to prevent chained auth attacks" do + session[:pending_oidc_auth] = new_user_auth + + post :create_user + + new_user = User.find_by(email: new_user_auth["email"]) + assert_not_nil new_user, "User should be created" + assert_nil new_user.password_digest, "JIT user should have nil password_digest" + assert new_user.sso_only?, "JIT user should be SSO-only" + end + + test "JIT user cannot authenticate with local password" do + session[:pending_oidc_auth] = new_user_auth + + post :create_user + + new_user = User.find_by(email: new_user_auth["email"]) + + # Attempting to authenticate should return nil (no password set) + assert_nil User.authenticate_by( + email: new_user.email, + password: "anypassword" + ), "SSO-only user should not authenticate with password" + end end diff --git a/test/controllers/password_resets_controller_test.rb b/test/controllers/password_resets_controller_test.rb index 42417c1c9..288c8a57e 100644 --- a/test/controllers/password_resets_controller_test.rb +++ b/test/controllers/password_resets_controller_test.rb @@ -48,4 +48,43 @@ class PasswordResetsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to new_session_path assert_equal "Password reset via Sure is disabled. Please reset your password through your identity provider.", flash[:alert] end + + # Security: SSO-only users should not receive password reset emails + test "create does not send email for SSO-only user" do + sso_user = users(:sso_only) + assert sso_user.sso_only?, "Test user should be SSO-only" + + assert_no_enqueued_emails do + post password_reset_path, params: { email: sso_user.email } + end + + # Should still redirect to pending to prevent email enumeration + assert_redirected_to new_password_reset_url(step: "pending") + end + + test "create sends email for user with local password" do + assert @user.has_local_password?, "Test user should have local password" + + assert_enqueued_emails 1 do + post password_reset_path, params: { email: @user.email } + end + + assert_redirected_to new_password_reset_url(step: "pending") + end + + # Security: SSO-only users cannot set password via reset + test "update blocks password setting for SSO-only user" do + sso_user = users(:sso_only) + token = sso_user.generate_token_for(:password_reset) + + patch password_reset_path(token: token), + params: { user: { password: "NewSecure1!", password_confirmation: "NewSecure1!" } } + + assert_redirected_to new_session_path + assert_equal "Your account uses SSO for authentication. Please contact your administrator to manage your credentials.", flash[:alert] + + # Verify password was not set + sso_user.reload + assert_nil sso_user.password_digest, "SSO-only user should still have nil password_digest" + end end diff --git a/test/fixtures/oidc_identities.yml b/test/fixtures/oidc_identities.yml index c2fbdb404..030cab934 100644 --- a/test/fixtures/oidc_identities.yml +++ b/test/fixtures/oidc_identities.yml @@ -19,3 +19,14 @@ jakob_google: first_name: Jakob last_name: Dylan last_authenticated_at: <%= 2.days.ago %> + +sso_only_identity: + user: sso_only + provider: openid_connect + uid: sso-only-uid-12345 + info: + email: sso-user@example.com + name: SSO User + first_name: SSO + last_name: User + last_authenticated_at: <%= 1.day.ago %> diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index b0987b1e6..a3694a0ed 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -43,6 +43,17 @@ new_email: last_name: User email: user@example.com unconfirmed_email: new@example.com - password_digest: $2a$12$XoNBo/cMCyzpYtvhrPAhsubG21mELX48RAcjSVCRctW8dG8wrDIla + password_digest: $2a$12$XoNBo/cMCyzpYtvhrPAhsubG21mELX48RAcjSVCRctW8dG8wrDIla onboarded_at: <%= Time.current %> + ai_enabled: true + +# SSO-only user: created via JIT provisioning, no local password +sso_only: + family: empty + first_name: SSO + last_name: User + email: sso-user@example.com + password_digest: ~ + role: admin + onboarded_at: <%= 1.day.ago %> ai_enabled: true \ No newline at end of file diff --git a/test/models/user_test.rb b/test/models/user_test.rb index c8d16889d..b4558744e 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -276,4 +276,59 @@ class UserTest < ActiveSupport::TestCase assert_not @user.dashboard_section_collapsed?("net_worth_chart"), "Should return false when section key is missing from collapsed_sections" end + + # SSO-only user security tests + test "sso_only? returns true for user with OIDC identity and no password" do + sso_user = users(:sso_only) + assert_nil sso_user.password_digest + assert sso_user.oidc_identities.exists? + assert sso_user.sso_only? + end + + test "sso_only? returns false for user with password and OIDC identity" do + # family_admin has both password and OIDC identity + assert @user.password_digest.present? + assert @user.oidc_identities.exists? + assert_not @user.sso_only? + end + + test "sso_only? returns false for user with password but no OIDC identity" do + user_without_oidc = users(:empty) + assert user_without_oidc.password_digest.present? + assert_not user_without_oidc.oidc_identities.exists? + assert_not user_without_oidc.sso_only? + end + + test "has_local_password? returns true when password_digest is present" do + assert @user.has_local_password? + end + + test "has_local_password? returns false when password_digest is nil" do + sso_user = users(:sso_only) + assert_not sso_user.has_local_password? + end + + test "user can be created without password when skip_password_validation is true" do + user = User.new( + email: "newssuser@example.com", + first_name: "New", + last_name: "SSO User", + skip_password_validation: true, + family: families(:empty) + ) + assert user.valid?, user.errors.full_messages.to_sentence + assert user.save + assert_nil user.password_digest + end + + test "user requires password on create when skip_password_validation is false" do + user = User.new( + email: "needspassword@example.com", + first_name: "Needs", + last_name: "Password", + family: families(:empty) + ) + assert_not user.valid? + assert_includes user.errors[:password], "can't be blank" + end end