From fff3a6d9a34b91d22a8994fff3ab9231e6811627 Mon Sep 17 00:00:00 2001 From: nisha-shaikh Date: Wed, 14 Aug 2024 13:25:20 +0000 Subject: [PATCH] AO3-6754 Restrict access to API tokens (#4871) * AO3-6754 Added authorization and policy on controller to ensure only superadmin can access /api paths * AO3-6754 Only show Manage API Tokens in header for super admins * AO3-6754 Remove Manage from API Tokens heading * AO3-6754 Rename browser titles for api tokens * AO3-6754 Add authorization for POST api token endpoints * AO3-6754 Add newline to end of file * AO3-6754 Normalize I18n en files * AO3-6754 override page_subtitle instead of page_title * AO3-6754 authorize ApiKey in one line * AO3-6754 use index policy to show/hide API Tokens link * AO3-6754 use better naming for note key in form * AO3-6754 normalize i18n file --- app/controllers/admin/api_controller.rb | 13 +- app/policies/api_key_policy.rb | 14 + app/views/admin/_header.html.erb | 4 +- app/views/admin/api/index.html.erb | 26 +- config/locales/views/en.yml | 27 +- spec/controllers/admin/api_controller_spec.rb | 481 +++++++++++++----- 6 files changed, 412 insertions(+), 153 deletions(-) create mode 100644 app/policies/api_key_policy.rb diff --git a/app/controllers/admin/api_controller.rb b/app/controllers/admin/api_controller.rb index fa24a7b09c3..e6efb99333a 100644 --- a/app/controllers/admin/api_controller.rb +++ b/app/controllers/admin/api_controller.rb @@ -2,12 +2,14 @@ class Admin::ApiController < Admin::BaseController before_action :check_for_cancel, only: [:create, :update] def index + @page_subtitle = t(".page_title") @api_keys = if params[:query] sql_query = "%" + params[:query] + "%" ApiKey.where("name LIKE ?", sql_query).order("name").paginate(page: params[:page]) else ApiKey.order("name").paginate(page: params[:page]) end + authorize @api_keys end def show @@ -15,10 +17,12 @@ def show end def new - @api_key = ApiKey.new + @page_subtitle = t(".page_title") + @api_key = authorize ApiKey.new end def create + authorize ApiKey # Use provided api key params if available otherwise fallback to empty # ApiKey object @api_key = params[:api_key].nil? ? ApiKey.new : ApiKey.new(api_key_params) @@ -31,11 +35,12 @@ def create end def edit - @api_key = ApiKey.find(params[:id]) + @page_subtitle = t(".page_title") + @api_key = authorize ApiKey.find(params[:id]) end def update - @api_key = ApiKey.find(params[:id]) + @api_key = authorize ApiKey.find(params[:id]) if @api_key.update(api_key_params) flash[:notice] = ts("Access token was successfully updated") redirect_to action: "index" @@ -45,7 +50,7 @@ def update end def destroy - @api_key = ApiKey.find(params[:id]) + @api_key = authorize ApiKey.find(params[:id]) @api_key.destroy redirect_to(admin_api_path) end diff --git a/app/policies/api_key_policy.rb b/app/policies/api_key_policy.rb new file mode 100644 index 00000000000..52640c39719 --- /dev/null +++ b/app/policies/api_key_policy.rb @@ -0,0 +1,14 @@ +class ApiKeyPolicy < ApplicationPolicy + PERMITTED_ROLES = %w[superadmin].freeze + + def index? + user_has_roles?(PERMITTED_ROLES) + end + + alias show? index? + alias new? index? + alias edit? index? + alias create? index? + alias update? index? + alias destroy? index? +end diff --git a/app/views/admin/_header.html.erb b/app/views/admin/_header.html.erb index e2e16dca705..419db932728 100644 --- a/app/views/admin/_header.html.erb +++ b/app/views/admin/_header.html.erb @@ -68,6 +68,8 @@
  • <%= link_to t(".nav.activities"), admin_activities_path %>
    • <% end %> -
  • <%= link_to t(".nav.api_tokens"), admin_api_index_path %>
    • + <% if policy(ApiKey).index? %> +
  • <%= link_to t(".nav.api_tokens"), admin_api_index_path %>
    • + <% end %> diff --git a/app/views/admin/api/index.html.erb b/app/views/admin/api/index.html.erb index 74c8182c191..3e1f7e661a2 100644 --- a/app/views/admin/api/index.html.erb +++ b/app/views/admin/api/index.html.erb @@ -1,5 +1,5 @@
    -

    <%= ts("API Tokens") %>

    +

    <%= t(".page_heading") %>

    <%= render "navigation" %> @@ -9,25 +9,25 @@ <%= form_tag url_for(controller: "admin/api", action: "index"), method: :get, class: "search", role: "search" do %> -

    <%= ts("Search for an API token by name") %>

    +

    <%= t(".search_by_name") %>

    -
    <%= label_tag "query", ts("Name") %>
    +
    <%= label_tag "query", t(".search_box.label") %>
    <%= text_field_tag "query", params[:query] %>
    -

    <%= submit_tag ts("Find") %>

    +

    <%= submit_tag t(".actions.find") %>

    <% end %> - "> - +
    <%= ts("API Tokens") %>
    "> + - - - - - - + + + + + + @@ -40,7 +40,7 @@ diff --git a/config/locales/views/en.yml b/config/locales/views/en.yml index 3c71d374377..94402cbee30 100644 --- a/config/locales/views/en.yml +++ b/config/locales/views/en.yml @@ -178,6 +178,31 @@ en: required: Required when adding or removing a warning or suspension to an account. submit: Update heading: Record Warnings, Suspensions, or Notes + api: + edit: + page_title: Edit API Token + index: + actions: + find: Find + page_heading: API Tokens + page_title: API Tokens + search_box: + label: Name + search_by_name: Search for an API token by name + table: + actions: + edit: Edit + caption: API Tokens + headings: + actions: Actions + banned: Banned? + created: Created + name: Name + token: Token + updated: Updated + summary: Existing API tokens along with the dates they were created and updated and options for editing them. + new: + page_title: New API Token banners: index: actions: @@ -218,7 +243,7 @@ en: header: nav: activities: Activities - api_tokens: Manage API Tokens + api_tokens: API Tokens banned_emails: Banned Emails banners: Banners invitations: diff --git a/spec/controllers/admin/api_controller_spec.rb b/spec/controllers/admin/api_controller_spec.rb index 5701f9b881b..ae29f9c5470 100644 --- a/spec/controllers/admin/api_controller_spec.rb +++ b/spec/controllers/admin/api_controller_spec.rb @@ -29,38 +29,72 @@ end context "where admin is logged in" do - render_views - let(:admin) { FactoryBot.create(:admin) } + context "when admin does not have correct authorization" do + context "when admin has no role" do + let(:admin) { create(:admin, roles: []) } - before do - fake_login_admin(admin) - end + before do + fake_login_admin(admin) + end - let(:api_key_prefixes) { %w(a b c) } - let!(:api_keys) do - api_key_prefixes.each do |p| - FactoryBot.create(:api_key, name: "#{p}_key") + it "redirects with error" do + get :index, params: params + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end + + (Admin::VALID_ROLES - %w[superadmin]).each do |role| + context "when admin has #{role} role" do + let(:admin) { create(:admin, roles: [role]) } + + before do + fake_login_admin(admin) + end + + it "redirects with error" do + get :index, params: params + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end end end - context "where no query params are set" do - it "returns a successful response with all api keys" do - get :index, params: params - expect(response).to have_http_status(:success) + context "when admin is authorized with the superadmin role" do + render_views + let(:admin) { create(:admin, roles: ["superadmin"]) } + + before do + fake_login_admin(admin) + end + + let(:api_key_prefixes) { %w(a b c) } + let!(:api_keys) do api_key_prefixes.each do |p| - expect(response.body).to include("#{p}_key") + FactoryBot.create(:api_key, name: "#{p}_key") end end - end - - context "where query params are set" do - let(:params) { { query: "a_key" } } - it "returns a successful response with a filtered list of api keys" do - get :index, params: params - expect(response).to have_http_status(:success) - expect(response.body).to include("a_key") - expect(response.body).to_not include("b_key") - expect(response.body).to_not include("c_key") + + context "where no query params are set" do + it "returns a successful response with all api keys" do + get :index, params: params + expect(response).to have_http_status(:success) + api_key_prefixes.each do |p| + expect(response.body).to include("#{p}_key") + end + end + end + + context "where query params are set" do + let(:params) { { query: "a_key" } } + it "returns a successful response with a filtered list of api keys" do + get :index, params: params + expect(response).to have_http_status(:success) + expect(response.body).to include("a_key") + expect(response.body).to_not include("b_key") + expect(response.body).to_not include("c_key") + end end end end @@ -68,70 +102,138 @@ describe "GET #new" do context "where an admin is logged in" do - let(:admin) { FactoryBot.create(:admin) } + context "when admin does not have correct authorization" do + context "when admin has no role" do + let(:admin) { create(:admin, roles: []) } - before do - fake_login_admin(admin) + before do + fake_login_admin(admin) + end + + it "redirects with error" do + get :new + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end + + (Admin::VALID_ROLES - %w[superadmin]).each do |role| + context "when admin has #{role} role" do + let(:admin) { create(:admin, roles: [role]) } + + before do + fake_login_admin(admin) + end + + it "redirects with error" do + get :new + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end + end end - it "responds with the new api key form" do - get :new - expect(response).to have_http_status(:success) - assert_template :new + + context "when admin is authorized with the superadmin role" do + let(:admin) { create(:admin, roles: ["superadmin"]) } + + before do + fake_login_admin(admin) + end + it "responds with the new api key form" do + get :new + expect(response).to have_http_status(:success) + assert_template :new + end end end end describe "POST #create" do context "where an admin is logged in" do - let(:admin) { FactoryBot.create(:admin) } - let(:params) { {} } + context "when admin does not have correct authorization" do + context "when admin has no role" do + let(:admin) { create(:admin, roles: []) } - before do - fake_login_admin(admin) - end - - context "with an api key param" do - let(:api_key_name) { "api_key" } - let(:api_key_params) { { name: api_key_name } } - let(:params) { { api_key: api_key_params } } + before do + fake_login_admin(admin) + end - it "redirects to the homepage and notifies of the success" do - post :create, params: params - expect(ApiKey.where(name: api_key_name)).to_not be_empty - it_redirects_to_with_notice(admin_api_index_path, "New token successfully created") + it "redirects with error" do + post :create + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end end - end - - context "without an api key param" do - it "shows the new api view" do - post :create, params: params - expect(response).to have_http_status(:success) - assert_template :new + + (Admin::VALID_ROLES - %w[superadmin]).each do |role| + context "when admin has #{role} role" do + let(:admin) { create(:admin, roles: [role]) } + + before do + fake_login_admin(admin) + end + + it "redirects with error" do + post :create + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end end end - context "the save was unsuccessful" do - let(:api_key_name) { "api_key" } - let(:api_key_params) { { name: api_key_name } } - let(:params) { { api_key: api_key_params } } + context "when admin is authorized with the superadmin role" do + let(:admin) { create(:admin, roles: ["superadmin"]) } + let(:params) { {} } before do - allow_any_instance_of(ApiKey).to receive(:save).and_return(false) + fake_login_admin(admin) end - it "shows the new api view" do - post :create, params: params - expect(ApiKey.where(name: api_key_name)).to be_empty - assert_template :new + context "with an api key param" do + let(:api_key_name) { "api_key" } + let(:api_key_params) { { name: api_key_name } } + let(:params) { { api_key: api_key_params } } + + it "redirects to the homepage and notifies of the success" do + post :create, params: params + expect(ApiKey.where(name: api_key_name)).to_not be_empty + it_redirects_to_with_notice(admin_api_index_path, "New token successfully created") + end + end + + context "without an api key param" do + it "shows the new api view" do + post :create, params: params + expect(response).to have_http_status(:success) + assert_template :new + end end - end - context "cancel_button is present" do - let(:params) { { cancel_button: "Cancel" } } + context "the save was unsuccessful" do + let(:api_key_name) { "api_key" } + let(:api_key_params) { { name: api_key_name } } + let(:params) { { api_key: api_key_params } } - it "redirects to index" do - post :create, params: params - it_redirects_to admin_api_index_path + before do + allow_any_instance_of(ApiKey).to receive(:save).and_return(false) + end + + it "shows the new api view" do + post :create, params: params + expect(ApiKey.where(name: api_key_name)).to be_empty + assert_template :new + end + end + + context "cancel_button is present" do + let(:params) { { cancel_button: "Cancel" } } + + it "redirects to index" do + post :create, params: params + it_redirects_to admin_api_index_path + end end end end @@ -139,27 +241,64 @@ describe "GET #edit" do context "where an admin is logged in" do - let(:admin) { FactoryBot.create(:admin) } + context "when admin does not have correct authorization" do + let(:api_key_id) { 123 } + let!(:api_key) { FactoryBot.create(:api_key, id: api_key_id) } - before do - fake_login_admin(admin) - end + context "when admin has no role" do + let(:admin) { create(:admin, roles: []) } - context "where the api key exists" do - render_views - let!(:api_key) { FactoryBot.create(:api_key, name: "api_key") } + before do + fake_login_admin(admin) + end - it "populates the form with the api key" do - get :edit, params: { id: api_key.id } - expect(response).to have_http_status(:success) - expect(response.body).to include(api_key.name) + it "redirects with error" do + get :edit, params: { id: api_key_id } + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end + + (Admin::VALID_ROLES - %w[superadmin]).each do |role| + context "when admin has #{role} role" do + let(:admin) { create(:admin, roles: [role]) } + + before do + fake_login_admin(admin) + end + + it "redirects with error" do + get :edit, params: { id: api_key_id } + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end end end - context "where the api key can't be found" do - it "raises an error" do - assert_raises ActiveRecord::RecordNotFound do - get :edit, params: { id: 123 } + context "when admin is authorized with the superadmin role" do + let(:admin) { create(:admin, roles: ["superadmin"]) } + + before do + fake_login_admin(admin) + end + + context "where the api key exists" do + render_views + let!(:api_key) { FactoryBot.create(:api_key, name: "api_key") } + + it "populates the form with the api key" do + get :edit, params: { id: api_key.id } + expect(response).to have_http_status(:success) + expect(response.body).to include(api_key.name) + end + end + + context "where the api key can't be found" do + it "raises an error" do + assert_raises ActiveRecord::RecordNotFound do + get :edit, params: { id: 123 } + end end end end @@ -168,59 +307,96 @@ describe "POST #update" do context "where an admin is logged in" do - let(:admin) { FactoryBot.create(:admin) } + context "when admin does not have correct authorization" do + let(:api_key_id) { 123 } + let!(:api_key) { FactoryBot.create(:api_key, id: api_key_id) } + + context "when admin has no role" do + let(:admin) { create(:admin, roles: []) } - before do - fake_login_admin(admin) + before do + fake_login_admin(admin) + end + + it "redirects with error" do + post :update, params: { id: api_key_id, api_key: { name: "new_name" } } + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end + + (Admin::VALID_ROLES - %w[superadmin]).each do |role| + context "when admin has #{role} role" do + let(:admin) { create(:admin, roles: [role]) } + + before do + fake_login_admin(admin) + end + + it "redirects with error" do + post :update, params: { id: api_key_id, api_key: { name: "new_name" } } + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end + end end - context "where the api key exists" do - let(:api_key) { FactoryBot.create(:api_key) } - let(:new_name) { "new_name" } - let(:params) do - { - id: api_key.id, - api_key: { name: new_name } - } + context "when admin is authorized with the superadmin role" do + let(:admin) { create(:admin, roles: ["superadmin"]) } + + before do + fake_login_admin(admin) end - context "where the update is successful" do - it "updates the api key and redirects to index" do - expect(ApiKey.where(name: new_name)).to be_empty - post :update, params: params - expect(ApiKey.where(name: new_name)).to_not be_empty - it_redirects_to_with_notice(admin_api_index_path, "Access token was successfully updated") + context "where the api key exists" do + let(:api_key) { FactoryBot.create(:api_key) } + let(:new_name) { "new_name" } + let(:params) do + { + id: api_key.id, + api_key: { name: new_name } + } end - end - context "where the update is unsuccessful" do - before do - allow_any_instance_of(ApiKey).to receive(:update).and_return(false) + context "where the update is successful" do + it "updates the api key and redirects to index" do + expect(ApiKey.where(name: new_name)).to be_empty + post :update, params: params + expect(ApiKey.where(name: new_name)).to_not be_empty + it_redirects_to_with_notice(admin_api_index_path, "Access token was successfully updated") + end end - it "shows the edit view" do - post :update, params: { id: api_key.id, api_key: { name: "new_name" } } - expect(ApiKey.where(name: "new_name")).to be_empty - assert_template :edit + context "where the update is unsuccessful" do + before do + allow_any_instance_of(ApiKey).to receive(:update).and_return(false) + end + + it "shows the edit view" do + post :update, params: { id: api_key.id, api_key: { name: "new_name" } } + expect(ApiKey.where(name: "new_name")).to be_empty + assert_template :edit + end end end - end - context "where the api key doesn't exist" do - it "raises an error" do - assert_raises ActiveRecord::RecordNotFound do - post :update, params: { id: 123, api_key: { name: "new_name" } } + context "where the api key doesn't exist" do + it "raises an error" do + assert_raises ActiveRecord::RecordNotFound do + post :update, params: { id: 123, api_key: { name: "new_name" } } + end end end - end - context "cancel_button is true" do - let(:api_key_id) { 123 } - let!(:api_key) { FactoryBot.create(:api_key, id: api_key_id) } + context "cancel_button is true" do + let(:api_key_id) { 123 } + let!(:api_key) { FactoryBot.create(:api_key, id: api_key_id) } - it "redirects to index" do - post :update, params: { id: api_key_id, cancel_button: "Cancel" } - it_redirects_to admin_api_index_path + it "redirects to index" do + post :update, params: { id: api_key_id, cancel_button: "Cancel" } + it_redirects_to admin_api_index_path + end end end end @@ -228,27 +404,64 @@ describe "POST #destroy" do context "where an admin is logged in" do - let(:admin) { FactoryBot.create(:admin) } - - before do - fake_login_admin(admin) - end - - context "where the api key exists" do + context "when admin does not have correct authorization" do let(:api_key_id) { 123 } let!(:api_key) { FactoryBot.create(:api_key, id: api_key_id) } - it "destroys the api key, then redirects to edit" do - post :destroy, params: { id: api_key_id } - expect(ApiKey.where(id: api_key_id)).to be_empty - expect(response).to redirect_to admin_api_path + context "when admin has no role" do + let(:admin) { create(:admin, roles: []) } + + before do + fake_login_admin(admin) + end + + it "redirects with error" do + post :destroy, params: { id: api_key_id } + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end + + (Admin::VALID_ROLES - %w[superadmin]).each do |role| + context "when admin has #{role} role" do + let(:admin) { create(:admin, roles: [role]) } + + before do + fake_login_admin(admin) + end + + it "redirects with error" do + post :destroy, params: { id: api_key_id } + + it_redirects_to_with_error(root_path, "Sorry, only an authorized admin can access the page you were trying to reach.") + end + end end end - context "where the api key doesn't exist" do - it "raises an error" do - assert_raises ActiveRecord::RecordNotFound do - post :destroy, params: { id: 123 } + context "when admin is authorized with the superadmin role" do + let(:admin) { create(:admin, roles: ["superadmin"]) } + + before do + fake_login_admin(admin) + end + + context "where the api key exists" do + let(:api_key_id) { 123 } + let!(:api_key) { FactoryBot.create(:api_key, id: api_key_id) } + + it "destroys the api key, then redirects to edit" do + post :destroy, params: { id: api_key_id } + expect(ApiKey.where(id: api_key_id)).to be_empty + expect(response).to redirect_to admin_api_path + end + end + + context "where the api key doesn't exist" do + it "raises an error" do + assert_raises ActiveRecord::RecordNotFound do + post :destroy, params: { id: 123 } + end end end end
    <%= t(".table.caption") %>
    <%= ts("Name") %><%= ts("Token") %><%= ts("Banned?") %><%= ts("Created") %><%= ts("Updated") %><%= ts("Actions") %><%= t(".table.headings.name") %><%= t(".table.headings.token") %><%= t(".table.headings.banned") %><%= t(".table.headings.created") %><%= t(".table.headings.updated") %><%= t(".table.headings.actions") %>
    <%= api_key.updated_at %>
      -
    • <%= link_to ts("Edit"), edit_admin_api_path(api_key) %>
      • +
    • <%= link_to t(".table.actions.edit"), edit_admin_api_path(api_key) %>