tickets: fix bug that allow index of other users

2 files changed, 26 insertions, 11 deletions

diff --git a/engines/support/app/controllers/tickets_controller.rb b/engines/support/app/controllers/tickets_controller.rb
index fab26f3..1ccbd16 100644
--- a/engines/support/app/controllers/tickets_controller.rb
+++ b/engines/support/app/controllers/tickets_controller.rb
@@ -4,10 +4,10 @@ class TicketsController < ApplicationController
   respond_to :html, :json
   #has_scope :open, :type => boolean
 
+  before_filter :fetch_user
   before_filter :require_login, :only => [:index]
   before_filter :fetch_ticket, except: [:new, :create, :index]
-  before_filter :require_ticket_access, except: [:new, :create, :index]
-  before_filter :fetch_user
+  before_filter :require_ticket_access, except: [:new, :create]
   before_filter :set_title
 
   def new
@@ -129,14 +129,24 @@ class TicketsController < ApplicationController
   end
 
   def ticket_access?
-    admin? or
-      @ticket.created_by.blank? or
-      current_user.id == @ticket.created_by
+    admin? or (
+      @ticket &&
+      @ticket.created_by.blank?
+    ) or (
+      @ticket &&
+      @ticket.created_by == current_user.id
+    ) or (
+      @ticket.nil? &&
+      @user &&
+      @user.id == current_user.id
+    )
   end
 
   def fetch_user
     if params[:user_id]
       @user = User.find(params[:user_id])
+    else
+      @user = current_user
     end
   end
 
@@ -146,7 +156,7 @@ class TicketsController < ApplicationController
   def search_options(params)
     params.merge(
       :admin_status => params[:user_id] ? 'mine' : 'all',
-      :user_id      => @user ? @user.id : current_user.id,
+      :user_id      => @user.id,
       :is_admin     => admin?
     )
   end
diff --git a/engines/support/test/functional/tickets_controller_test.rb b/engines/support/test/functional/tickets_controller_test.rb
index 1d074cc..ebaa3a4 100644
--- a/engines/support/test/functional/tickets_controller_test.rb
+++ b/engines/support/test/functional/tickets_controller_test.rb
@@ -45,8 +45,7 @@ class TicketsControllerTest < ActionController::TestCase
     user = find_record :user
     ticket = find_record :ticket, :created_by => user.id
     get :show, :id => ticket.id
-    assert_response :redirect
-    assert_redirected_to login_url
+    assert_login_required
   end
 
   test "user tickets are visible to creator" do
@@ -57,13 +56,19 @@ class TicketsControllerTest < ActionController::TestCase
     assert_response :success
   end
 
-  test "other users tickets are not visible" do
+  test "ticket of other user is not visible" do
     other_user = find_record :user
     ticket = find_record :ticket, :created_by => other_user.id
     login
     get :show, :id => ticket.id
-    assert_response :redirect
-    assert_redirected_to home_url
+    assert_access_denied
+  end
+
+  test "ticket list of other user is not visible" do
+    other_user = find_record :user
+    login
+    get :index, :user_id => other_user.id
+    assert_access_denied
   end
 
   test "should create unauthenticated ticket" do