Skip to content

Commit a4569a6

Browse files
committed
Fix permissions for admin groups without analytics permissions.
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.
1 parent 313036f commit a4569a6

File tree

10 files changed

+271
-5
lines changed

10 files changed

+271
-5
lines changed

src/api-umbrella/web-app/app/helpers/admin/stats_helper.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ def aggregation_result(aggregation_name)
3131
bucket["percent"] = ((bucket["count"] / total) * 100).round
3232
end
3333

34-
if(other_hits > 0)
34+
if(other_hits && other_hits > 0)
3535
buckets << {
3636
"key" => "Other",
3737
"count" => other_hits,

src/api-umbrella/web-app/app/models/log_search/base.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,4 +63,8 @@ def aggregate_by_region!
6363
def aggregate_by_country!
6464
aggregate_by_region_field!(:request_ip_country)
6565
end
66+
67+
def none!
68+
@none = true
69+
end
6670
end

src/api-umbrella/web-app/app/models/log_search/elastic_search.rb

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,23 @@ def initialize(options = {})
4444
end
4545

4646
def result
47+
if @none
48+
raw_result = {
49+
"hits" => {
50+
"total" => 0,
51+
"hits" => [],
52+
},
53+
"aggregations" => {},
54+
}
55+
@query[:aggregations].each_key do |aggregation_name|
56+
raw_result["aggregations"][aggregation_name.to_s] ||= {}
57+
raw_result["aggregations"][aggregation_name.to_s]["buckets"] = []
58+
raw_result["aggregations"][aggregation_name.to_s]["doc_count"] = 0
59+
end
60+
@result = LogResult.factory(self, raw_result)
61+
return @result
62+
end
63+
4764
query_options = @query_options.merge({
4865
:index => indexes.join(","),
4966
:body => @query,

src/api-umbrella/web-app/app/models/log_search/sql.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,11 @@ def build_query(query)
9292
end
9393

9494
def result
95+
if @none
96+
@result = LogResult.factory(self, {})
97+
return @result
98+
end
99+
95100
if(@query_results.empty? || @queries[:default])
96101
execute_query(:default, @queries[:default] || {})
97102
end

src/api-umbrella/web-app/app/policies/log_search_policy.rb

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,14 @@ def resolve
2121
}
2222
end
2323

24-
scope.permission_scope!({
25-
"condition" => "OR",
26-
"rules" => rules,
27-
})
24+
if(rules.any?)
25+
scope.permission_scope!({
26+
"condition" => "OR",
27+
"rules" => rules,
28+
})
29+
else
30+
scope.none!
31+
end
2832
end
2933
end
3034
end
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
require_relative "../../../test_helper"
2+
3+
class Test::Apis::Admin::Stats::TestLogsAdminPermissions < Minitest::Test
4+
include ApiUmbrellaTestHelpers::AdminAuth
5+
include ApiUmbrellaTestHelpers::AdminPermissions
6+
include ApiUmbrellaTestHelpers::Setup
7+
8+
def setup
9+
super
10+
setup_server
11+
ElasticsearchHelper.clean_es_indices(["2015-01"])
12+
end
13+
14+
def test_default_permissions
15+
factory = :google_log_item
16+
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
17+
end
18+
19+
private
20+
21+
def make_request(factory, admin)
22+
ElasticsearchHelper.clean_es_indices(["2015-01"])
23+
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
24+
LogItem.gateway.refresh_index!
25+
26+
Typhoeus.get("https://127.0.0.1:9081/admin/stats/logs.json", http_options.deep_merge(admin_session(admin)).deep_merge({
27+
:params => {
28+
"start_at" => "2015-01-13",
29+
"end_at" => "2015-01-18",
30+
"interval" => "day",
31+
"start" => "0",
32+
"length" => "10",
33+
},
34+
}))
35+
end
36+
37+
def assert_admin_permitted(factory, admin)
38+
response = make_request(factory, admin)
39+
assert_response_code(200, response)
40+
data = MultiJson.load(response.body)
41+
assert_equal(1, data["recordsTotal"])
42+
assert_equal(1, data["recordsFiltered"])
43+
assert_equal(1, data["data"].length)
44+
end
45+
46+
def assert_admin_forbidden(factory, admin)
47+
response = make_request(factory, admin)
48+
assert_response_code(200, response)
49+
data = MultiJson.load(response.body)
50+
assert_equal(0, data["recordsTotal"])
51+
assert_equal(0, data["recordsFiltered"])
52+
assert_equal(0, data["data"].length)
53+
end
54+
end
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
require_relative "../../../test_helper"
2+
3+
class Test::Apis::Admin::Stats::TestMapAdminPermissions < Minitest::Test
4+
include ApiUmbrellaTestHelpers::AdminAuth
5+
include ApiUmbrellaTestHelpers::AdminPermissions
6+
include ApiUmbrellaTestHelpers::Setup
7+
8+
def setup
9+
super
10+
setup_server
11+
ElasticsearchHelper.clean_es_indices(["2014-11", "2015-01", "2015-03"])
12+
end
13+
14+
def test_default_permissions
15+
factory = :google_log_item
16+
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
17+
end
18+
19+
private
20+
21+
def make_request(factory, admin)
22+
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
23+
LogItem.gateway.refresh_index!
24+
25+
Typhoeus.get("https://127.0.0.1:9081/admin/stats/map.json", http_options.deep_merge(admin_session(admin)).deep_merge({
26+
:params => {
27+
"start_at" => "2015-01-13",
28+
"end_at" => "2015-01-18",
29+
"region" => "world",
30+
},
31+
}))
32+
end
33+
34+
def assert_admin_permitted(factory, admin)
35+
response = make_request(factory, admin)
36+
assert_response_code(200, response)
37+
data = MultiJson.load(response.body)
38+
assert_equal(1, data["map_regions"].length)
39+
assert_equal(1, data["regions"].length)
40+
end
41+
42+
def assert_admin_forbidden(factory, admin)
43+
response = make_request(factory, admin)
44+
assert_response_code(200, response)
45+
data = MultiJson.load(response.body)
46+
assert_equal(0, data["map_regions"].length)
47+
assert_equal(0, data["regions"].length)
48+
end
49+
end
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
require_relative "../../../test_helper"
2+
3+
class Test::Apis::Admin::Stats::TestSearchAdminPermissions < Minitest::Test
4+
include ApiUmbrellaTestHelpers::AdminAuth
5+
include ApiUmbrellaTestHelpers::AdminPermissions
6+
include ApiUmbrellaTestHelpers::Setup
7+
8+
def setup
9+
super
10+
setup_server
11+
ElasticsearchHelper.clean_es_indices(["2015-01"])
12+
end
13+
14+
def test_default_permissions
15+
factory = :google_log_item
16+
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
17+
end
18+
19+
private
20+
21+
def make_request(factory, admin)
22+
ElasticsearchHelper.clean_es_indices(["2015-01"])
23+
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
24+
LogItem.gateway.refresh_index!
25+
26+
Typhoeus.get("https://127.0.0.1:9081/admin/stats/search.json", http_options.deep_merge(admin_session(admin)).deep_merge({
27+
:params => {
28+
:search => "",
29+
:start_at => "2015-01-13",
30+
:end_at => "2015-01-18",
31+
:interval => "day",
32+
},
33+
}))
34+
end
35+
36+
def assert_admin_permitted(factory, admin)
37+
response = make_request(factory, admin)
38+
assert_response_code(200, response)
39+
data = MultiJson.load(response.body)
40+
assert_equal(1, data["stats"]["total_hits"])
41+
assert_equal(1, data["stats"]["total_users"])
42+
assert_equal(1, data["stats"]["total_ips"])
43+
assert_equal(1, data["aggregations"]["users"].length)
44+
assert_equal(1, data["aggregations"]["ips"].length)
45+
assert_equal(6, data["hits_over_time"].length)
46+
hits_over_time_total = data["hits_over_time"].map { |hit| hit["c"][1]["v"] }.sum
47+
assert_equal(1, hits_over_time_total)
48+
end
49+
50+
def assert_admin_forbidden(factory, admin)
51+
response = make_request(factory, admin)
52+
assert_response_code(200, response)
53+
data = MultiJson.load(response.body)
54+
assert_equal(0, data["stats"]["total_hits"])
55+
assert_nil(data["stats"]["average_response_time"])
56+
if(data["hits_over_time"].present?)
57+
assert_equal(0, data["stats"]["total_users"])
58+
assert_equal(0, data["stats"]["total_ips"])
59+
assert_equal(6, data["hits_over_time"].length)
60+
else
61+
assert_nil(data["stats"]["total_users"])
62+
assert_nil(data["stats"]["total_ips"])
63+
assert_equal(0, data["hits_over_time"].length)
64+
end
65+
hits_over_time_total = data["hits_over_time"].map { |hit| hit["c"][1]["v"] }.sum
66+
assert_equal(0, hits_over_time_total)
67+
end
68+
end
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
require_relative "../../../test_helper"
2+
3+
class Test::Apis::V1::Analytics::TestDrilldownAdminPermissions < Minitest::Test
4+
include ApiUmbrellaTestHelpers::AdminAuth
5+
include ApiUmbrellaTestHelpers::AdminPermissions
6+
include ApiUmbrellaTestHelpers::Setup
7+
8+
def setup
9+
super
10+
setup_server
11+
ElasticsearchHelper.clean_es_indices(["2015-01"])
12+
end
13+
14+
def test_default_permissions
15+
factory = :google_log_item
16+
assert_default_admin_permissions(factory, :required_permissions => ["analytics"])
17+
end
18+
19+
private
20+
21+
def make_request(factory, admin)
22+
ElasticsearchHelper.clean_es_indices(["2015-01"])
23+
FactoryGirl.create(factory, :request_at => Time.parse("2015-01-15T00:00:00Z").utc)
24+
LogItem.gateway.refresh_index!
25+
26+
Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/analytics/drilldown.json", http_options.deep_merge(admin_token(admin)).deep_merge({
27+
:params => {
28+
:search => "",
29+
:start_at => "2015-01-13",
30+
:end_at => "2015-01-18",
31+
:interval => "day",
32+
:prefix => "0/",
33+
},
34+
}))
35+
end
36+
37+
def assert_admin_permitted(factory, admin)
38+
response = make_request(factory, admin)
39+
assert_response_code(200, response)
40+
data = MultiJson.load(response.body)
41+
assert_equal(1, data["results"].length)
42+
assert_equal(1, data["results"][0]["hits"])
43+
assert_equal(6, data["hits_over_time"]["rows"].length)
44+
hits_over_time_total = data["hits_over_time"]["rows"].map { |hit| hit["c"][1]["v"] }.sum
45+
assert_equal(1, hits_over_time_total)
46+
end
47+
48+
def assert_admin_forbidden(factory, admin)
49+
response = make_request(factory, admin)
50+
assert_response_code(200, response)
51+
data = MultiJson.load(response.body)
52+
assert_equal(0, data["results"].length)
53+
if(data["hits_over_time"]["rows"].any?)
54+
assert_equal(6, data["hits_over_time"]["rows"].length)
55+
end
56+
hits_over_time_total = data["hits_over_time"]["rows"].map { |hit| if(hit["c"][1]) then hit["c"][1]["v"] else 0 end }.sum
57+
assert_equal(0, hits_over_time_total)
58+
end
59+
end

test/factories/log_items.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,5 +46,11 @@
4646
user_email '"><script class="xss-test">alert("12");</script>'
4747
user_registration_source '"><script class="xss-test">alert("13");</script>'
4848
end
49+
50+
factory :google_log_item do
51+
request_host "localhost"
52+
request_path "/google/hello/"
53+
request_hierarchy ["0/localhost/", "1/localhost/google/", "2/localhost/google/hello"]
54+
end
4955
end
5056
end

0 commit comments

Comments
 (0)