[PATCH conductor] bug 798516: prevent last admin deletion

Scott Seago sseago at redhat.com
Thu Sep 6 04:14:17 UTC 2012


On 09/05/2012 07:46 PM, Tzu-Mainn Chen wrote:
> ACK'd and pushed.  The BZ comments mentioned a potential flash message once a user takes away his own admin rights; I'm guessing that was deemed unnecessary. . . ?
>
> Mainn
Yeah, I'm setting the flash[:notice] but it wasn't coming through. At 
first I thought it was an issue with the redirect, but then I noticed 
that _nothing_ is displaying flash messages on the user profile page, so 
I suspect it's a layout/haml issue.

Scott
> ----- Original Message -----
>> From: "Scott Seago" <sseago at redhat.com>
>> To: aeolus-devel at lists.fedorahosted.org
>> Cc: "Scott Seago" <sseago at redhat.com>
>> Sent: Wednesday, September 5, 2012 10:41:05 AM
>> Subject: [PATCH conductor]  bug 798516: prevent last admin deletion
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=798516
>>
>> Don't allow deletion of admin permissions if there is only one admin
>> left.
>> Upon permission removal, redirect user to /account if the user's
>> own permissions to that page have been revoked.
>> ---
>>   src/app/controllers/application_controller.rb       |  5 +++++
>>   src/app/controllers/permissions_controller.rb       |  6 ++++--
>>   src/app/models/base_permission_object.rb            | 13
>>   +++++++++++++
>>   src/app/views/permissions/_permissions.html.haml    |  6 ++++--
>>   src/spec/controllers/permissions_controller_spec.rb |  2 +-
>>   5 files changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/app/controllers/application_controller.rb
>> b/src/app/controllers/application_controller.rb
>> index 2d7f325..64f3498 100644
>> --- a/src/app/controllers/application_controller.rb
>> +++ b/src/app/controllers/application_controller.rb
>> @@ -44,6 +44,11 @@ class ApplicationController <
>> ActionController::Base
>>     # permissions checking
>>   
>>     def handle_perm_error(error)
>> +    if params[:return_from_permission_change]
>> +      flash.now
>> +      redirect_to account_url
>> +      return
>> +    end
>>       handle_error(:error => error, :status => :forbidden,
>>                    :title =>
>>                    t('application_controller.access_denied'))
>>     end
>> diff --git a/src/app/controllers/permissions_controller.rb
>> b/src/app/controllers/permissions_controller.rb
>> index 33c2a7e..0bc90cb 100644
>> --- a/src/app/controllers/permissions_controller.rb
>> +++ b/src/app/controllers/permissions_controller.rb
>> @@ -220,7 +220,7 @@ class PermissionsController <
>> ApplicationController
>>       raise RuntimeError, "invalid permission object" if
>>       @permission_object.nil?
>>       unless @return_path
>>         if @permission_object ==
>>         BasePermissionObject.general_permission_scope
>> -        @return_path = permissions_path
>> +        @return_path =
>> permissions_path(:return_from_permission_change => true)
>>           set_admin_users_tabs 'permissions'
>>         else
>>           @return_path = send("#{@path_prefix}polymorphic_path",
>> @@ -230,7 +230,9 @@ class PermissionsController <
>> ApplicationController
>>                                 @polymorphic_path_extras) :
>>                               @permission_object,
>>                               @use_tabs == "yes" ? {:details_tab =>
>>                               :permissions,
>> -                              :only_tab => true} : {})
>> +                              :only_tab => true,
>> +                              :return_from_permission_change =>
>> true} :
>> +                             {:return_from_permission_change =>
>> true})
>>         end
>>       end
>>       require_privilege(required_role, @permission_object)
>> diff --git a/src/app/models/base_permission_object.rb
>> b/src/app/models/base_permission_object.rb
>> index c5d525a..faf5e53 100644
>> --- a/src/app/models/base_permission_object.rb
>> +++ b/src/app/models/base_permission_object.rb
>> @@ -56,4 +56,17 @@ class BasePermissionObject < ActiveRecord::Base
>>       [HardwareProfile, Catalog, Deployable, PoolFamily, Pool,
>>        Deployment, Instance, Provider, ProviderAccount]
>>     end
>> +
>> +  def self.global_admin_permission_count
>> +    self.general_permission_scope.permissions.includes(:role =>
>> :privileges).
>> +      where("privileges.target_type" => "BasePermissionObject",
>> +            "privileges.action" => Privilege::PERM_SET).size
>> +  end
>> +
>> +  def self.is_global_admin_perm(permission)
>> +    permission.role.privileges.where("privileges.target_type" =>
>> +                                       "BasePermissionObject",
>> +                                     "privileges.action" =>
>> +                                       Privilege::PERM_SET).size > 0
>> +  end
>>   end
>> diff --git a/src/app/views/permissions/_permissions.html.haml
>> b/src/app/views/permissions/_permissions.html.haml
>> index 4f2818d..6d8ab32 100644
>> --- a/src/app/views/permissions/_permissions.html.haml
>> +++ b/src/app/views/permissions/_permissions.html.haml
>> @@ -1,3 +1,5 @@
>> +- has_admin_perms = check_privilege(Privilege::PERM_SET)
>> +- prevent_admin_deletion = has_admin_perms && !(@show_inherited ||
>> @show_global) && (@permission_object ==
>> BasePermissionObject.general_permission_scope) &&
>> (BasePermissionObject.global_admin_permission_count == 1)
>>   - content_for :permissions_form_header do
>>     - if not(@show_inherited or @show_global) &&
>>     check_privilege(Privilege::PERM_SET)
>>       %li= link_to t('permissions.form.grant_access'),
>>       new_permission_path(:permission_object_type =>
>>       @permission_object.class.name, :permission_object_id =>
>>       @permission_object.id, :path_prefix => @path_prefix, :use_tabs
>>       => @use_tabs ? @use_tabs : (@tabs ? 'yes' : 'no')), { :class =>
>>       'button primary', :id => 'new_permission_button'}
>> @@ -51,11 +53,11 @@
>>     %tr{:class => cycle('nostripe','stripe')}
>>       - if not(@show_inherited or @show_global)
>>         %td{:class => 'checkbox'}
>> -        - if check_privilege(Privilege::PERM_SET)
>> +        - if has_admin_perms && !(prevent_admin_deletion &&
>> BasePermissionObject.is_global_admin_perm(permission))
>>             - selected = params[:select] == 'all'
>>             = check_box_tag "permission_selected[]", permission.id,
>>             selected, :id => "permission_checkbox_#{permission.id}"
>>       %td= link_to permission.entity.name,
>>       ((permission.entity.entity_target_type == "User") ?
>>       user_path(permission.entity.user) :
>>       user_group_path(permission.entity.user_group))
>> -    - if not(@show_inherited or @show_global) &&
>> check_privilege(Privilege::PERM_SET)
>> +    - if not(@show_inherited or @show_global) && has_admin_perms &&
>> !(prevent_admin_deletion &&
>> BasePermissionObject.is_global_admin_perm(permission))
>>         %td= select_tag "permission_role_selected[]",
>>         options_for_select(@roles.map {|r| [ t(r.name, :scope=>
>>         :role_defs, :default => r.name), "#{permission.id},#{r.id}" ]
>>         }, :selected => "#{permission.id},#{permission.role.id}",
>>         :disabled => @permission_object.permissions.where(["entity_id
>>         = ? and role_id != ?", permission.entity_id,
>>         permission.role_id]).collect {|p|
>>         "#{permission.id},#{p.role.id}"}), :id =>
>>         "permission_role_selected_#{permission.id}"
>>         :javascript
>>           $("#permission_role_selected_#{permission.id}").change(function
>>           () { $("#perm_edit_button").click(); } );
>> diff --git a/src/spec/controllers/permissions_controller_spec.rb
>> b/src/spec/controllers/permissions_controller_spec.rb
>> index a27d669..4538dc9 100644
>> --- a/src/spec/controllers/permissions_controller_spec.rb
>> +++ b/src/spec/controllers/permissions_controller_spec.rb
>> @@ -36,7 +36,7 @@ describe PermissionsController do
>>       post :multi_update, :permission_object_id => @deployable.id,
>>       :permission_object_type => @deployable.class.to_s,
>>           :permission_role_selected =>
>>           ["#{@permission.id},#{@new_role.id}"],
>>           :polymorphic_path_extras => { 'catalog_id' => @catalog.id}
>>   
>> -    response.should redirect_to catalog_deployable_path(@catalog,
>> @deployable)
>> +    response.should redirect_to catalog_deployable_path(@catalog,
>> @deployable, :return_from_permission_change => true)
>>     end
>>   
>>     it "should work for global role grants" do
>> --
>> 1.7.11.4
>>
>>




More information about the aeolus-devel mailing list