From 565a6572a791923b4bc9996ac8d3816b2678e4b8 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 17 Mar 2020 05:17:57 -0500 Subject: [PATCH] pundit: convert user name change requests to pundit. Fix discrepancy between index action and show action. The index action allowed members to see name changes for undeleted users, but the show action didn't. --- .../user_name_change_requests_controller.rb | 22 ++++------------- app/models/user.rb | 4 ++++ .../user_name_change_request_policy.rb | 13 ++++++++++ app/views/static/site_map.html.erb | 4 ++-- ...er_name_change_requests_controller_test.rb | 24 ++++++++++++------- 5 files changed, 39 insertions(+), 28 deletions(-) create mode 100644 app/policies/user_name_change_request_policy.rb 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