Skip to content

Commit

Permalink
Fix limited admin accounts not being able to publish website backends.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
GUI committed May 10, 2018
1 parent 547c51e commit 9091de9
Show file tree
Hide file tree
Showing 10 changed files with 223 additions and 5 deletions.
6 changes: 5 additions & 1 deletion src/api-umbrella/web-app/app/models/config_version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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|
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Expand Down
121 changes: 121 additions & 0 deletions test/apis/v1/config/test_publish_admin_permissions_websites.rb
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions test/factories/admin_groups.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
5 changes: 5 additions & 0 deletions test/factories/api_scopes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 4 additions & 0 deletions test/factories/website_backends.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions test/support/models/config_version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 9091de9

Please sign in to comment.