Merge pull request #5344 from amacarthur/thread-variable-fix

Fixing unsafe use of Thread.current variable :current_user

app/controllers/application_controller.rb

@@ -2,7 +2,7 @@ class ApplicationController < ActionController::Base
   before_filter :authenticate_user!
   before_filter :reject_blocked!
   before_filter :check_password_expiration
-  before_filter :set_current_user_for_thread
+  around_filter :set_current_user_for_thread
   before_filter :add_abilities
   before_filter :dev_tools if Rails.env == 'development'
   before_filter :default_headers
@@ -50,6 +50,11 @@ class ApplicationController < ActionController::Base
 
   def set_current_user_for_thread
     Thread.current[:current_user] = current_user
+    begin
+      yield
+    ensure
+      Thread.current[:current_user] = nil
+    end
   end
 
   def abilities

db/fixtures/development/09_issues.rb

@@ -11,6 +11,8 @@ Gitlab::Seeder.quiet do
     next unless user
 
     user_id = user.id
+
+    begin
       Thread.current[:current_user] = user
 
       Issue.seed(:id, [{
@@ -22,6 +24,9 @@ Gitlab::Seeder.quiet do
         milestone: project.milestones.sample,
         title: Faker::Lorem.sentence(6)
       }])
+    ensure
+      Thread.current[:current_user] = nil
+    end
     print('.')
   end
 

db/fixtures/development/10_merge_requests.rb

@@ -17,6 +17,7 @@ Gitlab::Seeder.quiet do
     next if branches.uniq.size < 2
 
     user_id = user.id
+    begin
       Thread.current[:current_user] = user
 
       MergeRequest.seed(:id, [{
@@ -30,6 +31,9 @@ Gitlab::Seeder.quiet do
         milestone: project.milestones.sample,
         title: Faker::Lorem.sentence(6)
       }])
+    ensure
+      Thread.current[:current_user] = nil
+    end
     print('.')
   end
 end

features/support/env.rb

@@ -51,4 +51,6 @@ Spinach.hooks.before_run do
   RSpec::Mocks::setup self
 
   include FactoryGirl::Syntax::Methods
+  MergeRequestObserver.any_instance.stub(current_user: create(:user))
 end
+

lib/api/helpers.rb

@@ -27,6 +27,15 @@ module API
       end
     end
 
+    def set_current_user_for_thread
+      Thread.current[:current_user] = current_user
+      begin
+        yield
+      ensure
+        Thread.current[:current_user] = nil
+      end
+    end
+
     def user_project
       @project ||= find_project(params[:id])
       @project || not_found!

lib/api/issues.rb

@@ -2,7 +2,6 @@ module API
   # Issues API
   class Issues < Grape::API
     before { authenticate! }
-    before { Thread.current[:current_user] = current_user }
 
     resource :issues do
       # Get currently authenticated user's issues
@@ -49,6 +48,7 @@ module API
       # Example Request:
       #   POST /projects/:id/issues
       post ":id/issues" do
+        set_current_user_for_thread do
           required_attributes! [:title]
           attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]
           attrs[:label_list] = params[:labels] if params[:labels].present?
@@ -60,6 +60,7 @@ module API
             not_found!
           end
         end
+      end
 
       # Update an existing issue
       #
@@ -75,6 +76,7 @@ module API
       # Example Request:
       #   PUT /projects/:id/issues/:issue_id
       put ":id/issues/:issue_id" do
+        set_current_user_for_thread do
           @issue = user_project.issues.find(params[:issue_id])
           authorize! :modify_issue, @issue
 
@@ -87,6 +89,7 @@ module API
             not_found!
           end
         end
+      end
 
       # Delete a project issue (deprecated)
       #

lib/api/merge_requests.rb

@@ -2,7 +2,6 @@ module API
   # MergeRequest API
   class MergeRequests < Grape::API
     before { authenticate! }
-    before { Thread.current[:current_user] = current_user }
 
     resource :projects do
       helpers do
@@ -70,6 +69,7 @@ module API
       #   POST /projects/:id/merge_requests
       #
       post ":id/merge_requests" do
+        set_current_user_for_thread do
           authorize! :write_merge_request, user_project
           required_attributes! [:source_branch, :target_branch, :title]
           attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id]
@@ -94,6 +94,7 @@ module API
             handle_merge_request_errors! merge_request.errors
           end
         end
+      end
 
       # Update MR
       #
@@ -109,6 +110,7 @@ module API
       #   PUT /projects/:id/merge_request/:merge_request_id
       #
       put ":id/merge_request/:merge_request_id" do
