Merge pull request #2592 from alphagov/fix-policies-for-user-suspension

Prevent Admins from suspending/unsuspending API users

app/controllers/suspensions_controller.rb

@@ -28,7 +28,9 @@ def update
 private
 
   def load_and_authorize_user
-    @user = User.find(params[:id])
+    @user = ApiUser.find_by(id: params[:id]) || User.find_by(id: params[:id])
+    raise ActiveRecord::RecordNotFound if @user.blank?
+
     authorize @user, :suspension?
   end
 end

app/policies/api_user_policy.rb

@@ -9,4 +9,5 @@ def new?
   alias_method :revoke?, :new?
   alias_method :manage_permissions?, :new?
   alias_method :manage_tokens?, :new?
+  alias_method :suspension?, :new?
 end

app/views/api_users/edit.html.erb

@@ -12,7 +12,9 @@
   User <strong><%= @api_user.status %></strong> &bull;
   Created <%= time_ago_in_words(@api_user.created_at) %> ago &bull;
   <%= link_to "Account access log", event_logs_user_path(@api_user) %> &bull;
-  <%= link_to "#{@api_user.suspended? ? "Uns" : "S"}uspend user", edit_suspension_path(@api_user) %>
+  <% if policy(@api_user).suspension? %>
+    <%= link_to "#{@api_user.suspended? ? "Uns" : "S"}uspend user", edit_suspension_path(@api_user) %>
+  <% end %>
 </p>
 
 <% if @api_user.suspended? and @api_user.reason_for_suspension.present? %>

test/controllers/suspensions_controller_test.rb

@@ -74,6 +74,14 @@ class SuspensionsControllerTest < ActionController::TestCase
       assert_equal "Negligence", another_user.reason_for_suspension
     end
 
+    should "not be able to control suspension of an API user" do
+      api_user = create(:api_user)
+      put :update, params: { id: api_user.id, user: { suspended: "1", reason_for_suspension: "Negligence" } }
+
+      assert_not_authorised
+      assert_not api_user.reload.suspended?
+    end
+
     should "redisplay the form if the reason is blank" do
       another_user = create(:user)
       put :update, params: { id: another_user.id, user: { suspended: "1", reason_for_suspension: "" } }

test/policies/api_user_policy_test.rb

@@ -4,7 +4,7 @@
 class ApiUserPolicyTest < ActiveSupport::TestCase
   include PolicyHelpers
 
-  %i[new create index edit update revoke].each do |permission_name|
+  %i[new create index edit update revoke manage_permissions manage_tokens suspension].each do |permission_name|
     context permission_name do
       should "allow only for superadmins" do
         assert permit?(create(:superadmin_user), User, permission_name)