On Wed, 2011-01-19 at 08:48 -0500, Scott Seago wrote:
Privileges are now a combination of a base action and a target object
type (i.e. "modify" on Pool instead of the flat string "pool_modify").
In addition, the modeling has changed -- instead of a privileges model that was only
low-level privileges with an explicit mapping to roles, the privileges model now tracks
the privileges assigned to each role, with no separate mapping table.
This will require a database reload as the seeds.rb required substantial changes in
role/privilege loading
Minor feedback inline, will test fully after rebase.
Signed-off-by: Scott Seago <sseago(a)redhat.com>
---
.../admin/provider_accounts_controller.rb | 11 +-
src/app/controllers/admin/providers_controller.rb | 15 +-
src/app/controllers/admin/settings_controller.rb | 15 +--
src/app/controllers/admin/users_controller.rb | 32 +---
src/app/controllers/application_controller.rb | 11 +-
src/app/controllers/builds_controller.rb | 10 +-
src/app/controllers/cloud_accounts_controller.rb | 20 ++--
src/app/controllers/dashboard_controller.rb | 10 --
.../controllers/hardware_profiles_controller.rb | 1 -
.../controllers/image_factory/builds_controller.rb | 8 +-
.../image_factory/templates_controller.rb | 66 ++++++---
src/app/controllers/instances_controller.rb | 12 +-
src/app/controllers/permissions_controller.rb | 14 +-
src/app/controllers/pools_controller.rb | 38 ++----
src/app/controllers/providers_controller.rb | 27 ++--
src/app/controllers/quotas_controller.rb | 8 +-
.../controllers/resources/instances_controller.rb | 10 +-
src/app/controllers/resources/pools_controller.rb | 15 +-
src/app/controllers/settings_controller.rb | 17 +--
src/app/controllers/templates_controller.rb | 50 ++++--
src/app/controllers/users_controller.rb | 79 ++++------
src/app/models/cloud_account.rb | 21 +++
src/app/models/instance.rb | 27 +++-
src/app/models/permission.rb | 6 +
src/app/models/permissioned_object.rb | 136 +++++++---------
src/app/models/privilege.rb | 168 +++++++++++++-------
src/app/models/role.rb | 7 +-
src/app/models/template.rb | 5 +
src/app/services/application_service.rb | 35 +++--
src/app/views/admin/users/_form.haml | 4 +-
src/app/views/admin/users/edit.haml | 2 +-
src/app/views/dashboard/summary.haml | 20 ++--
src/app/views/layouts/_main_nav.html.erb | 2 +-
src/app/views/permissions/list.haml | 6 +-
src/app/views/pools/show.haml | 2 +-
src/app/views/providers/accounts.haml | 4 +-
src/app/views/users/_form.haml | 4 +-
src/app/views/users/edit.haml | 2 +-
.../20110107000000_privilege_model_refactor.rb | 53 ++++++
src/db/seeds.rb | 163 ++++++++------------
src/features/support/custom.rb | 4 -
src/features/support/hooks.rb | 17 --
src/lib/tasks/cucumber.rake | 6 +-
src/spec/controllers/instance_controller_spec.rb | 12 +-
src/spec/controllers/users_controller_spec.rb | 7 -
src/spec/factories/base_permission_object.rb | 3 -
src/spec/factories/metadata_object.rb | 34 ----
src/spec/factories/permission.rb | 8 +-
src/spec/factories/pool.rb | 2 +-
src/spec/factories/pool_family.rb | 2 +-
src/spec/factories/user.rb | 5 +-
src/spec/fixtures/base_permission_objects.yml | 2 -
src/spec/fixtures/privileges.yml | 43 -----
src/spec/fixtures/roles.yml | 47 ------
src/spec/models/metadata_object_spec.rb | 2 +-
src/spec/models/permission_spec.rb | 58 +++++++
src/spec/models/pool_family_spec.rb | 4 +-
src/spec/services/registration_service_spec.rb | 14 +--
src/spec/spec_helper.rb | 2 +-
59 files changed, 705 insertions(+), 703 deletions(-)
create mode 100644 src/db/migrate/20110107000000_privilege_model_refactor.rb
delete mode 100644 src/spec/factories/base_permission_object.rb
delete mode 100644 src/spec/factories/metadata_object.rb
delete mode 100644 src/spec/fixtures/base_permission_objects.yml
delete mode 100644 src/spec/fixtures/privileges.yml
delete mode 100644 src/spec/fixtures/roles.yml
create mode 100644 src/spec/models/permission_spec.rb
diff --git a/src/app/controllers/admin/provider_accounts_controller.rb
b/src/app/controllers/admin/provider_accounts_controller.rb
index 2c331f7..960f4f5 100644
--- a/src/app/controllers/admin/provider_accounts_controller.rb
+++ b/src/app/controllers/admin/provider_accounts_controller.rb
@@ -8,6 +8,7 @@ class Admin::ProviderAccountsController < ApplicationController
def show
@tab_captions = ['Properties', 'Credentials', 'History',
'Permissions']
@account = CloudAccount.find(params[:id])
+ require_privilege(Privilege::ACTION_VIEW, @account)
As we discussed in irc, since this has been simplified, it may be more
concise to change these to be like:
Privilege::VIEW
@details_tab = params[:details_tab].blank? ?
'properties' : params[:details_tab]
if params.delete :test_account
@@ -34,7 +35,7 @@ class Admin::ProviderAccountsController < ApplicationController
def create
@provider = Provider.find(params[:provider_id])
- require_privilege(Privilege::ACCOUNT_MODIFY, @provider)
+ require_privilege(Privilege::ACTION_CREATE, CloudAccount, @provider)
@providers = Provider.all
@cloud_account = CloudAccount.new(params[:cloud_account])
@@ -73,13 +74,13 @@ class Admin::ProviderAccountsController < ApplicationController
@cloud_account = CloudAccount.find(params[:id])
@quota = @cloud_account.quota
@provider = @cloud_account.provider
- require_privilege(Privilege::ACCOUNT_MODIFY,@provider)
+ require_privilege(Privilege::ACTION_MODIFY,@cloud_account)
end
def update
@cloud_account = CloudAccount.find(params[:id])
@provider = @cloud_account.provider
- require_privilege(Privilege::ACCOUNT_MODIFY, @provider)
+ require_privilege(Privilege::ACTION_MODIFY,@cloud_account)
@quota = @cloud_account.quota
if params.delete :test_account
@@ -101,7 +102,9 @@ class Admin::ProviderAccountsController < ApplicationController
if (not params[:accounts_selected]) or (params[:accounts_selected].length == 0)
flash[:notice] = "You must select some accounts first."
else
- CloudAccount.destroy(params[:accounts_selected])
+ CloudAccount.find(params[:accounts_selected]).each do |account|
+ account.destroy if check_privilege(Privilege::ACTION_MODIFY, account)
+ end
end
redirect_to admin_provider_accounts_url
end
diff --git a/src/app/controllers/admin/providers_controller.rb
b/src/app/controllers/admin/providers_controller.rb
index aac2c1d..61a405b 100644
--- a/src/app/controllers/admin/providers_controller.rb
+++ b/src/app/controllers/admin/providers_controller.rb
@@ -6,19 +6,20 @@ class Admin::ProvidersController < ApplicationController
end
def new
- require_privilege(Privilege::PROVIDER_MODIFY)
+ require_privilege(Privilege::ACTION_CREATE, Provider)
@provider = Provider.new
kick_condor
end
def edit
@provider = Provider.find_by_id(params[:id])
- require_privilege(Privilege::PROVIDER_MODIFY, @provider)
+ require_privilege(Privilege::ACTION_MODIFY, @provider)
end
def show
@provider = Provider.find(params[:id])
@url_params = params.clone
+ require_privilege(Privilege::ACTION_VIEW, @provider)
@tab_captions = ['Properties', 'HW Profiles', 'Realms',
'Provider Accounts', 'Services','History','Permissions']
@details_tab = params[:details_tab].blank? ? 'properties' :
params[:details_tab]
respond_to do |format|
@@ -33,7 +34,7 @@ class Admin::ProvidersController < ApplicationController
end
def create
- require_privilege(Privilege::PROVIDER_MODIFY)
+ require_privilege(Privilege::ACTION_CREATE, Provider)
@provider = Provider.new(params[:provider])
if params[:test_connection]
test_connection(@provider)
@@ -52,8 +53,8 @@ class Admin::ProvidersController < ApplicationController
end
def update
- require_privilege(Privilege::PROVIDER_MODIFY)
@provider = Provider.find_by_id(params[:id])
+ require_privilege(Privilege::ACTION_MODIFY, @provider)
previous_cloud_type = @provider.cloud_type
@provider.update_attributes(params[:provider])
if params[:test_connection]
@@ -77,7 +78,9 @@ class Admin::ProvidersController < ApplicationController
end
def multi_destroy
- Provider.destroy(params[:provider_selected])
+ Provider.find(params[:provider_selected]).each do |provider|
+ provider.destroy if check_privilege(Privilege::ACTION_MODIFY, provider)
+ end
redirect_to admin_providers_url
end
@@ -97,7 +100,7 @@ class Admin::ProvidersController < ApplicationController
@header = [{ :name => "Provider name", :sort_attr => :name },
{ :name => "Provider URL", :sort_attr => :name }
]
- @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)
+ @providers = Provider.list_for_user(@current_user, Privilege::ACTION_VIEW)
@url_params = params.clone
end
end
diff --git a/src/app/controllers/admin/settings_controller.rb
b/src/app/controllers/admin/settings_controller.rb
index accf398..2b816d4 100644
--- a/src/app/controllers/admin/settings_controller.rb
+++ b/src/app/controllers/admin/settings_controller.rb
@@ -6,18 +6,12 @@ class Admin::SettingsController < ApplicationController
KEYS = [SELF_SERVICE_DEFAULT_QUOTA]
def self_service
- if !is_admin?
- raise PermissionError.new('You have insufficient privileges to perform
action.')
- return
- end
+ require_privilege(Privilege::ACTION_MODIFY)
@self_service_default_quota = MetadataObject.lookup(SELF_SERVICE_DEFAULT_QUOTA)
end
def general_settings
- if !is_admin?
- raise PermissionError.new('You have insufficient privileges to perform
action.')
- return
- end
+ require_privilege(Privilege::ACTION_MODIFY)
end
def update
@@ -47,9 +41,4 @@ class Admin::SettingsController < ApplicationController
redirect_to :action => 'self_service'
end
- private
- def is_admin?
- is_admin = @current_user.permissions.collect { |p| p.role }.find { |r| r.name ==
"Administrator" }
- return is_admin == nil ? false : true
- end
end
diff --git a/src/app/controllers/admin/users_controller.rb
b/src/app/controllers/admin/users_controller.rb
index 0e854a9..4648da9 100644
--- a/src/app/controllers/admin/users_controller.rb
+++ b/src/app/controllers/admin/users_controller.rb
@@ -1,9 +1,9 @@
class Admin::UsersController < ApplicationController
before_filter :require_user
- before_filter :only_admin, :only => [:index, :multi_destroy]
before_filter :load_users, :only => [:index, :show]
def new
+ require_privilege(Privilege::ACTION_CREATE, User) unless current_user.nil?
@user = User.new
@user.quota = Quota.new
end
@@ -13,8 +13,7 @@ class Admin::UsersController < ApplicationController
redirect_to :action => 'new' and return
end
- # TODO: Shouldn't it be if current_user.nil? instead?
- require_privilege(Privilege::USER_MODIFY) unless current_user.nil?
+ require_privilege(Privilege::ACTION_MODIFY, User) unless current_user.nil?
@user = User.new(params[:user])
@registration = RegistrationService.new(@user)
@@ -34,6 +33,7 @@ class Admin::UsersController < ApplicationController
def show
@user = User.find_by_id(params[:id]) || current_user
+ require_privilege(Privilege::ACTION_VIEW, User) unless current_user == @user
@quota_resources = @user.quota.quota_resources
@url_params = params.clone
@@ -52,15 +52,12 @@ class Admin::UsersController < ApplicationController
def edit
@user = User.find_by_id(params[:id]) || current_user
-
- if cannot_modify_different_user?(@user)
- flash[:notice] = "Invalid Permission to perform this operation"
- redirect_to dashboard_url and return
- end
+ require_privilege(Privilege::ACTION_MODIFY, User) unless @user == current_user
end
def update
@user = User.find_by_id(params[:id]) || current_user
+ require_privilege(Privilege::ACTION_MODIFY, User) unless @user == current_user
if params[:commit] == "Reset"
redirect_to edit_admin_user_url(@user) and return
@@ -68,20 +65,16 @@ class Admin::UsersController < ApplicationController
redirect_to dashboard_url and return unless @user
- if cannot_modify_different_user = cannot_modify_different_user?(@user)
- flash[:notice] = "Invalid Permission to perform this operation"
- redirect_to dashboard_url and return
- end
-
unless @user.update_attributes(params[:user])
render :action => 'edit' and return
else
flash[:notice] = "User updated!"
- redirect_to cannot_modify_different_user ? dashboard_url : admin_users_url
+ redirect_to (@user == current_user) ? dashboard_url : admin_users_url
end
end
def multi_destroy
+ require_privilege(Privilege::ACTION_MODIFY, User)
User.destroy(params[:user_selected])
redirect_to admin_users_url
end
@@ -101,15 +94,4 @@ class Admin::UsersController < ApplicationController
end
end
- def only_admin
- unless current_user.permissions.collect { |p| p.role }.find { |r| r.name ==
"Administrator" }
- flash[:notice] = "Invalid Permission to perform this operation"
- redirect_to dashboard_url
- end
- end
-
- def cannot_modify_different_user?(user)
- user && user != current_user &&
!BasePermissionObject.general_permission_scope.can_modify_users(current_user)
- end
-
end
diff --git a/src/app/controllers/application_controller.rb
b/src/app/controllers/application_controller.rb
index b77fe9c..b0386e0 100644
--- a/src/app/controllers/application_controller.rb
+++ b/src/app/controllers/application_controller.rb
@@ -60,13 +60,6 @@ class ApplicationController < ActionController::Base
'generic'
end
- perm_helper_string = ""
- Privilege::FULL_PRIVILEGE_LIST.each do |privilege|
- perm_helper_string += "def has_#{privilege}?(obj=@perm_obj); " +
- "check_privilege(\"#{privilege}\", obj) end; "
- end
- master_helper_module.module_eval perm_helper_string
-
helper_method :check_privilege
protected
@@ -162,8 +155,8 @@ class ApplicationController < ActionController::Base
def get_nav_items
if current_user.present?
- @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)
- @pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW)
+ @providers = Provider.list_for_user(@current_user, Privilege::ACTION_VIEW)
+ @pools = Pool.list_for_user(@current_user, Privilege::ACTION_VIEW)
end
end
diff --git a/src/app/controllers/builds_controller.rb
b/src/app/controllers/builds_controller.rb
index 7cf0ad6..e205171 100644
--- a/src/app/controllers/builds_controller.rb
+++ b/src/app/controllers/builds_controller.rb
@@ -1,12 +1,12 @@
class BuildsController < ApplicationController
- before_filter [:require_user, :check_permission]
+ before_filter [:require_user]
This may be a moot point once I can actually apply this patch, but would
it make sense to keep the filter and limit it to certain methods (like
new) so we dont have to call it in the body of the method? Same thought
applies to the version of this in image_factory/
def section_id
'build'
end
def index
- require_privilege(Privilege::IMAGE_VIEW)
+ require_privilege(Privilege::ACTION_VIEW, Template)
order = get_order('templates.name')
@running_images = Image.all(:include => :template, :conditions => ['status
IN (?)', Image::ACTIVE_STATES], :order => order)
@completed_images = Image.all(:include => :template, :conditions => {:status
=> Image::STATE_COMPLETE}, :order => order)
@@ -16,6 +16,7 @@ class BuildsController < ApplicationController
def new
raise "select template to build" unless id = params[:template_id]
@tpl = Template.find(id)
+ check_permission
if @tpl.imported
flash[:warning] = "Build imported template is not supported"
redirect_to templates_path
@@ -30,6 +31,7 @@ class BuildsController < ApplicationController
end
<snip>
diff --git a/src/app/controllers/pools_controller.rb
b/src/app/controllers/pools_controller.rb
index 47cdc77..fdc3650 100644
--- a/src/app/controllers/pools_controller.rb
+++ b/src/app/controllers/pools_controller.rb
@@ -44,16 +44,19 @@ class PoolsController < ApplicationController
def show
@pool = Pool.find(params[:id])
+ require_privilege(Privilege::ACTION_VIEW, @pool)
end
def edit
@pool = Pool.find(params[:id])
+ require_privilege(Privilege::ACTION_MODIFY, @pool)
end
def list
#FIXME: clean this up, many error cases here
@pool = Pool.find(params[:id])
- require_privilege(Privilege::INSTANCE_VIEW,@pool)
+ # FIXME: really we need perm-filtered list here
+ require_privilege(Privilege::ACTION_VIEW, Instance, @pool)
list_for_user wouldn't work here?
@pool.reload
@order_dir = params[:order_dir] == 'desc' ? 'desc' : 'asc'
<snip>
diff --git a/src/app/controllers/resources/instances_controller.rb
b/src/app/controllers/resources/instances_controller.rb
index 3bd0c45..241dbba 100644
--- a/src/app/controllers/resources/instances_controller.rb
+++ b/src/app/controllers/resources/instances_controller.rb
@@ -5,7 +5,7 @@ class Resources::InstancesController < ApplicationController
def new
@instance = Instance.new(params[:instance])
- #require_privilege(Privilege::INSTANCE_MODIFY, @instance.pool) if @instance.pool
+ #require_privilege(Privilege::ACTION_CREATE, Instance, @instance.pool) if
@instance.pool
Is this meant to still be commented out? ^
unless @instance.template
redirect_to select_template_resources_instances_path
@@ -36,7 +36,7 @@ class Resources::InstancesController < ApplicationController
@instance.owner = current_user
begin
- require_privilege(Privilege::INSTANCE_MODIFY,
+ require_privilege(Privilege::ACTION_CREATE, Instance,
Pool.find((a)instance.pool_id))
<snip>
Rest seems ok on first pass, will review more once I can apply.
-j