From 3dc8886beb7d3689c87d9aa1e5ad2d4c6c5b4c55 Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 15 Jan 2013 11:55:12 -0800 Subject: Refactoring of tickets controller to fetch the ticket in a before filter for relevant actions. --- help/app/controllers/tickets_controller.rb | 67 ++++++++++++++---------------- 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/help/app/controllers/tickets_controller.rb b/help/app/controllers/tickets_controller.rb index d4aa378..d47939e 100644 --- a/help/app/controllers/tickets_controller.rb +++ b/help/app/controllers/tickets_controller.rb @@ -6,6 +6,7 @@ class TicketsController < ApplicationController before_filter :set_strings before_filter :authorize, :only => [:index] + before_filter :fetch_ticket, :only => [:show, :update, :destroy] # don't now have an edit method def new @ticket = Ticket.new @@ -31,52 +32,44 @@ class TicketsController < ApplicationController =begin def edit - @ticket = Ticket.find(params[:id]) @ticket.comments.build # build ticket comments? end =end def show - @ticket = Ticket.find(params[:id]) @comment = TicketComment.new if !@ticket redirect_to tickets_path, :alert => "No such ticket" return end - ticket_access_denied? #authorize_ticket_access - # @ticket.comments.build - # build ticket comments? end def update - @ticket = Ticket.find(params[:id]) - if !ticket_access_denied? - if params[:post] #currently changes to title or is_open status - @ticket.attributes = params[:post] - # TODO: do we want to keep the history of title changes? one possibility was adding a comment that said something like 'user changed the title from a to b' - - else - params[:ticket][:comments_attributes] = nil if params[:ticket][:comments_attributes].values.first[:body].blank? #unset comments hash if no new comment was typed - @ticket.attributes = params[:ticket] #this will call comments_attributes= - @ticket.close if params[:commit] == @reply_close_str #this overrides is_open selection - # what if there is an update and no new comment? Confirm that there is a new comment to update posted_by: - @ticket.comments.last.posted_by = (current_user ? current_user.id : nil) if @ticket.comments_changed? #protecting posted_by isn't working, so this should protect it. - end - if @ticket.changed? and @ticket.save - flash[:notice] = 'Ticket was successfully updated.' - if @ticket.is_open || !logged_in? - respond_with @ticket - else #for closed tickets with authenticated users, redirect to index. - redirect_to tickets_path - end - else - #redirect_to [:show, @ticket] # - flash[:alert] = 'Ticket has not been changed' - redirect_to @ticket - #respond_with(@ticket) # why does this go to edit?? redirect??? + if params[:post] #currently changes to title or is_open status + @ticket.attributes = params[:post] + # TODO: do we want to keep the history of title changes? one possibility was adding a comment that said something like 'user changed the title from a to b' + + else + params[:ticket][:comments_attributes] = nil if params[:ticket][:comments_attributes].values.first[:body].blank? #unset comments hash if no new comment was typed + @ticket.attributes = params[:ticket] #this will call comments_attributes= + @ticket.close if params[:commit] == @reply_close_str #this overrides is_open selection + # what if there is an update and no new comment? Confirm that there is a new comment to update posted_by: + @ticket.comments.last.posted_by = (current_user ? current_user.id : nil) if @ticket.comments_changed? #protecting posted_by isn't working, so this should protect it. + end + if @ticket.changed? and @ticket.save + flash[:notice] = 'Ticket was successfully updated.' + if @ticket.is_open || !logged_in? + respond_with @ticket + else #for closed tickets with authenticated users, redirect to index. + redirect_to tickets_path end + else + #redirect_to [:show, @ticket] # + flash[:alert] = 'Ticket has not been changed' + redirect_to @ticket + #respond_with(@ticket) # why does this go to edit?? redirect??? end end @@ -88,7 +81,6 @@ class TicketsController < ApplicationController def destroy # should we allow non-admins to delete their own tickets? i don't think necessary. - @ticket = Ticket.find(params[:id]) @ticket.destroy if admin? redirect_to tickets_path end @@ -99,16 +91,19 @@ class TicketsController < ApplicationController @ticket and (admin? or !@ticket.created_by or (current_user and current_user.id == @ticket.created_by)) end - def ticket_access_denied? - access_denied unless ticket_access? - end - - def set_strings @post_reply_str = 'Post reply' #t :post_reply @reply_close_str = 'Reply and close' #t :reply_and_close end + def fetch_ticket + @ticket = Ticket.find(params[:id]) + if !@ticket and admin? + redirect_to tickets_path, :alert => "No such ticket" + return + end + access_denied unless ticket_access? + end # not using now, as we are using comment_attributes= from the Ticket model =begin def add_comment -- cgit v1.2.3 From e2021bdcc40b51ab5e571c97e882bba10dc80ad6 Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 15 Jan 2013 12:52:09 -0800 Subject: For both users and tickets, if the object is not found and the current user is an admin, they should see an alert that the object wasn't found, and be redirected to the current controller. If the object isn't found and the current user is not an admin, then we will continue to give an error about no access, so as not to leak information about what IDs do and don't exist. --- config/locales/en.yml | 1 + help/app/controllers/tickets_controller.rb | 2 +- users/app/controllers/users_controller.rb | 4 ++++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 179c14c..fc61c31 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3,3 +3,4 @@ en: hello: "Hello world" + no_such_thing: "No such %{thing}." diff --git a/help/app/controllers/tickets_controller.rb b/help/app/controllers/tickets_controller.rb index d47939e..b613088 100644 --- a/help/app/controllers/tickets_controller.rb +++ b/help/app/controllers/tickets_controller.rb @@ -99,7 +99,7 @@ class TicketsController < ApplicationController def fetch_ticket @ticket = Ticket.find(params[:id]) if !@ticket and admin? - redirect_to tickets_path, :alert => "No such ticket" + redirect_to tickets_path, :alert => t(:no_such_thing, :thing => 'ticket') return end access_denied unless ticket_access? diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index 79de630..3d5a6a7 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -49,6 +49,10 @@ class UsersController < ApplicationController def fetch_user @user = User.find_by_param(params[:id]) + if !@user and admin? + redirect_to users_path, :alert => t(:no_such_thing, :thing => 'user') + return + end access_denied unless admin? or (@user == current_user) end -- cgit v1.2.3 From dc16c6f8e5382f9e5470eb2a40081d41f4112437 Mon Sep 17 00:00:00 2001 From: jessib Date: Thu, 17 Jan 2013 11:25:39 -0800 Subject: Deal with corner case where we don't have authenticated user. Will write a test after merging in show view for users. --- users/app/controllers/users_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index 3d5a6a7..b705f47 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -2,6 +2,7 @@ class UsersController < ApplicationController skip_before_filter :verify_authenticity_token, :only => [:create] + before_filter :authorize before_filter :fetch_user, :only => [:edit, :update, :destroy] before_filter :set_anchor, :only => [:edit, :update] before_filter :authorize_admin, :only => [:index] @@ -48,6 +49,7 @@ class UsersController < ApplicationController protected def fetch_user + # authorize filter has been checked first, so won't get here unless authenticated @user = User.find_by_param(params[:id]) if !@user and admin? redirect_to users_path, :alert => t(:no_such_thing, :thing => 'user') -- cgit v1.2.3 From cce882a42cc0c139b75d932ea8ee42525e4fdb32 Mon Sep 17 00:00:00 2001 From: jessib Date: Thu, 17 Jan 2013 12:35:48 -0800 Subject: Should be able to create a user when not logged in. This isn't ready to merge, as there is an issue with logging in as an admin in the test. --- users/app/controllers/users_controller.rb | 2 +- users/test/functional/users_controller_test.rb | 26 ++++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index a8ba1ab..c0fe243 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -3,7 +3,7 @@ class UsersController < ApplicationController skip_before_filter :verify_authenticity_token, :only => [:create] - before_filter :authorize + before_filter :authorize, :only => [:show, :edit, :update, :destroy] before_filter :fetch_user, :only => [:show, :edit, :update, :destroy] before_filter :set_anchor, :only => [:edit, :update] before_filter :authorize_admin, :only => [:index] diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index 46db4d1..8c584ef 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -10,10 +10,12 @@ class UsersControllerTest < ActionController::TestCase end test "failed show without login" do - user = find_record :user + user = FactoryGirl.build(:user) + user.save get :show, :id => user.id assert_response :redirect assert_redirected_to login_path + user.destroy end test "user can see user" do @@ -42,7 +44,7 @@ class UsersControllerTest < ActionController::TestCase assert_response :success end - + test "user cannot see other user" do user = find_record :user, :email => nil, @@ -57,6 +59,26 @@ class UsersControllerTest < ActionController::TestCase assert_access_denied end + test "show for non-existing user" do + nonid = 'thisisnotanexistinguserid' + + # when unauthenticated: + get :show, :id => nonid + assert_access_denied(true, false) + + # when authenticated but not admin: + login + get :show, :id => nonid + assert_access_denied + + # when authenticated as admin: + # TODO: THIS IS failing to login and have admin? return true in users_controller. Will look into it later. + login :is_admin => true + get :show, :id => nonid + assert_response :redirect + assert_equal({:alert => "No such user."}, flash.to_hash) + assert_redirected_to users_path + end test "should create new user" do user_attribs = record_attributes_for :user -- cgit v1.2.3 From 444dbca4054ccfb7a82bb4df2a6369959ef6c9b2 Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 18 Jan 2013 07:38:13 +0100 Subject: minor: smalles fix ever - is_admin? has a questionmark --- users/test/functional/users_controller_test.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index 8c584ef..9fb06c9 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -72,8 +72,7 @@ class UsersControllerTest < ActionController::TestCase assert_access_denied # when authenticated as admin: - # TODO: THIS IS failing to login and have admin? return true in users_controller. Will look into it later. - login :is_admin => true + login :is_admin? => true get :show, :id => nonid assert_response :redirect assert_equal({:alert => "No such user."}, flash.to_hash) -- cgit v1.2.3