Commit 6c43aa6b authored by Tiago Botelho's avatar Tiago Botelho

Stops group approvers from leaking private group information

parent 89f2a74f
...@@ -105,6 +105,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap ...@@ -105,6 +105,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
@source_project = @merge_request.source_project @source_project = @merge_request.source_project
@commits = set_commits_for_rendering(@merge_request.commits) @commits = set_commits_for_rendering(@merge_request.commits)
@commit = @merge_request.diff_head_commit @commit = @merge_request.diff_head_commit
@mr_presenter = @merge_request.present(current_user: current_user)
@labels = LabelsFinder.new(current_user, project_id: @project.id).execute @labels = LabelsFinder.new(current_user, project_id: @project.id).execute
......
...@@ -341,6 +341,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -341,6 +341,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
@target_project = @merge_request.target_project @target_project = @merge_request.target_project
@target_branches = @merge_request.target_project.repository.branch_names @target_branches = @merge_request.target_project.repository.branch_names
@noteable = @merge_request @noteable = @merge_request
@mr_presenter = @merge_request.present(current_user: current_user)
end end
def finder_type def finder_type
......
...@@ -18,6 +18,7 @@ class ProjectsController < Projects::ApplicationController ...@@ -18,6 +18,7 @@ class ProjectsController < Projects::ApplicationController
before_action :tree, only: [:show], if: [:repo_exists?, :project_view_files?] before_action :tree, only: [:show], if: [:repo_exists?, :project_view_files?]
before_action :lfs_blob_ids, only: [:show], if: [:repo_exists?, :project_view_files?] before_action :lfs_blob_ids, only: [:show], if: [:repo_exists?, :project_view_files?]
before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export] before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export]
before_action :present_project, only: [:edit]
# Authorize # Authorize
before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export] before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export]
...@@ -442,4 +443,8 @@ class ProjectsController < Projects::ApplicationController ...@@ -442,4 +443,8 @@ class ProjectsController < Projects::ApplicationController
def whitelist_query_limiting def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42440') Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42440')
end end
def present_project
@project = @project.present(current_user: current_user)
end
end end
# frozen_string_literal: true # frozen_string_literal: true
class JoinedGroupsFinder < UnionFinder class JoinedGroupsFinder
def initialize(user) def initialize(user)
@user = user @user = user
end end
...@@ -8,19 +8,8 @@ class JoinedGroupsFinder < UnionFinder ...@@ -8,19 +8,8 @@ class JoinedGroupsFinder < UnionFinder
# Finds the groups of the source user, optionally limited to those visible to # Finds the groups of the source user, optionally limited to those visible to
# the current user. # the current user.
def execute(current_user = nil) def execute(current_user = nil)
segments = all_groups(current_user) @user.authorized_groups
.public_or_visible_to_user(current_user)
find_union(segments, Group).order_id_desc .order_id_desc
end
private
def all_groups(current_user)
groups = []
groups << @user.authorized_groups.visible_to_user(current_user) if current_user
groups << @user.authorized_groups.public_to_user(current_user)
groups
end end
end end
...@@ -84,8 +84,17 @@ class Group < Namespace ...@@ -84,8 +84,17 @@ class Group < Namespace
User.reference_pattern User.reference_pattern
end end
def visible_to_user(user) # WARNING: This method should never be used on its own
where(id: user.authorized_groups.select(:id).reorder(nil)) # please do make sure the number of rows you are filtering is small
# enough for this query
def public_or_visible_to_user(user)
return public_to_user unless user
public_for_user = public_to_user_arel(user)
visible_for_user = visible_to_user_arel(user)
public_or_visible = public_for_user.or(visible_for_user)
where(public_or_visible)
end end
def select_for_project_authorization def select_for_project_authorization
...@@ -97,6 +106,23 @@ class Group < Namespace ...@@ -97,6 +106,23 @@ class Group < Namespace
super super
end end
end end
private
def public_to_user_arel(user)
self.arel_table[:visibility_level]
.in(Gitlab::VisibilityLevel.levels_for_user(user))
end
def visible_to_user_arel(user)
groups_table = self.arel_table
authorized_groups = user.authorized_groups.as('authorized')
groups_table.project(1)
.from(authorized_groups)
.where(authorized_groups[:id].eq(groups_table[:id]))
.exists
end
end end
# Overrides notification_settings has_many association # Overrides notification_settings has_many association
......
...@@ -6,6 +6,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -6,6 +6,7 @@ class MergeRequest < ActiveRecord::Base
include Issuable include Issuable
include Noteable include Noteable
include Referable include Referable
include Presentable
include Elastic::MergeRequestsSearch include Elastic::MergeRequestsSearch
include IgnorableColumn include IgnorableColumn
include TimeTrackable include TimeTrackable
......
...@@ -692,10 +692,12 @@ class User < ActiveRecord::Base ...@@ -692,10 +692,12 @@ class User < ActiveRecord::Base
# Returns the groups a user has access to, either through a membership or a project authorization # Returns the groups a user has access to, either through a membership or a project authorization
def authorized_groups def authorized_groups
Group.from_union([ Group.unscoped do
groups, Group.from_union([
authorized_projects.joins(:namespace).select('namespaces.*') groups,
]) authorized_projects.joins(:namespace).select('namespaces.*')
])
end
end end
# Returns the groups a user is a member of, either directly or through a parent group # Returns the groups a user is a member of, either directly or through a parent group
......
= form_for [@project.namespace.becomes(Namespace), @project, @merge_request], = form_for [@project.namespace.becomes(Namespace), @project, @merge_request],
html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' }, html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' },
data: { markdown_version: @merge_request.cached_markdown_version } do |f| data: { markdown_version: @merge_request.cached_markdown_version } do |f|
= render 'shared/issuable/form', f: f, issuable: @merge_request = render 'shared/issuable/form', f: f, issuable: @merge_request, presenter: @mr_presenter
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
= link_to 'Change branches', mr_change_branches_path(@merge_request) = link_to 'Change branches', mr_change_branches_path(@merge_request)
%hr %hr
= form_for [@project.namespace.becomes(Namespace), @project, @merge_request], html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' } do |f| = form_for [@project.namespace.becomes(Namespace), @project, @merge_request], html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' } do |f|
= render 'shared/issuable/form', f: f, issuable: @merge_request, commits: @commits = render 'shared/issuable/form', f: f, issuable: @merge_request, commits: @commits, presenter: @mr_presenter
= f.hidden_field :source_project_id = f.hidden_field :source_project_id
= f.hidden_field :source_branch = f.hidden_field :source_branch
= f.hidden_field :target_project_id = f.hidden_field :target_project_id
......
- form = local_assigns.fetch(:f) - form = local_assigns.fetch(:f)
- commits = local_assigns[:commits] - commits = local_assigns[:commits]
- project = @target_project || @project - project = @target_project || @project
- presenter = local_assigns.fetch(:presenter, nil)
= form_errors(issuable) = form_errors(issuable)
...@@ -29,7 +30,7 @@ ...@@ -29,7 +30,7 @@
= render 'shared/issuable/form/metadata', issuable: issuable, form: form = render 'shared/issuable/form/metadata', issuable: issuable, form: form
= render_if_exists 'shared/issuable/approvals', issuable: issuable, form: form = render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form
= render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form = render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form
......
...@@ -5,4 +5,10 @@ class ApproverGroup < ActiveRecord::Base ...@@ -5,4 +5,10 @@ class ApproverGroup < ActiveRecord::Base
validates :group, presence: true validates :group, presence: true
delegate :users, to: :group delegate :users, to: :group
def self.filtered_approver_groups(approver_groups, user)
public_or_visible_groups = Group.public_or_visible_to_user(user) # rubocop:disable Cop/GroupPublicOrVisibleToUser
approver_groups.joins(:group).merge(public_or_visible_groups)
end
end end
module EE module EE
module MergeRequestPresenter module MergeRequestPresenter
include ::Gitlab::Utils::StrongMemoize
def approvals_path def approvals_path
if requires_approve? if requires_approve?
approvals_project_merge_request_path(project, merge_request) approvals_project_merge_request_path(project, merge_request)
end end
end end
def all_approvers_including_groups
strong_memoize(:all_approvers_including_groups) do
approvers = []
# approvers from direct assignment
approvers << merge_request.approvers_from_users
approvers << approvers_from_groups
approvers.flatten
end
end
def overall_approver_groups
if approvers_overwritten?
approver_groups
else
merge_request.target_project.present(current_user: current_user).approver_groups
end
end
def approvers_overwritten?
merge_request.approvers.to_a.any? || approver_groups.to_a.any?
end
private
def approvers_from_groups
overall_approver_groups.flat_map(&:users) - [merge_request.author]
end
def approver_groups
::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user)
end
end end
end end
...@@ -20,6 +20,10 @@ module EE ...@@ -20,6 +20,10 @@ module EE
anchors anchors
end end
def approver_groups
::ApproverGroup.filtered_approver_groups(project.approver_groups, current_user)
end
private private
def security_dashboard_data def security_dashboard_data
......
- issuable = local_assigns.fetch(:issuable) - issuable = local_assigns.fetch(:issuable)
- presenter = local_assigns.fetch(:presenter)
- form = local_assigns.fetch(:form) - form = local_assigns.fetch(:form)
- return unless issuable.is_a?(MergeRequest) - return unless issuable.is_a?(MergeRequest)
...@@ -13,7 +14,7 @@ ...@@ -13,7 +14,7 @@
Approvers Approvers
.col-sm-10 .col-sm-10
- if can_update_approvers - if can_update_approvers
- skip_users = [*issuable.all_approvers_including_groups, (author unless authors_can_approve)].compact - skip_users = [*presenter.all_approvers_including_groups, (author unless authors_can_approve)].compact
= users_select_tag("merge_request[approver_ids]", = users_select_tag("merge_request[approver_ids]",
multiple: true, multiple: true,
...@@ -21,12 +22,11 @@ ...@@ -21,12 +22,11 @@
email_user: true, email_user: true,
skip_users: skip_users, skip_users: skip_users,
project: issuable.target_project) project: issuable.target_project)
.form-text.text-muted .form-text.text-muted
This merge request must be approved by these users. This merge request must be approved by these users.
You can override the project settings by setting your own list of approvers. You can override the project settings by setting your own list of approvers.
- skip_groups = issuable.overall_approver_groups.pluck(:group_id) # rubocop: disable CodeReuse/ActiveRecord - skip_groups = presenter.overall_approver_groups.pluck(:group_id) # rubocop: disable CodeReuse/ActiveRecord
= groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true, project: issuable.target_project }, class: 'input-large') = groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true, project: issuable.target_project }, class: 'input-large')
.form-text.text-muted .form-text.text-muted
This merge request must be approved by members of these groups. This merge request must be approved by members of these groups.
...@@ -36,10 +36,10 @@ ...@@ -36,10 +36,10 @@
.card-header .card-header
Approvers Approvers
%ul.content-list.approver-list %ul.content-list.approver-list
- if issuable.all_approvers_including_groups.empty? - if presenter.all_approvers_including_groups.empty?
%li.no-approvers There are no approvers %li.no-approvers There are no approvers
- else - else
- unsaved_approvers = !issuable.approvers_overwritten? - unsaved_approvers = !presenter.approvers_overwritten?
- item_classes = unsaved_approvers ? ['unsaved-approvers'] : [] - item_classes = unsaved_approvers ? ['unsaved-approvers'] : []
- issuable.overall_approvers.each do |approver| - issuable.overall_approvers.each do |approver|
%li{ id: dom_id(approver.user), class: item_classes + ['approver'] } %li{ id: dom_id(approver.user), class: item_classes + ['approver'] }
...@@ -54,7 +54,7 @@ ...@@ -54,7 +54,7 @@
= link_to project_merge_request_approver_path(@project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-sm btn btn-remove", title: 'Remove approver' do = link_to project_merge_request_approver_path(@project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-sm btn btn-remove", title: 'Remove approver' do
= icon("sign-out") = icon("sign-out")
Remove Remove
- issuable.overall_approver_groups.each do |approver_group| - presenter.overall_approver_groups.each do |approver_group|
%li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] } %li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] }
Group: Group:
= link_to approver_group.group.name, approver_group.group = link_to approver_group.group.name, approver_group.group
......
---
title: Project groups approvers no longer leak private groups info
merge_request:
author:
type: security
...@@ -2,13 +2,15 @@ require 'spec_helper' ...@@ -2,13 +2,15 @@ require 'spec_helper'
describe 'Service Desk Setting', :js do describe 'Service Desk Setting', :js do
let(:project) { create(:project_empty_repo, :private, service_desk_enabled: false) } let(:project) { create(:project_empty_repo, :private, service_desk_enabled: false) }
let(:presenter) { project.present(current_user: user) }
let(:user) { create(:user) } let(:user) { create(:user) }
before do before do
project.add_maintainer(user) project.add_maintainer(user)
sign_in(user) sign_in(user)
allow(::EE::Gitlab::ServiceDesk).to receive(:enabled?).with(project: project).and_return(true) allow_any_instance_of(Project).to receive(:present).with(current_user: user).and_return(presenter)
allow(::EE::Gitlab::ServiceDesk).to receive(:enabled?).with(project: presenter).and_return(true)
allow(::Gitlab::IncomingEmail).to receive(:enabled?) { true } allow(::Gitlab::IncomingEmail).to receive(:enabled?) { true }
allow(::Gitlab::IncomingEmail).to receive(:supports_wildcard?) { true } allow(::Gitlab::IncomingEmail).to receive(:supports_wildcard?) { true }
......
...@@ -4,4 +4,24 @@ describe ApproverGroup do ...@@ -4,4 +4,24 @@ describe ApproverGroup do
subject { create(:approver_group) } subject { create(:approver_group) }
it { is_expected.to be_valid } it { is_expected.to be_valid }
describe '.filtered_approver_groups' do
let!(:project) { create(:project) }
let!(:user) { project.creator }
let!(:private_group) { create(:group, :private) }
let!(:public_approver_group) { create(:approver_group, target: project) }
let!(:private_approver_group) { create(:approver_group, target: project, group: private_group) }
subject { described_class.filtered_approver_groups(project.approver_groups, user) }
it { is_expected.to match_array([public_approver_group]) }
context 'when user has access to private group' do
before do
private_group.add_user(user, Gitlab::Access::DEVELOPER)
end
it { is_expected.to match_array([public_approver_group, private_approver_group]) }
end
end
end end
require 'spec_helper'
describe MergeRequestPresenter do
let(:resource) { create :merge_request, source_project: project }
let!(:project) { create(:project, :repository) }
let!(:user) { project.creator }
describe '#all_approvers_including_groups' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
let!(:approver) { create(:approver, target: resource) }
subject { described_class.new(resource, current_user: user).all_approvers_including_groups }
it { is_expected.to match_array([public_approver_group.users, approver.user].flatten) }
context 'when user has access to private group' do
before do
private_group.add_user(user, Gitlab::Access::DEVELOPER)
end
it do
approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]
is_expected.to match_array(approvers)
end
end
end
describe '#overall_approver_groups' do
subject { described_class.new(resource, current_user: user).overall_approver_groups }
context 'when approvers is overwritten' do
let!(:public_approver_group) { create(:approver_group, target: project) }
it { is_expected.to match_array(project.approver_groups) }
end
context 'when approvers is not overwritten' do
let!(:public_approver_group) { create(:approver_group, target: resource) }
it { is_expected.to match_array([public_approver_group]) }
end
end
describe '#approvers_overwritten?' do
subject { described_class.new(resource, current_user: user).approvers_overwritten? }
it { is_expected.to be_falsey }
context 'when merge request has user and group approvers' do
let!(:public_approver_group) { create(:approver_group, target: resource) }
let!(:approver) { create(:approver, user: user, target: resource) }
it { is_expected.to be_truthy }
end
context 'when merge request only has user approvers' do
let!(:approver) { create(:approver, user: user, target: resource) }
it { is_expected.to be_truthy }
end
context 'when merge request only has group approvers' do
let!(:public_approver_group) { create(:approver_group, target: resource) }
it { is_expected.to be_truthy }
end
end
end
...@@ -4,6 +4,7 @@ describe 'shared/issuable/_approvals.html.haml' do ...@@ -4,6 +4,7 @@ describe 'shared/issuable/_approvals.html.haml' do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { build(:project) } let(:project) { build(:project) }
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
let(:presenter) { merge_request.present(current_user: user) }
let(:form) { double('form') } let(:form) { double('form') }
before do before do
...@@ -18,14 +19,14 @@ describe 'shared/issuable/_approvals.html.haml' do ...@@ -18,14 +19,14 @@ describe 'shared/issuable/_approvals.html.haml' do
context 'has no approvers' do context 'has no approvers' do
it 'shows empty approvers list' do it 'shows empty approvers list' do
render 'shared/issuable/approvals', form: form, issuable: merge_request render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
expect(rendered).to have_text('There are no approvers') expect(rendered).to have_text('There are no approvers')
end end
context 'can override approvers' do context 'can override approvers' do
before do before do
render 'shared/issuable/approvals', form: form, issuable: merge_request render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
end end
it 'shows suggested approvers' do it 'shows suggested approvers' do
...@@ -44,7 +45,7 @@ describe 'shared/issuable/_approvals.html.haml' do ...@@ -44,7 +45,7 @@ describe 'shared/issuable/_approvals.html.haml' do
context 'can not override approvers' do context 'can not override approvers' do
before do before do
allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false) allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
render 'shared/issuable/approvals', form: form, issuable: merge_request render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
end end
it 'hides suggested approvers' do it 'hides suggested approvers' do
...@@ -69,10 +70,11 @@ describe 'shared/issuable/_approvals.html.haml' do ...@@ -69,10 +70,11 @@ describe 'shared/issuable/_approvals.html.haml' do
before do before do
assign(:approver, approver) assign(:approver, approver)
assign(:approver_group, approver_group) assign(:approver_group, approver_group)
assign(:presenter, merge_request.present(current_user: user))
end end
it 'shows approver in table' do it 'shows approver in table' do
render 'shared/issuable/approvals', form: form, issuable: merge_request, project: project render 'shared/issuable/approvals', form: form, issuable: merge_request, project: project, presenter: presenter
expect(rendered).to have_text(approver[:name]) expect(rendered).to have_text(approver[:name])
expect(rendered).to have_text(approver_group[:name]) expect(rendered).to have_text(approver_group[:name])
...@@ -80,7 +82,7 @@ describe 'shared/issuable/_approvals.html.haml' do ...@@ -80,7 +82,7 @@ describe 'shared/issuable/_approvals.html.haml' do
context 'can override approvers' do context 'can override approvers' do
it 'shows remove button for approver' do it 'shows remove button for approver' do
render 'shared/issuable/approvals', form: form, issuable: merge_request render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
expect(rendered).to have_css('.btn-remove') expect(rendered).to have_css('.btn-remove')
end end
...@@ -90,7 +92,7 @@ describe 'shared/issuable/_approvals.html.haml' do ...@@ -90,7 +92,7 @@ describe 'shared/issuable/_approvals.html.haml' do
it 'hides remove button' do it 'hides remove button' do
allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false) allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
render 'shared/issuable/approvals', form: form, issuable: merge_request render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
expect(rendered).not_to have_css('.btn-remove') expect(rendered).not_to have_css('.btn-remove')
end end
......
# frozen_string_literal: true
#
module RuboCop
module Cop
# Cop that blacklists the usage of Group.public_or_visible_to_user
class GroupPublicOrVisibleToUser < RuboCop::Cop::Cop
MSG = '`Group.public_or_visible_to_user` should be used with extreme care. ' \
'Please ensure that you are not using it on its own and that the amount ' \
'of rows being filtered is reasonable.'
def_node_matcher :public_or_visible_to_user?, <<~PATTERN
(send (const nil? :Group) :public_or_visible_to_user ...)
PATTERN
def on_send(node)
return unless public_or_visible_to_user?(node)
add_offense(node, location: :expression)
end
end
end
end
...@@ -38,3 +38,4 @@ require_relative 'cop/code_reuse/service_class' ...@@ -38,3 +38,4 @@ require_relative 'cop/code_reuse/service_class'
require_relative 'cop/code_reuse/presenter' require_relative 'cop/code_reuse/presenter'
require_relative 'cop/code_reuse/serializer' require_relative 'cop/code_reuse/serializer'
require_relative 'cop/code_reuse/active_record' require_relative 'cop/code_reuse/active_record'
require_relative 'cop/group_public_or_visible_to_user'
...@@ -169,22 +169,42 @@ describe Group do ...@@ -169,22 +169,42 @@ describe Group do
end end
end end
describe '.visible_to_user' do describe '.public_or_visible_to_user' do
let!(:group) { create(:group) } let!(:private_group) { create(:group, :private) }
let!(:user) { create(:user) } let!(:internal_group) { create(:group, :internal) }
subject { described_class.visible_to_user(user) } subject { described_class.public_or_visible_to_user(user) }
describe 'when the user has access to a group' do context 'when user is nil' do
before do let!(:user) { nil }
group.add_user(user, Gitlab::Access::MAINTAINER)
end
it { is_expected.to eq([group]) } it { is_expected.to match_array([group]) }
end end
describe 'when the user does not have access to any groups' do context 'when user' do
it { is_expected.to eq([]) } let!(:user) { create(:user) }
context 'when user does not have access to any private group' do
it { is_expected.to match_array([internal_group, group]) }
end
context 'when user is a member of private group' do
before do
private_group.add_user(user, Gitlab::Access::DEVELOPER)
end
it { is_expected.to match_array([private_group, internal_group, group]) }
end
context 'when user is a member of private subgroup', :postgresql do
let!(:private_subgroup) { create(:group, :private, parent: private_group) }
before do
private_subgroup.add_user(user, Gitlab::Access::DEVELOPER)
end
it { is_expected.to match_array([private_subgroup, internal_group, group]) }
end
end end
end end
......
...@@ -169,7 +169,7 @@ describe API::Groups do ...@@ -169,7 +169,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq(Group.visible_to_user(user1).order(:name).pluck(:name)) expect(response_groups).to eq(groups_visible_to_user(user1).order(:name).pluck(:name))
end end
it "sorts in descending order when passed" do it "sorts in descending order when passed" do
...@@ -178,7 +178,7 @@ describe API::Groups do ...@@ -178,7 +178,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq(Group.visible_to_user(user1).order(name: :desc).pluck(:name)) expect(response_groups).to eq(groups_visible_to_user(user1).order(name: :desc).pluck(:name))
end end
it "sorts by path in order_by param" do it "sorts by path in order_by param" do
...@@ -187,7 +187,7 @@ describe API::Groups do ...@@ -187,7 +187,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq(Group.visible_to_user(user1).order(:path).pluck(:name)) expect(response_groups).to eq(groups_visible_to_user(user1).order(:path).pluck(:name))
end end
it "sorts by id in the order_by param" do it "sorts by id in the order_by param" do
...@@ -196,7 +196,7 @@ describe API::Groups do ...@@ -196,7 +196,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq(Group.visible_to_user(user1).order(:id).pluck(:name)) expect(response_groups).to eq(groups_visible_to_user(user1).order(:id).pluck(:name))
end end
it "sorts also by descending id with pagination fix" do it "sorts also by descending id with pagination fix" do
...@@ -205,7 +205,7 @@ describe API::Groups do ...@@ -205,7 +205,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq(Group.visible_to_user(user1).order(id: :desc).pluck(:name)) expect(response_groups).to eq(groups_visible_to_user(user1).order(id: :desc).pluck(:name))
end end
it "sorts identical keys by id for good pagination" do it "sorts identical keys by id for good pagination" do
...@@ -225,6 +225,10 @@ describe API::Groups do ...@@ -225,6 +225,10 @@ describe API::Groups do
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort) expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort)
end end
def groups_visible_to_user(user)
Group.where(id: user.authorized_groups.select(:id).reorder(nil))
end
end end
context 'when using owned in the request' do context 'when using owned in the request' do
......
require 'spec_helper'
require 'rubocop'
require 'rubocop/rspec/support'
require_relative '../../../rubocop/cop/group_public_or_visible_to_user'
describe RuboCop::Cop::GroupPublicOrVisibleToUser do
include CopHelper
subject(:cop) { described_class.new }
it 'flags the use of Group.public_or_visible_to_user with a constant receiver' do
inspect_source('Group.public_or_visible_to_user')
expect(cop.offenses.size).to eq(1)
end
it 'does not flat the use of public_or_visible_to_user with a constant that is not Group' do
inspect_source('Project.public_or_visible_to_user')
expect(cop.offenses.size).to eq(0)
end
it 'does not flag the use of Group.public_or_visible_to_user with a send receiver' do
inspect_source('foo.public_or_visible_to_user')
expect(cop.offenses.size).to eq(0)
end
end
...@@ -12,6 +12,7 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do ...@@ -12,6 +12,7 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
assign(:hidden_commit_count, 0) assign(:hidden_commit_count, 0)
assign(:total_commit_count, merge_request.commits.count) assign(:total_commit_count, merge_request.commits.count)
assign(:project, merge_request.target_project) assign(:project, merge_request.target_project)
assign(:mr_presenter, merge_request.present(current_user: merge_request.author))
allow(view).to receive(:can?).and_return(true) allow(view).to receive(:can?).and_return(true)
allow(view).to receive(:url_for).and_return('#') allow(view).to receive(:url_for).and_return('#')
......
...@@ -24,6 +24,7 @@ describe 'projects/merge_requests/edit.html.haml' do ...@@ -24,6 +24,7 @@ describe 'projects/merge_requests/edit.html.haml' do
before do before do
assign(:project, project) assign(:project, project)
assign(:merge_request, closed_merge_request) assign(:merge_request, closed_merge_request)
assign(:mr_presenter, closed_merge_request.present(current_user: user))
allow(view).to receive(:can?).and_return(true) allow(view).to receive(:can?).and_return(true)
allow(view).to receive(:current_user) allow(view).to receive(:current_user)
......
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