diff options
author | Angus MacArthur <amacarthur@blackberry.com> | 2013-10-04 23:11:50 +0400 |
---|---|---|
committer | Angus MacArthur <amacarthur@blackberry.com> | 2013-10-16 09:20:53 +0400 |
commit | aefe2e952f33267ce38fb9270400f4f6f194d37b (patch) | |
tree | 3546807c2b7942585a41cfb1163dc5e6a69e40e0 | |
parent | a8eb525e72f6883a07539af9429ccd41dbc8698b (diff) |
Fixing unsafe use of Thread.current variable :current_user
-rw-r--r-- | app/controllers/application_controller.rb | 7 | ||||
-rw-r--r-- | db/fixtures/development/09_issues.rb | 25 | ||||
-rw-r--r-- | db/fixtures/development/10_merge_requests.rb | 30 | ||||
-rw-r--r-- | features/support/env.rb | 2 | ||||
-rw-r--r-- | lib/api/helpers.rb | 9 | ||||
-rw-r--r-- | lib/api/issues.rb | 39 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 79 | ||||
-rw-r--r-- | lib/api/milestones.rb | 34 | ||||
-rw-r--r-- | lib/api/notes.rb | 40 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 10 | ||||
-rw-r--r-- | spec/requests/api/issues_spec.rb | 12 | ||||
-rw-r--r-- | spec/requests/api/milestones_spec.rb | 12 | ||||
-rw-r--r-- | spec/requests/api/notes_spec.rb | 12 | ||||
-rw-r--r-- | spec/support/test_env.rb | 5 |
14 files changed, 196 insertions, 120 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 85b95862a17..cfa3cac5e88 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/db/fixtures/development/09_issues.rb b/db/fixtures/development/09_issues.rb index 31ba77254a3..1535c16939c 100644 --- a/db/fixtures/development/09_issues.rb +++ b/db/fixtures/development/09_issues.rb @@ -11,17 +11,22 @@ Gitlab::Seeder.quiet do next unless user user_id = user.id - Thread.current[:current_user] = user - Issue.seed(:id, [{ - id: i, - project_id: project.id, - author_id: user_id, - assignee_id: user_id, - state: ['opened', 'closed'].sample, - milestone: project.milestones.sample, - title: Faker::Lorem.sentence(6) - }]) + begin + Thread.current[:current_user] = user + + Issue.seed(:id, [{ + id: i, + project_id: project.id, + author_id: user_id, + assignee_id: user_id, + state: ['opened', 'closed'].sample, + milestone: project.milestones.sample, + title: Faker::Lorem.sentence(6) + }]) + ensure + Thread.current[:current_user] = nil + end print('.') end diff --git a/db/fixtures/development/10_merge_requests.rb b/db/fixtures/development/10_merge_requests.rb index 1e61ea28636..75f6e56985d 100644 --- a/db/fixtures/development/10_merge_requests.rb +++ b/db/fixtures/development/10_merge_requests.rb @@ -17,19 +17,23 @@ Gitlab::Seeder.quiet do next if branches.uniq.size < 2 user_id = user.id - Thread.current[:current_user] = user - - MergeRequest.seed(:id, [{ - id: i, - source_branch: branches.first, - target_branch: branches.last, - source_project_id: project.id, - target_project_id: project.id, - author_id: user_id, - assignee_id: user_id, - milestone: project.milestones.sample, - title: Faker::Lorem.sentence(6) - }]) + begin + Thread.current[:current_user] = user + + MergeRequest.seed(:id, [{ + id: i, + source_branch: branches.first, + target_branch: branches.last, + source_project_id: project.id, + target_project_id: project.id, + author_id: user_id, + assignee_id: user_id, + milestone: project.milestones.sample, + title: Faker::Lorem.sentence(6) + }]) + ensure + Thread.current[:current_user] = nil + end print('.') end end diff --git a/features/support/env.rb b/features/support/env.rb index 8798e62ea72..d27a73edab8 100644 --- a/features/support/env.rb +++ b/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 + diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 2b0c672c7fa..e09a4660668 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -31,6 +31,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! diff --git a/lib/api/issues.rb b/lib/api/issues.rb index a15203d1563..3d15c35b8cc 100644 --- a/lib/api/issues.rb +++ b/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,15 +48,17 @@ module API # Example Request: # POST /projects/:id/issues post ":id/issues" do - required_attributes! [:title] - attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id] - attrs[:label_list] = params[:labels] if params[:labels].present? - @issue = user_project.issues.new attrs - @issue.author = current_user - if @issue.save - present @issue, with: Entities::Issue - else - not_found! + 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? + @issue = user_project.issues.new attrs + @issue.author = current_user + if @issue.save + present @issue, with: Entities::Issue + else + not_found! + end end end @@ -75,16 +76,18 @@ module API # Example Request: # PUT /projects/:id/issues/:issue_id put ":id/issues/:issue_id" do - @issue = user_project.issues.find(params[:issue_id]) - authorize! :modify_issue, @issue + set_current_user_for_thread do + @issue = user_project.issues.find(params[:issue_id]) + authorize! :modify_issue, @issue - attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event] - attrs[:label_list] = params[:labels] if params[:labels].present? + attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event] + attrs[:label_list] = params[:labels] if params[:labels].present? - if @issue.update_attributes attrs - present @issue, with: Entities::Issue - else - not_found! + if @issue.update_attributes attrs + present @issue, with: Entities::Issue + else + not_found! + end end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index d690f1d07e7..3f4bec895bf 100644 --- a/lib/api/merge_requests.rb +++ b/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,28 +69,30 @@ module API # POST /projects/:id/merge_requests # post ":id/merge_requests" 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] - merge_request = user_project.merge_requests.new(attrs) - merge_request.author = current_user - merge_request.source_project = user_project - target_project_id = attrs[:target_project_id] - if not_fork?(target_project_id, user_project) - merge_request.target_project = user_project - else - if target_matches_fork(target_project_id,user_project) - merge_request.target_project = Project.find_by_id(attrs[:target_project_id]) + 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] + merge_request = user_project.merge_requests.new(attrs) + merge_request.author = current_user + merge_request.source_project = user_project + target_project_id = attrs[:target_project_id] + if not_fork?(target_project_id, user_project) + merge_request.target_project = user_project else - render_api_error!('(Bad Request) Specified target project that is not the source project, or the source fork of the project.', 400) + if target_matches_fork(target_project_id,user_project) + merge_request.target_project = Project.find_by_id(attrs[:target_project_id]) + else + render_api_error!('(Bad Request) Specified target project that is not the source project, or the source fork of the project.', 400) + end end - end - if merge_request.save - merge_request.reload_code - present merge_request, with: Entities::MergeRequest - else - handle_merge_request_errors! merge_request.errors + if merge_request.save + merge_request.reload_code + present merge_request, with: Entities::MergeRequest + else + handle_merge_request_errors! merge_request.errors + end end end @@ -109,17 +110,19 @@ module API # PUT /projects/:id/merge_request/:merge_request_id # put ":id/merge_request/:merge_request_id" 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]) + 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]) - authorize! :modify_merge_request, merge_request + authorize! :modify_merge_request, merge_request - if merge_request.update_attributes attrs - merge_request.reload_code - merge_request.mark_as_unchecked - present merge_request, with: Entities::MergeRequest - else - handle_merge_request_errors! merge_request.errors + if merge_request.update_attributes attrs + merge_request.reload_code + merge_request.mark_as_unchecked + present merge_request, with: Entities::MergeRequest + else + handle_merge_request_errors! merge_request.errors + end end end @@ -133,16 +136,18 @@ module API # POST /projects/:id/merge_request/:merge_request_id/comments # post ":id/merge_request/:merge_request_id/comments" do - required_attributes! [:note] + set_current_user_for_thread do + required_attributes! [:note] - merge_request = user_project.merge_requests.find(params[:merge_request_id]) - note = merge_request.notes.new(note: params[:note], project_id: user_project.id) - note.author = current_user + merge_request = user_project.merge_requests.find(params[:merge_request_id]) + note = merge_request.notes.new(note: params[:note], project_id: user_project.id) + note.author = current_user - if note.save - present note, with: Entities::MRNote - else - not_found! + if note.save + present note, with: Entities::MRNote + else + not_found! + end end end diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb index aee12e7dc40..f7e63b23093 100644 --- a/lib/api/milestones.rb +++ b/lib/api/milestones.rb @@ -40,15 +40,17 @@ module API # Example Request: # POST /projects/:id/milestones post ":id/milestones" do - authorize! :admin_milestone, user_project - required_attributes! [:title] + set_current_user_for_thread do + authorize! :admin_milestone, user_project + required_attributes! [:title] - attrs = attributes_for_keys [:title, :description, :due_date] - @milestone = user_project.milestones.new attrs - if @milestone.save - present @milestone, with: Entities::Milestone - else - not_found! + attrs = attributes_for_keys [:title, :description, :due_date] + @milestone = user_project.milestones.new attrs + if @milestone.save + present @milestone, with: Entities::Milestone + else + not_found! + end end end @@ -64,14 +66,16 @@ module API # Example Request: # PUT /projects/:id/milestones/:milestone_id put ":id/milestones/:milestone_id" do - authorize! :admin_milestone, user_project + set_current_user_for_thread do + authorize! :admin_milestone, user_project - @milestone = user_project.milestones.find(params[:milestone_id]) - attrs = attributes_for_keys [:title, :description, :due_date, :state_event] - if @milestone.update_attributes attrs - present @milestone, with: Entities::Milestone - else - not_found! + @milestone = user_project.milestones.find(params[:milestone_id]) + attrs = attributes_for_keys [:title, :description, :due_date, :state_event] + if @milestone.update_attributes attrs + present @milestone, with: Entities::Milestone + else + not_found! + end end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index cb2bc764476..f21907b1ffc 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -41,17 +41,19 @@ module API # Example Request: # POST /projects/:id/notes post ":id/notes" do - required_attributes! [:body] + set_current_user_for_thread do + required_attributes! [:body] - @note = user_project.notes.new(note: params[:body]) - @note.author = current_user + @note = user_project.notes.new(note: params[:body]) + @note.author = current_user - if @note.save - present @note, with: Entities::Note - else - # :note is exposed as :body, but :note is set on error - bad_request!(:note) if @note.errors[:note].any? - not_found! + if @note.save + present @note, with: Entities::Note + else + # :note is exposed as :body, but :note is set on error + bad_request!(:note) if @note.errors[:note].any? + not_found! + end end end @@ -97,17 +99,19 @@ 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 - required_attributes! [:body] + set_current_user_for_thread do + required_attributes! [:body] - @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) - @note = @noteable.notes.new(note: params[:body]) - @note.author = current_user - @note.project = user_project + @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) + @note = @noteable.notes.new(note: params[:body]) + @note.author = current_user + @note.project = user_project - if @note.save - present @note, with: Entities::Note - else - not_found! + if @note.save + present @note, with: Entities::Note + else + not_found! + end end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c7266007999..88ea6926790 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -27,14 +27,8 @@ require 'spec_helper' describe Project do - let(:user) { create(:user) } - - before do - enable_observers - Thread.current[:current_user] = user - end - - after { disable_observers } + before { enable_observers } + after { disable_observers } describe "Associations" do it { should belong_to(:group) } diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index e9422cd2643..c55025d72b5 100644 --- a/spec/requests/api/issues_spec.rb +++ b/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 diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb index e79ce083a46..febfc63921e 100644 --- a/spec/requests/api/milestones_spec.rb +++ b/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 diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index ba18b123039..6ed96eb97f3 100644 --- a/spec/requests/api/notes_spec.rb +++ b/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 diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 203e661f514..6bb5ce608f6 100644 --- a/spec/support/test_env.rb +++ b/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) |