Commit f6d8c63b authored by John Jarvis's avatar John Jarvis

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

parents 9ab1ac4f 24e8c3d1
......@@ -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
......
......@@ -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
......
......@@ -80,7 +80,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
......
......@@ -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
......@@ -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
......@@ -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')) }
......
......@@ -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?
......
......@@ -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
......
......@@ -175,6 +175,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
......
......@@ -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
......
......@@ -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
......
---
title: Add subresources removal to member destroy service
merge_request:
author:
type: security
---
title: Prevent private snippets from being embeddable
merge_request:
author:
type: security
......@@ -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
......
......@@ -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(
......
......@@ -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
......
......@@ -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
......
......@@ -423,4 +423,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
......@@ -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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment