diff --git a/app/controllers/admin/families_controller.rb b/app/controllers/admin/families_controller.rb new file mode 100644 index 000000000..e80b1e69b --- /dev/null +++ b/app/controllers/admin/families_controller.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Admin + class FamiliesController < Admin::BaseController + def destroy + family = Family.find(params[:id]) + authorize family + + if family.users.exists? + redirect_to admin_users_path, alert: t(".family_has_users") + return + end + + if family.destroy + redirect_to admin_users_path, notice: t(".success") + else + redirect_to admin_users_path, alert: family.errors.full_messages.to_sentence + end + end + end +end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 853faf6a1..15f34f456 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -8,7 +8,7 @@ module Admin authorize User scope = policy_scope(User) .left_joins(family: :subscription) - .includes(family: :subscription) + .includes(:oidc_identities, family: :subscription) scope = scope.where(role: params[:role]) if params[:role].present? scope = apply_trial_filter(scope) if params[:trial_status].present? @@ -28,8 +28,8 @@ module Admin @entries_count_by_family = Entry.joins(:account).where(accounts: { family_id: family_ids }).group("accounts.family_id").count user_ids = users.map(&:id).uniq - @last_login_by_user = Session.where(user_id: user_ids).group(:user_id).maximum(:created_at) - @sessions_count_by_user = Session.where(user_id: user_ids).group(:user_id).count + @last_login_by_user = User.where(id: user_ids).pluck(:id, :last_login_at).to_h + @sessions_count_by_user = User.where(id: user_ids).pluck(:id, :sessions_count).to_h @families_with_users = users.group_by(&:family).sort_by do |family, _users| -(@entries_count_by_family[family.id] || 0) @@ -39,6 +39,9 @@ module Admin .where(family_id: family_ids) .group_by(&:family_id) + @families = Family.order(:name, :created_at) + @unused_families = Family.left_joins(:users).where(users: { id: nil }).order(:name, :created_at) + @trials_expiring_in_7_days = Subscription .where(status: :trialing) .where(trial_ends_at: Time.current..7.days.from_now) @@ -53,19 +56,81 @@ module Admin def update authorize @user - if @user.update(user_params) + if demoting_last_super_admin? + redirect_to admin_users_path, alert: t(".last_super_admin_error") + return + end + + if membership_change_requested? && password_change_requested? + redirect_to admin_users_path, alert: t(".password_and_family_conflict") + return + end + + if password_change_requested? + errors = validate_password_criteria(user_params[:password]) + if errors.any? + redirect_to admin_users_path, alert: errors.join(" ") + return + end + end + + if membership_change_requested? + target_family = nil + + ActiveRecord::Base.transaction do + target_family = target_family_for_update + + if target_family.nil? + raise ActiveRecord::Rollback + end + + @user.transfer_to_family!(target_family, role: user_params[:role]) + end + + if target_family.nil? + redirect_to admin_users_path, alert: t(".family_required") + return + end + Rails.logger.info( - "[Admin::Users] Role changed - " \ + "[Admin::Users] Family changed - " \ + "by_user_id=#{Current.user.id} " \ + "target_user_id=#{@user.id} " \ + "new_family_id=#{@user.family_id} " \ + "new_role=#{@user.role}" + ) + + redirect_to admin_users_path, notice: t(".success_family") + elsif @user.update(user_update_attributes) + changes = [] + changes << :role if @user.saved_change_to_role? + changes << :password if @user.saved_change_to_password_digest? + + success_key = case changes + when [ :role, :password ] then ".success_role_and_password" + when [ :password ] then ".success_password" + else ".success_role" + end + + Rails.logger.info( + "[Admin::Users] User details changed (#{changes.join(', ')}) - " \ "by_user_id=#{Current.user.id} " \ "target_user_id=#{@user.id} " \ "new_role=#{@user.role}" ) - redirect_to admin_users_path, notice: t(".success") + + redirect_to admin_users_path, notice: t(success_key) else - redirect_to admin_users_path, alert: t(".failure") + redirect_to admin_users_path, alert: @user.errors.full_messages.to_sentence.presence || t(".failure") end + rescue ActiveRecord::RecordInvalid => e + redirect_to admin_users_path, alert: e.record.errors.full_messages.to_sentence + rescue ActiveRecord::RecordNotFound + redirect_to admin_users_path, alert: t(".failure") end + + def deletion # Same self-removal short-circuit as #destroy. UserPolicy#destroy? already # denies it, but Pundit::NotAuthorizedError is not rescued anywhere in this @@ -112,12 +177,69 @@ module Admin private + helper_method :family_label_for + def set_user @user = User.find(params[:id]) end def user_params - params.require(:user).permit(:role) + params.require(:user).permit(:role, :family_id, :new_family_name, :new_family_moniker, :password) + end + + def user_update_attributes + attrs = {} + attrs[:role] = user_params[:role] if user_params[:role].present? + if user_params[:password].present? && @user.has_local_password? + attrs[:password] = user_params[:password] + end + attrs + end + + def password_change_requested? + user_params[:password].present? && @user.has_local_password? + end + + def validate_password_criteria(password) + errors = [] + errors << t(".password_too_short") if password.length < 8 + errors << t(".password_missing_case") unless password.match?(/[A-Z]/) && password.match?(/[a-z]/) + errors << t(".password_missing_number") unless password.match?(/\d/) + errors << t(".password_missing_special") unless password.match?(/[!@#$%^&*(),.?":{}|<>]/) + errors + end + + def membership_change_requested? + return true if user_params[:new_family_name].to_s.strip.present? + return true if user_params[:family_id] == "new" + + user_params[:family_id].present? && user_params[:family_id] != @user.family_id.to_s + end + + def target_family_for_update + new_family_name = user_params[:new_family_name].to_s.strip + + if new_family_name.present? + Family.create!( + name: new_family_name, + moniker: user_params[:new_family_moniker].presence || "Family" + ) + elsif user_params[:family_id].present? && user_params[:family_id] != "new" + Family.find(user_params[:family_id]) + end + end + + def family_label_for(family) + return "" if family.nil? + + family.name.presence || "#{family.moniker_label} (#{family.id.to_s.first(8)})" + end + + def demoting_last_super_admin? + user_params[:role].present? && + @user.super_admin? && + user_params[:role] != "super_admin" && + User.where(role: :super_admin).where.not(id: @user.id).none? end def apply_trial_filter(scope) diff --git a/app/javascript/controllers/admin_user_family_select_controller.js b/app/javascript/controllers/admin_user_family_select_controller.js new file mode 100644 index 000000000..9509e9e13 --- /dev/null +++ b/app/javascript/controllers/admin_user_family_select_controller.js @@ -0,0 +1,21 @@ +import { Controller } from "@hotwired/stimulus"; + +export default class extends Controller { + static targets = ["select", "newFamilyFields", "nameInput"]; + + connect() { + this.toggle(); + } + + toggle() { + if (this.hasSelectTarget && this.hasNewFamilyFieldsTarget) { + const isNew = this.selectTarget.value === "new"; + this.newFamilyFieldsTarget.classList.toggle("hidden", !isNew); + if (isNew) { + if (this.hasNameInputTarget) this.nameInputTarget.focus(); + } else { + if (this.hasNameInputTarget) this.nameInputTarget.value = ""; + } + } + } +} diff --git a/app/models/account.rb b/app/models/account.rb index f812cab78..772f6a29c 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -669,12 +669,17 @@ class Account < ApplicationRecord if Current.user.present? && Current.user.family_id == family_id self.owner = Current.user else - self.owner = family&.users&.find_by(role: %w[admin super_admin]) || family&.users&.order(:created_at)&.first + self.owner = + family&.users&.where(role: "admin")&.order(:created_at)&.first || + family&.users&.where(role: "super_admin")&.order(:created_at)&.first || + family&.users&.order(:created_at)&.first end end def owner_belongs_to_family - return if User.where(id: owner_id, family_id: family_id).exists? + owner_user = User.lock.find_by(id: owner_id) + return if owner_user&.family_id == family_id + errors.add(:owner, :invalid, message: "must belong to the same family as the account") end diff --git a/app/models/family/subscribeable.rb b/app/models/family/subscribeable.rb index 9ac267f6c..e35517bdb 100644 --- a/app/models/family/subscribeable.rb +++ b/app/models/family/subscribeable.rb @@ -7,6 +7,7 @@ module Family::Subscribeable included do has_one :subscription, dependent: :destroy + before_destroy :cancel_or_reject_active_subscription scope :inactive_trial_for_cleanup, -> { cutoff_with_sub = CLEANUP_GRACE_PERIOD.ago @@ -113,4 +114,20 @@ module Family::Subscribeable entries.where(date: recent_window_start..trial_end).exists? end + + private + + def cancel_or_reject_active_subscription + return unless subscription&.stripe_id.present? + return if subscription.canceled? || subscription.incomplete_expired? + + begin + Provider::Registry.get_provider(:stripe).cancel_subscription(subscription.stripe_id) + rescue => e + Sentry.capture_exception(e) if defined?(Sentry) + Rails.logger.error "Failed to cancel Stripe subscription before family deletion: #{e.message}" + errors.add(:base, :cannot_delete_with_active_subscription, message: "Could not cancel active Stripe subscription. Please cancel it manually before deleting the family.") + throw(:abort) + end + end end diff --git a/app/models/oidc_identity.rb b/app/models/oidc_identity.rb index e3c2dcba1..deae3f943 100644 --- a/app/models/oidc_identity.rb +++ b/app/models/oidc_identity.rb @@ -104,7 +104,11 @@ class OidcIdentity < ApplicationRecord # Find the configured provider for this identity def provider_config - AuthConfig.sso_providers&.find { |p| p[:name] == provider || p[:id] == provider } + AuthConfig.sso_providers&.find do |p| + p_name = p[:name] || p["name"] + p_id = p[:id] || p["id"] + p_name == provider || p_id == provider + end end # Validate that the stored issuer matches the configured provider's issuer @@ -113,8 +117,9 @@ class OidcIdentity < ApplicationRecord return true if issuer.blank? # Backward compatibility for old records config = provider_config - return true if config.blank? || config[:issuer].blank? # No config to validate against + config_issuer = config&.dig(:issuer) || config&.dig("issuer") + return true if config_issuer.blank? # No config to validate against - issuer == config[:issuer] + issuer == config_issuer end end diff --git a/app/models/provider/stripe.rb b/app/models/provider/stripe.rb index 3a75dd257..6250e1bf5 100644 --- a/app/models/provider/stripe.rb +++ b/app/models/provider/stripe.rb @@ -67,6 +67,10 @@ class Provider::Stripe client.v1.customers.update(customer_id, metadata: metadata) end + def cancel_subscription(subscription_id) + client.v1.subscriptions.cancel(subscription_id) + end + private attr_reader :client, :webhook_secret diff --git a/app/models/session.rb b/app/models/session.rb index 7e44bd49d..891a72f24 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -6,7 +6,7 @@ class Session < ApplicationRecord encrypts :user_agent end - belongs_to :user + belongs_to :user, counter_cache: :sessions_count belongs_to :active_impersonator_session, -> { where(status: :in_progress) }, class_name: "ImpersonationSession", @@ -14,10 +14,13 @@ class Session < ApplicationRecord before_create :capture_session_info + after_create :update_user_last_login + def prev_transaction_page_params super || {} end + def get_preferred_tab(tab_key) data.dig("tab_preferences", tab_key) end @@ -36,4 +39,8 @@ class Session < ApplicationRecord self.ip_address = raw_ip self.ip_address_digest = Digest::SHA256.hexdigest(raw_ip.to_s) if raw_ip.present? end + + def update_user_last_login + user.update_columns(last_login_at: created_at) + end end diff --git a/app/models/user.rb b/app/models/user.rb index d1a994f0d..223cfde2f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -218,7 +218,7 @@ class User < ApplicationRecord # 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? + password_digest.nil? && oidc_identities.any? end # Check if user has a local password set (can authenticate locally) @@ -231,24 +231,52 @@ class User < ApplicationRecord # Deactivation validate :can_deactivate, if: -> { active_changed? && !active } + + # Super Admin Invariant + validate :ensure_not_last_super_admin, if: :losing_super_admin_privileges? + before_destroy :ensure_not_last_super_admin_on_destroy + after_update_commit :purge_later, if: -> { saved_change_to_active?(from: true, to: false) } def deactivate return true unless active? transaction do - if super_admin? - active_super_admins = User.where(role: :super_admin, active: true).lock.to_a - if active_super_admins.one? - errors.add(:base, :cannot_remove_last_super_admin) - raise ActiveRecord::Rollback - end - end - update(active: false, email: deactivated_email) end || false end + private + + def losing_super_admin_privileges? + (role_changed? && role_was == "super_admin" && role != "super_admin") || + (active_changed? && active_was == true && !active && role == "super_admin") + end + + def ensure_not_last_super_admin + return unless check_last_super_admin_invariant_failed? + + attribute = role_changed? ? :role : :base + errors.add(attribute, :cannot_remove_last_super_admin, message: I18n.t("admin.users.update.last_super_admin_error")) + end + + def ensure_not_last_super_admin_on_destroy + return unless role == "super_admin" && active? + + if check_last_super_admin_invariant_failed? + errors.add(:base, :cannot_remove_last_super_admin, message: I18n.t("admin.users.update.last_super_admin_error")) + throw(:abort) + end + end + + def check_last_super_admin_invariant_failed? + # Lock all active super admins in a consistent order to prevent deadlocks + locked_ids = User.where(role: :super_admin, active: true).order(:id).lock.pluck(:id) + locked_ids.size <= 1 && locked_ids.include?(id) + end + + public + # Permanent removal of another user, initiated by a super admin from the # instance users page. Reuses the sanctioned deactivate -> UserPurgeJob path # (which reassigns owned accounts, or destroys the family when this is its @@ -297,6 +325,59 @@ class User < ApplicationRecord UserPurgeJob.perform_later(self) end + def transfer_to_family!(new_family, role: self.role) + transaction do + lock! + + accounts_to_move = owned_accounts.to_a + provider_items_to_move = provider_items_for_transfer(accounts_to_move) + moving_default_account = accounts_to_move.any? { |account| account.id == default_account_id } + + account_shares.delete_all + + update!(family: new_family, role: role, default_account: moving_default_account ? default_account : nil) + + accounts_to_move.each do |account| + account.update!(family: new_family) + end + + AccountStatement.where(account: accounts_to_move).update_all(family_id: new_family.id, updated_at: Time.current) if accounts_to_move.any? + + provider_items_to_move.each do |provider_item| + provider_item.update!(family: new_family) + end + + new_family.auto_share_existing_accounts_with(self) + end + end + + def provider_items_for_transfer(accounts_to_move) + account_ids_to_move = accounts_to_move.map(&:id) + provider_items = accounts_to_move.flat_map do |account| + account.account_providers.includes(:provider).filter_map do |account_provider| + provider_item_for(account_provider.provider) + end + end.uniq + + provider_items.each do |provider_item| + linked_account_ids = provider_item.accounts.map(&:id) + next if linked_account_ids.all? { |account_id| account_ids_to_move.include?(account_id) } + + errors.add(:base, :provider_item_has_other_accounts) + raise ActiveRecord::RecordInvalid, self + end + + provider_items + end + + def provider_item_for(provider) + item_association = provider.class.reflect_on_all_associations(:belongs_to).find do |association| + association.name.to_s.end_with?("_item") && provider.respond_to?(association.name) + end + + provider.public_send(item_association.name) if item_association + end + def purge if last_user_in_family? family.destroy diff --git a/app/policies/family_policy.rb b/app/policies/family_policy.rb new file mode 100644 index 000000000..a1afbcaad --- /dev/null +++ b/app/policies/family_policy.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class FamilyPolicy < ApplicationPolicy + def destroy? + user&.super_admin? + end +end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index abab05920..ca9ef0087 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -7,8 +7,11 @@ class UserPolicy < ApplicationPolicy end def update? + user&.super_admin? + end + + def destroy? return false unless user&.super_admin? - # Prevent users from changing their own role (must be done by another super_admin) user.id != record.id end diff --git a/app/views/admin/users/deletion.html.erb b/app/views/admin/users/deletion.html.erb index 18b0cea68..f609c779f 100644 --- a/app/views/admin/users/deletion.html.erb +++ b/app/views/admin/users/deletion.html.erb @@ -10,7 +10,7 @@ autocomplete: "off", spellcheck: false, required: true, - class: "mb-4" %> + container_class: "mb-4" %>
<%= render DS::Button.new( diff --git a/app/views/admin/users/index.html.erb b/app/views/admin/users/index.html.erb index 326a0206b..8018f836b 100644 --- a/app/views/admin/users/index.html.erb +++ b/app/views/admin/users/index.html.erb @@ -95,7 +95,7 @@ <%= t(".table.user") %> <%= t(".table.last_login") %> <%= t(".table.session_count") %> - <%= t(".table.role") %> + <%= t(".table.actions") %> @@ -107,7 +107,26 @@ <%= user.initials %>
-

<%= user.display_name %>

+
+

<%= user.display_name %>

+ <% if user.id == Current.user.id %> + <%= t(".you") %> + <% end %> + <%= render DS::Pill.new(label: t(".roles.#{user.role}"), tone: user.super_admin? ? :success : :neutral) %> + <% if user.oidc_identities.any? %> + <% sso_providers = user.oidc_identities.map { |i| (i.provider_config&.dig(:label) || i.provider_config&.dig("label")).presence || i.provider.titleize }.uniq.join(", ") %> + <% provider_tooltip = t(".auth_types.sso_provider_tooltip", providers: sso_providers) %> + <% if user.sso_only? %> + <%= render DS::Pill.new(label: t(".auth_types.sso"), tone: :info) %> + <%= render DS::Tooltip.new(text: provider_tooltip, as: :span) %> + <% else %> + <%= render DS::Pill.new(label: t(".auth_types.local_and_sso"), tone: :info) %> + <%= render DS::Tooltip.new(text: provider_tooltip, as: :span) %> + <% end %> + <% else %> + <%= render DS::Pill.new(label: t(".auth_types.local"), tone: :neutral) %> + <% end %> +

<%= user.email %>

@@ -118,34 +137,118 @@ <%= number_with_delimiter(@sessions_count_by_user[user.id] || 0) %> - - <% if user.id == Current.user.id %> - <%= t(".you") %> - <% else %> -
- <%= form_with model: [:admin, user], method: :patch, class: "flex items-center", data: { controller: "auto-submit-form" } do |form| %> - <%= form.select :role, - options_for_select([ - [t(".roles.guest"), "guest"], - [t(".roles.member"), "member"], - [t(".roles.admin"), "admin"], - [t(".roles.super_admin"), "super_admin"] - ], user.role), - {}, - class: "text-sm rounded-lg border border-primary bg-container text-primary px-2 py-1", - data: { auto_submit_form_target: "auto" } %> - <% end %> - <% unless user.super_admin? && user.active? && @active_super_admin_count <= 1 %> - <%= render DS::Button.new( - text: t(".table.remove"), - href: deletion_admin_user_path(user), - method: :get, - frame: "modal", - variant: :outline_destructive, - size: :sm + + <%= render DS::Popover.new(variant: :icon, icon: "ellipsis-vertical", placement: "bottom-end", aria_label: t(".actions.manage_user")) do |popover| %> + <% popover.with_custom_content do %> + <% is_last_super_admin = user.super_admin? && User.where(role: :super_admin).where.not(id: user.id).none? %> +
+
+

<%= user.display_name %>

+

<%= user.email %>

+
+ + <%= form_with model: [:admin, user], method: :patch, class: "space-y-3" do |form| %> +
+ <%= form.label :role, t(".table.role"), class: "block text-xs font-medium text-secondary mb-1" %> + <% if is_last_super_admin %> + <%= form.select :role, + options_for_select([[t(".roles.super_admin"), "super_admin"]], "super_admin"), + {}, + disabled: true, + class: "w-full text-sm rounded-lg border border-primary bg-container-inset text-secondary px-2 py-1.5 cursor-not-allowed" %> + <%= form.hidden_field :role, value: "super_admin" %> +

<%= t(".actions.last_super_admin_role_locked") %>

+ <% else %> + <%= form.select :role, + options_for_select([ + [t(".roles.guest"), "guest"], + [t(".roles.member"), "member"], + [t(".roles.admin"), "admin"], + [t(".roles.super_admin"), "super_admin"] + ], user.role), + {}, + class: "w-full text-sm rounded-lg border border-primary bg-container text-primary px-2 py-1.5" %> + <% end %> +
+ +
+
+ <%= form.label :family_id, t(".table.family"), class: "block text-xs font-medium text-secondary mb-1" %> + <%= form.select :family_id, + options_for_select([ + [t(".table.keep_current_family"), ""], + [t(".table.create_new_family_option"), "new"] + ] + @families.map { |family| [family_label_for(family), family.id] }, user.family_id), + {}, + class: "w-full text-sm rounded-lg border border-primary bg-container text-primary px-2 py-1.5", + data: { + admin_user_family_select_target: "select", + action: "change->admin-user-family-select#toggle" + } %> +
+ + +
+ + <% if user.has_local_password? %> +
+ <%= form.label :password, t(".table.set_password"), class: "block text-xs font-medium text-secondary mb-1" %> + <%= form.password_field :password, + placeholder: t(".table.password_placeholder"), + autocomplete: "new-password", + class: "w-full text-sm rounded-lg border border-primary bg-container text-primary px-2 py-1.5" %> +
+ <% end %> + + <% if user.id == Current.user.id %> +
+

<%= t(".actions.self_edit_warning") %>

+
+ <% end %> + +
+ <%= render DS::Button.new( + variant: :primary, + size: :sm, + type: "submit", + confirm: (user.id == Current.user.id ? t(".actions.self_update_confirm") : nil), + text: t(".actions.update"), + class: "w-full justify-center" ) %> - <% end %> -
+
+ <% end %> + + <% unless user.id == Current.user.id || (user.super_admin? && user.active? && @active_super_admin_count <= 1) %> +
+ <%= render DS::Button.new( + text: t(".remove"), + href: deletion_admin_user_path(user), + method: :get, + frame: "modal", + variant: :outline_destructive, + size: :sm, + class: "w-full justify-center" + ) %> +
+ <% end %> +
+ <% end %> <% end %> @@ -175,7 +278,7 @@ <%= render DS::Button.new( text: t(".invitations.delete"), type: "submit", - variant: :outline_destructive, + variant: :destructive, size: :sm, data: { admin_invitation_delete_target: "button", @@ -207,6 +310,36 @@ <% end %> +
+

<%= t(".unused_families.title") %>

+ + <% if @unused_families.any? %> +
+ <% @unused_families.each do |family| %> +
+
+

<%= family.name.presence || t(".unnamed_family") %>

+

<%= t(".unused_families.family_id", id: family.id.to_s.first(8)) %>

+
+ + <%= render DS::Button.new( + variant: :outline_destructive, + size: :sm, + href: admin_family_path(family), + method: :delete, + confirm: t(".unused_families.delete_confirm"), + text: t(".unused_families.delete") + ) %> +
+ <% end %> +
+ <% else %> +
+ <%= t(".unused_families.empty") %> +
+ <% end %> +
+ <% if @sso_identity_blocks.any? %> <%= settings_section title: t(".removed_sso_identities.title"), collapsible: true, open: false do %>

<%= t(".removed_sso_identities.description") %>

diff --git a/config/brakeman.ignore b/config/brakeman.ignore index df46de563..463351253 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -118,13 +118,13 @@ { "warning_type": "Mass Assignment", "warning_code": 105, - "fingerprint": "01a88a0a17848e70999c17f6438a636b00e01da39a2c0aa0c46f20f0685c7202", + "fingerprint": "dba46648ab625bae43bd8126e4dcdba34b6828a064cedb5129c3c2e8607789c7", "check_name": "PermitAttributes", "message": "Potentially dangerous key allowed for mass assignment", "file": "app/controllers/admin/users_controller.rb", - "line": 35, + "line": 146, "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/", - "code": "params.require(:user).permit(:role)", + "code": "params.require(:user).permit(:role, :family_id, :new_family_name, :new_family_moniker, :password)", "render_path": null, "location": { "type": "method", diff --git a/config/locales/models/user/en.yml b/config/locales/models/user/en.yml index 3be84c0a2..bca600838 100644 --- a/config/locales/models/user/en.yml +++ b/config/locales/models/user/en.yml @@ -17,6 +17,7 @@ en: base: cannot_deactivate_admin_with_other_users: Admin cannot delete account while other users are present. Please delete all members first. + provider_item_has_other_accounts: User owns accounts linked to a provider connection that also has accounts owned by another user. Move or unlink those accounts before changing family/group membership. cannot_remove_last_super_admin: Cannot remove the last active super admin. Promote another user to super admin first. profile_image: diff --git a/config/locales/views/admin/users/en.yml b/config/locales/views/admin/users/en.yml index a29816a4b..88b433fdd 100644 --- a/config/locales/views/admin/users/en.yml +++ b/config/locales/views/admin/users/en.yml @@ -4,7 +4,7 @@ en: users: index: title: "User Management" - description: "Manage user roles for your instance. Super admins can access SSO provider settings and user management." + description: "Manage user roles, family/group membership, and account removal for your instance. Super admins can access SSO provider settings and user management." section_title: "Families / Groups" you: "(You)" trial_ends_at: "Trial ends" @@ -32,7 +32,28 @@ en: session_count: "Session count" never: "Never" role: "Role" - remove: "Remove" + family: "Family / Group" + keep_current_family: "Keep current family/group" + create_new_family_option: "+ Create new family/group" + new_family_name: "New family/group name" + new_family_name_placeholder: "Enter a name to create a new family/group" + new_family_moniker: "New family/group type" + set_password: "Set new password" + password_placeholder: "Leave blank to keep current" + actions: "Actions" + family_monikers: + family: "Family" + group: "Group" + actions: + manage_user: "Manage user" + update: "Update user" + delete_user: "Delete user" + self_edit_warning: "Changing your own role will modify your administrative privileges." + self_update_confirm: "Are you sure you want to update your own account role or family?" + last_super_admin_role_locked: "Role change is blocked because you are the last super admin in the system." + delete_confirm: "Are you sure you want to delete this user? This action cannot be undone." + delete_confirm_last_user: "Are you sure you want to delete this user? This is the last user in their family/group, so deleting them will also delete the entire family/group and all associated data. This action cannot be undone." + remove: "Delete User" role_descriptions_title: "Role Descriptions" removed_sso_identities: title: "Removed SSO identities" @@ -44,6 +65,11 @@ en: member: "Member" admin: "Admin" super_admin: "Super Admin" + auth_types: + sso: "SSO" + local: "Local" + local_and_sso: "Local + SSO" + sso_provider_tooltip: "SSO Provider: %{providers}" role_descriptions: guest: "Assistant-first experience with intentionally restricted permissions for intro workflows." member: "Basic user access. Can manage their own accounts, transactions, and settings." @@ -54,9 +80,25 @@ en: expires: "Expires %{date}" delete: "Delete" delete_all: "Delete All" + unused_families: + title: "Unused families / groups" + family_id: "Family ID: %{id}" + delete: "Delete unused family" + delete_confirm: "Are you sure you want to delete this unused family/group? This action cannot be undone." + empty: "No unused families/groups found." update: - success: "User role updated successfully." - failure: "Failed to update user role." + success_role: "User role updated successfully." + success_password: "User password updated successfully." + success_role_and_password: "User role and password updated successfully." + success_family: "User family/group membership updated successfully." + failure: "Failed to update user details (role or family/group membership)." + family_required: "Select an existing family/group or enter a name for a new one." + password_and_family_conflict: "Password and family/group membership cannot be updated at the same time." + password_too_short: "Password must be at least 8 characters long." + password_missing_case: "Password must include both uppercase and lowercase letters." + password_missing_number: "Password must include at least one number." + password_missing_special: "Password must include at least one special character." + last_super_admin_error: "Cannot demote the last super admin in the system." destroy: success: "User access revoked and permanent deletion scheduled." failure: "Failed to remove user." @@ -70,3 +112,8 @@ en: email_label: "User email" cancel: "Cancel" submit: "Permanently remove user" + families: + destroy: + family_has_active_subscription: "Family/group has an active subscription and cannot be deleted until the subscription is cancelled." + family_has_users: "Family/group still has users and cannot be deleted." + success: "Family/group deleted successfully." diff --git a/config/routes.rb b/config/routes.rb index d8e7d7066..3a9bc205a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -872,7 +872,7 @@ Rails.application.routes.draw do end resources :sso_identity_blocks, only: [ :destroy ] resources :invitations, only: [ :destroy ] - resources :families, only: [] do + resources :families, only: [ :destroy ] do member do delete :invitations, to: "invitations#destroy_all" end diff --git a/db/migrate/20260821034200_add_last_login_at_and_sessions_count_to_users.rb b/db/migrate/20260821034200_add_last_login_at_and_sessions_count_to_users.rb new file mode 100644 index 000000000..e10c84900 --- /dev/null +++ b/db/migrate/20260821034200_add_last_login_at_and_sessions_count_to_users.rb @@ -0,0 +1,19 @@ +class AddLastLoginAtAndSessionsCountToUsers < ActiveRecord::Migration[7.2] + def change + add_column :users, :last_login_at, :datetime, if_not_exists: true + add_column :users, :sessions_count, :integer, default: 0, null: false, if_not_exists: true + + # Backfill existing data from sessions table + reversible do |dir| + dir.up do + say_with_time "Backfilling last_login_at and sessions_count" do + execute <<-SQL + UPDATE users + SET last_login_at = (SELECT MAX(created_at) FROM sessions WHERE user_id = users.id), + sessions_count = (SELECT COUNT(*) FROM sessions WHERE user_id = users.id); + SQL + end + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index a66cb6050..40f629525 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2323,6 +2323,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_08_26_090000) do t.uuid "family_id", null: false t.string "first_name" t.text "goals", default: [], array: true + t.datetime "last_login_at" t.string "last_name" t.uuid "last_viewed_chat_id" t.string "locale" @@ -2335,6 +2336,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_08_26_090000) do t.string "role", default: "member", null: false t.datetime "rule_prompt_dismissed_at" t.boolean "rule_prompts_disabled", default: false + t.integer "sessions_count", default: 0, null: false t.datetime "set_onboarding_goals_at" t.datetime "set_onboarding_preferences_at" t.boolean "show_ai_sidebar", default: true diff --git a/test/controllers/admin/families_controller_test.rb b/test/controllers/admin/families_controller_test.rb new file mode 100644 index 000000000..fa6cc0f07 --- /dev/null +++ b/test/controllers/admin/families_controller_test.rb @@ -0,0 +1,39 @@ +require "test_helper" + +class Admin::FamiliesControllerTest < ActionDispatch::IntegrationTest + setup do + sign_in users(:sure_support_staff) + end + + test "destroy deletes an unused family" do + family = Family.create!(name: "Unused Family") + + assert_difference("Family.count", -1) do + delete admin_family_url(family) + end + + assert_redirected_to admin_users_url + end + + test "destroy does not delete a family with users" do + family = users(:family_admin).family + + assert_no_difference("Family.count") do + delete admin_family_url(family) + end + + assert_redirected_to admin_users_url + end + + test "destroy does not delete a family with an active subscription" do + family = Family.create!(name: "Subscribed Empty Family") + family.start_subscription!("sub_admin_cleanup") + + assert_no_difference("Family.count") do + delete admin_family_url(family) + end + + assert_redirected_to admin_users_url + assert_equal "Could not cancel active Stripe subscription. Please cancel it manually before deleting the family.", flash[:alert] + end +end diff --git a/test/controllers/admin/users_controller_test.rb b/test/controllers/admin/users_controller_test.rb index 5c39c2494..abc7588ba 100644 --- a/test/controllers/admin/users_controller_test.rb +++ b/test/controllers/admin/users_controller_test.rb @@ -49,6 +49,439 @@ class Admin::UsersControllerTest < ActionDispatch::IntegrationTest assert_match(/No subscription/, response.body, "Page should show 'No subscription' for families without one") end + test "index renders auth type pills for local and sso users" do + solo_family = Family.create!(name: "SSO Test Family") + sso_email = "unique-sso-user-#{SecureRandom.hex(4)}@example.com" + sso_user = User.create!( + family: solo_family, + email: sso_email, + first_name: "SSO", + last_name: "User", + skip_password_validation: true, + role: :member + ) + OidcIdentity.create!( + user: sso_user, + provider: "google", + uid: "google-12345" + ) + + get admin_users_url + assert_response :success + + # Locate each user's row and verify auth-type pills are scoped correctly + doc = Nokogiri::HTML(response.body) + sso_row = doc.at_css("tr:has(p:contains('#{sso_email}'))") + assert sso_row, "Expected a table row for SSO user #{sso_email}" + assert_match(/SSO/, sso_row.text) + assert_match(/SSO Provider: /, sso_row.text) + + local_email = users(:family_admin).email + local_row = doc.at_css("tr:has(p:contains('#{local_email}'))") + assert local_row, "Expected a table row for local user #{local_email}" + assert_match(/Local/, local_row.text) + end + + test "index exposes delete and family controls for super admins" do + Family.create!(name: "Unused Family") + + get admin_users_url + assert_response :success + + assert_match(/Delete User/, response.body) + assert_match(/New family\/group name/, response.body) + assert_match(/Unused families \/ groups/, response.body) + assert_match(/Delete unused family/, response.body) + end + + + + test "update can move a user to an existing family" do + target = users(:family_member) + destination_family = Family.create!(name: "New Admin Family") + destination_family.update!(default_account_sharing: "shared") + destination_member = User.create!( + family: destination_family, + email: "destination-member@example.com", + first_name: "Destination", + last_name: "Member", + password: "password", + role: :member + ) + shared_account = Account.create!(family: target.family, owner: users(:family_admin), name: "Shared", balance: 50, currency: "USD", accountable: Depository.new) + shared_account.share_with!(target, permission: "read_only") + + patch admin_user_url(target), params: { + user: { + role: "member", + family_id: destination_family.id, + new_family_name: "" + } + } + + assert_redirected_to admin_users_url + target.reload + + assert_equal destination_family, target.family + assert_equal "member", target.role + assert_equal 0, AccountShare.where(user: target).count + assert_equal 0, AccountShare.where(account: shared_account).where(user: target).count + end + + test "update can create a new family and move the user into it" do + target = users(:family_member) + + assert_difference("Family.count", 1) do + patch admin_user_url(target), params: { + user: { + role: "admin", + family_id: "", + new_family_name: "New Support Group", + new_family_moniker: "Group" + } + } + end + + assert_redirected_to admin_users_url + target.reload + + assert_equal "New Support Group", target.family.name + assert_equal "Group", target.family.moniker + assert_equal "admin", target.role + end + + test "update ignores whitespace-only new family name when existing family is selected" do + target = users(:family_member) + destination_family = Family.create!(name: "Destination Family") + + assert_no_difference("Family.count") do + patch admin_user_url(target), params: { + user: { + role: "member", + family_id: destination_family.id, + new_family_name: " ", + new_family_moniker: "Group" + } + } + end + + assert_redirected_to admin_users_url + target.reload + + assert_equal destination_family, target.family + assert_equal "member", target.role + end + + test "update rejects blank new family selection" do + target = users(:family_member) + original_family = target.family + + assert_no_difference("Family.count") do + patch admin_user_url(target), params: { + user: { + role: "member", + family_id: "new", + new_family_name: " ", + new_family_moniker: "Group" + } + } + end + + assert_redirected_to admin_users_url + assert_equal I18n.t("admin.users.update.family_required"), flash[:alert] + assert_equal original_family, target.reload.family + end + + test "update rolls back a new family when transfer validation fails" do + target = users(:family_member) + original_family = target.family + + assert_no_difference("Family.count") do + patch admin_user_url(target), params: { + user: { + role: "not-a-real-role", + family_id: "", + new_family_name: "Rollback Family", + new_family_moniker: "Group" + } + } + end + + assert_match(/Role is not included in the list/, flash[:alert]) + target.reload + + assert_equal original_family, target.family + assert_equal "member", target.role + end + + test "update shows failure when selected family does not exist" do + target = users(:family_member) + missing_family_id = SecureRandom.uuid + + missing_family_id = SecureRandom.uuid while Family.exists?(id: missing_family_id) + + patch admin_user_url(target), params: { + user: { + role: "member", + family_id: missing_family_id, + new_family_name: "" + } + } + + assert_redirected_to admin_users_url + assert_equal I18n.t("admin.users.update.failure"), flash[:alert] + end + + + + test "update allows super admin to change their own family" do + current_admin = users(:sure_support_staff) + new_family = Family.create!(name: "Self Move Family") + + patch admin_user_url(current_admin), params: { + user: { + role: "super_admin", + family_id: new_family.id + } + } + + assert_redirected_to admin_users_url + assert_equal new_family, current_admin.reload.family + end + + test "update prevents demoting the last super admin in the system" do + User.where(role: :super_admin).where.not(id: users(:sure_support_staff).id).update_all(role: :member) + current_admin = users(:sure_support_staff) + + patch admin_user_url(current_admin), params: { + user: { + role: "member", + family_id: current_admin.family_id + } + } + + assert_redirected_to admin_users_url + assert_match(/cannot demote the last super admin/i, flash[:alert]) + assert_equal "super_admin", current_admin.reload.role + end + + test "update can set a new password for a local user" do + target = users(:family_member) + assert target.has_local_password?, "Precondition: target must have a local password" + + new_password = "Secure1!pass" + patch admin_user_url(target), params: { + user: { + role: target.role, + password: new_password + } + } + + assert_redirected_to admin_users_url + assert_equal I18n.t("admin.users.update.success_password"), flash[:notice] + target.reload + assert target.authenticate(new_password), "User should authenticate with the new password" + end + + test "update shows descriptive notification for role change only" do + target = users(:family_member) + + patch admin_user_url(target), params: { + user: { + role: "admin", + password: "" + } + } + + assert_redirected_to admin_users_url + assert_equal I18n.t("admin.users.update.success_role"), flash[:notice] + end + + test "update shows descriptive notification for role and password change" do + target = users(:family_member) + assert target.has_local_password?, "Precondition: target must have a local password" + + patch admin_user_url(target), params: { + user: { + role: "admin", + password: "Secure1!pass" + } + } + + assert_redirected_to admin_users_url + assert_equal I18n.t("admin.users.update.success_role_and_password"), flash[:notice] + target.reload + assert_equal "admin", target.role + assert target.authenticate("Secure1!pass") + end + + test "update shows descriptive notification for family change" do + target = users(:family_member) + destination_family = Family.create!(name: "Notify Family") + + patch admin_user_url(target), params: { + user: { + role: target.role, + family_id: destination_family.id + } + } + + assert_redirected_to admin_users_url + assert_equal I18n.t("admin.users.update.success_family"), flash[:notice] + end + + test "update with blank password leaves existing password unchanged" do + target = users(:family_member) + old_digest = target.password_digest + + patch admin_user_url(target), params: { + user: { + role: target.role, + password: "" + } + } + + assert_redirected_to admin_users_url + assert_equal old_digest, target.reload.password_digest, "Password digest should not change when blank password is submitted" + end + + test "update with short password shows too short error" do + target = users(:family_member) + old_digest = target.password_digest + + patch admin_user_url(target), params: { + user: { role: target.role, password: "Aa1!xy" } + } + + assert_redirected_to admin_users_url + assert_match(/at least 8 characters/i, flash[:alert]) + assert_equal old_digest, target.reload.password_digest + end + + test "update with password missing uppercase or lowercase shows case error" do + target = users(:family_member) + old_digest = target.password_digest + + patch admin_user_url(target), params: { + user: { role: target.role, password: "alllower1!" } + } + + assert_redirected_to admin_users_url + assert_match(/uppercase and lowercase/i, flash[:alert]) + assert_equal old_digest, target.reload.password_digest + end + + test "update with password missing number shows number error" do + target = users(:family_member) + old_digest = target.password_digest + + patch admin_user_url(target), params: { + user: { role: target.role, password: "NoNumber!!" } + } + + assert_redirected_to admin_users_url + assert_match(/at least one number/i, flash[:alert]) + assert_equal old_digest, target.reload.password_digest + end + + test "update with password missing special character shows special char error" do + target = users(:family_member) + old_digest = target.password_digest + + patch admin_user_url(target), params: { + user: { role: target.role, password: "NoSpecial1a" } + } + + assert_redirected_to admin_users_url + assert_match(/special character/i, flash[:alert]) + assert_equal old_digest, target.reload.password_digest + end + + test "update with password failing multiple criteria shows all errors" do + target = users(:family_member) + old_digest = target.password_digest + + patch admin_user_url(target), params: { + user: { role: target.role, password: "short" } + } + + assert_redirected_to admin_users_url + assert_match(/at least 8 characters/i, flash[:alert]) + assert_match(/uppercase and lowercase/i, flash[:alert]) + assert_match(/at least one number/i, flash[:alert]) + assert_match(/special character/i, flash[:alert]) + assert_equal old_digest, target.reload.password_digest + end + + test "update ignores password param for SSO-only users" do + solo_family = Family.create!(name: "SSO Ignore Family") + sso_user = User.create!( + family: solo_family, + email: "sso-ignore-#{SecureRandom.hex(4)}@example.com", + first_name: "SSO", + last_name: "Ignore", + skip_password_validation: true, + role: :member + ) + OidcIdentity.create!(user: sso_user, provider: "google", uid: "ignore-#{SecureRandom.hex(4)}") + assert sso_user.sso_only?, "Precondition: user must be SSO-only" + assert_nil sso_user.password_digest + + patch admin_user_url(sso_user), params: { + user: { + role: sso_user.role, + password: "attempt_to_set_password" + } + } + + assert_redirected_to admin_users_url + assert_nil sso_user.reload.password_digest, "SSO-only user should not gain a local password" + end + + test "update blocks simultaneous family and password change" do + target = users(:family_member) + assert target.has_local_password?, "Precondition: target must have a local password" + original_family = target.family + destination_family = Family.create!(name: "Conflict Family") + old_digest = target.password_digest + + patch admin_user_url(target), params: { + user: { + role: target.role, + family_id: destination_family.id, + password: "Secure1!pass" + } + } + + assert_redirected_to admin_users_url + assert_equal I18n.t("admin.users.update.password_and_family_conflict"), flash[:alert] + target.reload + assert_equal original_family, target.family, "Family should not change when conflict is detected" + assert_equal old_digest, target.password_digest, "Password should not change when conflict is detected" + end + + test "index shows set password field for local users but not for SSO-only users" do + local_user = users(:family_member) + assert local_user.has_local_password?, "Precondition: local_user must have a local password" + + solo_family = Family.create!(name: "SSO Only Family") + sso_user = User.create!( + family: solo_family, + email: "sso-only-pwd-test-#{SecureRandom.hex(4)}@example.com", + first_name: "SSO", + last_name: "Only", + skip_password_validation: true, + role: :member + ) + OidcIdentity.create!(user: sso_user, provider: "google", uid: "pwd-test-#{SecureRandom.hex(4)}") + assert sso_user.sso_only?, "Precondition: sso_user must be SSO-only" + + get admin_users_url + assert_response :success + + assert_match(/Set new password/, response.body, "Should show set password field for local users") + end + test "index shows removed SSO identities with a recovery action" do block = SsoIdentityBlock.create!( provider: "openid_connect", diff --git a/test/controllers/registrations_controller_test.rb b/test/controllers/registrations_controller_test.rb index 8f40f7f1b..440ad7553 100644 --- a/test/controllers/registrations_controller_test.rb +++ b/test/controllers/registrations_controller_test.rb @@ -29,7 +29,7 @@ class RegistrationsControllerTest < ActionDispatch::IntegrationTest test "first user of instance becomes super_admin" do # Clear all users to simulate fresh instance - User.destroy_all + User.connection.disable_referential_integrity { User.delete_all } assert_difference "User.count", +1 do post registration_url, params: { user: { diff --git a/test/models/account_test.rb b/test/models/account_test.rb index e3b94e31d..4b03520c6 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -16,6 +16,24 @@ class AccountTest < ActiveSupport::TestCase end end + test "default owner prefers a family admin before a super admin" do + family = families(:empty) + admin = users(:empty) + super_admin = users(:sure_support_staff) + + Current.reset + + account = family.accounts.create!( + name: "Unowned test account", + balance: 0, + currency: "USD", + accountable: Depository.new + ) + + assert_equal admin, account.owner + assert_not_equal super_admin, account.owner + end + test "create_and_sync calls sync_later by default" do Account.any_instance.expects(:sync_later).once diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 0daaf5143..28a37b876 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -681,6 +681,66 @@ class UserTest < ActiveSupport::TestCase assert_nil @user.default_account_for_transactions end + test "transfer_to_family! clears a shared default account" do + user = users(:family_member) + user.update!(role: "admin", default_account: accounts(:depository)) + + new_family = Family.create!(name: "Transferred Family") + + user.transfer_to_family!(new_family, role: "admin") + + user.reload + + assert_equal new_family, user.family + assert_nil user.default_account_id + assert_nil user.default_account_for_transactions + end + + test "transfer_to_family! moves owned account provider items and statements" do + user = users(:family_member) + source_family = user.family + new_family = Family.create!(name: "Transferred Provider Family") + account = Account.create!(family: source_family, owner: user, name: "Synced Checking", balance: 100, currency: "USD", accountable: Depository.new) + plaid_item = PlaidItem.create!(family: source_family, plaid_id: "item_transfer_#{SecureRandom.hex(4)}", access_token: "token", name: "Transfer Bank") + plaid_account = PlaidAccount.create!(plaid_item: plaid_item, plaid_id: "acct_transfer_#{SecureRandom.hex(4)}", name: "Transfer Checking", plaid_type: "depository", currency: "USD", current_balance: 100) + AccountProvider.create!(account: account, provider: plaid_account) + statement = AccountStatement.create_from_upload!( + family: source_family, + account: account, + file: uploaded_file(filename: "transfer-statement.csv", content_type: "text/csv", content: "date,amount\n2026-01-01,10\n") + ) + + user.transfer_to_family!(new_family, role: "admin") + + assert_equal new_family, user.reload.family + assert_equal new_family, account.reload.family + assert_equal new_family, plaid_item.reload.family + assert_equal new_family, statement.reload.family + end + + test "transfer_to_family! rejects provider items linked to accounts outside the transfer" do + user = users(:family_member) + other_user = users(:family_admin) + source_family = user.family + new_family = Family.create!(name: "Rejected Provider Family") + moved_account = Account.create!(family: source_family, owner: user, name: "Moved Synced", balance: 100, currency: "USD", accountable: Depository.new) + remaining_account = Account.create!(family: source_family, owner: other_user, name: "Remaining Synced", balance: 200, currency: "USD", accountable: Depository.new) + plaid_item = PlaidItem.create!(family: source_family, plaid_id: "item_reject_#{SecureRandom.hex(4)}", access_token: "token", name: "Shared Bank") + moved_plaid_account = PlaidAccount.create!(plaid_item: plaid_item, plaid_id: "acct_reject_moved_#{SecureRandom.hex(4)}", name: "Moved Checking", plaid_type: "depository", currency: "USD", current_balance: 100) + remaining_plaid_account = PlaidAccount.create!(plaid_item: plaid_item, plaid_id: "acct_reject_remaining_#{SecureRandom.hex(4)}", name: "Remaining Checking", plaid_type: "depository", currency: "USD", current_balance: 200) + AccountProvider.create!(account: moved_account, provider: moved_plaid_account) + AccountProvider.create!(account: remaining_account, provider: remaining_plaid_account) + + error = assert_raises(ActiveRecord::RecordInvalid) do + user.transfer_to_family!(new_family, role: "admin") + end + + assert_includes error.record.errors[:base], I18n.t("activerecord.errors.models.user.attributes.base.provider_item_has_other_accounts") + assert_equal source_family, user.reload.family + assert_equal source_family, moved_account.reload.family + assert_equal source_family, plaid_item.reload.family + 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) @@ -739,7 +799,7 @@ class UserTest < ActiveSupport::TestCase # First user role assignment tests test "role_for_new_family_creator returns super_admin when no users exist" do # Delete all users to simulate fresh instance - User.destroy_all + User.connection.disable_referential_integrity { User.delete_all } assert_equal :super_admin, User.role_for_new_family_creator end @@ -865,4 +925,24 @@ class UserTest < ActiveSupport::TestCase assert_not Family.exists?(family.id) assert_not ActiveStorage::Attachment.exists?(export_attachment_id) end + + test "cannot demote the last super admin in the system" do + User.where(role: :super_admin).update_all(role: :member) + solo_super_admin = users(:sure_support_staff) + solo_super_admin.update!(role: :super_admin) + + solo_super_admin.role = :member + assert_not solo_super_admin.valid? + assert_includes solo_super_admin.errors[:role], "Cannot demote the last super admin in the system." + end + + test "can demote super admin if another super admin exists" do + admin1 = users(:family_admin) + admin1.update!(role: :super_admin) + + admin2 = users(:sure_support_staff) + admin2.update!(role: :super_admin) + + assert admin1.update(role: :member) + end end diff --git a/test/policies/family_policy_test.rb b/test/policies/family_policy_test.rb new file mode 100644 index 000000000..e2a81099b --- /dev/null +++ b/test/policies/family_policy_test.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require "test_helper" + +class FamilyPolicyTest < ActiveSupport::TestCase + def setup + @super_admin = users(:sure_support_staff) + + @regular_user = users(:family_member) + @regular_user.update!(role: :member) + + @family = families(:empty) + end + + test "super admin can destroy family" do + assert FamilyPolicy.new(@super_admin, @family).destroy? + end + + test "regular user cannot destroy family" do + assert_not FamilyPolicy.new(@regular_user, @family).destroy? + end + + test "nil user cannot destroy family" do + assert_not FamilyPolicy.new(nil, @family).destroy? + end +end diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index aecb2f8d3..0f1bca97f 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -30,8 +30,16 @@ class UserPolicyTest < ActiveSupport::TestCase assert UserPolicy.new(@super_admin, @regular_user).update? end - test "super admin cannot update themselves" do - assert_not UserPolicy.new(@super_admin, @super_admin).update? + test "super admin can update themselves" do + assert UserPolicy.new(@super_admin, @super_admin).update? + end + + test "super admin can delete another user" do + assert UserPolicy.new(@super_admin, @regular_user).destroy? + end + + test "super admin cannot delete themselves" do + assert_not UserPolicy.new(@super_admin, @super_admin).destroy? end test "regular user cannot update anyone" do diff --git a/test/system/admin_user_removals_test.rb b/test/system/admin_user_removals_test.rb index 30733bfd9..36672a6c7 100644 --- a/test/system/admin_user_removals_test.rb +++ b/test/system/admin_user_removals_test.rb @@ -19,8 +19,9 @@ class AdminUserRemovalsTest < ApplicationSystemTestCase find("details", text: @target.family.name).find("summary").click within find("tr", text: @target_email) do - click_on "Remove" + find("button[aria-haspopup='dialog']").click end + click_on "Delete User" within "dialog[open]" do assert_text "This immediately revokes access" diff --git a/test/test_helper.rb b/test/test_helper.rb index d73d3706d..969cfcce8 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -91,6 +91,7 @@ module ActiveSupport # Add more helper methods to be used by all tests here... def sign_in(user) post sessions_path, params: { email: user.email, password: user_password_test } + Current.session = user.sessions.order(:created_at).last end def ensure_tailwind_build