diff options
-rw-r--r-- | CHANGELOG.md | 8 | ||||
-rw-r--r-- | Gemfile | 2 | ||||
-rw-r--r-- | Gemfile.lock | 4 | ||||
-rw-r--r-- | VERSION | 2 | ||||
-rw-r--r-- | app/models/namespace.rb | 12 | ||||
-rw-r--r-- | lib/api/deploy_keys.rb | 10 | ||||
-rw-r--r-- | lib/api/helpers.rb | 6 | ||||
-rw-r--r-- | lib/api/merge_request_diffs.rb | 8 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 25 | ||||
-rw-r--r-- | lib/api/notes.rb | 26 | ||||
-rw-r--r-- | lib/api/subscriptions.rb | 4 | ||||
-rw-r--r-- | lib/api/todos.rb | 2 | ||||
-rw-r--r-- | spec/features/projects/import_export/namespace_export_file_spec.rb | 62 | ||||
-rw-r--r-- | spec/models/namespace_spec.rb | 11 | ||||
-rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 27 | ||||
-rw-r--r-- | spec/requests/api/notes_spec.rb | 12 | ||||
-rw-r--r-- | spec/requests/api/todos_spec.rb | 15 |
17 files changed, 192 insertions, 44 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index de6530edc0d..870ad3cfc5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 8.14.7 (2017-01-21) + +- Ensure export files are removed after a namespace is deleted. +- Don't allow project guests to subscribe to merge requests through the API. (Robert Schilling) +- Prevent users from creating notes on resources they can't access. +- Prevent users from deleting system deploy keys via the project deploy key API. +- Upgrade omniauth gem to 1.3.2. + ## 8.14.6 (2017-01-10) - Update the gitlab-markup gem to the version 1.5.1. !8509 @@ -19,7 +19,7 @@ gem 'pg', '~> 0.18.2', group: :postgres # Authentication libraries gem 'devise', '~> 4.2' gem 'doorkeeper', '~> 4.2.0' -gem 'omniauth', '~> 1.3.1' +gem 'omniauth', '~> 1.3.2' gem 'omniauth-auth0', '~> 1.4.1' gem 'omniauth-azure-oauth2', '~> 0.0.6' gem 'omniauth-bitbucket', '~> 0.0.2' diff --git a/Gemfile.lock b/Gemfile.lock index ee0f959cce3..a652a3ac67b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -428,7 +428,7 @@ GEM octokit (4.3.0) sawyer (~> 0.7.0, >= 0.5.3) oj (2.17.4) - omniauth (1.3.1) + omniauth (1.3.2) hashie (>= 1.2, < 4) rack (>= 1.0, < 3) omniauth-auth0 (1.4.1) @@ -899,7 +899,7 @@ DEPENDENCIES oauth2 (~> 1.2.0) octokit (~> 4.3.0) oj (~> 2.17.4) - omniauth (~> 1.3.1) + omniauth (~> 1.3.2) omniauth-auth0 (~> 1.4.1) omniauth-azure-oauth2 (~> 0.0.6) omniauth-bitbucket (~> 0.0.2) @@ -1 +1 @@ -8.14.6 +8.14.7 diff --git a/app/models/namespace.rb b/app/models/namespace.rb index b67049f0f55..aaabeeecdb8 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -111,6 +111,8 @@ class Namespace < ActiveRecord::Base Gitlab::UploadsTransfer.new.rename_namespace(path_was, path) + remove_exports! + # If repositories moved successfully we need to # send update instructions to users. # However we cannot allow rollback since we moved namespace dir @@ -174,5 +176,15 @@ class Namespace < ActiveRecord::Base GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path) end end + + remove_exports! + end + + def remove_exports! + Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) + end + + def export_path + File.join(Gitlab::ImportExport.storage_path, path_was) end end diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 85360730841..f6cb17bafd8 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -105,15 +105,19 @@ module API present key.deploy_key, with: Entities::SSHKey end - desc 'Delete existing deploy key of currently authenticated user' do + desc 'Delete deploy key for a project' do success Key end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end delete ":id/#{path}/:key_id" do - key = user_project.deploy_keys.find(params[:key_id]) - key.destroy + key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) + if key + key.destroy + else + not_found!('Deploy Key') + end end end end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 66090a64853..1bc682b0694 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -82,6 +82,12 @@ module API IssuesFinder.new(current_user, project_id: user_project.id).find(id) end + def find_merge_request_with_access(id, access_level = :read_merge_request) + merge_request = user_project.merge_requests.find(id) + authorize! access_level, merge_request + merge_request + end + def paginate(relation) relation.page(params[:page]).per(params[:per_page].to_i).tap do |data| add_pagination_headers(data) diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index 07435d78468..bc3d69f6904 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -15,10 +15,8 @@ module API end get ":id/merge_requests/:merge_request_id/versions" do - merge_request = user_project.merge_requests. - find(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) - authorize! :read_merge_request, merge_request present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff end @@ -34,10 +32,8 @@ module API end get ":id/merge_requests/:merge_request_id/versions/:version_id" do - merge_request = user_project.merge_requests. - find(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) - authorize! :read_merge_request, merge_request present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 2f1b274689a..b9ecf2de94b 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -111,8 +111,8 @@ module API success Entities::MergeRequest end get path do - merge_request = user_project.merge_requests.find(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project end @@ -120,8 +120,8 @@ module API success Entities::RepoCommit end get "#{path}/commits" do - merge_request = user_project.merge_requests.find(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request.commits, with: Entities::RepoCommit end @@ -129,8 +129,8 @@ module API success Entities::MergeRequestChanges end get "#{path}/changes" do - merge_request = user_project.merge_requests.find(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request, with: Entities::MergeRequestChanges, current_user: current_user end @@ -147,8 +147,7 @@ module API :milestone_id, :labels, :state_event end put path do - merge_request = user_project.merge_requests.find(params.delete(:merge_request_id)) - authorize! :update_merge_request, merge_request + merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request) mr_params = declared_params(include_missing: false) @@ -219,10 +218,7 @@ module API success Entities::MRNote end get "#{path}/comments" do - merge_request = user_project.merge_requests.find(params[:merge_request_id]) - - authorize! :read_merge_request, merge_request - + merge_request = find_merge_request_with_access(params[:merge_request_id]) present paginate(merge_request.notes.fresh), with: Entities::MRNote end @@ -234,8 +230,7 @@ module API requires :note, type: String, desc: 'The text of the comment' end post "#{path}/comments" do - merge_request = user_project.merge_requests.find(params[:merge_request_id]) - authorize! :create_note, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id], :create_note) opts = { note: params[:note], @@ -256,7 +251,7 @@ module API success Entities::MRNote end get "#{path}/closes_issues" do - merge_request = user_project.merge_requests.find(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user)) present paginate(issues), with: issue_entity(user_project), current_user: current_user end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index b255b47742b..34f7b6a367e 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -70,21 +70,27 @@ module API required_attributes! [:body] opts = { - note: params[:body], - noteable_type: noteables_str.classify, - noteable_id: params[:noteable_id] + note: params[:body], + noteable_type: noteables_str.classify, + noteable_id: params[:noteable_id] } - if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user) - opts[:created_at] = params[:created_at] - end + noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id]) + + if can?(current_user, noteable_read_ability_name(noteable), noteable) + if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user) + opts[:created_at] = params[:created_at] + end - note = ::Notes::CreateService.new(user_project, current_user, opts).execute + note = ::Notes::CreateService.new(user_project, current_user, opts).execute - if note.valid? - present note, with: Entities::const_get(note.class.name) + if note.valid? + present note, with: Entities::const_get(note.class.name) + else + not_found!("Note #{note.errors.messages}") + end else - not_found!("Note #{note.errors.messages}") + not_found!("Note") end end diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb index 10749b34004..e11d7537cc9 100644 --- a/lib/api/subscriptions.rb +++ b/lib/api/subscriptions.rb @@ -3,8 +3,8 @@ module API before { authenticate! } subscribable_types = { - 'merge_request' => proc { |id| user_project.merge_requests.find(id) }, - 'merge_requests' => proc { |id| user_project.merge_requests.find(id) }, + 'merge_request' => proc { |id| find_merge_request_with_access(id, :update_merge_request) }, + 'merge_requests' => proc { |id| find_merge_request_with_access(id, :update_merge_request) }, 'issues' => proc { |id| find_project_issue(id) }, 'labels' => proc { |id| find_project_label(id) }, } diff --git a/lib/api/todos.rb b/lib/api/todos.rb index 832b04a3bb1..86d79d60247 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -4,7 +4,7 @@ module API before { authenticate! } ISSUABLE_TYPES = { - 'merge_requests' => ->(id) { user_project.merge_requests.find(id) }, + 'merge_requests' => ->(id) { find_merge_request_with_access(id) }, 'issues' => ->(id) { find_project_issue(id) } } diff --git a/spec/features/projects/import_export/namespace_export_file_spec.rb b/spec/features/projects/import_export/namespace_export_file_spec.rb new file mode 100644 index 00000000000..d0bafc6168c --- /dev/null +++ b/spec/features/projects/import_export/namespace_export_file_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +feature 'Import/Export - Namespace export file cleanup', feature: true, js: true do + let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } + let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } + + let(:project) { create(:empty_project) } + + background do + allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) + end + + after do + FileUtils.rm_rf(export_path, secure: true) + end + + context 'admin user' do + before do + login_as(:admin) + end + + context 'moving the namespace' do + scenario 'removes the export file' do + setup_export_project + + old_export_path = project.export_path.dup + + expect(File).to exist(old_export_path) + + project.namespace.update(path: 'new_path') + + expect(File).not_to exist(old_export_path) + end + end + + context 'deleting the namespace' do + scenario 'removes the export file' do + setup_export_project + + old_export_path = project.export_path.dup + + expect(File).to exist(old_export_path) + + project.namespace.destroy + + expect(File).not_to exist(old_export_path) + end + end + + def setup_export_project + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Export project') + + click_link 'Export project' + + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Download export') + end + end +end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 431b3e4435f..bc9245e80ff 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -69,6 +69,7 @@ describe Namespace, models: true do new_path = @namespace.path + "_new" allow(@namespace).to receive(:path_was).and_return(@namespace.path) allow(@namespace).to receive(:path).and_return(new_path) + expect(@namespace).to receive(:remove_exports!) expect(@namespace.move_dir).to be_truthy end @@ -91,11 +92,17 @@ describe Namespace, models: true do let!(:project) { create(:project, namespace: namespace) } let!(:path) { File.join(Gitlab.config.repositories.storages.default, namespace.path) } - before { namespace.destroy } - it "removes its dirs when deleted" do + namespace.destroy + expect(File.exist?(path)).to be(false) end + + it 'removes the exports folder' do + expect(namespace).to receive(:remove_exports!) + + namespace.destroy + end end describe '.find_by_path_or_name' do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 3ecf3eea5f5..1fe77cb0ffb 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -601,6 +601,15 @@ describe API::API, api: true do expect(json_response.first['title']).to eq(issue.title) expect(json_response.first['id']).to eq(issue.id) end + + it 'returns 403 if the user has no access to the merge request' do + guest = create(:user) + project.team << [guest, :guest] + + get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/closes_issues", guest) + + expect(response).to have_http_status(403) + end end describe 'POST :id/merge_requests/:merge_request_id/subscription' do @@ -622,6 +631,15 @@ describe API::API, api: true do expect(response).to have_http_status(404) end + + it 'returns 403 if user has no access to read code' do + guest = create(:user) + project.team << [guest, :guest] + + post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest) + + expect(response).to have_http_status(403) + end end describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do @@ -643,6 +661,15 @@ describe API::API, api: true do expect(response).to have_http_status(404) end + + it 'returns 403 if user has no access to read code' do + guest = create(:user) + project.team << [guest, :guest] + + delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest) + + expect(response).to have_http_status(403) + end end def mr_with_later_created_and_updated_at_time diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index b71a4c5a56e..c57d47c3c57 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -264,6 +264,18 @@ describe API::API, api: true do end end + context 'when user does not have access to read the noteable' do + it 'responds with 404' do + project = create(:empty_project, :private) { |p| p.team << [user, :guest] } + issue = create(:issue, :confidential, project: project) + + post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), + body: 'Foo' + + expect(response).to have_http_status(404) + end + end + context 'when user does not have access to create noteable' do let(:private_issue) { create(:issue, project: create(:project, :private)) } diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 887a2ba5b84..e8248086df6 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -183,12 +183,25 @@ describe API::Todos, api: true do expect(response.status).to eq(404) end + + it 'returns an error if the issuable is not accessible' do + guest = create(:user) + project_1.team << [guest, :guest] + + post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.id}/todo", guest) + + if issuable_type == 'merge_requests' + expect(response).to have_http_status(403) + else + expect(response).to have_http_status(404) + end + end end describe 'POST :id/issuable_type/:issueable_id/todo' do context 'for an issue' do it_behaves_like 'an issuable', 'issues' do - let(:issuable) { create(:issue, author: author_1, project: project_1) } + let(:issuable) { create(:issue, :confidential, author: author_1, project: project_1) } end end |