From 81d949f656979429835e0b6059a10a44faba488b Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Fri, 7 Sep 2018 13:29:19 +0100 Subject: [PATCH 1/2] Applies the CE backport of EE#657 --- .../merge_requests/creations_controller.rb | 1 + .../projects/merge_requests_controller.rb | 1 + app/controllers/projects_controller.rb | 5 +++ app/finders/joined_groups_finder.rb | 19 ++------- app/models/group.rb | 30 ++++++++++++- app/models/merge_request.rb | 1 + app/models/user.rb | 10 +++-- .../projects/merge_requests/_form.html.haml | 2 +- .../creations/_new_submit.html.haml | 2 +- app/views/shared/issuable/_form.html.haml | 3 +- .../cop/group_public_or_visible_to_user.rb | 22 ++++++++++ rubocop/rubocop.rb | 1 + spec/models/group_spec.rb | 42 ++++++++++++++----- spec/requests/api/groups_spec.rb | 14 ++++--- .../group_public_or_visible_to_user_spec.rb | 28 +++++++++++++ .../creations/_new_submit.html.haml_spec.rb | 1 + .../merge_requests/edit.html.haml_spec.rb | 1 + 17 files changed, 143 insertions(+), 40 deletions(-) create mode 100644 rubocop/cop/group_public_or_visible_to_user.rb create mode 100644 spec/rubocop/cop/group_public_or_visible_to_user_spec.rb 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) From 487ac98a2a7cebb1bdef3a5038c926c56380881c Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Fri, 14 Sep 2018 16:59:28 +0100 Subject: [PATCH 2/2] Ports new refactor from EE to CE --- .../projects/merge_requests/creations_controller.rb | 3 +++ .../projects/merge_requests_controller.rb | 3 +++ app/mailers/emails/merge_requests.rb | 12 +++++++++--- app/views/notify/new_merge_request_email.html.haml | 2 +- app/views/notify/new_merge_request_email.text.erb | 2 +- 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 92e7d7fdebd..20216c1f7a1 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -103,6 +103,9 @@ 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 + + # FIXME: We have to assign a presenter to another instance variable + # due to class_name checks being made with issuable classes @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 dbe710f58e6..d4fcd5a108b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -331,6 +331,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @target_project = @merge_request.target_project @target_branches = @merge_request.target_project.repository.branch_names @noteable = @merge_request + + # FIXME: We have to assign a presenter to another instance variable + # due to class_name checks being made with issuable classes @mr_presenter = @merge_request.present(current_user: current_user) end diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 67af0a4eb98..be085496731 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -3,13 +3,14 @@ module Emails module MergeRequests def new_merge_request_email(recipient_id, merge_request_id, reason = nil) - setup_merge_request_mail(merge_request_id, recipient_id) + setup_merge_request_mail(merge_request_id, recipient_id, present: true) mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason)) end def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil) - setup_merge_request_mail(merge_request_id, recipient_id) + setup_merge_request_mail(merge_request_id, recipient_id, present: true) + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end @@ -75,11 +76,16 @@ module Emails private - def setup_merge_request_mail(merge_request_id, recipient_id) + def setup_merge_request_mail(merge_request_id, recipient_id, present: false) @merge_request = MergeRequest.find(merge_request_id) @project = @merge_request.project @target_url = project_merge_request_url(@project, @merge_request) + if present + recipient = User.find(recipient_id) + @mr_presenter = @merge_request.present(current_user: recipient) + end + @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key) end diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml index dd6a84e503d..5acd45b74a7 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/app/views/notify/new_merge_request_email.html.haml @@ -9,7 +9,7 @@ %p Assignee: #{@merge_request.assignee_name} -= render_if_exists 'notify/merge_request_approvers', merge_request: @merge_request += render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter - if @merge_request.description %div diff --git a/app/views/notify/new_merge_request_email.text.erb b/app/views/notify/new_merge_request_email.text.erb index d5b8f8d764f..754f4bca1cd 100644 --- a/app/views/notify/new_merge_request_email.text.erb +++ b/app/views/notify/new_merge_request_email.text.erb @@ -5,6 +5,6 @@ New Merge Request <%= @merge_request.to_reference %> <%= merge_path_description(@merge_request, 'to') %> Author: <%= @merge_request.author_name %> Assignee: <%= @merge_request.assignee_name %> -<%= render_if_exists 'notify/merge_request_approvers', merge_request: @merge_request %> +<%= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter %> <%= @merge_request.description %>