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 54dca6b3b..2eb87913f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -48,10 +48,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..ebfec287a 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?