From 4e1edc92254170d540603edf3cce821b542171cb Mon Sep 17 00:00:00 2001
From: Dhurba baral <dhurba87@gmail.com>
Date: Wed, 21 Jun 2023 21:07:25 +0545
Subject: [PATCH] allows super_admin user to remove super_admin role (#19590)

* Skips scheduled posts in featured post count

* Allows removing super_admin role

* adds namespace resolution op

* Adds pundit policy

* Adds rspec for role policy

* Fixes test cases and minor refactoring

* Removes dead code

---------

Co-authored-by: Lawrence <lawrence@forem.com>
---
 app/controllers/admin/users_controller.rb     |  8 +++---
 app/models/role.rb                            |  6 +++++
 app/policies/role_policy.rb                   | 11 ++++++++
 app/services/users/remove_role.rb             |  8 ------
 .../admin/users/show/overview/_roles.html.erb | 11 +++++---
 .../show/overview/_tag_moderation.html.erb    |  2 +-
 config/locales/services/en.yml                |  1 -
 config/locales/services/fr.yml                |  1 -
 config/locales/views/admin/en.yml             |  1 +
 config/locales/views/admin/fr.yml             |  1 +
 .../adminFlows/users/manageRoles.spec.js      | 23 -----------------
 spec/factories/roles.rb                       |  5 ++++
 spec/models/role_spec.rb                      | 21 ++++++++++++++++
 spec/policies/role_policy_spec.rb             | 25 +++++++++++++++++++
 spec/requests/admin/users_manage_spec.rb      | 25 ++++++-------------
 spec/services/users/remove_role_spec.rb       | 13 ----------
 16 files changed, 92 insertions(+), 70 deletions(-)
 create mode 100644 app/policies/role_policy.rb
 create mode 100644 spec/factories/roles.rb
 create mode 100644 spec/policies/role_policy_spec.rb

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 3139647e0..2bcb2c7b9 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -90,19 +90,21 @@ module Admin
     end
 
     def destroy
-      role = params[:role].to_sym
+      role = Role.find(params[:role_id])
+      authorize(role, :remove_role?)
+
       resource_type = params[:resource_type]
 
       @user = User.find(params[:user_id])
 
       response = ::Users::RemoveRole.call(user: @user,
-                                          role: role,
+                                          role: role.name,
                                           resource_type: resource_type,
                                           admin: current_user)
       if response.success
         flash[:success] =
           I18n.t("admin.users_controller.role_removed",
-                 role: role.to_s.humanize.titlecase) # TODO: [@yheuhtozr] need better role i18n
+                 role: role.name.to_s.humanize.titlecase) # TODO: [@yheuhtozr] need better role i18n
       else
         flash[:danger] = response.error_message
       end
diff --git a/app/models/role.rb b/app/models/role.rb
index 75aed3465..7e0729098 100644
--- a/app/models/role.rb
+++ b/app/models/role.rb
@@ -19,6 +19,12 @@ class Role < ApplicationRecord
     workshop_pass
   ].freeze
 
+  ROLES.each do |role|
+    define_method("#{role}?") do
+      name == role
+    end
+  end
+
   has_and_belongs_to_many :users, join_table: :users_roles # rubocop:disable Rails/HasAndBelongsToMany
 
   belongs_to :resource,
diff --git a/app/policies/role_policy.rb b/app/policies/role_policy.rb
new file mode 100644
index 000000000..e68cb7ab8
--- /dev/null
+++ b/app/policies/role_policy.rb
@@ -0,0 +1,11 @@
+class RolePolicy < ApplicationPolicy
+  def remove_role?
+    return false if record.suspended?
+
+    if user.super_admin?
+      true
+    else
+      user.admin? && !record.super_admin?
+    end
+  end
+end
diff --git a/app/services/users/remove_role.rb b/app/services/users/remove_role.rb
index 8eb1a1a90..f7f2c85f7 100644
--- a/app/services/users/remove_role.rb
+++ b/app/services/users/remove_role.rb
@@ -15,7 +15,6 @@ module Users
     end
 
     def call
-      return response if super_admin_role?(role)
       return response if user_current_user?(user)
 
       if resource_type && user.remove_role(role, resource_type)
@@ -33,13 +32,6 @@ module Users
 
     attr_reader :user, :role, :resource_type, :admin, :response
 
