summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/projects/merge_requests/creations_controller.rb1
-rw-r--r--app/controllers/projects/merge_requests_controller.rb1
-rw-r--r--app/controllers/projects_controller.rb5
-rw-r--r--app/finders/joined_groups_finder.rb19
-rw-r--r--app/models/group.rb30
-rw-r--r--app/models/merge_request.rb1
-rw-r--r--app/models/user.rb10
-rw-r--r--app/views/projects/merge_requests/_form.html.haml2
-rw-r--r--app/views/projects/merge_requests/creations/_new_submit.html.haml2
-rw-r--r--app/views/shared/issuable/_form.html.haml3
-rw-r--r--rubocop/cop/group_public_or_visible_to_user.rb22
-rw-r--r--rubocop/rubocop.rb1
-rw-r--r--spec/models/group_spec.rb42
-rw-r--r--spec/requests/api/groups_spec.rb14
-rw-r--r--spec/rubocop/cop/group_public_or_visible_to_user_spec.rb28
-rw-r--r--spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb1
-rw-r--r--spec/views/projects/merge_requests/edit.html.haml_spec.rb1
17 files changed, 143 insertions, 40 deletions
diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb
index 2ccb3896857..92e7d7fdebd 100644
--- a/app/controllers/projects/merge_requests/creations_controller.rb
+++ b/app/controllers/projects/merge_requests/creations_controller.rb
@@ -103,6 +103,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
@source_project = @merge_request.source_project
@commits = set_commits_for_rendering(@merge_request.commits)
@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
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 75a85fafa3f..dbe710f58e6 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -331,6 +331,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
@target_project = @merge_request.target_project
@target_branches = @merge_request.target_project.repository.branch_names
@noteable = @merge_request
+ @mr_presenter = @merge_request.present(current_user: current_user)
end
def finder_type
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 7352c5e9bec..a9417369ca2 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -16,6 +16,7 @@ class ProjectsController < Projects::ApplicationController
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 :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export]
+ before_action :present_project, only: [:edit]
# Authorize
before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export]
@@ -433,4 +434,8 @@ class ProjectsController < Projects::ApplicationController
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42440')
end
+
+ def present_project
+ @project = @project.present(current_user: current_user)
+ end
end
diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb
index 18cc6891ca4..4d8128dd824 100644
--- a/app/finders/joined_groups_finder.rb
+++ b/app/finders/joined_groups_finder.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-class JoinedGroupsFinder < UnionFinder
+class JoinedGroupsFinder
def initialize(user)
@user = user
end
@@ -8,19 +8,8 @@ class JoinedGroupsFinder < UnionFinder
# Finds the groups of the source user, optionally limited to those visible to
# the current user.
def execute(current_user = nil)
- segments = all_groups(current_user)
-
- find_union(segments, Group).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
+ @user.authorized_groups
+ .public_or_visible_to_user(current_user)
+ .order_id_desc
end
end
diff --git a/app/models/group.rb b/app/models/group.rb
index 62af20d2142..612c546ca57 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -82,8 +82,17 @@ class Group < Namespace
User.reference_pattern
end
- def visible_to_user(user)
- where(id: user.authorized_groups.select(:id).reorder(nil))
+ # WARNING: This method should never be used on its own
+ # 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
def select_for_project_authorization
@@ -95,6 +104,23 @@ class Group < Namespace
super
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
# Overrides notification_settings has_many association
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index dd5d494997d..0481a4a3d28 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -6,6 +6,7 @@ class MergeRequest < ActiveRecord::Base
include Issuable
include Noteable
include Referable
+ include Presentable
include IgnorableColumn
include TimeTrackable
include ManualInverseAssociation
diff --git a/app/models/user.rb b/app/models/user.rb
index eeac87e2e52..cd3b1c95b7e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -674,10 +674,12 @@ class User < ActiveRecord::Base
# Returns the groups a user has access to, either through a membership or a project authorization
def authorized_groups
- Group.from_union([
- groups,
- authorized_projects.joins(:namespace).select('namespaces.*')
- ])
+ Group.unscoped do
+ Group.from_union([
+ groups,
+ authorized_projects.joins(:namespace).select('namespaces.*')
+ ])
+ end
end
# Returns the groups a user is a member of, either directly or through a parent group
diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml
index 5a59f956cb5..13b967beba1 100644
--- a/app/views/projects/merge_requests/_form.html.haml
+++ b/app/views/projects/merge_requests/_form.html.haml
@@ -1,4 +1,4 @@
= form_for [@project.namespace.becomes(Namespace), @project, @merge_request],
html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' },
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
diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml
index d5c4134dee2..464f8fa65e9 100644
--- a/app/views/projects/merge_requests/creations/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/creations/_new_submit.html.haml
@@ -11,7 +11,7 @@
= link_to 'Change branches', mr_change_branches_path(@merge_request)
%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|
- = 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_branch
= f.hidden_field :target_project_id
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index 5b28a43a361..b33c758b464 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -1,6 +1,7 @@
- form = local_assigns.fetch(:f)
- commits = local_assigns[:commits]
- project = @target_project || @project
+- presenter = local_assigns.fetch(:presenter, nil)
= form_errors(issuable)
@@ -29,7 +30,7 @@
= 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
diff --git a/rubocop/cop/group_public_or_visible_to_user.rb b/rubocop/cop/group_public_or_visible_to_user.rb
new file mode 100644
index 00000000000..beda0b7f8ba
--- /dev/null
+++ b/rubocop/cop/group_public_or_visible_to_user.rb
@@ -0,0 +1,22 @@
+# 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
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index ff929c7b6ce..76d6037706e 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -38,3 +38,4 @@ require_relative 'cop/code_reuse/service_class'
require_relative 'cop/code_reuse/presenter'
require_relative 'cop/code_reuse/serializer'
require_relative 'cop/code_reuse/active_record'
+require_relative 'cop/group_public_or_visible_to_user'
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 0729eb99e78..1bf8f89e126 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -169,22 +169,42 @@ describe Group do
end
end
- describe '.visible_to_user' do
- let!(:group) { create(:group) }
- let!(:user) { create(:user) }
+ describe '.public_or_visible_to_user' do
+ let!(:private_group) { create(:group, :private) }
+ 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
- before do
- group.add_user(user, Gitlab::Access::MAINTAINER)
- end
+ context 'when user is nil' do
+ let!(:user) { nil }
- it { is_expected.to eq([group]) }
+ it { is_expected.to match_array([group]) }
end
- describe 'when the user does not have access to any groups' do
- it { is_expected.to eq([]) }
+ context 'when user' do
+ 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
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 3a8948f8477..3802b5c6848 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -155,7 +155,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers
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
it "sorts in descending order when passed" do
@@ -164,7 +164,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers
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
it "sorts by path in order_by param" do
@@ -173,7 +173,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers
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
it "sorts by id in the order_by param" do
@@ -182,7 +182,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers
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
it "sorts also by descending id with pagination fix" do
@@ -191,7 +191,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers
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
it "sorts identical keys by id for good pagination" do
@@ -211,6 +211,10 @@ describe API::Groups do
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)
end
+
+ def groups_visible_to_user(user)
+ Group.where(id: user.authorized_groups.select(:id).reorder(nil))
+ end
end
context 'when using owned in the request' do
diff --git a/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb
new file mode 100644
index 00000000000..7b5235a3da7
--- /dev/null
+++ b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb
@@ -0,0 +1,28 @@
+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
diff --git a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
index 8befae39d3a..0206928a211 100644
--- a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
+++ b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
@@ -12,6 +12,7 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
assign(:hidden_commit_count, 0)
assign(:total_commit_count, merge_request.commits.count)
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(:url_for).and_return('#')
diff --git a/spec/views/projects/merge_requests/edit.html.haml_spec.rb b/spec/views/projects/merge_requests/edit.html.haml_spec.rb
index 9b74a7e1946..c13eab30054 100644
--- a/spec/views/projects/merge_requests/edit.html.haml_spec.rb
+++ b/spec/views/projects/merge_requests/edit.html.haml_spec.rb
@@ -24,6 +24,7 @@ describe 'projects/merge_requests/edit.html.haml' do
before do
assign(:project, project)
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(:current_user)
View Lorry Controller Status