From cfe91c25c9a3861421f559227518b4e9a8b8ea5e Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Tue, 1 Apr 2025 17:04:13 +0200 Subject: [PATCH 1/4] Add admin role, make sure only admins can remove and promote other users --- app/controllers/accounts/users_controller.rb | 19 ++++++--- app/models/user.rb | 4 -- app/models/user/role.rb | 6 ++- app/views/accounts/users/_user.html.erb | 13 +++--- .../accounts/users_controller_test.rb | 41 +++++++++++++++++++ test/fixtures/users.yml | 2 +- test/models/user/role_test.rb | 10 +++++ test/test_helpers/session_test_helper.rb | 1 + 8 files changed, 79 insertions(+), 17 deletions(-) create mode 100644 test/controllers/accounts/users_controller_test.rb create mode 100644 test/models/user/role_test.rb diff --git a/app/controllers/accounts/users_controller.rb b/app/controllers/accounts/users_controller.rb index 3763b4c21..7243677de 100644 --- a/app/controllers/accounts/users_controller.rb +++ b/app/controllers/accounts/users_controller.rb @@ -1,14 +1,19 @@ class Accounts::UsersController < ApplicationController - before_action :set_user, only: %i[ destroy ] - before_action :ensure_permission_to_remove, only: :destroy + before_action :set_user, only: %i[ update destroy ] + before_action :ensure_permission_to_administer_user, only: %i[ update destroy ] def index @users = Current.account.users.active end + def update + @user.update(role_params) + redirect_to account_users_path + end + def destroy @user.deactivate - redirect_to users_path + redirect_to account_users_path end private @@ -16,7 +21,11 @@ class Accounts::UsersController < ApplicationController @user = Current.account.users.active.find(params[:id]) end - def ensure_permission_to_remove - head :forbidden unless Current.user.can_remove?(@user) + def ensure_permission_to_administer_user + head :forbidden unless Current.user.can_administer?(@user) + end + + def role_params + { role: params.require(:user)[:role].presence_in(%w[ member admin ]) || "member" } end end diff --git a/app/models/user.rb b/app/models/user.rb index 57a8e636c..54edb7c56 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -49,10 +49,6 @@ class User < ApplicationRecord Current.user == self end - def can_remove?(other) - other != self - end - private def deactived_email_address email_address.sub(/@/, "-deactivated-#{SecureRandom.uuid}@") diff --git a/app/models/user/role.rb b/app/models/user/role.rb index 101f285d2..625361037 100644 --- a/app/models/user/role.rb +++ b/app/models/user/role.rb @@ -2,7 +2,7 @@ module User::Role extend ActiveSupport::Concern included do - enum :role, %i[ member system ].index_by(&:itself), scopes: false + enum :role, %i[ admin member system ].index_by(&:itself), scopes: false scope :member, -> { where(role: :member) } scope :without_system, -> { where.not(role: :system) } @@ -16,4 +16,8 @@ module User::Role end end end + + def can_administer?(other) + admin? && other != self + end end diff --git a/app/views/accounts/users/_user.html.erb b/app/views/accounts/users/_user.html.erb index a66de7d15..9498a1d42 100644 --- a/app/views/accounts/users/_user.html.erb +++ b/app/views/accounts/users/_user.html.erb @@ -7,15 +7,16 @@ - + <%= form_with model: user, url: account_user_path(user), data: { controller: "form" }, method: :patch do | form | %> + + <% end %> <%# FIXME: Move this Current.user check to a stimulus controller that just checks for admin? or the like we so we can cache user list %> <%= button_to account_user_path(user), method: :delete, class: "btn btn--small btn--negative flex-item-no-shrink", - disabled: !Current.user.can_remove?(user), + disabled: !Current.user.can_administer?(user), data: { turbo_confirm: "Are you sure you want to permanently remove this person from the account?" } do %> <%= icon_tag "minus" %> Remove <%= user.name %> from the account diff --git a/test/controllers/accounts/users_controller_test.rb b/test/controllers/accounts/users_controller_test.rb new file mode 100644 index 000000000..dcda0ba99 --- /dev/null +++ b/test/controllers/accounts/users_controller_test.rb @@ -0,0 +1,41 @@ +require "test_helper" + +class Accounts::UsersControllerTest < ActionDispatch::IntegrationTest + setup do + sign_in_as :kevin + end + + test "update" do + assert_not users(:david).admin? + + put account_user_url(users(:david)), params: { user: { role: "admin" } } + + assert_redirected_to account_users_path + assert users(:david).reload.admin? + end + + test "can't promote to special roles" do + assert_no_changes -> { users(:david).reload.role } do + put account_user_url(users(:david)), params: { user: { role: "system" } } + end + end + + test "destroy" do + assert_difference -> { User.active.count }, -1 do + delete account_user_url(users(:david)) + end + + assert_redirected_to account_users_path + assert_nil User.active.find_by(id: users(:david).id) + end + + test "non-admins cannot perform actions" do + sign_in_as :jz + + put account_user_url(users(:david)), params: { user: { role: "admin" } } + assert_response :forbidden + + delete account_user_url(users(:david)) + assert_response :forbidden + end +end diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index d81fd8333..8c15709e1 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -19,4 +19,4 @@ kevin: name: Kevin email_address: kevin@37signals.com password_digest: <%= digest %> - role: member + role: admin diff --git a/test/models/user/role_test.rb b/test/models/user/role_test.rb new file mode 100644 index 000000000..4f3ad0177 --- /dev/null +++ b/test/models/user/role_test.rb @@ -0,0 +1,10 @@ +require "test_helper" + +class User::RoleTest < ActiveSupport::TestCase + test "can administer others?" do + assert users(:kevin).can_administer?(users(:jz)) + + assert_not users(:kevin).can_administer?(users(:kevin)) + assert_not users(:jz).can_administer?(users(:kevin)) + end +end diff --git a/test/test_helpers/session_test_helper.rb b/test/test_helpers/session_test_helper.rb index 5666c5b28..35e29d71f 100644 --- a/test/test_helpers/session_test_helper.rb +++ b/test/test_helpers/session_test_helper.rb @@ -4,6 +4,7 @@ module SessionTestHelper end def sign_in_as(user) + cookies[:session_token] = nil user = users(user) unless user.is_a? User post session_url, params: { email_address: user.email_address, password: "secret123456" } assert cookies[:session_token].present? From e07bd966f564408c4c8d3d34ddd35c57f98f9ada Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Tue, 1 Apr 2025 17:05:58 +0200 Subject: [PATCH 2/4] Enable morphing so that we get smooth page refreshes when redirecting back --- app/views/layouts/application.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 57f76180a..ee46e2cfc 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -11,6 +11,7 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> <%= tag.meta name: "current-user-id", content: Current.user.id if Current.user %> + <% turbo_refreshes_with method: :morph, scroll: :preserve %> <% turbo_refreshes_with method: :morph, scroll: :preserve %> From 84c4003b670d6222a5b600b738ae157b85d860e5 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Tue, 1 Apr 2025 18:47:37 +0200 Subject: [PATCH 3/4] Remove morphing since it's not compatible with House --- app/views/layouts/application.html.erb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index ee46e2cfc..57f76180a 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -11,7 +11,6 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> <%= tag.meta name: "current-user-id", content: Current.user.id if Current.user %> - <% turbo_refreshes_with method: :morph, scroll: :preserve %> <% turbo_refreshes_with method: :morph, scroll: :preserve %> From c02262867ed4c766697e96fcde0f2b40b58ed730 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 3 Apr 2025 17:45:11 +0200 Subject: [PATCH 4/4] Fix merge issue --- app/views/accounts/users/_user.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/accounts/users/_user.html.erb b/app/views/accounts/users/_user.html.erb index 9498a1d42..ebfec287a 100644 --- a/app/views/accounts/users/_user.html.erb +++ b/app/views/accounts/users/_user.html.erb @@ -9,7 +9,7 @@ <%= form_with model: user, url: account_user_path(user), data: { controller: "form" }, method: :patch do | form | %> <% end %>