From dfaa140ed3b2d221d03d298040a94392fd6673b0 Mon Sep 17 00:00:00 2001
From: Brian Austin <13002992+brianjaustin@users.noreply.github.com>
Date: Fri, 29 Nov 2024 00:19:26 +0100
Subject: [PATCH] AO3-6697 Limit roles that can edit known issues (#4891)

* AO3-6697 Limit roles that can edit known issues

* Tests without weird workarounds

* Test consolidation and formatting cleanup

* Make things translatable

* Add missing tests

* Hide posts button in admin nav when unauthorised

* Fix issue from merge
---
 app/controllers/known_issues_controller.rb    |  19 +-
 app/policies/known_issue_policy.rb            |  16 ++
 app/views/admin/_admin_nav.html.erb           |  26 +--
 app/views/admin/_header.html.erb              |   4 +-
 app/views/known_issues/index.html.erb         |   2 +-
 config/locales/views/en.yml                   |   6 +
 features/admins/admin_post_issues.feature     |  39 +++-
 features/step_definitions/admin_steps.rb      |  10 +-
 .../known_issues_controller_spec.rb           | 187 ++++++++++++++++++
 9 files changed, 269 insertions(+), 40 deletions(-)
 create mode 100644 app/policies/known_issue_policy.rb
 create mode 100644 spec/controllers/known_issues_controller_spec.rb

diff --git a/app/controllers/known_issues_controller.rb b/app/controllers/known_issues_controller.rb
index 4f6bbe6cd30..793daff088e 100644
--- a/app/controllers/known_issues_controller.rb
+++ b/app/controllers/known_issues_controller.rb
@@ -1,5 +1,4 @@
 class KnownIssuesController < ApplicationController
-
   before_action :admin_only, except: [:index]
 
   # GET /known_issues
@@ -9,25 +8,24 @@ def index
 
   # GET /known_issues/1
   def show
-    @known_issue = KnownIssue.find(params[:id])
+    @known_issue = authorize KnownIssue.find(params[:id])
   end
 
   # GET /known_issues/new
   def new
-    @known_issue = KnownIssue.new
+    @known_issue = authorize KnownIssue.new
   end
 
   # GET /known_issues/1/edit
   def edit
-    @known_issue = KnownIssue.find(params[:id])
+    @known_issue = authorize KnownIssue.find(params[:id])
   end
 
   # POST /known_issues
   def create
-    @known_issue = KnownIssue.new(known_issue_params)
-
+    @known_issue = authorize KnownIssue.new(known_issue_params)
     if @known_issue.save
-      flash[:notice] = 'Known issue was successfully created.'
+      flash[:notice] = "Known issue was successfully created."
       redirect_to(@known_issue)
     else
       render action: "new"
@@ -36,10 +34,9 @@ def create
 
   # PUT /known_issues/1
   def update
-    @known_issue = KnownIssue.find(params[:id])
-
+    @known_issue = authorize KnownIssue.find(params[:id])
     if @known_issue.update(known_issue_params)
-      flash[:notice] = 'Known issue was successfully updated.'
+      flash[:notice] = "Known issue was successfully updated."
       redirect_to(@known_issue)
     else
       render action: "edit"
@@ -48,7 +45,7 @@ def update
 
   # DELETE /known_issues/1
   def destroy
-    @known_issue = KnownIssue.find(params[:id])
+    @known_issue = authorize KnownIssue.find(params[:id])
     @known_issue.destroy
     redirect_to(known_issues_path)
   end
diff --git a/app/policies/known_issue_policy.rb b/app/policies/known_issue_policy.rb
new file mode 100644
index 00000000000..a74de07e65b
--- /dev/null
+++ b/app/policies/known_issue_policy.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class KnownIssuePolicy < ApplicationPolicy
+  MANAGE_ROLES = %w[superadmin support].freeze
+
+  def admin_index?
+    user_has_roles?(MANAGE_ROLES)
+  end
+
+  alias destroy? admin_index?
+  alias edit? admin_index?
+  alias create? admin_index?
+  alias new? admin_index?
+  alias show? admin_index?
+  alias update? admin_index?
+end
diff --git a/app/views/admin/_admin_nav.html.erb b/app/views/admin/_admin_nav.html.erb
index 24f6abc0738..e705ea49bb8 100644
--- a/app/views/admin/_admin_nav.html.erb
+++ b/app/views/admin/_admin_nav.html.erb
@@ -1,12 +1,13 @@
-<h3 class="landmark heading"><%= ts("Admin Navigation") %></h3>
+<h3 class="landmark heading"><%= t(".landmark") %></h3>
 <ul class="navigation actions" role="navigation">
   <li>
-    <%= span_if_current ts("AO3 News", key: "header"), admin_posts_path %>
-   </li>
-  <li>
-    <%= span_if_current ts("Post AO3 News", key: "header"),
-                        new_admin_post_path %>
-</li>
+    <%= span_if_current t(".ao3_news"), admin_posts_path %>
+  </li>
+  <% if policy(AdminPost).can_post? %>
+    <li>
+      <%= span_if_current t(".post_ao3_news"), new_admin_post_path %>
+    </li>
+  <% end %>
   <% if params[:controller] == "admin_posts" && params[:action] == "edit" %>
     <li>
       <%= link_to t(".news.delete_post"),
@@ -18,8 +19,7 @@
   <% if params[:controller] == "archive_faqs" && params[:action] == "edit" &&
       Globalize.locale.to_s == "en" %>
     <li>
-      <%= link_to ts("Reorder Questions"),
-                  manage_archive_faq_questions_path(@archive_faq) %>
+      <%= link_to t(".faq.reorder_questions"), manage_archive_faq_questions_path(@archive_faq) %>
     </li>
   <% end %>
 
@@ -28,9 +28,11 @@
       <%= span_if_current t(".archive_faq"), archive_faqs_path %>
     </li>
   <% end %>
-  <li>
-    <%= span_if_current ts("Known Issues", key: "header"), known_issues_path %>
-  </li>
+  <% if policy(KnownIssue).admin_index? %>
+    <li>
+      <%= span_if_current t(".known_issues"), known_issues_path %>
+    </li>
+  <% end %>
   <% if policy(:wrangling).new? %>
     <li>
       <%= span_if_current t(".wrangling_guidelines"), wrangling_guidelines_path %>
diff --git a/app/views/admin/_header.html.erb b/app/views/admin/_header.html.erb
index a657ac5ef53..9c7ae29136f 100644
--- a/app/views/admin/_header.html.erb
+++ b/app/views/admin/_header.html.erb
@@ -30,7 +30,9 @@
         <% if policy(ArchiveFaq).translation_access? %>
           <li><%= link_to t(".nav.posts.faqs"), archive_faqs_path %></li>
         <% end %>
-        <li><%= link_to t(".nav.posts.known_issues"), known_issues_path %></li>
+        <% if policy(KnownIssue).admin_index? %>
+          <li><%= link_to t(".nav.posts.known_issues"), known_issues_path %></li>
+        <% end %>
         <% if policy(:wrangling).new? %>
           <li><%= link_to t(".nav.posts.wrangling_guidelines"), wrangling_guidelines_path %></li>
         <% end %>
diff --git a/app/views/known_issues/index.html.erb b/app/views/known_issues/index.html.erb
index acd5616f914..6d550cdb5e6 100644
--- a/app/views/known_issues/index.html.erb
+++ b/app/views/known_issues/index.html.erb
@@ -1,5 +1,5 @@
 <!--Descriptive page name, messages and instructions-->
-<% if logged_in_as_admin? %>
+<% if policy(KnownIssue).admin_index? %>
   <%= render :partial => "admin_index" %>
 <% else %>
   <h2 class="heading"><%= ts("Known Issues") %></h2>
diff --git a/config/locales/views/en.yml b/config/locales/views/en.yml
index 211b9ece894..98566ffcc69 100644
--- a/config/locales/views/en.yml
+++ b/config/locales/views/en.yml
@@ -92,9 +92,15 @@ en:
           requests: Manage Requests
         page_heading: Invite New Users
     admin_nav:
+      ao3_news: AO3 News
       archive_faq: Archive FAQ
+      faq:
+        reorder_questions: Reorder Questions
+      known_issues: Known Issues
+      landmark: Admin Navigation
       news:
         delete_post: Delete Post
+      post_ao3_news: Post AO3 News
       wrangling_guidelines: Wrangling Guidelines
     admin_options:
       delete:
diff --git a/features/admins/admin_post_issues.feature b/features/admins/admin_post_issues.feature
index fdfdb508fbe..cdedac3a957 100644
--- a/features/admins/admin_post_issues.feature
+++ b/features/admins/admin_post_issues.feature
@@ -3,9 +3,9 @@ Feature: Admin Actions to Post Known Issues
   As an an admin
   I want to be able to report known issues
 
-  Scenario: Post known issues
-    When I am logged in as an admin
-      And I follow "Admin Posts"
+  Scenario Outline: Authorized admin posts, edits, and deletes known issues
+    Given I am logged in as a "<role>" admin
+    When I follow "Admin Posts"
       And I follow "Known Issues" within "#header"
       And I follow "make a new known issues post"
       And I fill in "known_issue_title" with "First known problem"
@@ -18,15 +18,36 @@ Feature: Admin Actions to Post Known Issues
       And I follow "Known Issues" within "#header"
       And I follow "Show"
     Then I should see "First known problem"
-
-  Scenario: Edit known issues
-    Given I have posted known issues
     When I edit known issues
     Then I should see "Known issue was successfully updated"
       And I should not see "First known problem"
       And I should see "This is a bit of a problem, and this is too"
-
-  Scenario: Delete known issues
-    Given I have posted known issues
     When I delete known issues
     Then I should not see "First known problem"
+
+    Examples:
+    | role       |
+    | support    |
+    | superadmin |
+
+  Scenario Outline: Links to edit and create known issues are not shown to unauthorized admins
+    Given I have posted known issues
+      And I am logged in as a "<role>" admin
+    When I follow "Admin Posts"
+    Then I should not see "Known Issues" within "#header"
+    When I go to the known issues page
+    Then I should not see "Edit" within ".actions"
+
+    Examples:
+    | role                       |
+    | board                      |
+    | board_assistants_team      |
+    | communications             |
+    | development_and_membership |
+    | docs                       |
+    | elections                  |
+    | legal                      |
+    | translation                |
+    | tag_wrangling              |
+    | policy_and_abuse           |
+    | open_doors                 |
diff --git a/features/step_definitions/admin_steps.rb b/features/step_definitions/admin_steps.rb
index b0826105a44..fa8e38d8926 100644
--- a/features/step_definitions/admin_steps.rb
+++ b/features/step_definitions/admin_steps.rb
@@ -102,8 +102,8 @@
   click_button("Update")
 end
 
-Given /^I have posted known issues$/ do
-  step %{I am logged in as an admin}
+Given "I have posted known issues" do
+  step %{I am logged in as a super admin}
   step %{I follow "Admin Posts"}
   step %{I follow "Known Issues" within "#header"}
   step %{I follow "make a new known issues post"}
@@ -299,8 +299,7 @@
   Resque.enqueue(AdminSetting, :check_queue)
 end
 
-When /^I edit known issues$/ do
-  step %{I am logged in as an admin}
+When "I edit known issues" do
   step %{I follow "Admin Posts"}
   step %{I follow "Known Issues" within "#header"}
   step %{I follow "Edit"}
@@ -309,8 +308,7 @@
   step %{I press "Post"}
 end
 
-When /^I delete known issues$/ do
-  step %{I am logged in as an admin}
+When "I delete known issues" do
   step %{I follow "Admin Posts"}
   step %{I follow "Known Issues" within "#header"}
   step %{I follow "Delete"}
diff --git a/spec/controllers/known_issues_controller_spec.rb b/spec/controllers/known_issues_controller_spec.rb
new file mode 100644
index 00000000000..13b797656b9
--- /dev/null
+++ b/spec/controllers/known_issues_controller_spec.rb
@@ -0,0 +1,187 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe KnownIssuesController do
+  include LoginMacros
+  include RedirectExpectationHelper
+
+  allowed_roles = %w[superadmin support]
+
+  shared_examples "denies access to unauthorized admins" do
+    context "when logged in as an admin with no role" do
+      let(:admin) { create(:admin) }
+
+      it "redirects with an error" do
+        it_redirects_to_with_error(root_url, "Sorry, only an authorized admin can access the page you were trying to reach.")
+      end
+    end
+
+    (Admin::VALID_ROLES - allowed_roles).each do |admin_role|
+      context "when logged in as an admin with role #{admin_role}" do
+        let(:admin) { create(:admin, roles: [admin_role]) }
+
+        it "redirects with an error" do
+          it_redirects_to_with_error(root_url, "Sorry, only an authorized admin can access the page you were trying to reach.")
+        end
+      end
+    end
+  end
+
+  describe "GET #show" do
+    let(:known_issue) { create(:known_issue) }
+
+    it_behaves_like "denies access to unauthorized admins" do
+      before do
+        fake_login_admin(admin)
+        get :show, params: { id: known_issue.id }
+      end
+    end
+
+    allowed_roles.each do |admin_role|
+      context "when logged in as an admin with role #{admin_role}" do
+        let(:admin) { create(:admin, roles: [admin_role]) }
+
+        before do
+          fake_login_admin(admin)
+        end
+
+        it "allows access" do
+          get :show, params: { id: known_issue.id }
+          expect(response).to have_http_status(:success)
+        end
+      end
+    end
+  end
+
+  describe "GET #new" do
+    it_behaves_like "denies access to unauthorized admins" do
+      before do
+        fake_login_admin(admin)
+        get :new
+      end
+    end
+
+    allowed_roles.each do |admin_role|
+      context "when logged in as an admin with role #{admin_role}" do
+        let(:admin) { create(:admin, roles: [admin_role]) }
+
+        before do
+          fake_login_admin(admin)
+        end
+
+        it "allows access" do
+          get :new
+          expect(response).to have_http_status(:success)
+        end
+      end
+    end
+  end
+
+  describe "GET #edit" do
+    let(:known_issue) { create(:known_issue) }
+
+    it_behaves_like "denies access to unauthorized admins" do
+      before do
+        fake_login_admin(admin)
+        get :edit, params: { id: known_issue.id }
+      end
+    end
+
+    allowed_roles.each do |admin_role|
+      context "when logged in as an admin with role #{admin_role}" do
+        let(:admin) { create(:admin, roles: [admin_role]) }
+
+        before do
+          fake_login_admin(admin)
+        end
+
+        it "allows access" do
+          get :edit, params: { id: known_issue.id }
+          expect(response).to have_http_status(:success)
+        end
+      end
+    end
+  end
+
+  describe "POST #create" do
+    let(:params) { { known_issue: attributes_for(:known_issue) } }
+
+    it_behaves_like "denies access to unauthorized admins" do
+      before do
+        fake_login_admin(admin)
+        post :create, params: params
+      end
+    end
+
+    allowed_roles.each do |admin_role|
+      context "when logged in as an admin with role #{admin_role}" do
+        let(:admin) { create(:admin, roles: [admin_role]) }
+
+        before do
+          fake_login_admin(admin)
+        end
+
+        it "creates a known issue" do
+          expect { post :create, params: params }
+            .to change { KnownIssue.count }
+            .by(1)
+        end
+      end
+    end
+  end
+
+  describe "PUT #update" do
+    let(:known_issue) { create(:known_issue) }
+    let(:params) { { id: known_issue.id, known_issue: { title: "Brand new title" } } }
+
+    it_behaves_like "denies access to unauthorized admins" do
+      before do
+        fake_login_admin(admin)
+        put :update, params: params
+      end
+    end
+
+    allowed_roles.each do |admin_role|
+      context "when logged in as an admin with role #{admin_role}" do
+        let(:admin) { create(:admin, roles: [admin_role]) }
+
+        before do
+          fake_login_admin(admin)
+        end
+
+        it "updates the known issue successfully" do
+          put :update, params: params
+          expect(known_issue.reload.title).to eq("Brand new title")
+        end
+      end
+    end
+  end
+
+  describe "DELETE #destroy" do
+    let(:known_issue) { create(:known_issue) }
+
+    it_behaves_like "denies access to unauthorized admins" do
+      before do
+        fake_login_admin(admin)
+        delete :destroy, params: { id: known_issue.id }
+      end
+    end
+
+    allowed_roles.each do |admin_role|
+      context "when logged in as an admin with role #{admin_role}" do
+        let(:admin) { create(:admin, roles: [admin_role]) }
+
+        before do
+          fake_login_admin(admin)
+        end
+
+        it "deletes the known issue" do
+          delete :destroy, params: { id: known_issue.id }
+          expect { known_issue.reload }
+            .to raise_exception(ActiveRecord::RecordNotFound)
+        end
+      end
+    end
+  end
+end