From 71230223c7671b93f40b0a67204e0ea87f0c7434 Mon Sep 17 00:00:00 2001 From: Alberto Bellotti Date: Mon, 15 Jan 2018 17:08:07 -0500 Subject: [PATCH] Merge pull request #275 from chrisarcand/settings-crud Add Settings API for servers, zones, and regions (cherry picked from commit 3f407638b474b1ff8b2945e9a13666d1fb061201) https://bugzilla.redhat.com/show_bug.cgi?id=1535605 --- app/controllers/api/base_controller.rb | 39 +++++++ app/controllers/api/base_controller/parser.rb | 11 +- app/controllers/api/settings_controller.rb | 2 + config/api.yml | 3 + config/routes.rb | 9 ++ spec/requests/api/regions_spec.rb | 105 ++++++++++++++++++ spec/requests/servers_spec.rb | 103 +++++++++++++++++ spec/requests/zones_spec.rb | 103 +++++++++++++++++ 8 files changed, 373 insertions(+), 2 deletions(-) create mode 100644 spec/requests/servers_spec.rb create mode 100644 spec/requests/zones_spec.rb diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index bd54baeefdde..fbb10110ef6c 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -39,8 +39,47 @@ class BaseController < ActionController::API respond_to :json rescue_from_api_errors + def settings + id = @req.collection_id + type = @req.collection + klass = collection_class(@req.collection) + resource = resource_search(id, type, klass) + + case @req.method + when :patch + raise ForbiddenError, "You are not authorized to edit settings." unless super_admin? + resource.add_settings_for_resource(@req.json_body) + when :delete + raise ForbiddenError, "You are not authorized to remove settings." unless super_admin? + resource.remove_settings_path_for_resource(*@req.json_body) + head :no_content + return + end + + if super_admin? || current_user.role_allows?(:identifier => 'ops_settings') + render :json => whitelist_settings(resource.settings_for_resource.to_hash) + else + raise ForbiddenError, "You are not authorized to view settings." + end + end + private + def current_user + User.current_user + end + + def super_admin? + current_user.super_admin_user? + end + + def whitelist_settings(settings) + return settings if super_admin? + + whitelisted_categories = ApiConfig.collections[:settings][:categories] + settings.with_indifferent_access.slice(*whitelisted_categories) + end + def set_gettext_locale FastGettext.set_locale(LocaleResolver.resolve(User.current_user, headers)) end diff --git a/app/controllers/api/base_controller/parser.rb b/app/controllers/api/base_controller/parser.rb index b392badaeb97..2043c8e9ba31 100644 --- a/app/controllers/api/base_controller/parser.rb +++ b/app/controllers/api/base_controller/parser.rb @@ -22,7 +22,7 @@ def validate_api_request # Method Validation for the collection or sub-collection specified if cname && ctype mname = @req.method - unless collection_config.supports_http_method?(cname, mname) || mname == :options + unless collection_config.supports_http_method?(cname, mname) || ignore_http_method_validation? raise BadRequestError, "Unsupported HTTP Method #{mname} for the #{ctype} #{cname} specified" end end @@ -37,7 +37,7 @@ def validate_optional_collection_classes end def validate_api_action - return unless @req.collection + return if @req.collection.blank? || ignore_http_method_validation? send("validate_#{@req.method}_method") end @@ -173,6 +173,12 @@ def parse_fetch_tenant(data) private + def ignore_http_method_validation? + settings_request = @req.subcollection == 'settings' && collection_option?(:settings) + + settings_request || @req.method == :options + end + # # For Posts we need to support actions, let's validate those # @@ -277,6 +283,7 @@ def validate_api_request_collection def validate_api_request_subcollection(cname, ctype) # Sub-Collection Validation for the specified Collection if cname && @req.subcollection + return [cname, ctype] if @req.subcollection == 'settings' && collection_option?(:settings) return [cname, ctype] if collection_option?(:arbitrary_resource_path) ctype = "Sub-Collection" unless collection_config.subcollection?(cname, @req.subcollection) diff --git a/app/controllers/api/settings_controller.rb b/app/controllers/api/settings_controller.rb index 1c2b652b2869..1bb8695d73b9 100644 --- a/app/controllers/api/settings_controller.rb +++ b/app/controllers/api/settings_controller.rb @@ -14,6 +14,8 @@ def show private def whitelist_settings(settings) + return settings if super_admin? + result_hash = {} ApiConfig.collections[:settings][:categories].each do |category_path| result_hash.deep_merge!(settings_entry_to_hash(category_path, entry_value(settings, category_path))) diff --git a/config/api.yml b/config/api.yml index 249d6902050d..ded77d4a5169 100644 --- a/config/api.yml +++ b/config/api.yml @@ -1440,6 +1440,7 @@ :identifier: region :options: - :collection + - :settings :verbs: *gp :klass: MiqRegion :collection_actions: @@ -1682,6 +1683,7 @@ :description: EVM Servers :options: - :collection + - :settings :verbs: *g :klass: MiqServer :service_catalogs: @@ -2474,6 +2476,7 @@ :identifier: zone :options: - :collection + - :settings :verbs: *gp :klass: Zone :collection_actions: diff --git a/config/routes.rb b/config/routes.rb index a07814d2ee1a..241527d3f311 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -64,6 +64,15 @@ end end end + + if collection.options.include?(:settings) + match( + "/:c_id/settings", + :to => "#{collection_name}#settings", + :via => %w[get patch delete], + :as => "#{collection_name.to_s.singularize}_settings", + ) + end end end end diff --git a/spec/requests/api/regions_spec.rb b/spec/requests/api/regions_spec.rb index 4f16817d5c01..3f0c7e51fa9d 100644 --- a/spec/requests/api/regions_spec.rb +++ b/spec/requests/api/regions_spec.rb @@ -40,4 +40,109 @@ "id" => region.id ) end + + describe "/api/regions/:id/settings" do + let(:region_number) { ApplicationRecord.my_region_number + 1 } + let(:id) { ApplicationRecord.id_in_region(1, region_number) } + let(:region) { FactoryGirl.create(:miq_region, :id => id, :region => region_number) } + let(:zone) { FactoryGirl.create(:zone, :id => id) } + let!(:server) { EvmSpecHelper.remote_miq_server(:id => id, :zone => zone) } + let(:original_timeout) { region.settings_for_resource[:api][:authentication_timeout] } + let(:super_admin) { FactoryGirl.create(:user, :role => 'super_administrator', :userid => 'alice', :password => 'alicepassword') } + + it "shows the settings to an authenticated user with the proper role" do + api_basic_authorize(:ops_settings) + + get(api_region_settings_url(nil, region)) + + expect(response).to have_http_status(:ok) + end + + it "does not allow an authenticated user who doesn't have the proper role to view the settings" do + api_basic_authorize + + get(api_region_settings_url(nil, region)) + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to view the settings" do + get(api_region_settings_url(nil, region)) + + expect(response).to have_http_status(:unauthorized) + end + + it "permits updates to settings for an authenticated super-admin user" do + api_basic_authorize(:user => super_admin.userid, :password => super_admin.password) + + expect { + patch(api_region_settings_url(nil, region), :params => {:api => {:authentication_timeout => "1337.minutes"}}) + }.to change { region.settings_for_resource[:api][:authentication_timeout] }.from(original_timeout).to("1337.minutes") + + expect(response.parsed_body).to include("api" => a_hash_including("authentication_timeout" => "1337.minutes")) + expect(response).to have_http_status(:ok) + end + + it "does not allow an authenticated non-super-admin user to update settings" do + api_basic_authorize + + expect { + patch(api_region_settings_url(nil, region), :params => {:api => {:authentication_timeout => "10.minutes"}}) + }.not_to change { region.settings_for_resource[:api][:authentication_timeout] } + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to update the settings" do + expect { + patch(api_region_settings_url(nil, region), :params => {:api => {:authentication_timeout => "10.minutes"}}) + }.not_to change { region.settings_for_resource[:api][:authentication_timeout] } + + expect(response).to have_http_status(:unauthorized) + end + + context "with an existing settings change" do + before do + region.add_settings_for_resource("api" => {"authentication_timeout" => "7331.minutes"}) + end + + it "allows an authenticated super-admin user to delete settings" do + api_basic_authorize(:user => super_admin.userid, :password => super_admin.password) + expect(region.settings_for_resource["api"]["authentication_timeout"]).to eq("7331.minutes") + + expect { + delete( + api_region_settings_url(nil, region), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.to change { region.settings_for_resource["api"]["authentication_timeout"] }.from("7331.minutes").to("30.seconds") + + expect(response).to have_http_status(:no_content) + end + + it "does not allow an authenticated non-super-admin user to delete settings" do + api_basic_authorize + + expect { + delete( + api_region_settings_url(nil, region), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.not_to change { region.settings_for_resource["api"]["authentication_timeout"] } + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to delete settings`" do + expect { + delete( + api_region_settings_url(nil, region), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.not_to change { region.settings_for_resource["api"]["authentication_timeout"] } + + expect(response).to have_http_status(:unauthorized) + end + end + end end diff --git a/spec/requests/servers_spec.rb b/spec/requests/servers_spec.rb new file mode 100644 index 000000000000..63bcb7d86aa3 --- /dev/null +++ b/spec/requests/servers_spec.rb @@ -0,0 +1,103 @@ +RSpec.describe "Servers" do + describe "/api/servers/:id/settings" do + let(:server) { FactoryGirl.create(:miq_server) } + let(:original_timeout) { server.settings_for_resource[:api][:authentication_timeout] } + let(:super_admin) { FactoryGirl.create(:user, :role => 'super_administrator', :userid => 'alice', :password => 'alicepassword') } + + + it "shows the settings to an authenticated user with the proper role" do + api_basic_authorize(:ops_settings) + + get(api_server_settings_url(nil, server)) + + expect(response).to have_http_status(:ok) + end + + it "does not allow an authenticated user who doesn't have the proper role to view the settings" do + api_basic_authorize + + get(api_server_settings_url(nil, server)) + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to view the settings" do + get(api_server_settings_url(nil, server)) + + expect(response).to have_http_status(:unauthorized) + end + + it "permits updates to settings for an authenticated super-admin user" do + api_basic_authorize(:user => super_admin.userid, :password => super_admin.password) + + expect { + patch(api_server_settings_url(nil, server), :params => {:api => {:authentication_timeout => "1337.minutes"}}) + }.to change { server.settings_for_resource[:api][:authentication_timeout] }.from(original_timeout).to("1337.minutes") + + expect(response.parsed_body).to include("api" => a_hash_including("authentication_timeout" => "1337.minutes")) + expect(response).to have_http_status(:ok) + end + + it "does not allow an authenticated non-super-admin user to update settings" do + api_basic_authorize + + expect { + patch(api_server_settings_url(nil, server), :params => {:api => {:authentication_timeout => "10.minutes"}}) + }.not_to change { server.settings_for_resource[:api][:authentication_timeout] } + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to update the settings" do + expect { + patch(api_server_settings_url(nil, server), :params => {:api => {:authentication_timeout => "10.minutes"}}) + }.not_to change { server.settings_for_resource[:api][:authentication_timeout] } + + expect(response).to have_http_status(:unauthorized) + end + + context "with an existing settings change" do + before do + server.add_settings_for_resource("api" => {"authentication_timeout" => "7331.minutes"}) + end + + it "allows an authenticated super-admin user to delete settings" do + api_basic_authorize(:user => super_admin.userid, :password => super_admin.password) + expect(server.settings_for_resource["api"]["authentication_timeout"]).to eq("7331.minutes") + + expect { + delete( + api_server_settings_url(nil, server), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.to change { server.settings_for_resource["api"]["authentication_timeout"] }.from("7331.minutes").to("30.seconds") + + expect(response).to have_http_status(:no_content) + end + + it "does not allow an authenticated non-super-admin user to delete settings" do + api_basic_authorize + + expect { + delete( + api_server_settings_url(nil, server), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.not_to change { server.settings_for_resource["api"]["authentication_timeout"] } + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to delete settings`" do + expect { + delete( + api_server_settings_url(nil, server), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.not_to change { server.settings_for_resource["api"]["authentication_timeout"] } + + expect(response).to have_http_status(:unauthorized) + end + end + end +end diff --git a/spec/requests/zones_spec.rb b/spec/requests/zones_spec.rb new file mode 100644 index 000000000000..5957ff88628d --- /dev/null +++ b/spec/requests/zones_spec.rb @@ -0,0 +1,103 @@ +RSpec.describe "Zones" do + describe "/api/zones/:id/settings" do + let(:zone) { FactoryGirl.create(:zone) } + let(:original_timeout) { zone.settings_for_resource[:api][:authentication_timeout] } + let(:super_admin) { FactoryGirl.create(:user, :role => 'super_administrator', :userid => 'alice', :password => 'alicepassword') } + + it "shows the settings to an authenticated user with the proper role" do + api_basic_authorize(:ops_settings) + + get(api_zone_settings_url(nil, zone)) + + expect(response).to have_http_status(:ok) + end + + it "does not allow an authenticated user who doesn't have the proper role to view the settings" do + api_basic_authorize + + get(api_zone_settings_url(nil, zone)) + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to view the settings" do + get(api_zone_settings_url(nil, zone)) + + expect(response).to have_http_status(:unauthorized) + end + + it "permits updates to settings for an authenticated super-admin user" do + api_basic_authorize(:user => super_admin.userid, :password => super_admin.password) + + expect { + patch(api_zone_settings_url(nil, zone), :params => {:api => {:authentication_timeout => "1337.minutes"}}) + }.to change { zone.settings_for_resource[:api][:authentication_timeout] }.from(original_timeout).to("1337.minutes") + + expect(response.parsed_body).to include("api" => a_hash_including("authentication_timeout" => "1337.minutes")) + expect(response).to have_http_status(:ok) + end + + it "does not allow an authenticated non-super-admin user to update settings" do + api_basic_authorize + + expect { + patch(api_zone_settings_url(nil, zone), :params => {:api => {:authentication_timeout => "10.minutes"}}) + }.not_to change { zone.settings_for_resource[:api][:authentication_timeout] } + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to update the settings" do + expect { + patch(api_zone_settings_url(nil, zone), :params => {:api => {:authentication_timeout => "10.minutes"}}) + }.not_to change { zone.settings_for_resource[:api][:authentication_timeout] } + + expect(response).to have_http_status(:unauthorized) + end + + context "with an existing settings change" do + before do + zone.add_settings_for_resource("api" => {"authentication_timeout" => "7331.minutes"}) + end + + it "allows an authenticated super-admin user to delete settings" do + api_basic_authorize(:user => super_admin.userid, :password => super_admin.password) + expect(zone.settings_for_resource["api"]["authentication_timeout"]).to eq("7331.minutes") + + expect { + delete( + api_zone_settings_url(nil, zone), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.to change { zone.settings_for_resource["api"]["authentication_timeout"] }.from("7331.minutes").to("30.seconds") + + expect(response).to have_http_status(:no_content) + end + + it "does not allow an authenticated non-super-admin user to delete settings" do + api_basic_authorize + + expect { + delete( + api_zone_settings_url(nil, zone), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.not_to change { zone.settings_for_resource["api"]["authentication_timeout"] } + + expect(response).to have_http_status(:forbidden) + end + + it "does not allow an unauthenticated user to delete settings`" do + expect { + delete( + api_zone_settings_url(nil, zone), + :params => %i[api authentication_timeout].to_json # => hack because Rails will interpret these as query params in a DELETE + ) + }.not_to change { zone.settings_for_resource["api"]["authentication_timeout"] } + + expect(response).to have_http_status(:unauthorized) + end + end + end + +end