diff --git a/app/controllers/user_name_change_requests_controller.rb b/app/controllers/user_name_change_requests_controller.rb index c1646f66a..e93278492 100644 --- a/app/controllers/user_name_change_requests_controller.rb +++ b/app/controllers/user_name_change_requests_controller.rb @@ -1,37 +1,25 @@ class UserNameChangeRequestsController < ApplicationController - before_action :member_only, :only => [:index, :show, :new, :create] respond_to :html, :json, :xml def new - @change_request = UserNameChangeRequest.new(change_request_params) + @change_request = authorize UserNameChangeRequest.new(permitted_attributes(UserNameChangeRequest)) respond_with(@change_request) end def create - @change_request = UserNameChangeRequest.create_with(user: CurrentUser.user, original_name: CurrentUser.name).create(change_request_params) + @change_request = authorize UserNameChangeRequest.new(user: CurrentUser.user, original_name: CurrentUser.name) + @change_request.update(permitted_attributes(@change_request)) flash[:notice] = "Your name has been changed" if @change_request.valid? respond_with(@change_request, location: profile_path) end def show - @change_request = UserNameChangeRequest.find(params[:id]) - check_privileges!(@change_request) + @change_request = authorize UserNameChangeRequest.find(params[:id]) respond_with(@change_request) end def index - @change_requests = UserNameChangeRequest.visible(CurrentUser.user).order("id desc").paginate(params[:page], :limit => params[:limit]) + @change_requests = authorize UserNameChangeRequest.visible(CurrentUser.user).order("id desc").paginate(params[:page], :limit => params[:limit]) respond_with(@change_requests) end - - private - - def check_privileges!(change_request) - return if CurrentUser.is_admin? - raise User::PrivilegeError if change_request.user_id != CurrentUser.user.id - end - - def change_request_params - params.fetch(:user_name_change_request, {}).permit(%i[desired_name desired_name_confirmation]) - end end diff --git a/app/models/user.rb b/app/models/user.rb index 7896eb15f..31203ddfd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -322,6 +322,10 @@ class User < ApplicationRecord User.level_string(value || level) end + def is_deleted? + name.match?(/\Auser_[0-9]+~*\z/) + end + def is_anonymous? level == Levels::ANONYMOUS end diff --git a/app/policies/user_name_change_request_policy.rb b/app/policies/user_name_change_request_policy.rb new file mode 100644 index 000000000..a7f38a9cf --- /dev/null +++ b/app/policies/user_name_change_request_policy.rb @@ -0,0 +1,13 @@ +class UserNameChangeRequestPolicy < ApplicationPolicy + def index? + user.is_member? + end + + def show? + user.is_admin? || (user.is_member? && !record.user.is_deleted?) || (record.user == user) + end + + def permitted_attributes + [:desired_name, :desired_name_confirmation] + end +end diff --git a/app/views/static/site_map.html.erb b/app/views/static/site_map.html.erb index 0f81103cd..bdb558770 100644 --- a/app/views/static/site_map.html.erb +++ b/app/views/static/site_map.html.erb @@ -121,7 +121,7 @@ <% else %>
  • <%= link_to "Profile", profile_path %>
  • <%= link_to "Settings", settings_path %>
  • - <% if CurrentUser.is_gold? %> + <% if policy(UserNameChangeRequest).create? %>
  • <%= link_to "Change name", new_user_name_change_request_path %>
  • <% end %>
  • <%= link_to "Delete account", maintenance_user_deletion_path %>
  • @@ -150,7 +150,7 @@
  • <%= link_to("Jobs", delayed_jobs_path) %>
  • <%= link_to("Bulk Update Requests", bulk_update_requests_path) %>
  • - <% if CurrentUser.is_member? %> + <% if policy(UserNameChangeRequest).index? %>
  • <%= link_to("User Name Change Requests", user_name_change_requests_path) %>
  • <% end %> diff --git a/test/functional/user_name_change_requests_controller_test.rb b/test/functional/user_name_change_requests_controller_test.rb index 553217b05..b9276d24b 100644 --- a/test/functional/user_name_change_requests_controller_test.rb +++ b/test/functional/user_name_change_requests_controller_test.rb @@ -26,6 +26,7 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest context "show action" do setup do @change_request = as(@user) { create(:user_name_change_request, user_id: @user.id) } + @user.update!(name: "user_#{@user.id}") end should "render" do @@ -33,7 +34,7 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest assert_response :success end - context "when the current user is not an admin and does not own the request" do + context "when the current user is not an admin, doesn't own the request, and the other user is deleted" do should "fail" do @another_user = create(:user) get_auth user_name_change_request_path(@change_request), @another_user @@ -42,15 +43,20 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest end end - context "for actions restricted to admins" do - context "index action" do - should "render" do - post_auth user_name_change_requests_path, @user, params: { user_name_change_request: { desired_name: "zun", desired_name_confirmation: "zun" }} - get user_name_change_requests_path + context "index action" do + should "allows members to see name changes" do + create(:user_name_change_request) + get_auth user_name_change_requests_path, @user - assert_response :success - assert_select "table tbody tr", 1 - end + assert_response :success + assert_select "table tbody tr", 1 + end + + should "not allow anonymous users to see name changes" do + create(:user_name_change_request) + get user_name_change_requests_path + + assert_response 403 end end end