diff options
author | Bob Van Landuyt <bob@gitlab.com> | 2018-09-24 17:47:23 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-09-24 17:54:48 +0300 |
commit | 5a20bb1db2c46c2f06ebbb9401e8bf0e6787d856 (patch) | |
tree | a84cadacb0b3b3ad4c7eb1bf238ea47b08b1db61 /app | |
parent | d6df5da26ac8ccf0ea4e81a9949f4a0512fd9a57 (diff) |
Merge branch 'security-11-3-6881-project-group-approvers-leaks-private-group-info-ce' into 'security-11-3'
[11.3] CE: Project group approvers leaks private group info
See merge request gitlab/gitlabhq!2525
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/projects/merge_requests/creations_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/projects_controller.rb | 5 | ||||
-rw-r--r-- | app/finders/joined_groups_finder.rb | 21 | ||||
-rw-r--r-- | app/mailers/emails/merge_requests.rb | 12 | ||||
-rw-r--r-- | app/models/group.rb | 30 | ||||
-rw-r--r-- | app/models/merge_request.rb | 1 | ||||
-rw-r--r-- | app/models/user.rb | 8 | ||||
-rw-r--r-- | app/views/notify/new_merge_request_email.html.haml | 2 | ||||
-rw-r--r-- | app/views/notify/new_merge_request_email.text.erb | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_form.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/creations/_new_submit.html.haml | 2 | ||||
-rw-r--r-- | app/views/shared/issuable/_form.html.haml | 3 |
13 files changed, 68 insertions, 28 deletions
diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 03d0290ac1d..7ed13bf1948 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -104,6 +104,10 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap @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 set_pipeline_variables diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d31b58972ca..c4a58746ad2 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -330,6 +330,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @source_project = @merge_request.source_project @target_project = @merge_request.target_project @target_branches = @merge_request.target_project.repository.branch_names + + # 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 def finder_type diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index e40f3dab882..ad0dbca84af 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -14,6 +14,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] @@ -423,4 +424,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 47174980258..4d8128dd824 100644 --- a/app/finders/joined_groups_finder.rb +++ b/app/finders/joined_groups_finder.rb @@ -1,4 +1,6 @@ -class JoinedGroupsFinder < UnionFinder +# frozen_string_literal: true + +class JoinedGroupsFinder def initialize(user) @user = user end @@ -6,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/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 70f65d4e58d..31451adcdf9 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 @@ -73,11 +74,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/models/group.rb b/app/models/group.rb index 106a1f4a94c..60e50f13af8 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 396647a14ae..231a2911488 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 568ec101016..671657388fb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -642,10 +642,12 @@ class User < ActiveRecord::Base # Returns the groups a user has access to, either through a membership or a project authorization def authorized_groups - union = Gitlab::SQL::Union - .new([groups.select(:id), authorized_projects.select(:namespace_id)]) + Group.unscoped do + union = Gitlab::SQL::Union + .new([groups.select(:id), authorized_projects.select(:namespace_id)]) - Group.where("namespaces.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection + Group.where("namespaces.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection + end end # Returns the groups a user is a member of, either directly or through a parent group 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 %> 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 b49e47a7266..17ce1c2b08b 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 |