Skip to content

Commit

Permalink
Fix permissions for admin groups without analytics permissions.
Browse files Browse the repository at this point in the history
If a limited admin without superuser privileges belonged to only admin
groups without any analytics permissions, then it was possible they
could inadvertently be granted access to all analytics data (outside of
just the admin groups they did belong to without analytics permissions).

This was caused when the LogSearchPolicy scope was applied, since if
there were no analytics scopes, then no conditions were being applied
(rather than returning no results).

This fixes the policy scope and adds missing permission tests to all the
analytics endpoints to test for this scenario.
  • Loading branch information
GUI committed Mar 5, 2018
1 parent 313036f commit a4569a6
Show file tree
Hide file tree
Showing 10 changed files with 271 additions and 5 deletions.
2 changes: 1 addition & 1 deletion src/api-umbrella/web-app/app/helpers/admin/stats_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def aggregation_result(aggregation_name)
bucket["percent"] = ((bucket["count"] / total) * 100).round
end

if(other_hits > 0)
if(other_hits && other_hits > 0)
buckets << {
"key" => "Other",
"count" => other_hits,
Expand Down
4 changes: 4 additions & 0 deletions src/api-umbrella/web-app/app/models/log_search/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,8 @@ def aggregate_by_region!
def aggregate_by_country!
aggregate_by_region_field!(:request_ip_country)
end

def none!
@none = true
end
end
17 changes: 17 additions & 0 deletions src/api-umbrella/web-app/app/models/log_search/elastic_search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,23 @@ def initialize(options = {})
end

def result
if @none
raw_result = {
"hits" => {
"total" => 0,
"hits" => [],
},
"aggregations" => {},
}
@query[:aggregations].each_key do |aggregation_name|
raw_result["aggregations"][aggregation_name.to_s] ||= {}
raw_result["aggregations"][aggregation_name.to_s]["buckets"] = []
raw_result["aggregations"][aggregation_name.to_s]["doc_count"] = 0
end
@result = LogResult.factory(self, raw_result)
return @result
end

query_options = @query_options.merge({
:index => indexes.join(","),
:body => @query,
Expand Down
5 changes: 5 additions & 0 deletions src/api-umbrella/web-app/app/models/log_search/sql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ def build_query(query)
end

def result
if @none
@result = LogResult.factory(self, {})
return @result
end

if(@query_results.empty? || @queries[:default])
execute_query(:default, @queries[:default] || {})
end
Expand Down
12 changes: 8 additions & 4 deletions src/api-umbrella/web-app/app/policies/log_search_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ def resolve
}
end

scope.permission_scope!({
"condition" => "OR",
"rules" => rules,
})
if(rules.any?)
scope.permission_scope!({
"condition" => "OR",
"rules" => rules,
})
else
scope.none!
end
end
end
end
Expand Down
54 changes: 54 additions & 0 deletions test/apis/admin/stats/test_logs_admin_permissions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
require_relative "../../../test_helper"

class Test::Apis::Admin::Stats::TestLogsAdminPermissions < Minitest::Test
include ApiUmbrellaTestHelpers::AdminAuth
include ApiUmbrellaTestHelpers::AdminPermissions
include ApiUmbrellaTestHelpers::Setup

def setup
super
setup_server
ElasticsearchHelper.clean_es_indices(["2015-01"])
end

def test_default_permissions
factory = :google_log_item
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
end

private

def make_request(factory, admin)
ElasticsearchHelper.clean_es_indices(["2015-01"])
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
LogItem.gateway.refresh_index!

Typhoeus.get("https://127.0.0.1:9081/admin/stats/logs.json", http_options.deep_merge(admin_session(admin)).deep_merge({
:params => {
"start_at" => "2015-01-13",
"end_at" => "2015-01-18",
"interval" => "day",
"start" => "0",
"length" => "10",
},
}))
end

def assert_admin_permitted(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(1, data["recordsTotal"])
assert_equal(1, data["recordsFiltered"])
assert_equal(1, data["data"].length)
end

def assert_admin_forbidden(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(0, data["recordsTotal"])
assert_equal(0, data["recordsFiltered"])
assert_equal(0, data["data"].length)
end
end
49 changes: 49 additions & 0 deletions test/apis/admin/stats/test_map_admin_permissions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
require_relative "../../../test_helper"

class Test::Apis::Admin::Stats::TestMapAdminPermissions < Minitest::Test
include ApiUmbrellaTestHelpers::AdminAuth
include ApiUmbrellaTestHelpers::AdminPermissions
include ApiUmbrellaTestHelpers::Setup

def setup
super
setup_server
ElasticsearchHelper.clean_es_indices(["2014-11", "2015-01", "2015-03"])
end

def test_default_permissions
factory = :google_log_item
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
end

private

def make_request(factory, admin)
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
LogItem.gateway.refresh_index!

Typhoeus.get("https://127.0.0.1:9081/admin/stats/map.json", http_options.deep_merge(admin_session(admin)).deep_merge({
:params => {
"start_at" => "2015-01-13",
"end_at" => "2015-01-18",
"region" => "world",
},
}))
end

def assert_admin_permitted(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(1, data["map_regions"].length)
assert_equal(1, data["regions"].length)
end

def assert_admin_forbidden(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(0, data["map_regions"].length)
assert_equal(0, data["regions"].length)
end
end
68 changes: 68 additions & 0 deletions test/apis/admin/stats/test_search_admin_permissions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
require_relative "../../../test_helper"

class Test::Apis::Admin::Stats::TestSearchAdminPermissions < Minitest::Test
include ApiUmbrellaTestHelpers::AdminAuth
include ApiUmbrellaTestHelpers::AdminPermissions
include ApiUmbrellaTestHelpers::Setup

def setup
super
setup_server
ElasticsearchHelper.clean_es_indices(["2015-01"])
end

def test_default_permissions
factory = :google_log_item
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
end

private

def make_request(factory, admin)
ElasticsearchHelper.clean_es_indices(["2015-01"])
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
LogItem.gateway.refresh_index!

Typhoeus.get("https://127.0.0.1:9081/admin/stats/search.json", http_options.deep_merge(admin_session(admin)).deep_merge({
:params => {
:search => "",
:start_at => "2015-01-13",
:end_at => "2015-01-18",
:interval => "day",
},
}))
end

def assert_admin_permitted(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(1, data["stats"]["total_hits"])
assert_equal(1, data["stats"]["total_users"])
assert_equal(1, data["stats"]["total_ips"])
assert_equal(1, data["aggregations"]["users"].length)
assert_equal(1, data["aggregations"]["ips"].length)
assert_equal(6, data["hits_over_time"].length)
hits_over_time_total = data["hits_over_time"].map { |hit| hit["c"][1]["v"] }.sum
assert_equal(1, hits_over_time_total)
end

def assert_admin_forbidden(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(0, data["stats"]["total_hits"])
assert_nil(data["stats"]["average_response_time"])
if(data["hits_over_time"].present?)
assert_equal(0, data["stats"]["total_users"])
assert_equal(0, data["stats"]["total_ips"])
assert_equal(6, data["hits_over_time"].length)
else
assert_nil(data["stats"]["total_users"])
assert_nil(data["stats"]["total_ips"])
assert_equal(0, data["hits_over_time"].length)
end
hits_over_time_total = data["hits_over_time"].map { |hit| hit["c"][1]["v"] }.sum
assert_equal(0, hits_over_time_total)
end
end
59 changes: 59 additions & 0 deletions test/apis/v1/analytics/test_drilldown_admin_permissions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
require_relative "../../../test_helper"

class Test::Apis::V1::Analytics::TestDrilldownAdminPermissions < Minitest::Test
include ApiUmbrellaTestHelpers::AdminAuth
include ApiUmbrellaTestHelpers::AdminPermissions
include ApiUmbrellaTestHelpers::Setup

def setup
super
setup_server
ElasticsearchHelper.clean_es_indices(["2015-01"])
end

def test_default_permissions
factory = :google_log_item
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
end

private

def make_request(factory, admin)
ElasticsearchHelper.clean_es_indices(["2015-01"])
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
LogItem.gateway.refresh_index!

Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/analytics/drilldown.json", http_options.deep_merge(admin_token(admin)).deep_merge({
:params => {
:search => "",
:start_at => "2015-01-13",
:end_at => "2015-01-18",
:interval => "day",
:prefix => "0/",
},
}))
end

def assert_admin_permitted(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(1, data["results"].length)
assert_equal(1, data["results"][0]["hits"])
assert_equal(6, data["hits_over_time"]["rows"].length)
hits_over_time_total = data["hits_over_time"]["rows"].map { |hit| hit["c"][1]["v"] }.sum
assert_equal(1, hits_over_time_total)
end

def assert_admin_forbidden(factory, admin)
response = make_request(factory, admin)
assert_response_code(200, response)
data = MultiJson.load(response.body)
assert_equal(0, data["results"].length)
if(data["hits_over_time"]["rows"].any?)
assert_equal(6, data["hits_over_time"]["rows"].length)
end
hits_over_time_total = data["hits_over_time"]["rows"].map { |hit| if(hit["c"][1]) then hit["c"][1]["v"] else 0 end }.sum
assert_equal(0, hits_over_time_total)
end
end
6 changes: 6 additions & 0 deletions test/factories/log_items.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,11 @@
user_email '"><script class="xss-test">alert("12");</script>'
user_registration_source '"><script class="xss-test">alert("13");</script>'
end

factory :google_log_item do
request_host "localhost"
request_path "/google/hello/"
request_hierarchy ["0/localhost/", "1/localhost/google/", "2/localhost/google/hello"]
end
end
end

0 comments on commit a4569a6

Please sign in to comment.