Merge branch 'security-11-4' of dev.gitlab.org:gitlab/gitlabhq into 11-4-stable

19 files changed, 243 insertions, 22 deletions

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index ca713192c9e..6402e01ddc0 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -35,7 +35,9 @@ module MembershipActions
 
     respond_to do |format|
       format.html do
-        message = "User was successfully removed from #{source_type}."
+        source = source_type == 'group' ? 'group and any subresources' : source_type
+
+        message = "User was successfully removed from #{source}."
         redirect_to members_page_url, notice: message
       end
 
diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index a44acb12bdf..255f1f3569a 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -75,7 +75,14 @@ class Projects::SnippetsController < Projects::ApplicationController
       format.json do
         render_blob_json(blob)
       end
-      format.js { render 'shared/snippets/show'}
+
+      format.js do
+        if @snippet.embeddable?
+          render 'shared/snippets/show'
+        else
+          head :not_found
+        end
+      end
     end
   end
 
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index 694c3a59e2b..9fe1f9a2bde 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -84,7 +84,13 @@ class SnippetsController < ApplicationController
         render_blob_json(blob)
       end
 
-      format.js { render 'shared/snippets/show' }
+      format.js do
+        if @snippet.embeddable?
+          render 'shared/snippets/show'
+        else
+          head :not_found
+        end
+      end
     end
   end
 
diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb
index 5a21403bc5e..75106e62ccf 100644
--- a/app/helpers/members_helper.rb
+++ b/app/helpers/members_helper.rb
@@ -18,12 +18,13 @@ module MembersHelper
         "remove #{member.user.name} from"
       end
 
-    "#{text} #{action} the #{member.source.human_name} #{member.real_source_type.humanize(capitalize: false)}?"
+    "#{text} #{action} the #{member.source.human_name} #{source_text(member)}?"
   end
 
   def remove_member_title(member)
     action = member.request? ? 'Deny access request' : 'Remove user'
-    "#{action} from #{member.real_source_type.humanize(capitalize: false)}"
+
+    "#{action} from #{source_text(member)}"
   end
 
   def leave_confirmation_message(member_source)
@@ -35,4 +36,14 @@ module MembersHelper
     options = params.slice(:search, :sort).merge(options)
     "#{request.path}?#{options.to_param}"
   end
+
+  private
+
+  def source_text(member)
+    type = member.real_source_type.humanize(capitalize: false)
+
+    return type if member.request? || member.invite? || type != 'group'
+
+    'group and any subresources'
+  end
 end
diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb
index c7d31f3469d..a20c47ed91a 100644
--- a/app/helpers/snippets_helper.rb
+++ b/app/helpers/snippets_helper.rb
@@ -130,12 +130,4 @@ module SnippetsHelper
 
     link_to external_snippet_icon('download'), download_url, class: 'btn', target: '_blank', title: 'Download', rel: 'noopener noreferrer'
   end
-
-  def public_snippet?
-    if @snippet.project_id?
-      can?(nil, :read_project_snippet, @snippet)
-    else
-      can?(nil, :read_personal_snippet, @snippet)
-    end
-  end
 end
diff --git a/app/models/member.rb b/app/models/member.rb
index 0696ea46c8b..99da80b5f93 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -76,6 +76,7 @@ class Member < ActiveRecord::Base
   scope :owners,  -> { active.where(access_level: OWNER) }
   scope :owners_and_maintainers,  -> { active.where(access_level: [OWNER, MAINTAINER]) }
   scope :owners_and_masters,  -> { owners_and_maintainers } # @deprecated
+  scope :with_user, -> (user) { where(user: user) }
 
   scope :order_name_asc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'ASC')) }
   scope :order_name_desc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'DESC')) }
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index fc49ee7ac8c..2c9e1ba1d80 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -12,6 +12,8 @@ class GroupMember < Member
   validates :source_type, format: { with: /\ANamespace\z/ }
   default_scope { where(source_type: SOURCE_TYPE) }
 
+  scope :in_groups, ->(groups) { where(source_id: groups.select(:id)) }
+
   after_create :update_two_factor_requirement, unless: :invite?
   after_destroy :update_two_factor_requirement, unless: :invite?
 
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 537f2a3a231..5fa38ee74dd 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -14,6 +14,10 @@ class ProjectMember < Member
   default_scope { where(source_type: SOURCE_TYPE) }
 
   scope :in_project, ->(project) { where(source_id: project.id) }
