commit 355bce36288ec6cd9f5a66b656f8a84158f2a8e1
Author: Ohad Levy <ohadlevy@gmail.com>
Date:   Wed Dec 25 11:10:16 2013 +0200

    fixes #3930 - refactoring of edit_self implementation.

diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb
index 6c669601b..da2ef784c 100644
--- a/app/controllers/api/v1/users_controller.rb
+++ b/app/controllers/api/v1/users_controller.rb
@@ -1,6 +1,8 @@
 module Api
   module V1
     class UsersController < V1::BaseController
+      include Foreman::Controller::UsersMixin
+
       before_filter :find_resource, :only => %w{show update destroy}
 
       api :GET, "/users/", "List all users."
@@ -36,10 +38,7 @@ module Api
       end
 
       def create
-        admin = params[:user].delete(:admin)
-        @user = User.new(params[:user]) { |u| u.admin = admin }
         if @user.save
-          @user.roles << Role.find_by_name("Anonymous") unless @user.roles.map(&:name).include? "Anonymous"
           process_success
         else
           process_resource_error
@@ -63,18 +62,9 @@ module Api
       end
 
       def update
-        admin = params[:user].has_key?(:admin) ? params[:user].delete(:admin) : nil
-        # Remove keys for restricted variables when the user is editing their own account
-        if @user == User.current
-          for key in params[:user].keys
-            params[:user].delete key unless %w{password_confirmation password mail firstname lastname}.include? key
-          end
-        end
         if @user.update_attributes(params[:user])
-          # Only an admin can update admin attribute of another use
-          # this is required, as the admin field is blacklisted above
-          @user.update_attribute(:admin, admin) if User.current.admin and !admin.nil?
-          @user.roles << Role.find_by_name("Anonymous") unless @user.roles.map(&:name).include? "Anonymous"
+          update_sub_hostgroups_owners
+
           process_success
         else
           process_resource_error
diff --git a/app/controllers/api/v2/users_controller.rb b/app/controllers/api/v2/users_controller.rb
index dbcc15c46..a5812022f 100644
--- a/app/controllers/api/v2/users_controller.rb
+++ b/app/controllers/api/v2/users_controller.rb
@@ -1,7 +1,7 @@
 module Api
   module V2
     class UsersController < V2::BaseController
-
+      include Foreman::Controller::UsersMixin
       include Api::Version2
       include Api::TaxonomyScope
       before_filter :find_resource, :only => %w{show update destroy}
@@ -39,10 +39,7 @@ module Api
       end
 
       def create
-        admin = params[:user].delete(:admin)
-        @user = User.new(params[:user]) { |u| u.admin = admin }
         if @user.save
-          @user.roles << Role.find_by_name("Anonymous") unless @user.roles.map(&:name).include? "Anonymous"
           process_success
         else
           process_resource_error
@@ -66,18 +63,9 @@ module Api
       end
 
       def update
-        admin = params[:user].has_key?(:admin) ? params[:user].delete(:admin) : nil
-        # Remove keys for restricted variables when the user is editing their own account
-        if @user == User.current
-          for key in params[:user].keys
-            params[:user].delete key unless %w{password_confirmation password mail firstname lastname}.include? key
-          end
-        end
         if @user.update_attributes(params[:user])
-          # Only an admin can update admin attribute of another use
-          # this is required, as the admin field is blacklisted above
-          @user.update_attribute(:admin, admin) if User.current.admin and !admin.nil?
-          @user.roles << Role.find_by_name("Anonymous") unless @user.roles.map(&:name).include? "Anonymous"
+          update_sub_hostgroups_owners
+
           process_success
         else
           process_resource_error
diff --git a/app/controllers/concerns/foreman/controller/authentication.rb b/app/controllers/concerns/foreman/controller/authentication.rb
index 029bea908..34889b93e 100644
--- a/app/controllers/concerns/foreman/controller/authentication.rb
+++ b/app/controllers/concerns/foreman/controller/authentication.rb
@@ -40,9 +40,7 @@ module Foreman::Controller::Authentication
   end
 
   def authorized