-    def super_admin_role?(role)
-      return false if role != :super_admin
-
-      response.error_message = I18n.t("services.users.remove_role.remove_super")
-      true
-    end
-
     def user_current_user?(user)
       return false if user.id != admin.id
 
diff --git a/app/views/admin/users/show/overview/_roles.html.erb b/app/views/admin/users/show/overview/_roles.html.erb
index a43a82a4f..3791e711d 100644
--- a/app/views/admin/users/show/overview/_roles.html.erb
+++ b/app/views/admin/users/show/overview/_roles.html.erb
@@ -17,9 +17,9 @@
   </div>
 <% else %>
   <ul class="flex flex-wrap gap-2">
-    <% @user.roles.reject { |role| role.name == "tag_moderator" }.each do |role| %>
+    <% @user.roles.reject(&:tag_moderator?).each do |role| %>
       <li>
-        <% if role.name == "banned" || role.name == "suspended" || role.name == "super_admin" || @user.id == current_user.id %>
+        <% if !policy(role).remove_role? || @user.id == current_user.id %>
           <button
             class="c-pill c-pill--description-icon crayons-tooltip__activator cursor-help"
             type="button"
@@ -29,7 +29,12 @@
             <span data-testid="tooltip" class="crayons-tooltip__content"><%= t("views.admin.users.overview.roles.locked") %></span>
           </button>
         <% else %>
-          <%= button_to url_for(action: :destroy, user_id: @user.id, role: role.name.to_sym, resource_type: role.resource_type), method: :delete, data: { confirm: t("views.admin.users.overview.roles.remove_confirm") }, class: "c-pill c-pill--action-icon" do %>
+          <% confirm_dialog = if role.super_admin?
+                                t("views.admin.users.overview.roles.remove_confirm_super_admin")
+                              else
+                                t("views.admin.users.overview.roles.remove_confirm")
+                              end %>
+          <%= button_to url_for(action: :destroy, user_id: @user.id, role_id: role.id, resource_type: role.resource_type), method: :delete, data: { confirm: confirm_dialog }, class: "c-pill c-pill--action-icon" do %>
             <span class="screen-reader-only"><%= t("views.admin.users.overview.roles.remove") %></span>
               <%= t("views.admin.users.overview.roles.name.#{role.name}", default: role.resource_name ? role.resource_name.to_s : role_display_name(role)) %>
             <%= crayons_icon_tag(:x, class: "c-pill__action-icon", aria_hidden: true, width: 18, height: 18) %>
diff --git a/app/views/admin/users/show/overview/_tag_moderation.html.erb b/app/views/admin/users/show/overview/_tag_moderation.html.erb
index 0518d322b..d18de3bba 100644
--- a/app/views/admin/users/show/overview/_tag_moderation.html.erb
+++ b/app/views/admin/users/show/overview/_tag_moderation.html.erb
@@ -9,7 +9,7 @@
   </p>
 </header>
 
-<% moderated_tags = @user.roles.select { |role| role.name == "tag_moderator" } %>
+<% moderated_tags = @user.roles.select(&:tag_moderator?) %>
 
 <% if moderated_tags.any? %>
   <ul class="flex flex-wrap gap-2 mb-4">
diff --git a/config/locales/services/en.yml b/config/locales/services/en.yml
index a6bd57a39..1075daf99 100644
--- a/config/locales/services/en.yml
+++ b/config/locales/services/en.yml
@@ -110,5 +110,4 @@ en:
     users:
       remove_role:
         remove_self: Admins cannot remove roles from themselves.
-        remove_super: Super Admin roles cannot be removed.
         error: There was an issue removing this role. %{e_message}
diff --git a/config/locales/services/fr.yml b/config/locales/services/fr.yml
index 87312440c..ed402510e 100644
--- a/config/locales/services/fr.yml
+++ b/config/locales/services/fr.yml
@@ -108,5 +108,4 @@ fr:
     users:
       remove_role:
         remove_self: Les administrateurs ne peuvent pas se retirer des rôles.
-        remove_super: Les rôles de Super Admin ne peuvent pas être supprimés.
         error: Un problème est survenu lors de la suppression de ce rôle. %{e_message}
diff --git a/config/locales/views/admin/en.yml b/config/locales/views/admin/en.yml
index 0f07cb324..b403e7cad 100644
--- a/config/locales/views/admin/en.yml
+++ b/config/locales/views/admin/en.yml
@@ -265,6 +265,7 @@ en:
             locked: You can't remove this role.
             remove: "Remove role:"
             remove_confirm: Are you sure?
