Skip to content

Commit

Permalink
Fixes and improvements for admin permissions.
Browse files Browse the repository at this point in the history
- Allows limited admins to create sub-scopes underneath their current
  scopes (18F/api.data.gov#135).
- Allows limited admins to create new groups utilizing the scopes they
  have access to (18F/api.data.gov#339).
- Fixes potential security issues where a limited admin with knowledge
  of internal record UUIDs could overwrite records they didn't
  originally have access to with data they do have access to. Since this
  hinges upon knowing the random UUIDs of the records the admin doesn't
  have access view, this hopefully shouldn't be likely.
- Refactor most of the admin permission tests to ensure better
  consistency and coverage. There's now a shared baseline of permission
  checks we can easily apply across all admin resource types to ensure
  basic permission checks. The same permission checks are also now
  tested across all CRUD actions (rather than requiring different tests
  be written for each CRUD action).
  • Loading branch information
GUI committed May 22, 2016
1 parent 50cb660 commit e1a6b34
Show file tree
Hide file tree
Showing 30 changed files with 2,139 additions and 1,345 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def destroy
private

def save!
authorize(@admin_group) unless(@admin_group.new_record?)
@admin_group.assign_attributes(params[:admin_group], :as => :admin)
authorize(@admin_group)
@admin_group.save
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ def destroy
private

def save!
authorize(@admin) unless(@admin.new_record?)
@admin.assign_attributes(params[:admin], :as => :admin)
authorize(@admin)
@admin.save
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ def destroy
private

def save!
authorize(@api_scope) unless(@api_scope.new_record?)
@api_scope.assign_attributes(params[:api_scope], :as => :admin)
authorize(@api_scope)
@api_scope.save
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ def move_after
private

def save!
authorize(@api) unless(@api.new_record?)
@api.assign_nested_attributes(params[:api], :as => :admin)
authorize(@api)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ def update
private

def assign_attributes!
authorize(@api_user) unless(@api_user.new_record?)

assign_options = {}
if(admin_signed_in?)
assign_options[:as] = :admin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def destroy
private

def save!
authorize(@website_backend) unless(@website_backend.new_record?)
@website_backend.assign_attributes(params[:website_backend], :as => :admin)
authorize(@website_backend)
@website_backend.save
Expand Down
16 changes: 16 additions & 0 deletions src/api-umbrella/web-app/app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,18 @@
module ApplicationHelper
def web_admin_ajax_api_user
user = ApiUser.where(:email => "web.admin.ajax@internal.apiumbrella").order_by(:created_at.asc).first
unless(user)
user = ApiUser.create!({
:email => "web.admin.ajax@internal.apiumbrella",
:first_name => "API Umbrella Admin",
:last_name => "Key",
:use_description => "An API key for the API Umbrella admin to use for internal ajax requests.",
:terms_and_conditions => "1",
:registration_source => "seed",
:settings_attributes => { :rate_limit_mode => "unlimited" },
}, :without_protection => true)
end

user
end
end
30 changes: 30 additions & 0 deletions src/api-umbrella/web-app/app/models/admin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,48 @@ def can_any?(permissions)
end
end

# Fetch all the groups this admin belongs to that has a certain permission.
def groups_with_permission(permission)
self.groups.select do |group|
group.can?(permission)
end
end

# Fetch all the API scopes this admin belongs to (through their group
# membership) that has a certain permission.
def api_scopes_with_permission(permission)
self.groups_with_permission(permission).map do |group|
group.api_scopes
end.flatten.compact.uniq
end

# Fetch all the API scopes this admin belongs to that has a certain
# permission. Differing from #api_scopes_with_permission, this also includes
# any nested duplicative scopes.
#
# For example, if the user were explicitly granted permissions on a
# "api.example.com/" scope, this would also return any other sub-scopes that
# might exist, like "api.example.com/foo" (even if the admin account didn't
# have explicit permissions on that scope). This can be useful when needing a
# full list of scope IDs that the admin can operate on (since our prefix
# based approach means there might be other scopes that exist, but haven't
# been explicitly granted permissions to).
def nested_api_scopes_with_permission(permission)
query_scopes = []
self.api_scopes_with_permission(permission).each do |api_scope|
query_scopes << {
:host => api_scope.host,
:path_prefix => api_scope.path_prefix_matcher,
}
end

if(query_scopes.any?)
ApiScope.or(query_scopes).to_a
else
[]
end
end

def apply_omniauth(omniauth)
if(omniauth["extra"]["attributes"])
extra = omniauth["extra"]["attributes"].first
Expand Down
14 changes: 14 additions & 0 deletions src/api-umbrella/web-app/app/models/api_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ class ApiScope
:format => {
:with => CommonValidations::URL_PREFIX_FORMAT,
:message => :invalid_url_prefix_format,
},
:uniqueness => {
:scope => :host,
}

# Mass assignment security
Expand All @@ -46,4 +49,15 @@ def display_name
def root?
(self.path_prefix.blank? || self.path_prefix == "/")
end

def self.find_or_create_by_instance!(other)
attributes = other.attributes.slice("host", "path_prefix")
record = self.where(attributes).first
unless(record)
record = other
record.save!
end

record
end
end
1 change: 1 addition & 0 deletions src/api-umbrella/web-app/app/models/api_user_role.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ def self.all
all = user_roles + all_api_roles
all.uniq!
all.reject! { |role| role.blank? }
all.sort!

all
end
Expand Down
4 changes: 2 additions & 2 deletions src/api-umbrella/web-app/app/policies/admin_group_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ def resolve
scope.all
else
api_scope_ids = []
user.api_scopes_with_permission("admin_manage").each do |api_scope|
user.nested_api_scopes_with_permission("admin_manage").each do |api_scope|
api_scope_ids << api_scope.id
end

Expand All @@ -20,7 +20,7 @@ def show?
allowed = true
else
current_user_scope_ids = []
user.api_scopes_with_permission("admin_manage").each do |current_user_scope|
user.nested_api_scopes_with_permission("admin_manage").each do |current_user_scope|
current_user_scope_ids << current_user_scope.id
end

Expand Down
4 changes: 2 additions & 2 deletions src/api-umbrella/web-app/app/policies/admin_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ def resolve
scope.all
else
api_scope_ids = []
user.api_scopes_with_permission("admin_manage").each do |api_scope|
user.nested_api_scopes_with_permission("admin_manage").each do |api_scope|
api_scope_ids << api_scope.id
end

Expand All @@ -23,7 +23,7 @@ def show?
allowed = false
else
current_user_scope_ids = []
user.api_scopes_with_permission("admin_manage").each do |current_user_scope|
user.nested_api_scopes_with_permission("admin_manage").each do |current_user_scope|
current_user_scope_ids << current_user_scope.id
end

Expand Down
26 changes: 24 additions & 2 deletions src/api-umbrella/web-app/app/policies/api_scope_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,35 @@ def resolve
if(user.superuser?)
scope.all
else
scope.none
query_scopes = []
user.api_scopes_with_permission("admin_manage").each do |api_scope|
query_scopes << {
:host => api_scope.host,
:path_prefix => api_scope.path_prefix_matcher,
}
end

if(query_scopes.any?)
scope.or(query_scopes)
else
scope.none
end
end
end
end

def show?
user.superuser?
allowed = false
if(user.superuser?)
allowed = true
else
api_scopes = user.api_scopes_with_permission("admin_manage")
allowed = api_scopes.any? do |api_scope|
(record.host == api_scope.host && api_scope.path_prefix_matcher.match(record.path_prefix))
end
end

allowed
end

def update?
Expand Down
6 changes: 2 additions & 4 deletions src/api-umbrella/web-app/app/views/layouts/admin.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<title>API Umbrella Admin</title>
<%= stylesheet_link_tag "admin" %>
<%= javascript_tag do %>
webAdminAjaxApiKey = <%= ApiUser.where(:email => 'web.admin.ajax@internal.apiumbrella').order_by(:created_at.asc).first.api_key.to_json.html_safe %>;
webAdminAjaxApiKey = <%= web_admin_ajax_api_user.api_key.to_json.html_safe %>;
currentAdmin = <%= current_admin.attributes.slice("username", "superuser").to_json.html_safe %>
<% end %>
<link href='//fonts.googleapis.com/css?family=Oswald:400,700' rel='stylesheet' type='text/css'>
Expand Down Expand Up @@ -47,9 +47,7 @@
<li><%= link_to(%(<i class="fa fa-user"></i> #{t("admin.nav.admin_accounts")}).html_safe, '/admin/#/admins') %></li>
<li class="divider"></li>
<li role="presentation" class="dropdown-header"><%= t("admin.nav.permissions_management") %></li>
<% if(current_admin.superuser?) %>
<li><%= link_to(%(<i class="fa fa-lock"></i> #{t("admin.nav.api_scopes")}).html_safe, '/admin/#/api_scopes') %></li>
<% end %>
<li><%= link_to(%(<i class="fa fa-lock"></i> #{t("admin.nav.api_scopes")}).html_safe, '/admin/#/api_scopes') %></li>
<li><%= link_to(%(<i class="fa fa-group"></i> #{t("admin.nav.admin_groups")}).html_safe, '/admin/#/admin_groups') %></li>
<% end %>
</ul>
Expand Down
10 changes: 0 additions & 10 deletions src/api-umbrella/web-app/db/fixtures/api_users.rb

This file was deleted.

Loading

0 comments on commit e1a6b34

Please sign in to comment.