-    User.current.allowed_to?(
-      :controller => params[:controller].gsub(/::/, "_").underscore,
-      :action     => params[:action])
+    User.current.allowed_to?(params.slice(:controller, :action, :id))
   end
 
   def require_login
diff --git a/app/controllers/concerns/foreman/controller/users_mixin.rb b/app/controllers/concerns/foreman/controller/users_mixin.rb
new file mode 100644
index 000000000..63def7983
--- /dev/null
+++ b/app/controllers/concerns/foreman/controller/users_mixin.rb
@@ -0,0 +1,47 @@
+module Foreman::Controller::UsersMixin
+  extend ActiveSupport::Concern
+
+  included do
+    before_filter :set_admin_on_creation, :only => :create
+    before_filter :clear_params_on_update, :update_admin_flag, :only => :update
+  end
+
+  protected
+  def set_admin_on_creation
+    admin = params[:user].delete :admin
+    @user = User.new(params[:user]) { |u| u.admin = admin }
+  end
+
+  def clear_params_on_update
+    find_resource
+    if params[:user]
+      @admin = params[:user].has_key?(:admin) ? params[:user].delete(:admin) : nil
+      # Remove keys for restricted variables when the user is editing their own account
+      if editing_self?
+        params[:user].slice!(:password_confirmation, :password, :mail, :firstname, :lastname, :locale)
+
+        # Remove locale from the session when set to "Browser Locale" and editing self
+        session.delete(:locale) if params[:user][:locale].try(:empty?)
+      end
+    end
+  end
+
+  def update_admin_flag
+    # Only an admin can update admin attribute of another user
+    # this is required, as the admin field is blacklisted above
+    @user.admin = @admin if User.current.admin && !@admin.nil?
+  end
+
+  def editing_self?
+    @editing_self ||= User.current.editing_self?(params.slice(:controller, :action, :id))
+  end
+
+  def update_sub_hostgroups_owners
+    return if params[:user]['hostgroup_ids'].empty?
+    hostgroup_ids = params[:user]['hostgroup_ids'].reject(&:empty?).map(&:to_i)
+    return if hostgroup_ids.empty?
+
+    sub_hg = Hostgroup.where(:id => hostgroup_ids).map(&:subtree).flatten.reject { |hg| hg.user_ids.include?(@user.id) }
+    sub_hg.each { |hg| hg.users << @user }
+  end
+end
\ No newline at end of file
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 0bf1545ce..db1594f60 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1,14 +1,13 @@
 class UsersController < ApplicationController
   include Foreman::Controller::AutoCompleteSearch
+  include Foreman::Controller::UsersMixin
 
-  before_filter :find_user, :only => [:edit, :update, :destroy]
+  before_filter :find_resource, :only => [:edit, :update, :destroy]
   skip_before_filter :require_mail, :only => [:edit, :update, :logout]
   skip_before_filter :require_login, :authorize, :session_expiry, :update_activity_time, :set_taxonomy, :set_gettext_locale_db, :only => [:login, :logout, :extlogout]
   skip_before_filter :authorize, :only => :extlogin
   after_filter       :update_activity_time, :only => :login
 
-  attr_accessor :editing_self
-
   def index
     begin
       users = User.search_for(params[:search], :order => params[:order])
@@ -24,10 +23,7 @@ class UsersController < ApplicationController
   end
 
   def create
-    admin = params[:user].delete :admin
-    @user = User.new(params[:user]){|u| u.admin = admin }
     if @user.save
-      @user.roles << Role.find_by_name("Anonymous") unless @user.roles.map(&:name).include? "Anonymous"
       process_success
     else
       process_error
@@ -35,6 +31,7 @@ class UsersController < ApplicationController
   end
 
   def edit
+    editing_self?
     if @user.user_facts.count == 0
       user_fact = @user.user_facts.build :operator => "==", :andor => "or"
       user_fact.fact_name_id = FactName.first.id if FactName.first
@@ -42,31 +39,13 @@ class UsersController < ApplicationController
   end
 
   def update
