Merge branch '8-14-stable' of gitlab.com:gitlab-org/gitlab-ce into 8-14-stable

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
diff --git a/Gemfile b/Gemfile
index 8c695f1a467..067b8d738b7 100644
--- a/Gemfile
+++ b/Gemfile
@@ -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)
diff --git a/VERSION b/VERSION
index 33b5bdccd8d..7a7d5650735 100644
--- a/VERSION
+++ b/VERSION
@@ -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