+  scope :in_namespaces, ->(groups) do
+    joins('INNER JOIN projects ON projects.id = members.source_id')
+      .where('projects.namespace_id in (?)', groups.select(:id))
+  end
 
   class << self
     # Add users to projects with passed access option
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index 1c5846b4023..964a66efe62 100644
--- a/app/models/snippet.rb
+++ b/app/models/snippet.rb
@@ -140,6 +140,12 @@ class Snippet < ActiveRecord::Base
     :visibility_level
   end
 
+  def embeddable?
+    ability = project_id? ? :read_project_snippet : :read_personal_snippet
+
+    Ability.allowed?(nil, ability, self)
+  end
+
   def notes_with_associations
     notes.includes(:author)
   end
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index ae0c644e6c0..828871125a1 100644
--- a/app/services/members/destroy_service.rb
+++ b/app/services/members/destroy_service.rb
@@ -2,9 +2,11 @@
 
 module Members
   class DestroyService < Members::BaseService
-    def execute(member, skip_authorization: false)
+    def execute(member, skip_authorization: false, skip_subresources: false)
       raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member)
 
+      @skip_auth = skip_authorization
+
       return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
 
       member.destroy
@@ -15,6 +17,7 @@ module Members
         notification_service.decline_access_request(member)
       end
 
+      delete_subresources(member) unless skip_subresources
       enqueue_delete_todos(member)
 
       after_execute(member: member)
@@ -24,6 +27,35 @@ module Members
 
     private
 
+    def delete_subresources(member)
+      return unless member.is_a?(GroupMember) && member.user && member.group
+
+      delete_project_members(member)
+      delete_subgroup_members(member) if Group.supports_nested_groups?
+    end
+
+    def delete_project_members(member)
+      groups = member.group.self_and_descendants
+
+      ProjectMember.in_namespaces(groups).with_user(member.user).each do |project_member|
+        self.class.new(current_user).execute(project_member, skip_authorization: @skip_auth)
+      end
+    end
+
+    def delete_subgroup_members(member)
+      groups = member.group.descendants
+
+      GroupMember.in_groups(groups).with_user(member.user).each do |group_member|
+        self.class.new(current_user).execute(group_member, skip_authorization: @skip_auth, skip_subresources: true)
+      end
+    end
+
+    def enqueue_delete_todos(member)
+      type = member.is_a?(GroupMember) ? 'Group' : 'Project'
+      # don't enqueue immediately to prevent todos removal in case of a mistake
+      TodosDestroyer::EntityLeaveWorker.perform_in(1.hour, member.user_id, member.source_id, type)
+    end
+
     def can_destroy_member?(member)
       can?(current_user, destroy_member_permission(member), member)
     end
diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml
index 10bfc30492a..a43296aa806 100644
--- a/app/views/shared/snippets/_header.html.haml
+++ b/app/views/shared/snippets/_header.html.haml
@@ -30,7 +30,7 @@
   - if @snippet.updated_at != @snippet.created_at
     = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago', exclude_author: true)
 
-  - if public_snippet?
+  - if @snippet.embeddable?
     .embed-snippet
       .input-group
         .input-group-prepend
diff --git a/changelogs/unreleased/fix-security-group-user-removal.yml b/changelogs/unreleased/fix-security-group-user-removal.yml
new file mode 100644
index 00000000000..09d09a96f84
--- /dev/null
+++ b/changelogs/unreleased/fix-security-group-user-removal.yml
@@ -0,0 +1,5 @@
+---
+title: Add subresources removal to member destroy service
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-48259-private-snippet.yml b/changelogs/unreleased/security-48259-private-snippet.yml
new file mode 100644
index 00000000000..6cf1e5dc694
--- /dev/null
+++ b/changelogs/unreleased/security-48259-private-snippet.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent private snippets from being embeddable
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb
index 362d5cc4514..01882383656 100644
--- a/spec/controllers/groups/group_members_controller_spec.rb
+++ b/spec/controllers/groups/group_members_controller_spec.rb
@@ -118,7 +118,7 @@ describe Groups::GroupMembersController do
         it '[HTML] removes user from members' do
           delete :destroy, group_id: group, id: member
 
-          expect(response).to set_flash.to 'User was successfully removed from group.'
+          expect(response).to set_flash.to 'User was successfully removed from group and any subresources.'
           expect(response).to redirect_to(group_group_members_path(group))
           expect(group.members).not_to include member
         end
diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb
index 9c383bd7628..70bf182cdee 100644
--- a/spec/controllers/projects/snippets_controller_spec.rb
+++ b/spec/controllers/projects/snippets_controller_spec.rb
@@ -371,6 +371,46 @@ describe Projects::SnippetsController do
     end
   end
 