-    # Remove keys for restricted variables when the user is editing their own account
-    if editing_self
-      for key in params[:user].keys
-        params[:user].delete key unless %w{password_confirmation password mail firstname lastname locale}.include? key
-      end
-      User.current.editing_self = true
-    end
-
-    # Only an admin can update admin attribute of another user
-    # this is required, as the admin field is blacklisted above
-    admin = params[:user].delete :admin
-    @user.admin = admin if User.current.admin
-
     if @user.update_attributes(params[:user])
-      @user.roles << Role.find_by_name("Anonymous") unless @user.roles.map(&:name).include? "Anonymous"
-      hostgroup_ids = params[:user]["hostgroup_ids"].reject(&:empty?).map(&:to_i) unless params[:user]["hostgroup_ids"].empty?
-      update_hostgroups_owners(hostgroup_ids) unless hostgroup_ids.empty?
-      process_success editing_self ? { :success_redirect => hosts_path } : {}
+      update_sub_hostgroups_owners
+
+      process_success((editing_self? && !current_user.allowed_to?({:controller => 'users', :action => 'index'})) ? { :success_redirect => hosts_path } : {})
     else
       process_error
     end
-    User.current.editing_self = false if editing_self
-
-    # Remove locale from the session when set to "Browser Locale" and editing self
-    session.delete(:locale) if params[:user][:locale].try(:empty?) and params[:id].to_i == User.current.id
   end
 
   def destroy
@@ -122,29 +101,9 @@ class UsersController < ApplicationController
   end
 
   private
-  def authorize(ctrl = params[:controller], action = params[:action])
-    # Editing self is true when the user is granted access to just their own account details
-
-    if action == 'auto_complete_search' and User.current.allowed_to?({:controller => ctrl, :action => 'index'})
-      return true
-    end
-
-    self.editing_self = false
-    return true if User.current.allowed_to?({:controller => ctrl, :action => action})
-    if (action =~ /edit|update/ and params[:id].to_i == User.current.id)
-      return self.editing_self = true
-    else
-      deny_access and return
-    end
-  end
-
-  def find_user
-    @user = User.find(params[:id])
-  end
 
-  def update_hostgroups_owners(hostgroup_ids)
-    subhostgroups = Hostgroup.where(:id => hostgroup_ids).map(&:subtree).flatten.reject { |hg| hg.users.include?(@user) }
-    subhostgroups.each { |subhs| subhs.users << @user }
+  def find_resource
+    @user ||= User.find(params[:id])
   end
 
   def login_user(user)
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index d77d0b5f1..5523ee9e1 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -96,8 +96,8 @@ module ApplicationHelper
   # Return true if user is authorized for controller/action, otherwise false
   # +controller+ : String or symbol for the controller
   # +action+     : String or symbol for the action
-  def authorized_for(controller, action)
-    User.current.allowed_to?({:controller => controller.to_s.gsub(/::/, "_").underscore, :action => action}) rescue false
+  def authorized_for(controller, action, id = nil)
+    User.current.allowed_to?({:controller => controller, :action => action, :id => id}) rescue false
   end
 
   # Display a link if user is authorized, otherwise a string
@@ -107,9 +107,12 @@ module ApplicationHelper
   #             :auth_action : String or Symbol representing the action to be used for authorization checks
   # +html_options+ : Hash containing html options for the link or span
   def link_to_if_authorized(name, options = {}, html_options = {})
-    auth_action = options.delete :auth_action
-    enable_link = authorized_for(options[:controller] || params[:controller], auth_action || options[:action])
-    if enable_link
+    auth_options = {
+      :controller => options[:controller]         || params[:controller],
+      :action     => options.delete(:auth_action) || options[:action],
+      :id         => options[:id]
+    }
+    if User.current.allowed_to?(auth_options)
       link_to name, options, html_options
     else
       link_to_function name, nil, html_options.merge!(:class => "#{html_options[:class]} disabled", :disabled => true)
diff --git a/app/models/concerns/authorization.rb b/app/models/concerns/authorization.rb
index c64f48f46..bd8afaf50 100644
--- a/app/models/concerns/authorization.rb
+++ b/app/models/concerns/authorization.rb
@@ -26,11 +26,20 @@ module Authorization
 
     klass   = self.class.name.downcase
     klasses   = self.class.name.tableize
