mirror of
https://github.com/we-promise/sure.git
synced 2026-09-05 23:01:26 +00:00
feat: Super Admins can Delete Users and Modify Families/Groups (#2868)
* Add admin family management features and tests
- Implement FamiliesController with destroy action to delete unused families.
- Add localization for success and error messages related to family deletion.
- Create FamiliesControllerTest to ensure proper functionality of family deletion.
- Update UserPolicyTest to include permissions for super admins to delete users.
- Enhance UsersControllerTest with tests for user family management, including moving users between families and creating new families.
* feat(users): enhance user management with family transfer validation and improved delete warnings
* Simplify user management actions column and combine family options
Move heavy user edit forms from table rows into a DS::Popover action
menu, add role badges to the user column, combine family migration and
creation inputs with a Stimulus controller, enable self-family
transfer for super admins, and add safety guards against demoting the
last super admin in the system.
* feat: add authentication type pills to admin user index to display SSO and local login status
* Add set password feature for local users in admin user management
- Add password field in action popover for users with local password login
- Enforce all registration password criteria (min 8 chars, mixed case, digit, special char)
- Block simultaneous family and password updates with clear error
- Show descriptive success notifications (role, password, both, family)
- Ignore password param for SSO-only users
- Add comprehensive tests for all password validation paths
* Resolve DS Drift Patrol findings and CI scan failures
- Wrap auth-type pills in DS::Tooltip instead of native title= attribute
- Add actions.manage_user key to locale and drop redundant default: fallbacks
- Fix RuboCop style offenses in Admin::UsersController
- Update Brakeman ignore entry fingerprint for Admin::UsersController#user_params
* fix: update badge query to target DS::Pill structure
* Fix DS::Tooltip misuse hiding SSO auth-type pill in admin users view
The SSO pill was passed as a block to DS::Tooltip, which caused it to
render inside the hidden div[role="tooltip"] instead of being visible.
The text: option ("SSO Provider: ...") was also silently ignored because
tooltip_content returns content (the block) over @text when a block is
given.
Fix: render the SSO/Local+SSO pill directly as visible content and pass
DS::Tooltip with no block so text: is used as the tooltip popup. An info
icon now appears next to the pill and shows the provider name on hover.
Fixes test: Admin::UsersControllerTest#test_index_renders_auth_type_pills_for_local_and_sso_users
* Remove redundant default: fallback from role pill i18n lookup
All admin.users.index.roles.{guest,member,admin,super_admin} keys are
defined in the locale file and used elsewhere in the same view without
a default:. The fallback was redundant for every valid role and would
silently mask a missing or renamed key instead of raising in
development.
Drop the default: user.role.humanize argument so that any future
missing key surfaces immediately as I18n::MissingTranslationData.
* Revert unrelated JS/schema/split churn; fix transfer_to_family! default role
- Revert 62 JS files (Biome formatter and unrelated controller changes)
- Revert db/schema.rb dump churn (no new migrations in this branch)
- Revert unrelated split transaction view changes (edit/new.html.erb)
- Fix User#transfer_to_family! role default: role: role evaluates to nil
when omitted; use explicit self.role to read model attribute
Keeps the PR focused on user/family management (~18-20 files).
* Fix last login and session count in admin user management
Store last_login_at and sessions_count directly on the users table
so they remain accurate after a user logs out.
- Add migration to add last_login_at (datetime) and sessions_count
(integer, default 0) columns to users, with backfill from sessions
- Add counter_cache: :sessions_count to Session#belongs_to :user so
the count auto-increments/decrements on session create/destroy
- Add after_create callback on Session to stamp user.last_login_at
- Update Admin::UsersController to read both values from users table
instead of aggregating Session rows (which disappear on logout)
* Fix user management PR pending CI items
* Keep test current session after sign in
* Address PR review comments for user transfers
* refactor: update user removal label to "Delete User" and standardize component attribute naming
* Address PR Review Feedback for User Management
* test: Fix families and users controller tests for user management PR
* Limit PR 2868 schema diff
* Fix PR 2868 user management CI failures
---------
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: sure-admin <sure-admin@splashblot.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
This commit is contained in:
co-authored by
sure-admin
Juan José Mata
parent
99e0994309
commit
bb835b9793
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user