From fc84783de1fd90e7a21f754363bdefb1c29fa368 Mon Sep 17 00:00:00 2001 From: Holger Just Date: Mon, 1 Aug 2011 23:28:58 +0200 Subject: [PATCH 1/2] [#124] Validate user status changes --- app/models/user.rb | 19 +++++++++++++++++++ test/unit/user_test.rb | 8 ++++++++ 2 files changed, 27 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 2be5eade..6f46a31e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -69,6 +69,7 @@ class User < Principal validates_length_of :mail, :maximum => 60, :allow_nil => true validates_confirmation_of :password, :allow_nil => true validates_inclusion_of :mail_notification, :in => MAIL_NOTIFICATION_OPTIONS.collect(&:first), :allow_blank => true + validates_inclusion_of :status, :in => [STATUS_ANONYMOUS, STATUS_ACTIVE, STATUS_REGISTERED, STATUS_LOCKED] named_scope :in_group, lambda {|group| group_id = group.is_a?(Group) ? group.id : group.to_i @@ -525,6 +526,24 @@ class User < Principal if !password.nil? && password.size < Setting.password_min_length.to_i errors.add(:password, :too_short, :count => Setting.password_min_length.to_i) end + + # Status + if !new_record? && status_changed? + case status_was + when nil + # initial setting is always save + true + when STATUS_ANONYMOUS + # never allow a state change of the anonymous user + false + when STATUS_REGISTERED + [STATUS_ACTIVE, STATUS_LOCKED].include? status + when STATUS_ACTIVE + [STATUS_LOCKED].include? status + when STATUS_LOCKED + [STATUS_ACTIVE].include? status + end || errors.add(:status, :inclusion) + end end private diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 2ff63a67..fe392c58 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -173,6 +173,14 @@ class UserTest < ActiveSupport::TestCase assert_equal nil, user end + def test_error_on_active_to_registered + user = User.try_to_login("jsmith", "jsmith") + assert_equal @jsmith, user + + @jsmith.status = User::STATUS_REGISTERED + assert !@jsmith.save + end + context ".try_to_login" do context "with good credentials" do should "return the user" do From 54b4fdf1aa65e4507a08b850a4b30e7f75951185 Mon Sep 17 00:00:00 2001 From: Holger Just Date: Tue, 2 Aug 2011 02:04:47 +0200 Subject: [PATCH 2/2] [#124] Allow to delete users with STATE_REGISTERED --- app/controllers/users_controller.rb | 22 +++++++++- app/helpers/users_helper.rb | 19 +++++++-- app/models/user.rb | 5 +++ config/routes.rb | 3 +- test/functional/users_controller_test.rb | 25 +++++++++++ test/integration/api_test/users_test.rb | 54 ++++++++++++++++++------ 6 files changed, 107 insertions(+), 21 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 6060e129..8b872d81 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -15,8 +15,8 @@ class UsersController < ApplicationController layout 'admin' before_filter :require_admin, :except => :show - before_filter :find_user, :only => [:show, :edit, :update, :edit_membership, :destroy_membership] - accept_key_auth :index, :show, :create, :update + before_filter :find_user, :only => [:show, :edit, :update, :destroy, :edit_membership, :destroy_membership] + accept_key_auth :index, :show, :create, :update, :destroy include SortHelper include CustomFieldsHelper @@ -177,6 +177,24 @@ class UsersController < ApplicationController redirect_to :controller => 'users', :action => 'edit', :id => @user end + verify :method => :delete, :only => :destroy, :render => {:nothing => true, :status => :method_not_allowed } + def destroy + # Only allow to delete users with STATUS_REGISTERED for now + # It is assumed that these users are not yet references in any way + # from other objects. + return render_403 unless @user.deletable? + + @user.destroy + respond_to do |format| + format.html { + flash[:notice] = l(:notice_successful_delete) + redirect_back_or_default(:action => 'index') + } + format.api { head :ok } + end + end + + def edit_membership @membership = Member.edit_membership(params[:membership_id], params[:membership], @user) @membership.save if request.post? diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 876175bf..6c99877d 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -36,13 +36,26 @@ module UsersHelper def change_status_link(user) url = {:controller => 'users', :action => 'update', :id => user, :page => params[:page], :status => params[:status], :tab => nil} + links = [] if user.locked? - link_to l(:button_unlock), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock' + links << link_to(l(:button_unlock), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock') elsif user.registered? - link_to l(:button_activate), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock' + links << link_to(l(:button_activate), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock') elsif user != User.current - link_to l(:button_lock), url.merge(:user => {:status => User::STATUS_LOCKED}), :method => :put, :class => 'icon icon-lock' + links << link_to(l(:button_lock), url.merge(:user => {:status => User::STATUS_LOCKED}), :method => :put, :class => 'icon icon-lock') end + + if user.deletable? + links << link_to( + l(:button_delete), {:controller => 'users', :action => 'destroy', :id => user}, + :method => :delete, + :confirm => l(:text_are_you_sure), + :title => l(:button_delete), + :class => 'icon icon-del' + ) + end + + links.join(" ") end def user_settings_tabs diff --git a/app/models/user.rb b/app/models/user.rb index 6f46a31e..784f2529 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -207,6 +207,11 @@ class User < Principal update_attribute(:status, STATUS_LOCKED) end + def deletable? + registered? && last_login_on.nil? + end + + # Returns true if +clear_password+ is the correct user's password, otherwise false def check_password?(clear_password) if auth_source_id.present? diff --git a/config/routes.rb b/config/routes.rb index 971966ac..dae8069d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -136,8 +136,7 @@ ActionController::Routing::Routes.draw do |map| map.resources :users, :member => { :edit_membership => :post, :destroy_membership => :post - }, - :except => [:destroy] + } # For nice "roadmap" in the url for the index action map.connect 'projects/:project_id/roadmap', :controller => 'versions', :action => 'index' diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index bff91192..41345765 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -269,6 +269,31 @@ class UsersControllerTest < ActionController::TestCase assert u.check_password?('newpass') end + def test_destroy + u = User.new(:firstname => 'Death', :lastname => 'Row', :mail => 'death.row@example.com', :language => 'en') + u.login = 'death.row' + u.status = User::STATUS_REGISTERED + u.save! + + delete :destroy, :id => u.id + assert_redirected_to :action => 'index' + # make sure that the user was actually destroyed + assert_raises(ActiveRecord::RecordNotFound) { u.reload } + end + + def test_failing_destroy + u = User.new(:firstname => 'Surviving', :lastname => 'Patient', :mail => 'surviving.patient@example.com', :language => 'en') + u.login = 'surviving.patient' + u.status = User::STATUS_ACTIVE + u.save! + + delete :destroy, :id => u.id + assert_response :forbidden + # make sure the user is still around + assert !u.reload.destroyed? + end + + def test_edit_membership post :edit_membership, :id => 2, :membership_id => 1, :membership => { :role_ids => [2]} diff --git a/test/integration/api_test/users_test.rb b/test/integration/api_test/users_test.rb index 6196d2ce..56085d21 100644 --- a/test/integration/api_test/users_test.rb +++ b/test/integration/api_test/users_test.rb @@ -240,26 +240,52 @@ class ApiTest::UsersTest < ActionController::IntegrationTest end end end + end - context "DELETE /users/2" do - context ".xml" do - should "not be allowed" do - assert_no_difference('User.count') do - delete '/users/2.xml' - end + context "DELETE /users/:temp:" do + context ".xml" do + should "delete the user" do + u = User.new(:firstname => 'Death', :lastname => 'Row', :mail => 'death.row@example.com', :language => 'en') + u.login = 'death.row' + u.status = User::STATUS_REGISTERED + u.save! - assert_response :method_not_allowed + assert_difference('User.count',-1) do + delete "/users/#{u.id}.xml", {}, :authorization => credentials('admin') end + + assert_response :success + assert_nil User.find_by_id(u.id) end - context ".json" do - should "not be allowed" do - assert_no_difference('User.count') do - delete '/users/2.json' - end - - assert_response :method_not_allowed + should "not delete active user" do + assert_difference('User.count',0) do + delete "/users/2.xml", {}, :authorization => credentials('jsmith') end + assert_response :forbidden + end + end + + context ".json" do + should "delete the user" do + u = User.new(:firstname => 'Death', :lastname => 'Row', :mail => 'death.row@example.com', :language => 'en') + u.login = 'death.row' + u.status = User::STATUS_REGISTERED + u.save! + + assert_difference('User.count',-1) do + delete "/users/#{u.id}.json", {}, :authorization => credentials('admin') + end + + assert_response :success + assert_nil User.find_by_id(u.id) + end + + should "not delete active user" do + assert_difference('User.count',0) do + delete "/users/2.json", {}, :authorization => credentials('jsmith') + end + assert_response :forbidden end end end