+    #TODO: Extract all fo the specific implementations into each individual class
     klasses.gsub!(/auth_source.*/, "authenticators")
     klasses.gsub!(/common_parameters.*/, "global_variables")
     klasses.gsub!(/lookup_key.*/, "external_variables")
     klasses.gsub!(/lookup_value.*/, "external_variables")
-    return true if User.current and User.current.allowed_to?("#{operation}_#{klasses}".to_sym)
+    # editing own user is a special case
+    if User.current
+      action = if klass == 'user'
+                 { :controller => 'users', :action => operation }
+               else
+                 "#{operation}_#{klasses}".to_sym
+               end
+      return true if User.current.allowed_to?(action)
+    end
 
     errors.add :base, _("You do not have permission to %{operation} this %{klass}") % { :operation => operation, :klass => klass }
     @permission_failed = operation
diff --git a/app/models/user.rb b/app/models/user.rb
index c515b5bee..0a2c71bf5 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -8,8 +8,9 @@ class User < ActiveRecord::Base
   self.auditing_enabled = !(File.basename($0) == "rake" && ARGV.include?("db:migrate"))
 
   attr_protected :password_hash, :password_salt, :admin
-  attr_accessor :password, :password_confirmation, :editing_self
+  attr_accessor :password, :password_confirmation
   before_destroy EnsureNotUsedBy.new(:direct_hosts, :hostgroups), :ensure_admin_is_not_deleted
+  after_commit :ensure_default_role
 
   belongs_to :auth_source
   has_many :auditable_changes, :class_name => '::Audit', :as => :user
@@ -188,9 +189,13 @@ class User < ActiveRecord::Base
   # action can be:
   # * a parameter-like Hash (eg. :controller => 'projects', :action => 'edit')
   # * a permission Symbol (eg. :edit_project)
-  def allowed_to?(action, options={})
+  def allowed_to?(action)
     return true if admin?