+  describe "GET #show for embeddable content" do
+    let(:project_snippet) { create(:project_snippet, snippet_permission, project: project, author: user) }
+
+    before do
+      sign_in(user)
+
+      get :show, namespace_id: project.namespace, project_id: project, id: project_snippet.to_param, format: :js
+    end
+
+    context 'when snippet is private' do
+      let(:snippet_permission) { :private }
+
+      it 'responds with status 404' do
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'when snippet is public' do
+      let(:snippet_permission) { :public }
+
+      it 'responds with status 200' do
+        expect(assigns(:snippet)).to eq(project_snippet)
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
+    context 'when the project is private' do
+      let(:project) { create(:project_empty_repo, :private) }
+
+      context 'when snippet is public' do
+        let(:project_snippet) { create(:project_snippet, :public, project: project, author: user) }
+
+        it 'responds with status 404' do
+          expect(assigns(:snippet)).to eq(project_snippet)
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+  end
+
   describe 'GET #raw' do
     let(:project_snippet) do
       create(
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb
index 9effe47ab05..f3d6493786e 100644
--- a/spec/controllers/snippets_controller_spec.rb
+++ b/spec/controllers/snippets_controller_spec.rb
@@ -80,6 +80,12 @@ describe SnippetsController do
             expect(assigns(:snippet)).to eq(personal_snippet)
             expect(response).to have_gitlab_http_status(200)
           end
+
+          it 'responds with status 404 when embeddable content is requested' do
+            get :show, id: personal_snippet.to_param, format: :js
+
+            expect(response).to have_gitlab_http_status(404)
+          end
         end
       end
 
@@ -106,6 +112,12 @@ describe SnippetsController do
           expect(assigns(:snippet)).to eq(personal_snippet)
           expect(response).to have_gitlab_http_status(200)
         end
+
+        it 'responds with status 404 when embeddable content is requested' do
+          get :show, id: personal_snippet.to_param, format: :js
+
+          expect(response).to have_gitlab_http_status(404)
+        end
       end
 
       context 'when not signed in' do
@@ -131,6 +143,13 @@ describe SnippetsController do
           expect(assigns(:snippet)).to eq(personal_snippet)
           expect(response).to have_gitlab_http_status(200)
         end
+
+        it 'responds with status 200 when embeddable content is requested' do
+          get :show, id: personal_snippet.to_param, format: :js
+
+          expect(assigns(:snippet)).to eq(personal_snippet)
+          expect(response).to have_gitlab_http_status(200)
+        end
       end
 
       context 'when not signed in' do
diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb
index 4590904c93d..908e8960f37 100644
--- a/spec/helpers/members_helper_spec.rb
+++ b/spec/helpers/members_helper_spec.rb
@@ -16,7 +16,7 @@ describe MembersHelper do
     it { expect(remove_member_message(project_member_invite)).to eq "Are you sure you want to revoke the invitation for #{project_member_invite.invite_email} to join the #{project.full_name} project?" }
     it { expect(remove_member_message(project_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{project.full_name} project?" }
     it { expect(remove_member_message(project_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{project.full_name} project?" }
-    it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group?" }
+    it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group and any subresources?" }
     it { expect(remove_member_message(group_member_invite)).to eq "Are you sure you want to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group?" }
     it { expect(remove_member_message(group_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{group.name} group?" }
     it { expect(remove_member_message(group_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{group.name} group?" }
@@ -33,7 +33,7 @@ describe MembersHelper do
 
     it { expect(remove_member_title(project_member)).to eq 'Remove user from project' }
     it { expect(remove_member_title(project_member_request)).to eq 'Deny access request from project' }
-    it { expect(remove_member_title(group_member)).to eq 'Remove user from group' }
+    it { expect(remove_member_title(group_member)).to eq 'Remove user from group and any subresources' }
     it { expect(remove_member_title(group_member_request)).to eq 'Deny access request from group' }
   end
 
diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb
index e09d89d235d..da61a4075ed 100644
--- a/spec/models/snippet_spec.rb
+++ b/spec/models/snippet_spec.rb
@@ -212,4 +212,41 @@ describe Snippet do
       expect(blob.data).to eq(snippet.content)
     end
   end
+
+  describe '#embeddable?' do
+    context 'project snippet' do
+      [
+        { project: :public,   snippet: :public,   embeddable: true },
+        { project: :internal, snippet: :public,   embeddable: false },
+        { project: :private,  snippet: :public,   embeddable: false },
+        { project: :public,   snippet: :internal, embeddable: false },
+        { project: :internal, snippet: :internal, embeddable: false },
+        { project: :private,  snippet: :internal, embeddable: false },
+        { project: :public,   snippet: :private,  embeddable: false },
+        { project: :internal, snippet: :private,  embeddable: false },
+        { project: :private,  snippet: :private,  embeddable: false }
+      ].each do |combination|
+        it 'only returns true when both project and snippet are public' do
+          project = create(:project, combination[:project])
+          snippet = create(:project_snippet, combination[:snippet], project: project)
+
+          expect(snippet.embeddable?).to eq(combination[:embeddable])
+        end
+      end
+    end
+
+    context 'personal snippet' do
+      [
+        { snippet: :public,   embeddable: true },
+        { snippet: :internal, embeddable: false },
+        { snippet: :private,  embeddable: false }
+      ].each do |combination|
+        it 'only returns true when snippet is public' do
+          snippet = create(:personal_snippet, combination[:snippet])
+
+          expect(snippet.embeddable?).to eq(combination[:embeddable])
+        end
+      end
+    end
+  end
 end
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb
index 5aa7165e135..e872a537761 100644
--- a/spec/services/members/destroy_service_spec.rb
+++ b/spec/services/members/destroy_service_spec.rb
@@ -69,14 +69,14 @@ describe Members::DestroyService do
     it 'calls Member#after_decline_request' do
       expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member)
 
-      described_class.new(current_user).execute(member)
+      described_class.new(current_user).execute(member, opts)
     end
 
     context 'when current user is the member' do
       it 'does not call Member#after_decline_request' do
         expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member)
 
-        described_class.new(member_user).execute(member)
+        described_class.new(member_user).execute(member, opts)
       end
     end
   end
@@ -159,7 +159,7 @@ describe Members::DestroyService do
       end
 
       it_behaves_like 'a service destroying a member' do
-        let(:opts) { { skip_authorization: true } }
+        let(:opts) { { skip_authorization: true, skip_subresources: true } }
         let(:member) { group_project.requesters.find_by(user_id: member_user.id) }
       end
 
@@ -168,12 +168,14 @@ describe Members::DestroyService do
       end
 
       it_behaves_like 'a service destroying a member' do
-        let(:opts) { { skip_authorization: true } }
+        let(:opts) { { skip_authorization: true, skip_subresources: true  } }
         let(:member) { group.requesters.find_by(user_id: member_user.id) }
       end
     end
 
     context 'when current user can destroy the given access requester' do
+      let(:opts) { { skip_subresources: true } }
+
       before do
         group_project.add_maintainer(current_user)
         group.add_owner(current_user)
@@ -229,4 +231,54 @@ describe Members::DestroyService do
       end
     end
   end
+
+  context 'subresources' do
+    let(:user) { create(:user) }
+    let(:member_user) { create(:user) }
+    let(:opts) { {} }
+
+    let(:group) { create(:group, :public) }
+    let(:subgroup) { create(:group, parent: group) }
+    let(:subsubgroup) { create(:group, parent: subgroup) }
+    let(:subsubproject) { create(:project, group: subsubgroup) }
+
+    let(:group_project) { create(:project, :public, group: group) }
+    let(:control_project) { create(:project, group: subsubgroup) }
+
+    before do
+      create(:group_member, :developer, group: subsubgroup, user: member_user)
+
+      subsubproject.add_developer(member_user)
+      control_project.add_maintainer(user)
+      group.add_owner(user)
+
+      group_member = create(:group_member, :developer, group: group, user: member_user)
+
+      described_class.new(user).execute(group_member, opts)
+    end
+
+    it 'removes the project membership' do
+      expect(group_project.members.map(&:user)).not_to include(member_user)
+    end
+
+    it 'removes the group membership' do
+      expect(group.members.map(&:user)).not_to include(member_user)
+    end
+
+    it 'removes the subgroup membership', :postgresql do
+      expect(subgroup.members.map(&:user)).not_to include(member_user)
+    end
+
+    it 'removes the subsubgroup membership', :postgresql do
+      expect(subsubgroup.members.map(&:user)).not_to include(member_user)
+    end
+
+    it 'removes the subsubproject membership', :postgresql do
+      expect(subsubproject.members.map(&:user)).not_to include(member_user)
+    end
+
+    it 'does not remove the user from the control project' do
+      expect(control_project.members.map(&:user)).to include(user)
+    end
+  end
 end