From 9091de98edf40920733cfc91e7101456a9d604fd Mon Sep 17 00:00:00 2001 From: Nick Muerdter Date: Wed, 9 May 2018 21:51:13 -0600 Subject: [PATCH] Fix limited admin accounts not being able to publish website backends. Pending changes for website backends weren't being returned to admin accounts with limited scopes (only authorized on specific domains). The reason was that we were inadvertently applying the API backend policy scope to this permission check instead of the website backend policy (so the API backend policy would never consider the website backend authorized). This fixes that issue and adds more test coverage for the admin permissions surrounding website backends in the config publishing process. --- .../web-app/app/models/config_version.rb | 6 +- .../app/policies/website_backend_policy.rb | 9 +- ...pending_changes_admin_permissions_apis.rb} | 2 +- ...ding_changes_admin_permissions_websites.rb | 74 +++++++++++ ...=> test_publish_admin_permissions_apis.rb} | 2 +- ...test_publish_admin_permissions_websites.rb | 121 ++++++++++++++++++ test/factories/admin_groups.rb | 4 + test/factories/api_scopes.rb | 5 + test/factories/website_backends.rb | 4 + test/support/models/config_version.rb | 1 + 10 files changed, 223 insertions(+), 5 deletions(-) rename test/apis/v1/config/{test_pending_changes_admin_permissions.rb => test_pending_changes_admin_permissions_apis.rb} (99%) create mode 100644 test/apis/v1/config/test_pending_changes_admin_permissions_websites.rb rename test/apis/v1/config/{test_publish_admin_permissions.rb => test_publish_admin_permissions_apis.rb} (98%) create mode 100644 test/apis/v1/config/test_publish_admin_permissions_websites.rb diff --git a/src/api-umbrella/web-app/app/models/config_version.rb b/src/api-umbrella/web-app/app/models/config_version.rb index 3aa9a7a45..4de8d443d 100644 --- a/src/api-umbrella/web-app/app/models/config_version.rb +++ b/src/api-umbrella/web-app/app/models/config_version.rb @@ -70,7 +70,11 @@ def self.pending_changes(current_admin) end if(current_admin) - pending_records = ApiPolicy::Scope.new(current_admin, pending_records).resolve("backend_publish") + if(category == "website_backends") + pending_records = WebsiteBackendPolicy::Scope.new(current_admin, pending_records).resolve("backend_publish") + else + pending_records = ApiPolicy::Scope.new(current_admin, pending_records).resolve("backend_publish") + end end pending_records = pending_records.sorted.all diff --git a/src/api-umbrella/web-app/app/policies/website_backend_policy.rb b/src/api-umbrella/web-app/app/policies/website_backend_policy.rb index 8ad5eb1d2..fcce64cf4 100644 --- a/src/api-umbrella/web-app/app/policies/website_backend_policy.rb +++ b/src/api-umbrella/web-app/app/policies/website_backend_policy.rb @@ -1,10 +1,15 @@ class WebsiteBackendPolicy < ApplicationPolicy class Scope < Scope - def resolve + def resolve(permission = "backend_manage") if(user.superuser?) scope.all else - api_scopes = user.api_scopes_with_permission("backend_manage") + api_scopes = [] + if(permission == :any) + api_scopes = user.api_scopes + else + api_scopes = user.api_scopes_with_permission(permission) + end query_scopes = [] api_scopes.each do |api_scope| diff --git a/test/apis/v1/config/test_pending_changes_admin_permissions.rb b/test/apis/v1/config/test_pending_changes_admin_permissions_apis.rb similarity index 99% rename from test/apis/v1/config/test_pending_changes_admin_permissions.rb rename to test/apis/v1/config/test_pending_changes_admin_permissions_apis.rb index 6aa16c51f..b82e36057 100644 --- a/test/apis/v1/config/test_pending_changes_admin_permissions.rb +++ b/test/apis/v1/config/test_pending_changes_admin_permissions_apis.rb @@ -1,6 +1,6 @@ require_relative "../../../test_helper" -class Test::Apis::V1::Config::TestPendingChangesAdminPermissions < Minitest::Test +class Test::Apis::V1::Config::TestPendingChangesAdminPermissionsApis < Minitest::Test include ApiUmbrellaTestHelpers::AdminAuth include ApiUmbrellaTestHelpers::Setup include Minitest::Hooks diff --git a/test/apis/v1/config/test_pending_changes_admin_permissions_websites.rb b/test/apis/v1/config/test_pending_changes_admin_permissions_websites.rb new file mode 100644 index 000000000..ec2d22ff0 --- /dev/null +++ b/test/apis/v1/config/test_pending_changes_admin_permissions_websites.rb @@ -0,0 +1,74 @@ +require_relative "../../../test_helper" + +class Test::Apis::V1::Config::TestPendingChangesAdminPermissionsWebsites < Minitest::Test + include ApiUmbrellaTestHelpers::AdminAuth + include ApiUmbrellaTestHelpers::Setup + include Minitest::Hooks + + def setup + super + setup_server + Api.delete_all + WebsiteBackend.delete_all + ConfigVersion.delete_all + + @localhost_website = FactoryBot.create(:website_backend) + @example_com_website = FactoryBot.create(:example_com_website_backend) + ConfigVersion.publish!(ConfigVersion.pending_config) + end + + def after_all + super + default_config_version_needed + end + + def test_all_websites_for_superuser + response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/config/pending_changes.json", http_options.deep_merge(admin_token)) + + assert_response_code(200, response) + data = MultiJson.load(response.body) + website_ids = data["config"]["website_backends"]["identical"].map { |website| website["pending"]["_id"] } + assert_includes(website_ids, @localhost_website.id) + assert_includes(website_ids, @example_com_website.id) + end + + def test_permitted_websites_for_limited_admin + localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:localhost_root_admin_group, :backend_publish_permission)]) + response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/config/pending_changes.json", http_options.deep_merge(admin_token(localhost_admin))) + + assert_response_code(200, response) + data = MultiJson.load(response.body) + website_ids = data["config"]["website_backends"]["identical"].map { |website| website["pending"]["_id"] } + assert_includes(website_ids, @localhost_website.id) + end + + def test_excludes_forbidden_websites + localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:localhost_root_admin_group, :backend_publish_permission)]) + response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/config/pending_changes.json", http_options.deep_merge(admin_token(localhost_admin))) + + assert_response_code(200, response) + data = MultiJson.load(response.body) + website_ids = data["config"]["website_backends"]["identical"].map { |website| website["pending"]["_id"] } + refute_includes(website_ids, @example_com_website.id) + end + + def test_exclude_websites_without_publish_permission + unauthorized_localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:localhost_root_admin_group, :backend_manage_permission)]) + response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/config/pending_changes.json", http_options.deep_merge(admin_token(unauthorized_localhost_admin))) + + assert_response_code(200, response) + data = MultiJson.load(response.body) + website_ids = data["config"]["website_backends"]["identical"].map { |website| website["pending"]["_id"] } + assert_equal(0, website_ids.length) + end + + def test_excludes_admins_without_root_url_permissions + localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:google_admin_group, :backend_publish_permission)]) + response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/config/pending_changes.json", http_options.deep_merge(admin_token(localhost_admin))) + + assert_response_code(200, response) + data = MultiJson.load(response.body) + website_ids = data["config"]["website_backends"]["identical"].map { |website| website["pending"]["_id"] } + assert_equal(0, website_ids.length) + end +end diff --git a/test/apis/v1/config/test_publish_admin_permissions.rb b/test/apis/v1/config/test_publish_admin_permissions_apis.rb similarity index 98% rename from test/apis/v1/config/test_publish_admin_permissions.rb rename to test/apis/v1/config/test_publish_admin_permissions_apis.rb index b0ebb86dd..b5f621db6 100644 --- a/test/apis/v1/config/test_publish_admin_permissions.rb +++ b/test/apis/v1/config/test_publish_admin_permissions_apis.rb @@ -1,6 +1,6 @@ require_relative "../../../test_helper" -class Test::Apis::V1::Config::TestPublishAdminPermissions < Minitest::Test +class Test::Apis::V1::Config::TestPublishAdminPermissionsApis < Minitest::Test include ApiUmbrellaTestHelpers::AdminAuth include ApiUmbrellaTestHelpers::Setup include Minitest::Hooks diff --git a/test/apis/v1/config/test_publish_admin_permissions_websites.rb b/test/apis/v1/config/test_publish_admin_permissions_websites.rb new file mode 100644 index 000000000..7e5050b8e --- /dev/null +++ b/test/apis/v1/config/test_publish_admin_permissions_websites.rb @@ -0,0 +1,121 @@ +require_relative "../../../test_helper" + +class Test::Apis::V1::Config::TestPublishAdminPermissionsWebsites < Minitest::Test + include ApiUmbrellaTestHelpers::AdminAuth + include ApiUmbrellaTestHelpers::Setup + include Minitest::Hooks + + def setup + super + setup_server + Api.delete_all + WebsiteBackend.delete_all + ConfigVersion.delete_all + + @localhost_website = FactoryBot.create(:website_backend) + @example_com_website = FactoryBot.create(:example_com_website_backend) + end + + def after_all + super + default_config_version_needed + end + + def test_superusers_publish_anything + config = { + :website_backends => { + @localhost_website.id => { :publish => "1" }, + @example_com_website.id => { :publish => "1" }, + }, + } + + response = Typhoeus.post("https://127.0.0.1:9081/api-umbrella/v1/config/publish.json", http_options.deep_merge(admin_token).deep_merge({ + :headers => { "Content-Type" => "application/x-www-form-urlencoded" }, + :body => { :config => config }, + })) + + assert_response_code(201, response) + active_config = ConfigVersion.active_config + assert_equal(2, active_config["website_backends"].length) + assert_equal([ + @localhost_website.id, + @example_com_website.id, + ].sort, active_config["website_backends"].map { |api| api["_id"] }.sort) + end + + def test_allow_limited_admins_publish_permitted_websites + config = { + :website_backends => { + @localhost_website.id => { :publish => "1" }, + }, + } + + localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:localhost_root_admin_group, :backend_publish_permission)]) + response = Typhoeus.post("https://127.0.0.1:9081/api-umbrella/v1/config/publish.json", http_options.deep_merge(admin_token(localhost_admin)).deep_merge({ + :headers => { "Content-Type" => "application/x-www-form-urlencoded" }, + :body => { :config => config }, + })) + + assert_response_code(201, response) + active_config = ConfigVersion.active_config + assert_equal(1, active_config["website_backends"].length) + assert_equal(@localhost_website.id, active_config["website_backends"].first["_id"]) + end + + def test_reject_limited_admins_publish_forbidden_website_backends + config = { + :website_backends => { + @example_com_website.id => { :publish => "1" }, + }, + } + + localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:localhost_root_admin_group, :backend_publish_permission)]) + response = Typhoeus.post("https://127.0.0.1:9081/api-umbrella/v1/config/publish.json", http_options.deep_merge(admin_token(localhost_admin)).deep_merge({ + :headers => { "Content-Type" => "application/x-www-form-urlencoded" }, + :body => { :config => config }, + })) + + assert_response_code(403, response) + data = MultiJson.load(response.body) + assert_equal(["errors"], data.keys) + assert_nil(ConfigVersion.active_config) + end + + def test_reject_limited_admins_without_publish_permission + config = { + :website_backends => { + @localhost_website.id => { :publish => "1" }, + }, + } + + unauthorized_localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:localhost_root_admin_group, :backend_manage_permission)]) + response = Typhoeus.post("https://127.0.0.1:9081/api-umbrella/v1/config/publish.json", http_options.deep_merge(admin_token(unauthorized_localhost_admin)).deep_merge({ + :headers => { "Content-Type" => "application/x-www-form-urlencoded" }, + :body => { :config => config }, + })) + + assert_response_code(403, response) + data = MultiJson.load(response.body) + assert_equal(["errors"], data.keys) + assert_nil(ConfigVersion.active_config) + end + + def test_reject_limited_admins_without_root_url_permission + config = { + :website_backends => { + @localhost_website.id => { :publish => "1" }, + }, + } + + localhost_admin = FactoryBot.create(:limited_admin, :groups => [FactoryBot.create(:google_admin_group, :backend_publish_permission)]) + response = Typhoeus.post("https://127.0.0.1:9081/api-umbrella/v1/config/publish.json", http_options.deep_merge(admin_token(localhost_admin)).deep_merge({ + :headers => { "Content-Type" => "application/x-www-form-urlencoded" }, + :body => { :config => config }, + })) + + assert_response_code(403, response) + data = MultiJson.load(response.body) + assert_equal(["errors"], data.keys) + assert_nil(ConfigVersion.active_config) + end +end diff --git a/test/factories/admin_groups.rb b/test/factories/admin_groups.rb index d703265e5..bd5083974 100644 --- a/test/factories/admin_groups.rb +++ b/test/factories/admin_groups.rb @@ -77,5 +77,9 @@ ] end end + + factory :example_com_admin_group do + api_scopes { [ApiScope.find_or_create_by_instance!(FactoryBot.build(:example_com_root_api_scope))] } + end end end diff --git a/test/factories/api_scopes.rb b/test/factories/api_scopes.rb index dce948fa6..06e2ff1c5 100644 --- a/test/factories/api_scopes.rb +++ b/test/factories/api_scopes.rb @@ -35,5 +35,10 @@ factory :bing_maps_api_scope do path_prefix "/bing/maps" end + + factory :example_com_root_api_scope do + host "example.com" + path_prefix "/" + end end end diff --git a/test/factories/website_backends.rb b/test/factories/website_backends.rb index a7ee19387..b9410ec5d 100644 --- a/test/factories/website_backends.rb +++ b/test/factories/website_backends.rb @@ -4,5 +4,9 @@ backend_protocol "http" server_host "example.com" server_port 80 + + factory :example_com_website_backend do + frontend_host "example.com" + end end end diff --git a/test/support/models/config_version.rb b/test/support/models/config_version.rb index ddbada456..73d3683f9 100644 --- a/test/support/models/config_version.rb +++ b/test/support/models/config_version.rb @@ -23,6 +23,7 @@ def self.active_config def self.pending_config { "apis" => Api.order_by(:sort_order.asc).all.map { |api| Hash[api.attributes] }, + "website_backends" => WebsiteBackend.order_by(:frontend_host.asc).all.map { |api| Hash[api.attributes] }, } end