+            remove_confirm_super_admin: You are removing super admin access from this account. Do this carefully and always ensure you retain access to one or more super admin accounts
             name:
               admin: Admin
               codeland_admin: Codeland Admin
diff --git a/config/locales/views/admin/fr.yml b/config/locales/views/admin/fr.yml
index 24fbf5319..2e518b115 100644
--- a/config/locales/views/admin/fr.yml
+++ b/config/locales/views/admin/fr.yml
@@ -265,6 +265,7 @@ fr:
             locked: You can't remove this role.
             remove: "Remove role:"
             remove_confirm: Are you sure?
+            remove_confirm_super_admin: You are removing super admin access from this account. Do this carefully and always ensure you retain access to one or more super admin accounts
             name:
               admin: Admin
               codeland_admin: Codeland Admin
diff --git a/cypress/e2e/seededFlows/adminFlows/users/manageRoles.spec.js b/cypress/e2e/seededFlows/adminFlows/users/manageRoles.spec.js
index 9068e837d..4983372b7 100644
--- a/cypress/e2e/seededFlows/adminFlows/users/manageRoles.spec.js
+++ b/cypress/e2e/seededFlows/adminFlows/users/manageRoles.spec.js
@@ -97,29 +97,6 @@ describe('Manage User Roles', () => {
         );
         checkUserStatus('Good standing');
       });