+        set_current_user_for_thread do
           attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event]
           merge_request = user_project.merge_requests.find(params[:merge_request_id])
 
@@ -122,6 +124,7 @@ module API
             handle_merge_request_errors! merge_request.errors
           end
         end
+      end
 
       # Post comment to merge request
       #
@@ -133,6 +136,7 @@ module API
       #   POST /projects/:id/merge_request/:merge_request_id/comments
       #
       post ":id/merge_request/:merge_request_id/comments" do
+        set_current_user_for_thread do
           required_attributes! [:note]
 
           merge_request = user_project.merge_requests.find(params[:merge_request_id])
@@ -145,6 +149,7 @@ module API
             not_found!
           end
         end
+      end
 
     end
   end

lib/api/milestones.rb

@@ -40,6 +40,7 @@ module API
       # Example Request:
       #   POST /projects/:id/milestones
       post ":id/milestones" do
+        set_current_user_for_thread do
           authorize! :admin_milestone, user_project
           required_attributes! [:title]
 
@@ -51,6 +52,7 @@ module API
             not_found!
           end
         end
+      end
 
       # Update an existing project milestone
       #
@@ -64,6 +66,7 @@ module API
       # Example Request:
       #   PUT /projects/:id/milestones/:milestone_id
       put ":id/milestones/:milestone_id" do
+        set_current_user_for_thread do
           authorize! :admin_milestone, user_project
 
           @milestone = user_project.milestones.find(params[:milestone_id])
@@ -76,4 +79,5 @@ module API
         end
       end
     end
+  end
 end

lib/api/notes.rb

@@ -41,6 +41,7 @@ module API
       # Example Request:
       #   POST /projects/:id/notes
       post ":id/notes" do
+        set_current_user_for_thread do
           required_attributes! [:body]
 
           @note = user_project.notes.new(note: params[:body])
@@ -54,6 +55,7 @@ module API
             not_found!
           end
         end
+      end
 
       NOTEABLE_TYPES.each do |noteable_type|
         noteables_str = noteable_type.to_s.underscore.pluralize
@@ -97,6 +99,7 @@ module API
         #   POST /projects/:id/issues/:noteable_id/notes
         #   POST /projects/:id/snippets/:noteable_id/notes
         post ":id/#{noteables_str}/:#{noteable_id_str}/notes" do
+          set_current_user_for_thread do
             required_attributes! [:body]
 
             @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"])
@@ -113,4 +116,5 @@ module API
         end
       end
     end
+  end
 end

spec/models/project_spec.rb

@@ -27,13 +27,7 @@
 require 'spec_helper'
 
 describe Project do
-  let(:user) { create(:user) }
-
-  before do
-    enable_observers
-    Thread.current[:current_user] = user
-  end
-
+  before { enable_observers }
   after { disable_observers }
 
   describe "Associations" do

spec/requests/api/issues_spec.rb

@@ -100,4 +100,16 @@ describe API::API do
       response.status.should == 405
     end
   end
+
+  describe "PUT /projects/:id/issues/:issue_id to test observer on close" do
+    before { enable_observers }
+    after { disable_observers }
+
+    it "should create an activity event when an issue is closed" do
+      Event.should_receive(:create)
+
+      put api("/projects/#{project.id}/issues/#{issue.id}", user),
+        state_event: "close"
+    end
+  end
 end

spec/requests/api/milestones_spec.rb

@@ -90,4 +90,16 @@ describe API::API do
       json_response['state'].should == 'closed'
     end
   end
+
+  describe "PUT /projects/:id/milestones/:milestone_id to test observer on close" do
+    before { enable_observers }
+    after { disable_observers }
+
+    it "should create an activity event when an milestone is closed" do
+      Event.should_receive(:create)
+
+      put api("/projects/#{project.id}/milestones/#{milestone.id}", user),
+          state_event: 'close'
+    end
+  end
 end

spec/requests/api/notes_spec.rb

@@ -176,4 +176,16 @@ describe API::API do
       end
     end
   end
+
+  describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
+    before { enable_observers }
+    after { disable_observers }
+
+    it "should create an activity event when an issue note is created" do
+      Event.should_receive(:create)
+
+      post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
+    end
+  end
+
 end

spec/support/test_env.rb

@@ -84,6 +84,11 @@ module TestEnv
     Repository.any_instance.stub(
       size: 12.45
     )
+
+    ActivityObserver.any_instance.stub(
+        current_user: double("current_user", id: 1)
+    )
+
   end
 
   def clear_repo_dir(namespace, name)