Avoid user delete admin access of current user logged

Fix #456

Avoid user delete admin access of current user logged
Fix #456
Cyril Mougel
1 parent a7870164
Showing 4 changed files with 59 additions and 26 deletions Show diff stats
CHANGELOG.md
app/controllers/users_controller.rb
config/locales/en.yml
spec/controllers/users_controller_spec.rb
@@ -5,6 +5,8 @@
 - Update some gems ([@shingara][])
 - [#492][] Improve some Pjax call ([@nfedyashev][])
 - [#428][] Add the support of Unfuddle Tracker ([@parallel588][])
+- Avoid to delete his own user ([@shingara][])
+- [#456] Avoid to delete admin access of current user logged ([@shingara][])
 ### Bug Fixes
@@ -48,6 +50,7 @@
 [#428]: https://github.com/errbit/errbit/issues/428
 [#453]: https://github.com/errbit/errbit/issues/453
 [#455]: https://github.com/errbit/errbit/issues/455
+[#456]: https://github.com/errbit/errbit/issues/456
 [#457]: https://github.com/errbit/errbit/issues/457
 [#460]: https://github.com/errbit/errbit/issues/460
 [#466]: https://github.com/errbit/errbit/issues/466
@@ -25,26 +25,25 @@ class UsersController &lt; ApplicationController
   end
   def update
-    # Devise Hack
-    # if params[:user][:password].blank? && params[:user][:password_confirmation].blank?
-    #   params[:user].delete(:password)
-    #   params[:user].delete(:password_confirmation)
-    # end
-
     if user.update_attributes(user_params)
-      flash[:success] = "#{user.name}'s information was successfully updated"
+      flash[:success] = I18n.t('controllers.users.flash.update.success', :name => user.name)
       redirect_to user_path(user)
     else
       render :edit
     end
   end
+  ##
+  # Destroy the user pass in args
+  #
+  # @param [ String ] id the id of user we want delete
+  #
   def destroy
     if user == current_user
       flash[:error] = I18n.t('controllers.users.flash.destroy.error')
     else
       UserDestroy.new(user).destroy
-      flash[:success] = "That's sad. #{user.name} is no longer part of your team."
+      flash[:success] = I18n.t('controllers.users.flash.destroy.success', :name => user.name)
     end
     redirect_to users_path
   end
@@ -62,14 +61,19 @@ class UsersController &lt; ApplicationController
     end
   def user_params
-    params[:user] ? params.require(:user).permit(*user_permit_params) : {}
+    @user_params ||= params[:user] ? params.require(:user).permit(*user_permit_params) : {}
   end
   def user_permit_params
-    @user_permit_params ||= [:name, :username, :email, :github_login, :per_page, :time_zone, :password, :password_confirmation]
-    @user_permit_params << :admin if current_user.admin?
+    @user_permit_params ||= [:name,:username, :email, :github_login, :per_page, :time_zone]
+    @user_permit_params << :admin if current_user.admin? && current_user.id != params[:id]
+    @user_permit_params |= [:password, :password_confirmation] if user_password_params.values.all?{|pa| !pa.blank? }
     @user_permit_params
   end
+  def user_password_params
+    @user_password_params ||= params[:user] ? params.require(:user).permit(:password, :password_confirmation) : {}
+  end
+
 end
@@ -22,3 +22,5 @@ en:
         destroy:
           success: "That's sad. %{name} is no longer part of your team."
           error: "You can't delete yourself"
+        update:
+          success: "%{name}'s information was successfully updated."
@@ -183,24 +183,18 @@ describe UsersController do
     context "PUT /users/:id" do
       context "when the update is successful" do
+        before {
+          put :update, :id => user.to_param, :user => user_params
+        }
-        it "sets a message to display" do
-          put :update, :id => user.to_param, :user => {:name => 'Kermit'}
-          request.flash[:success].should include('updated')
-        end
-
-        it "redirects to the user's page" do
-          put :update, :id => user.to_param, :user => {:name => 'Kermit'}
-          response.should redirect_to(user_path(user))
-        end
-
-        it "should be able to make user an admin" do
-          put :update, :id => user.to_param, :user => {:admin => true}
-          response.should be_redirect
-          User.find(controller.user.to_param).admin.should be_true
+        context "with normal params" do
+          let(:user_params) { {:name => 'Kermit'} }
+          it "sets a message to display" do
+            expect(request.flash[:success]).to eq I18n.t('controllers.users.flash.update.success', :name => user.name)
+            expect(response).to redirect_to(user_path(user))
+          end
         end
       end
-
       context "when the update is unsuccessful" do
         it "renders the edit page" do
@@ -238,6 +232,36 @@ describe UsersController do
         end
       end
     end
+
+    describe "#user_params" do
+      context "with current user not admin" do
+        before {
+          controller.stub(:current_user).and_return(user)
+          controller.stub(:params).and_return(ActionController::Parameters.new(user_param))
+        }
+        let(:user_param) { {'user' => { :name => 'foo', :admin => true }} }
+        it 'not have admin field' do
+          expect(controller.send(:user_params)).to eq ({'name' => 'foo'})
+        end
+        context "with password and password_confirmation empty?" do
+          let(:user_param) { {'user' => { :name => 'foo', 'password' => '', 'password_confirmation' => '' }} }
+          it 'not have password and password_confirmation field' do
+            expect(controller.send(:user_params)).to eq ({'name' => 'foo'})
+          end
+        end
+      end
+
+      context "with current user admin" do
+        it 'have admin field'
+        context "with password and password_confirmation empty?" do
+          it 'not have password and password_confirmation field'
+        end
+        context "on his own user" do
+          it 'not have admin field'
+        end
+      end
+    end
+
   end
 end
	@@ -22,3 +22,5 @@ en:		@@ -22,3 +22,5 @@ en:
22	destroy:	22	destroy:
23	success: "That's sad. %{name} is no longer part of your team."	23	success: "That's sad. %{name} is no longer part of your team."
24	error: "You can't delete yourself"	24	error: "You can't delete yourself"
		25	+ update:
		26	+ success: "%{name}'s information was successfully updated."
	@@ -183,24 +183,18 @@ describe UsersController do		@@ -183,24 +183,18 @@ describe UsersController do
183		183
184	context "PUT /users/:id" do	184	context "PUT /users/:id" do
185	context "when the update is successful" do	185	context "when the update is successful" do
		186	+ before {
		187	+ put :update, :id => user.to_param, :user => user_params
		188	+ }
186		189
187	- it "sets a message to display" do
188	- put :update, :id => user.to_param, :user => {:name => 'Kermit'}
189	- request.flash[:success].should include('updated')
190	- end
191	-
192	- it "redirects to the user's page" do
193	- put :update, :id => user.to_param, :user => {:name => 'Kermit'}
194	- response.should redirect_to(user_path(user))
195	- end
196	-
197	- it "should be able to make user an admin" do
198	- put :update, :id => user.to_param, :user => {:admin => true}
199	- response.should be_redirect
200	- User.find(controller.user.to_param).admin.should be_true	190	+ context "with normal params" do
		191	+ let(:user_params) { {:name => 'Kermit'} }
		192	+ it "sets a message to display" do
		193	+ expect(request.flash[:success]).to eq I18n.t('controllers.users.flash.update.success', :name => user.name)
		194	+ expect(response).to redirect_to(user_path(user))
		195	+ end
201	end	196	end
202	end	197	end
203	-
204	context "when the update is unsuccessful" do	198	context "when the update is unsuccessful" do
205		199
206	it "renders the edit page" do	200	it "renders the edit page" do
	@@ -238,6 +232,36 @@ describe UsersController do		@@ -238,6 +232,36 @@ describe UsersController do
238	end	232	end
239	end	233	end
240	end	234	end
		235	+
		236	+ describe "#user_params" do
		237	+ context "with current user not admin" do
		238	+ before {
		239	+ controller.stub(:current_user).and_return(user)
		240	+ controller.stub(:params).and_return(ActionController::Parameters.new(user_param))
		241	+ }
		242	+ let(:user_param) { {'user' => { :name => 'foo', :admin => true }} }
		243	+ it 'not have admin field' do
		244	+ expect(controller.send(:user_params)).to eq ({'name' => 'foo'})
		245	+ end
		246	+ context "with password and password_confirmation empty?" do
		247	+ let(:user_param) { {'user' => { :name => 'foo', 'password' => '', 'password_confirmation' => '' }} }
		248	+ it 'not have password and password_confirmation field' do
		249	+ expect(controller.send(:user_params)).to eq ({'name' => 'foo'})
		250	+ end
		251	+ end
		252	+ end
		253	+
		254	+ context "with current user admin" do
		255	+ it 'have admin field'
		256	+ context "with password and password_confirmation empty?" do
		257	+ it 'not have password and password_confirmation field'
		258	+ end
		259	+ context "on his own user" do
		260	+ it 'not have admin field'
		261	+ end
		262	+ end
		263	+ end
		264	+
241	end	265	end
242		266
243	end	267	end