Ensure plugin params are set before checking exclusion, and remove unused variable. #1602

Merged
merged 6 commits into from Apr 22, 2012

Projects

None yet

3 participants

@robyurkowski

This will cause issues in exceedingly rare circumstances.

@robyurkowski

...to clarify, the issue, not the fix, will cause issues.

(p'arndt)

@parndt parndt and 1 other commented on an outdated diff Apr 18, 2012
...on/app/controllers/refinery/admin/users_controller.rb
# Prevent the current user from locking themselves out of the User manager
- if current_refinery_user.id == @user.id and (params[:user][:plugins].exclude?("refinery_users") || @selected_role_names.map(&:downcase).exclude?("refinery"))
+ if current_refinery_user.id == @user.id and ((params[:user][:plugins].present? && params[:user][:plugins].exclude?("refinery_users")) || @selected_role_names.map(&:downcase).exclude?("refinery"))
@parndt
parndt Apr 18, 2012

Would you care to refactor this entire condition? It's really confusing.

@parndt
parndt Apr 18, 2012

Plus I think we should consider using && instead of and. What do you think?

@robyurkowski
robyurkowski Apr 18, 2012

Definitely on the last, and I'm going to reformat it, if not refactor it — I'm not really sure how you would make it more concise.

@parndt parndt commented on an outdated diff Apr 19, 2012
...on/app/controllers/refinery/admin/users_controller.rb
@@ -50,10 +50,14 @@ def update
unless current_refinery_user.has_role?(:superuser) and Refinery::Authentication.superuser_can_assign_roles
@selected_role_names = @user.roles.collect(&:title)
end
- @selected_plugin_names = params[:user][:plugins]
+ @selected_plugins = params[:user][:plugins]
+
+ # Prevent the current user from locking themselves out of the User manager or backend
+ if current_refinery_user.id == @user.id && # If editing self
+ ((@selected_plugins.present? && # If we're submitting plugins
+ @selected_plugins.exclude?("refinery_users")) || # And we're removing user plugin access
+ @selected_role_names.map(&:downcase).exclude?("refinery")) # Or we're removing the refinery role
@parndt
parndt Apr 19, 2012

blank line alert

@ugisozols ugisozols commented on an outdated diff Apr 19, 2012
...on/app/controllers/refinery/admin/users_controller.rb
@@ -50,10 +50,14 @@ def update
unless current_refinery_user.has_role?(:superuser) and Refinery::Authentication.superuser_can_assign_roles
@selected_role_names = @user.roles.collect(&:title)
end
- @selected_plugin_names = params[:user][:plugins]
@ugisozols
ugisozols Apr 19, 2012

@selected_plugin_names is used in the form partial and it'll throw an exception if you remove refinery_users node and try to save the form.

@robyurkowski

Ah, sorry, guys, I've been asleep. Nice catches.

@ugisozols
Refinery member

plucked!

@parndt parndt merged commit 48485bb into master Apr 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment