Add latest changes from gitlab-org/security/gitlab@14-5-stable-ee

16 files changed, 147 insertions, 58 deletions

diff --git a/app/models/todo.rb b/app/models/todo.rb
index 742b8fd2a9d..cfcb2201b80 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -69,7 +69,7 @@ class Todo < ApplicationRecord
   scope :for_type, -> (type) { where(target_type: type) }
   scope :for_target, -> (id) { where(target_id: id) }
   scope :for_commit, -> (id) { where(commit_id: id) }
-  scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: :route }]) }
+  scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: [:route, :owner] }]) }
   scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) }
 
   enum resolved_by_action: { system_done: 0, api_all_done: 1, api_done: 2, mark_all_done: 3, mark_done: 4 }, _prefix: :resolved_by
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index 39ce26526e6..ed5a0f24ed0 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -17,7 +17,9 @@ class IssuablePolicy < BasePolicy
     enable :read_issue
     enable :update_issue
     enable :reopen_issue
-    enable :read_merge_request
+  end
+
+  rule { can?(:read_merge_request) & assignee_or_author }.policy do
     enable :update_merge_request
     enable :reopen_merge_request
   end
diff --git a/lib/api/merge_request_approvals.rb b/lib/api/merge_request_approvals.rb
index dd49624c74f..71ca8331ed6 100644
--- a/lib/api/merge_request_approvals.rb
+++ b/lib/api/merge_request_approvals.rb
@@ -26,8 +26,6 @@ module API
         #   GET /projects/:id/merge_requests/:merge_request_iid/approvals
         desc 'List approvals for merge request'
         get 'approvals', urgency: :low do
-          not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
           merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
           present_approval(merge_request)
diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb
index 470f78a7dc2..8fa7138af42 100644
--- a/lib/api/merge_request_diffs.rb
+++ b/lib/api/merge_request_diffs.rb
@@ -23,8 +23,6 @@ module API
         use :pagination
       end
       get ":id/merge_requests/:merge_request_iid/versions" do
-        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present paginate(merge_request.merge_request_diffs.order_id_desc), with: Entities::MergeRequestDiff
@@ -41,8 +39,6 @@ module API
       end
 
       get ":id/merge_requests/:merge_request_iid/versions/:version_id" do
-        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present_cached merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull, cache_context: nil
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 21c1b7969aa..96d1a69c03a 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -264,8 +264,6 @@ module API
         success Entities::MergeRequest
       end
       get ':id/merge_requests/:merge_request_iid', feature_category: :code_review do
-        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present merge_request,
@@ -282,8 +280,6 @@ module API
         success Entities::UserBasic
       end
       get ':id/merge_requests/:merge_request_iid/participants', feature_category: :code_review do
-        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         participants = ::Kaminari.paginate_array(merge_request.participants)
@@ -295,8 +291,6 @@ module API
         success Entities::Commit
       end
       get ':id/merge_requests/:merge_request_iid/commits', feature_category: :code_review do
-        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         commits =
@@ -378,8 +372,6 @@ module API
         success Entities::MergeRequestChanges
       end
       get ':id/merge_requests/:merge_request_iid/changes', feature_category: :code_review do
-        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present merge_request,
@@ -395,8 +387,6 @@ module API
       get ':id/merge_requests/:merge_request_iid/pipelines', feature_category: :continuous_integration do
         pipelines = merge_request_pipelines_with_access
 
-        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
-
         present paginate(pipelines), with: Entities::Ci::PipelineBasic
       end
 
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index 57a6ee0bebb..1bc3e25a46c 100644
--- a/lib/api/todos.rb
+++ b/lib/api/todos.rb
@@ -29,10 +29,6 @@ module API
         post ":id/#{type}/:#{type_id_str}/todo" do
           issuable = instance_exec(params[type_id_str], &finder)
 
-          unless can?(current_user, :read_merge_request, issuable.project)
-            not_found!(type.split("_").map(&:capitalize).join(" "))
-          end
-
           todo = TodoService.new.mark_todo(issuable, current_user).first
 
           if todo
diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb
index ce886cb8738..dd7ec361dd8 100644
--- a/lib/gitlab/import_export/members_mapper.rb
+++ b/lib/gitlab/import_export/members_mapper.rb
@@ -52,11 +52,20 @@ module Gitlab
 
         @importable.members.destroy_all # rubocop: disable Cop/DestroyAll
 
-        relation_class.create!(user: @user, access_level: highest_access_level, source_id: @importable.id, importing: true)
+        relation_class.create!(user: @user, access_level: importer_access_level, source_id: @importable.id, importing: true)
       rescue StandardError => e
         raise e, "Error adding importer user to #{@importable.class} members. #{e.message}"
       end
 
+      def importer_access_level
+        if @importable.parent.is_a?(::Group) && !@user.admin?
+          lvl = @importable.parent.max_member_access_for_user(@user, only_concrete_membership: true)
+          [lvl, highest_access_level].min
+        else
+          highest_access_level
+        end
+      end
+
       def user_already_member?
         member = @importable.members&.first
 
diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb
index cf528b414c0..abada97fb10 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -65,7 +65,7 @@ RSpec.describe Dashboard::TodosController do
         project_2 = create(:project, namespace: user.namespace)
         project_2.add_developer(user)
         merge_request_2 = create(:merge_request, source_project: project_2)
-        create(:todo, project: project, author: author, user: user, target: merge_request_2)
+        create(:todo, project: project_2, author: author, user: user, target: merge_request_2)
 
         expect { get :index }.not_to exceed_query_limit(control)
         expect(response).to have_gitlab_http_status(:ok)
diff --git a/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb
index 473dbf5ecc5..ce6607f6a26 100644
--- a/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb
@@ -10,8 +10,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::ImportExport::Group::RelationTreeRestorer do
-  let_it_be(:group) { create(:group) }
-  let_it_be(:importable) { create(:group, parent: group) }
+  let(:group) { create(:group).tap { |g| g.add_owner(user) } }
+  let(:importable) { create(:group, parent: group) }
 
   include_context 'relation tree restorer shared context' do
     let(:importable_name) { nil }
diff --git a/spec/lib/gitlab/import_export/members_mapper_spec.rb b/spec/lib/gitlab/import_export/members_mapper_spec.rb
index 847d6b5d1ed..8b9ca90a280 100644
--- a/spec/lib/gitlab/import_export/members_mapper_spec.rb
+++ b/spec/lib/gitlab/import_export/members_mapper_spec.rb
@@ -267,6 +267,66 @@ RSpec.describe Gitlab::ImportExport::MembersMapper do
       end
     end
 
+    context 'when importer is not an admin' do
+      let(:user) { create(:user) }
+      let(:group) { create(:group) }
+      let(:members_mapper) do
+        described_class.new(
+          exported_members: [], user: user, importable: importable)
+      end
+
+      shared_examples_for 'it fetches the access level from parent group' do
+        before do
+          group.add_users([user], group_access_level)
+        end
+
+        it "and resolves it correctly" do
+          members_mapper.map
+          expect(member_class.find_by_user_id(user.id).access_level).to eq(resolved_access_level)
+        end
+      end
+
+      context 'and the imported project is part of a group' do
+        let(:importable) { create(:project, namespace: group) }
+        let(:member_class) { ProjectMember }
+
+        it_behaves_like 'it fetches the access level from parent group' do
+          let(:group_access_level) { GroupMember::DEVELOPER }
+          let(:resolved_access_level) { ProjectMember::DEVELOPER }
+        end
+
+        it_behaves_like 'it fetches the access level from parent group' do
+          let(:group_access_level) { GroupMember::MAINTAINER }
+          let(:resolved_access_level) { ProjectMember::MAINTAINER }
+        end
+
+        it_behaves_like 'it fetches the access level from parent group' do
+          let(:group_access_level) { GroupMember::OWNER }
+          let(:resolved_access_level) { ProjectMember::MAINTAINER }
+        end
+      end
+
+      context 'and the imported group is part of another group' do
+        let(:importable) { create(:group, parent: group) }
+        let(:member_class) { GroupMember }
+
+        it_behaves_like 'it fetches the access level from parent group' do
+          let(:group_access_level) { GroupMember::DEVELOPER }
+          let(:resolved_access_level) { GroupMember::DEVELOPER }
+        end
+
+        it_behaves_like 'it fetches the access level from parent group' do
+          let(:group_access_level) { GroupMember::MAINTAINER }
+          let(:resolved_access_level) { GroupMember::MAINTAINER }
+        end
+
+        it_behaves_like 'it fetches the access level from parent group' do
+          let(:group_access_level) { GroupMember::OWNER }
+          let(:resolved_access_level) { GroupMember::OWNER }
+        end
+      end
+    end
+
     context 'when importable is Group' do
       include_examples 'imports exported members' do
         let(:source_type) { 'Namespace' }
diff --git a/spec/lib/gitlab/import_export/project/relation_factory_spec.rb b/spec/lib/gitlab/import_export/project/relation_factory_spec.rb
index 49df2313924..80ba50976af 100644
--- a/spec/lib/gitlab/import_export/project/relation_factory_spec.rb
+++ b/spec/lib/gitlab/import_export/project/relation_factory_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::ImportExport::Project::RelationFactory, :use_clean_rails_memory_store_caching do
-  let(:group) { create(:group) }
+  let(:group) { create(:group).tap { |g| g.add_maintainer(importer_user) } }
   let(:project) { create(:project, :repository, group: group) }
   let(:members_mapper) { double('members_mapper').as_null_object }
   let(:admin) { create(:admin) }
diff --git a/spec/lib/gitlab/import_export/project/relation_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project/relation_tree_restorer_spec.rb
index 5ebace263ba..577f1e46db6 100644
--- a/spec/lib/gitlab/import_export/project/relation_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project/relation_tree_restorer_spec.rb
@@ -101,7 +101,7 @@ RSpec.describe Gitlab::ImportExport::Project::RelationTreeRestorer do
     it_behaves_like 'import project successfully'
 
     context 'with logging of relations creation' do
-      let_it_be(:group) { create(:group) }
+      let_it_be(:group) { create(:group).tap { |g| g.add_maintainer(user) } }
       let_it_be(:importable) do
         create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project', group: group)
       end
@@ -118,7 +118,7 @@ RSpec.describe Gitlab::ImportExport::Project::RelationTreeRestorer do
 
     context 'when inside a group' do
       let_it_be(:group) do
-        create(:group, :disabled_and_unoverridable)
+        create(:group, :disabled_and_unoverridable).tap { |g| g.add_maintainer(user) }
       end
 
       before do
diff --git a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
index cd3d29f1a51..6bb6be07749 100644
--- a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
@@ -674,6 +674,7 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do
           # Project needs to be in a group for visibility level comparison
           # to happen
           group = create(:group)
+          group.add_maintainer(user)
           project.group = group
 
           project.create_import_data(data: { override_params: { visibility_level: Gitlab::VisibilityLevel::INTERNAL.to_s } })
@@ -715,13 +716,19 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do
       end
 
       context 'with a project that has a group' do
+        let(:group) do
+          create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE).tap do |g|
+            g.add_maintainer(user)
+          end
+        end
+
         let!(:project) do
           create(:project,
                  :builds_disabled,
                  :issues_disabled,
                  name: 'project',
                  path: 'project',
-                 group: create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE))
+                 group: group)
         end
 
         before do
@@ -750,13 +757,14 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do
       end
 
       context 'with existing group models' do
+        let(:group) { create(:group).tap { |g| g.add_maintainer(user) } }
         let!(:project) do
           create(:project,
                  :builds_disabled,
                  :issues_disabled,
                  name: 'project',
                  path: 'project',
-                 group: create(:group))
+                 group: group)
         end
 
         before do
@@ -785,13 +793,14 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do
       end
 
       context 'with clashing milestones on IID' do
+        let(:group) { create(:group).tap { |g| g.add_maintainer(user) } }
         let!(:project) do
           create(:project,
                  :builds_disabled,
                  :issues_disabled,
                  name: 'project',
                  path: 'project',
-                 group: create(:group))
+                 group: group)
         end
 
         before do
@@ -870,7 +879,7 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do
       context 'with group visibility' do
         before do
           group = create(:group, visibility_level: group_visibility)
-
+          group.add_users([user], GroupMember::MAINTAINER)
           project.update(group: group)
         end
 
diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb
index b94df4d4374..e05de25f182 100644
--- a/spec/policies/merge_request_policy_spec.rb
+++ b/spec/policies/merge_request_policy_spec.rb
@@ -5,10 +5,11 @@ require 'spec_helper'
 RSpec.describe MergeRequestPolicy do
   include ExternalAuthorizationServiceHelpers
 
-  let(:guest) { create(:user) }
-  let(:author) { create(:user) }
-  let(:developer) { create(:user) }
-  let(:non_team_member) { create(:user) }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:author) { create(:user) }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:non_team_member) { create(:user) }
+
   let(:project) { create(:project, :public) }
 
   def permissions(user, merge_request)
@@ -50,15 +51,31 @@ RSpec.describe MergeRequestPolicy do
   end
 
   context 'when merge request is public' do
-    context 'and user is anonymous' do
-      let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
 
+    context 'and user is anonymous' do
       subject { permissions(nil, merge_request) }
 
       it do
         is_expected.to be_disallowed(:create_todo, :update_subscription)
       end
     end
+
+    describe 'the author, who became a guest' do
+      subject { permissions(author, merge_request) }
+
+      it do
+        is_expected.to be_allowed(:update_merge_request)
+      end
+
+      it do
+        is_expected.to be_allowed(:reopen_merge_request)
+      end
+
+      it do
+        is_expected.to be_allowed(:approve_merge_request)
+      end
+    end
   end
 
   context 'when merge requests have been disabled' do
@@ -107,6 +124,12 @@ RSpec.describe MergeRequestPolicy do
       it_behaves_like 'a denied user'
     end
 
+    describe 'the author' do
+      subject { author }
+
+      it_behaves_like 'a denied user'
+    end
+
     describe 'a developer' do
       subject { developer }
 
diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb
index c9deb84ff98..c6b4f50afae 100644
--- a/spec/requests/api/todos_spec.rb
+++ b/spec/requests/api/todos_spec.rb
@@ -378,30 +378,36 @@ RSpec.describe API::Todos do
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
-
-    it 'returns an error if the issuable author does not have access' do
-      project_1.add_guest(issuable.author)
-
-      post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.iid}/todo", issuable.author)
-
-      expect(response).to have_gitlab_http_status(:not_found)
-    end
   end
 
   describe 'POST :id/issuable_type/:issueable_id/todo' do
     context 'for an issue' do
-      it_behaves_like 'an issuable', 'issues' do
-        let_it_be(:issuable) do
-          create(:issue, :confidential, author: author_1, project: project_1)
-        end
+      let_it_be(:issuable) do
+        create(:issue, :confidential, project: project_1)
+      end
+
+      it_behaves_like 'an issuable', 'issues'
+
+      it 'returns an error if the issue author does not have access' do
+        post api("/projects/#{project_1.id}/issues/#{issuable.iid}/todo", issuable.author)
+
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
     context 'for a merge request' do
-      it_behaves_like 'an issuable', 'merge_requests' do
-        let_it_be(:issuable) do
-          create(:merge_request, :simple, source_project: project_1)
-        end
+      let_it_be(:issuable) do
+        create(:merge_request, :simple, source_project: project_1)
+      end
+
+      it_behaves_like 'an issuable', 'merge_requests'
+
+      it 'returns an error if the merge request author does not have access' do
+        project_1.add_guest(issuable.author)
+
+        post api("/projects/#{project_1.id}/merge_requests/#{issuable.iid}/todo", issuable.author)
+
+        expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
   end
diff --git a/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb b/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb
index e6f9e5a434c..28813a23fed 100644
--- a/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb
+++ b/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb
@@ -14,10 +14,10 @@ RSpec.shared_examples 'rejects user from accessing merge request info' do
     project.add_guest(user)
   end
 
-  it 'returns a 404 error' do
+  it 'returns a 403 error' do
     get api(url, user)
 
-    expect(response).to have_gitlab_http_status(:not_found)
-    expect(json_response['message']).to eq('404 Merge Request Not Found')
+    expect(response).to have_gitlab_http_status(:forbidden)
+    expect(json_response['message']).to eq('403 Forbidden')
   end
 end