-    return true if editing_self
+    if action.is_a? Hash
+      # normalize controller name
+      action[:controller] = action[:controller].to_s.gsub(/::/, "_").sub(/^\//,'').underscore
+      return true if editing_self?(action)
+    end
     roles.detect {|role| role.allowed_to?(action)}.present?
   end
 
@@ -235,6 +240,12 @@ class User < ActiveRecord::Base
     @role_ids_was ||= role_ids
   end
 
+  def editing_self?(options = {})
+    options[:controller].to_s == 'users' &&
+      options[:action] =~ /edit|update/ &&
+      options[:id].to_i == self.id
+  end
+
   private
 
   def prepare_password
@@ -328,4 +339,9 @@ class User < ActiveRecord::Base
       errors.add :admin, _("You can't change Administrator flag")
     end
   end
+
+  def ensure_default_role
+    role = Role.find_by_name('Anonymous')
+    self.roles << role unless self.role_ids.include?(role.id)
+  end
 end
diff --git a/app/services/menu/item.rb b/app/services/menu/item.rb
index 1ebe472f8..419d6a4f8 100644
--- a/app/services/menu/item.rb
+++ b/app/services/menu/item.rb
@@ -33,10 +33,7 @@ module Menu
     end
 
     def authorized?
-      User.current.allowed_to?({
-        :controller => url_hash[:controller].to_s.gsub(/::/, "_").underscore,
-        :action => url_hash[:action]
-      })
+      User.current.allowed_to?(url_hash.slice(:controller, :action, :id))
     rescue => error
       Rails.logger.error "#{error.message} (#{error.class})\n#{error.backtrace.join("\n")}"
       false
diff --git a/app/services/menu/loader.rb b/app/services/menu/loader.rb
index 94e5b09f7..cd908e902 100644
--- a/app/services/menu/loader.rb
+++ b/app/services/menu/loader.rb
@@ -5,7 +5,7 @@ module Menu
         menu.item :logout,               :caption => N_('Sign out'),
                   :url_hash => {:controller => '/users', :action => 'logout'}
         menu.item :my_account,           :caption => N_('My account'),
-                  :url_hash => {:controller => '/users', :action => 'edit', :id => Proc.new { User.current }}
+                  :url_hash => {:controller => '/users', :action => 'edit', :id => Proc.new { User.current.id }}
       end
 
       Manager.map :admin_menu do |menu|
diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb
index 438beb3fb..891e17993 100644
--- a/app/views/users/index.html.erb
+++ b/app/views/users/index.html.erb
@@ -15,7 +15,7 @@
   </tr>
   <% for user in @users %>
     <tr>
-      <td><%= gravatar_image_tag user.mail, :class => "gravatar" %> <%=link_to_if_authorized h(user.login), hash_for_edit_user_path(:id => user) %></td>
+      <td><%= gravatar_image_tag user.mail, :class => "gravatar" %> <%=link_to_if_authorized h(user.login), hash_for_edit_user_path(:id => user.id) %></td>
       <td><%=h user.firstname %></td>
       <td><%=h user.lastname %></td>
       <td><%=h user.mail %></td>
diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb
index fadaebdaf..0287b756f 100644
--- a/test/functional/users_controller_test.rb
+++ b/test/functional/users_controller_test.rb
@@ -114,12 +114,13 @@ class UsersControllerTest < ActionController::TestCase
 
   test "should modify session when locale is updated" do
     User.current = User.admin
-    put :update, {:id => User.admin.id, :user => { :locale => "cs" } }, set_session_user
+    put :update, { :id => User.admin.id, :user => { :locale => "cs" } }, set_session_user
     assert_redirected_to users_url
-    assert User.admin.locale == "cs"
+    assert_equal "cs", User.admin.locale
+
     put :update, { :id => User.admin.id, :user => { :locale => "" } }, set_session_user
-    assert User.admin.locale.nil?
-    assert session[:locale].nil?
+    assert_nil User.admin.locale
+    assert_nil session[:locale]
   end
 
   test "should not delete same user" do
@@ -135,7 +136,7 @@ class UsersControllerTest < ActionController::TestCase
 
   test 'user with viewer rights should fail to edit a user' do
     get :edit, {:id => User.first.id}
-    assert_equal @response.status, 403
+    assert_response 403
   end
 
   test 'user with viewer rights should succeed in viewing users' do
@@ -220,4 +221,28 @@ class UsersControllerTest < ActionController::TestCase
     assert_redirected_to edit_user_path(User.find_by_login('ares'))
   end
 
+  test 'non admin user should edit itself' do
+    User.current = users(:one)
+    get :edit, { :id => User.current.id }
+    assert_response :success
+  end
+
+  test 'non admin user should be able to update itself' do
+    User.current = users(:one)
+    put :update, { :id => users(:one).id, :user => { :firstname => 'test' } }
+    assert_response :success
+  end
+
+  test 'non admin user should not be able to edit another user' do
+    User.current = users(:one)
+    get :edit, { :id => users(:two) }
+    assert_response 403
+  end
+
+  test 'non admin user should not be able to update another user' do
+    User.current = users(:one)
+    put :update, { :id => users(:two).id, :user => { :firstname => 'test' } }
+    assert_response 403
+  end
+
 end
diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb
index 98d43a45b..6d61de219 100644
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -288,8 +288,8 @@ class UserTest < ActiveSupport::TestCase
 
   test "user with destroy permissions should not be able to edit" do
     setup_user "destroy"
-    record      =  users(:one)
-    record.login = "renamed"
+    record       = users(:two)
+    record.login = 'renamed'
     assert !record.save
     assert record.valid?
   end
@@ -411,4 +411,24 @@ class UserTest < ActiveSupport::TestCase
 
   end
 
+  test 'user should allow editing self?' do
+    User.current = users(:one)
+
+    # edit self
+    options = {:controller => 'users', :action => 'edit', :id => User.current.id}
+    assert User.current.editing_self?(options)
+
+    # update self
+    options = {:controller => 'users', :action => 'update', :id => User.current.id}
+    assert User.current.editing_self?(options)
+
+    # update someone else
+    options = {:controller => 'users', :action => 'update', :id => users(:two).id}
+    assert_not User.current.editing_self?(options)
+
+    # update for another controller
+    options = {:controller => 'hosts', :action => 'update', :id => User.current.id}
+    assert_not User.current.editing_self?(options)
+  end
+
 end