-
-      it('should not remove the Super Admin role', () => {
-        checkUserStatus('Trusted');
-
-        openRolesModal().within(() => {
-          cy.findByRole('combobox', { name: 'Role' }).select('Super Admin');
-          cy.findByRole('textbox', { name: 'Add a note to this action:' }).type(
-            'some reason',
-          );
-          cy.findByRole('button', { name: 'Add' }).click();
-        });
-
-        cy.findByRole('button', {
-          name: `Super Admin You can't remove this role.`,
-        })
-          .as('superAdminButton')
-          .click()
-          .within(() => {
-            cy.findByText(`You can't remove this role.`).should('exist');
-          });
-
-        cy.get('@superAdminButton').should('exist');
-      });
     });
 
     describe('Adding Roles', () => {
diff --git a/spec/factories/roles.rb b/spec/factories/roles.rb
new file mode 100644
index 000000000..637705415
--- /dev/null
+++ b/spec/factories/roles.rb
@@ -0,0 +1,5 @@
+FactoryBot.define do
+  factory :role do
+    name { "admin" }
+  end
+end
diff --git a/spec/models/role_spec.rb b/spec/models/role_spec.rb
index f3dc0a51d..ad98ad64d 100644
--- a/spec/models/role_spec.rb
+++ b/spec/models/role_spec.rb
@@ -15,4 +15,25 @@ RSpec.describe Role do
       expect(described_class::ROLES).to match_array(expected_roles)
     end
   end
+
+  describe "#super_admin?" do
+    let(:role) { create(:role, name: role_name) }
+    let(:role_name) { "super_admin" }
+
+    it "has a visible method" do
+      expect(role.respond_to?(:super_admin?)).to be true
+    end
+
+    it "returns true" do
+      expect(role.super_admin?).to be true
+    end
+
+    context "with different role" do
+      let(:role_name) { "admin" }
+
+      it "returns false" do
+        expect(role.super_admin?).to be false
+      end
+    end
+  end
 end
diff --git a/spec/policies/role_policy_spec.rb b/spec/policies/role_policy_spec.rb
new file mode 100644
index 000000000..6c625315d
--- /dev/null
+++ b/spec/policies/role_policy_spec.rb
@@ -0,0 +1,25 @@
+require "rails_helper"
+
+describe RolePolicy do
+  subject(:role) { described_class }
+
+  let(:role_suspended) { build(:role, name: "suspended") }
+  let(:role_trusted) { build(:role, name: "trusted") }
+  let(:role_super_admin) { build(:role, name: "super_admin") }
+  let(:super_admin_user) { build(:user, :super_admin) }
+  let(:admin_user) { build(:user, :admin) }
+
+  permissions :remove_role? do
+    it "denies access if current role is suspended" do
+      expect(role).not_to permit(super_admin_user, role_suspended)
+    end
+
+    it "grants access if user is super admin" do
+      expect(role).to permit(super_admin_user, role_trusted)
+    end
+
+    it "denies access if user is admin and role is super_admin" do
+      expect(role).not_to permit(admin_user, role_super_admin)
+    end
+  end
+end
diff --git a/spec/requests/admin/users_manage_spec.rb b/spec/requests/admin/users_manage_spec.rb
index 1dccb5da7..232b68db8 100644
--- a/spec/requests/admin/users_manage_spec.rb
+++ b/spec/requests/admin/users_manage_spec.rb
@@ -1,6 +1,7 @@
 require "rails_helper"
 
 RSpec.describe "Admin::Users" do
+  # rubocop:disable RSpec/IndexedLet
   let!(:user) { create(:user, twitter_username: nil, old_username: "username") }
   let!(:user2) { create(:user, twitter_username: "Twitter") }
   let(:user3) { create(:user) }
@@ -174,10 +175,10 @@ RSpec.describe "Admin::Users" do
     end
 
     it "removes non-admin roles from non-super_admin users", :aggregate_failures do
-      user.add_role(:trusted)
+      role = user.add_role(:trusted)
 
       expect do
-        delete admin_user_path(user.id), params: { user_id: user.id, role: :trusted }
+        delete admin_user_path(user.id), params: { user_id: user.id, role_id: role.id }
       end.to change(user.roles, :count).by(-1)
 
       expect(user.has_trusted_role?).to be false
@@ -185,12 +186,12 @@ RSpec.describe "Admin::Users" do
     end
 
     it "removes the correct resource_admin_role from non-super_admin users", :aggregate_failures do
-      user.add_role(:single_resource_admin, Comment)
+      role = user.add_role(:single_resource_admin, Comment)
       user.add_role(:single_resource_admin, Broadcast)
 
       expect do
         delete admin_user_path(user.id),
-               params: { user_id: user.id, role: :single_resource_admin, resource_type: Comment }
+               params: { user_id: user.id, role_id: role.id, resource_type: Comment }
       end.to change(user.roles, :count).by(-1)
 
       expect(user.single_resource_admin_for?(Comment)).to be false
@@ -198,22 +199,11 @@ RSpec.describe "Admin::Users" do
       expect(request.flash["success"]).to include("successfully removed from the user!")
     end
 
-    it "does not allow super_admin roles to be removed", :aggregate_failures do
-      user.add_role(:super_admin)
-
-      expect do
-        delete admin_user_path(user.id), params: { user_id: user.id, role: :super_admin }
-      end.not_to change(user.roles, :count)
-
-      expect(user.super_admin?).to be true
-      expect(request.flash["danger"]).to include("cannot be removed.")
-    end
-
     it "does not allow a admins to remove a role from themselves", :aggregate_failures do
-      super_admin.add_role(:trusted)
+      role = super_admin.add_role(:trusted)
 
       expect do
-        delete admin_user_path(super_admin.id), params: { user_id: super_admin.id, role: :trusted }
+        delete admin_user_path(super_admin.id), params: { user_id: super_admin.id, role_id: role.id }
       end.not_to change(super_admin.roles, :count)
 
       expect(super_admin.trusted?).to be true
@@ -308,4 +298,5 @@ RSpec.describe "Admin::Users" do
       expect(super_admin.reload.unspent_credits_count).to eq 5
     end
   end
+  # rubocop:enable RSpec/IndexedLet
 end
diff --git a/spec/services/users/remove_role_spec.rb b/spec/services/users/remove_role_spec.rb
index 71a95206e..3a383cbbe 100644
--- a/spec/services/users/remove_role_spec.rb
+++ b/spec/services/users/remove_role_spec.rb
@@ -3,19 +3,6 @@ require "rails_helper"
 RSpec.describe Users::RemoveRole, type: :service do
   let(:current_user) { create(:user, :admin) }
 
-  context "when user is a super_admin" do
-    it "does not remove super_admin roles and raises an error", :aggregate_failures do
-      super_admin = create(:user, :super_admin)
-      role = super_admin.roles.first.name.to_sym
-      resource_type = nil
-      args = { user: super_admin, role: role, resource_type: resource_type, admin: current_user }
-      role_removal = described_class.call(**args)
-
-      expect(role_removal.success).to be false
-      expect(role_removal.error_message).to eq "Super Admin roles cannot be removed."
-    end
-  end
-
   context "when current_user" do
     it "does not remove roles and raises an error", :aggregate_failures do
       role = current_user.roles.first