diff options
Diffstat (limited to 'app/finders')
| -rw-r--r-- | app/finders/autocomplete_users_finder.rb | 22 | ||||
| -rw-r--r-- | app/finders/concerns/finder_methods.rb | 51 | ||||
| -rw-r--r-- | app/finders/concerns/finder_with_cross_project_access.rb | 70 | ||||
| -rw-r--r-- | app/finders/events_finder.rb | 4 | ||||
| -rw-r--r-- | app/finders/issuable_finder.rb | 122 | ||||
| -rw-r--r-- | app/finders/issues_finder.rb | 47 | ||||
| -rw-r--r-- | app/finders/labels_finder.rb | 29 | ||||
| -rw-r--r-- | app/finders/members_finder.rb | 57 | ||||
| -rw-r--r-- | app/finders/merge_request_target_project_finder.rb | 2 | ||||
| -rw-r--r-- | app/finders/merge_requests_finder.rb | 28 | ||||
| -rw-r--r-- | app/finders/milestones_finder.rb | 2 | ||||
| -rw-r--r-- | app/finders/notes_finder.rb | 2 | ||||
| -rw-r--r-- | app/finders/snippets_finder.rb | 75 | ||||
| -rw-r--r-- | app/finders/todos_finder.rb | 5 | ||||
| -rw-r--r-- | app/finders/user_recent_events_finder.rb | 33 |
15 files changed, 424 insertions, 125 deletions
diff --git a/app/finders/autocomplete_users_finder.rb b/app/finders/autocomplete_users_finder.rb index c3f5358b577..e8a03947f59 100644 --- a/app/finders/autocomplete_users_finder.rb +++ b/app/finders/autocomplete_users_finder.rb @@ -1,6 +1,12 @@ class AutocompleteUsersFinder + # The number of users to display in the results is hardcoded to 20, and + # pagination is not supported. This ensures that performance remains + # consistent and removes the need for implementing keyset pagination to ensure + # good performance. + LIMIT = 20 + attr_reader :current_user, :project, :group, :search, :skip_users, - :page, :per_page, :author_id, :params + :author_id, :params def initialize(params:, current_user:, project:, group:) @current_user = current_user @@ -8,8 +14,6 @@ class AutocompleteUsersFinder @group = group @search = params[:search] @skip_users = params[:skip_users] - @page = params[:page] - @per_page = params[:per_page] @author_id = params[:author_id] @params = params end @@ -20,7 +24,7 @@ class AutocompleteUsersFinder items = items.reorder(:name) items = items.search(search) if search.present? items = items.where.not(id: skip_users) if skip_users.present? - items = items.page(page).per(per_page) + items = items.limit(LIMIT) if params[:todo_filter].present? && current_user items = items.todo_authors(current_user.id, params[:todo_state_filter]) @@ -52,9 +56,13 @@ class AutocompleteUsersFinder end def users_from_project - user_ids = project.team.users.pluck(:id) - user_ids << author_id if author_id.present? + if author_id.present? + union = Gitlab::SQL::Union + .new([project.authorized_users, User.where(id: author_id)]) - User.where(id: user_ids) + User.from("(#{union.to_sql}) #{User.table_name}") + else + project.authorized_users + end end end diff --git a/app/finders/concerns/finder_methods.rb b/app/finders/concerns/finder_methods.rb new file mode 100644 index 00000000000..2e905fa5750 --- /dev/null +++ b/app/finders/concerns/finder_methods.rb @@ -0,0 +1,51 @@ +module FinderMethods + def find_by!(*args) + raise_not_found_unless_authorized execute.find_by!(*args) + end + + def find_by(*args) + if_authorized execute.find_by(*args) + end + + def find(*args) + raise_not_found_unless_authorized model.find(*args) + end + + private + + def raise_not_found_unless_authorized(result) + result = if_authorized(result) + + raise ActiveRecord::RecordNotFound.new("Couldn't find #{model}") unless result + + result + end + + def if_authorized(result) + # Return the result if the finder does not perform authorization checks. + # this is currently the case in the `MilestoneFinder` + return result unless respond_to?(:current_user) + + if can_read_object?(result) + result + else + nil + end + end + + def can_read_object?(object) + # When there's no policy, we'll allow the read, this is for example the case + # for Todos + return true unless DeclarativePolicy.has_policy?(object) + + model_name = object&.model_name || model.model_name + + Ability.allowed?(current_user, :"read_#{model_name.singular}", object) + end + + # This fetches the model from the `ActiveRecord::Relation` but does not + # actually execute the query. + def model + execute.model + end +end diff --git a/app/finders/concerns/finder_with_cross_project_access.rb b/app/finders/concerns/finder_with_cross_project_access.rb new file mode 100644 index 00000000000..92bf98d7cd2 --- /dev/null +++ b/app/finders/concerns/finder_with_cross_project_access.rb @@ -0,0 +1,70 @@ +# Module to prepend into finders to specify wether or not the finder requires +# cross project access +# +# This module depends on the finder implementing the following methods: +# +# - `#execute` should return an `ActiveRecord::Relation` +# - `#current_user` the user that requires access (or nil) +module FinderWithCrossProjectAccess + extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override + + prepended do + extend Gitlab::CrossProjectAccess::ClassMethods + end + + override :execute + def execute(*args) + check = Gitlab::CrossProjectAccess.find_check(self) + original = super + + return original unless check + return original if should_skip_cross_project_check || can_read_cross_project? + + if check.should_run?(self) + original.model.none + else + original + end + end + + # We can skip the cross project check for finding indivitual records. + # this would be handled by the `can?(:read_*, result)` call in `FinderMethods` + # itself. + override :find_by! + def find_by!(*args) + skip_cross_project_check { super } + end + + override :find_by + def find_by(*args) + skip_cross_project_check { super } + end + + override :find + def find(*args) + skip_cross_project_check { super } + end + + private + + attr_accessor :should_skip_cross_project_check + + def skip_cross_project_check + self.should_skip_cross_project_check = true + + yield + ensure + # The find could raise an `ActiveRecord::RecordNotFound`, after which we + # still want to re-enable the check. + self.should_skip_cross_project_check = false + end + + def can_read_cross_project? + Ability.allowed?(current_user, :read_cross_project) + end + + def can_read_project?(project) + Ability.allowed?(current_user, :read_project, project) + end +end diff --git a/app/finders/events_finder.rb b/app/finders/events_finder.rb index 46ecbaba73a..8676925a540 100644 --- a/app/finders/events_finder.rb +++ b/app/finders/events_finder.rb @@ -1,6 +1,10 @@ class EventsFinder + prepend FinderMethods + prepend FinderWithCrossProjectAccess attr_reader :source, :params, :current_user + requires_cross_project_access unless: -> { source.is_a?(Project) } + # Used to filter Events # # Arguments: diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 0fe3000ca01..9dd6634b38f 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -21,36 +21,46 @@ # my_reaction_emoji: string # class IssuableFinder + prepend FinderWithCrossProjectAccess + include FinderMethods include CreatedAtFilter - NONE = '0'.freeze + requires_cross_project_access unless: -> { project? } - SCALAR_PARAMS = %i[ - assignee_id - assignee_username - author_id - author_username - authorized_only - due_date - group_id - iids - label_name - milestone_title - my_reaction_emoji - non_archived - project_id - scope - search - sort - state - include_subgroups - ].freeze - ARRAY_PARAMS = { label_name: [], iids: [], assignee_username: [] }.freeze - - VALID_PARAMS = (SCALAR_PARAMS + [ARRAY_PARAMS]).freeze + NONE = '0'.freeze attr_accessor :current_user, :params + def self.scalar_params + @scalar_params ||= %i[ + assignee_id + assignee_username + author_id + author_username + authorized_only + group_id + iids + label_name + milestone_title + my_reaction_emoji + non_archived + project_id + scope + search + sort + state + include_subgroups + ] + end + + def self.array_params + @array_params ||= { label_name: [], iids: [], assignee_username: [] } + end + + def self.valid_params + @valid_params ||= scalar_params + [array_params] + end + def initialize(current_user, params = {}) @current_user = current_user @params = params @@ -58,6 +68,15 @@ class IssuableFinder def execute items = init_collection + items = filter_items(items) + + # Filtering by project HAS TO be the last because we use the project IDs yielded by the issuable query thus far + items = by_project(items) + + sort(items) + end + + def filter_items(items) items = by_scope(items) items = by_created_at(items) items = by_state(items) @@ -65,24 +84,11 @@ class IssuableFinder items = by_search(items) items = by_assignee(items) items = by_author(items) - items = by_due_date(items) items = by_non_archived(items) items = by_iids(items) items = by_milestone(items) items = by_label(items) - items = by_my_reaction_emoji(items) - - # Filtering by project HAS TO be the last because we use the project IDs yielded by the issuable query thus far - items = by_project(items) - sort(items) - end - - def find(*params) - execute.find(*params) - end - - def find_by(*params) - execute.find_by(*params) + by_my_reaction_emoji(items) end def row_count @@ -114,10 +120,6 @@ class IssuableFinder counts end - def find_by!(*params) - execute.find_by!(*params) - end - def group return @group if defined?(@group) @@ -396,42 +398,6 @@ class IssuableFinder items end - def by_due_date(items) - if due_date? - if filter_by_no_due_date? - items = items.without_due_date - elsif filter_by_overdue? - items = items.due_before(Date.today) - elsif filter_by_due_this_week? - items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week) - elsif filter_by_due_this_month? - items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month) - end - end - - items - end - - def filter_by_no_due_date? - due_date? && params[:due_date] == Issue::NoDueDate.name - end - - def filter_by_overdue? - due_date? && params[:due_date] == Issue::Overdue.name - end - - def filter_by_due_this_week? - due_date? && params[:due_date] == Issue::DueThisWeek.name - end - - def filter_by_due_this_month? - due_date? && params[:due_date] == Issue::DueThisMonth.name - end - - def due_date? - params[:due_date].present? && klass.column_names.include?('due_date') - end - def label_names if labels? params[:label_name].is_a?(String) ? params[:label_name].split(',') : params[:label_name] diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 98831f5be4a..d65c620e75a 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -16,12 +16,17 @@ # sort: string # my_reaction_emoji: string # public_only: boolean +# due_date: date or '0', '', 'overdue', 'week', or 'month' # class IssuesFinder < IssuableFinder CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER + def self.scalar_params + @scalar_params ||= super + [:due_date] + end + def klass - Issue + Issue.includes(:author) end def with_confidentiality_access_check @@ -52,6 +57,46 @@ class IssuesFinder < IssuableFinder params.fetch(:public_only, false) end + def filter_items(items) + by_due_date(super) + end + + def by_due_date(items) + if due_date? + if filter_by_no_due_date? + items = items.without_due_date + elsif filter_by_overdue? + items = items.due_before(Date.today) + elsif filter_by_due_this_week? + items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week) + elsif filter_by_due_this_month? + items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month) + end + end + + items + end + + def filter_by_no_due_date? + due_date? && params[:due_date] == Issue::NoDueDate.name + end + + def filter_by_overdue? + due_date? && params[:due_date] == Issue::Overdue.name + end + + def filter_by_due_this_week? + due_date? && params[:due_date] == Issue::DueThisWeek.name + end + + def filter_by_due_this_month? + due_date? && params[:due_date] == Issue::DueThisMonth.name + end + + def due_date? + params[:due_date].present? + end + def user_can_see_all_confidential_issues? return @user_can_see_all_confidential_issues if defined?(@user_can_see_all_confidential_issues) diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb index 1427cdaa382..780c0fdb03e 100644 --- a/app/finders/labels_finder.rb +++ b/app/finders/labels_finder.rb @@ -1,6 +1,10 @@ class LabelsFinder < UnionFinder + prepend FinderWithCrossProjectAccess + include FinderMethods include Gitlab::Utils::StrongMemoize + requires_cross_project_access unless: -> { project? } + def initialize(current_user, params = {}) @current_user = current_user @params = params @@ -35,7 +39,7 @@ class LabelsFinder < UnionFinder end end elsif only_group_labels? - label_ids << Label.where(group_id: group.id) + label_ids << Label.where(group_id: group_ids) else label_ids << Label.where(group_id: projects.group_ids) label_ids << Label.where(project_id: projects.select(:id)) @@ -55,13 +59,22 @@ class LabelsFinder < UnionFinder items.where(title: title) end - def group - strong_memoize(:group) do - group = Group.find(params[:group_id]) - authorized_to_read_labels?(group) && group + def group_ids + strong_memoize(:group_ids) do + groups_user_can_read_labels(groups_to_include).map(&:id) end end + def groups_to_include + group = Group.find(params[:group_id]) + groups = [group] + + groups += group.ancestors if params[:include_ancestor_groups].present? + groups += group.descendants if params[:include_descendant_groups].present? + + groups + end + def group? params[:group_id].present? end @@ -116,4 +129,10 @@ class LabelsFinder < UnionFinder Ability.allowed?(current_user, :read_label, label_parent) end + + def groups_user_can_read_labels(groups) + DeclarativePolicy.user_scope do + groups.select { |group| authorized_to_read_labels?(group) } + end + end end diff --git a/app/finders/members_finder.rb b/app/finders/members_finder.rb index af24045886e..4734d97b8c7 100644 --- a/app/finders/members_finder.rb +++ b/app/finders/members_finder.rb @@ -10,26 +10,59 @@ class MembersFinder def execute project_members = project.project_members project_members = project_members.non_invite unless can?(current_user, :admin_project, project) - wheres = ["members.id IN (#{project_members.select(:id).to_sql})"] if group - # We need `.where.not(user_id: nil)` here otherwise when a group has an - # invitee, it would make the following query return 0 rows since a NULL - # user_id would be present in the subquery - # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values - non_null_user_ids = project_members.where.not(user_id: nil).select(:user_id) - group_members = GroupMembersFinder.new(group).execute - group_members = group_members.where.not(user_id: non_null_user_ids) - group_members = group_members.non_invite unless can?(current_user, :admin_group, group) + group_members = group_members.non_invite - wheres << "members.id IN (#{group_members.select(:id).to_sql})" - end + union = Gitlab::SQL::Union.new([project_members, group_members], remove_duplicates: false) - Member.where(wheres.join(' OR ')) + sql = distinct_on(union) + + Member.includes(:user).from("(#{sql}) AS #{Member.table_name}") + else + project_members + end end def can?(*args) Ability.allowed?(*args) end + + private + + def distinct_on(union) + # We're interested in a list of members without duplicates by user_id. + # We prefer project members over group members, project members should go first. + if Gitlab::Database.postgresql? + <<~SQL + SELECT DISTINCT ON (user_id, invite_email) member_union.* + FROM (#{union.to_sql}) AS member_union + ORDER BY user_id, + invite_email, + CASE + WHEN type = 'ProjectMember' THEN 1 + WHEN type = 'GroupMember' THEN 2 + ELSE 3 + END + SQL + else + # Older versions of MySQL do not support window functions (and DISTINCT ON is postgres-specific). + <<~SQL + SELECT t1.* + FROM (#{union.to_sql}) AS t1 + JOIN ( + SELECT + COALESCE(user_id, -1) AS user_id, + COALESCE(invite_email, 'NULL') AS invite_email, + MIN(CASE WHEN type = 'ProjectMember' THEN 1 WHEN type = 'GroupMember' THEN 2 ELSE 3 END) AS type_number + FROM + (#{union.to_sql}) AS t3 + GROUP BY COALESCE(user_id, -1), COALESCE(invite_email, 'NULL') + ) AS t2 ON COALESCE(t1.user_id, -1) = t2.user_id + AND COALESCE(t1.invite_email, 'NULL') = t2.invite_email + AND CASE WHEN t1.type = 'ProjectMember' THEN 1 WHEN t1.type = 'GroupMember' THEN 2 ELSE 3 END = t2.type_number + SQL + end + end end diff --git a/app/finders/merge_request_target_project_finder.rb b/app/finders/merge_request_target_project_finder.rb index 189eb3847eb..f358938344e 100644 --- a/app/finders/merge_request_target_project_finder.rb +++ b/app/finders/merge_request_target_project_finder.rb @@ -1,4 +1,6 @@ class MergeRequestTargetProjectFinder + include FinderMethods + attr_reader :current_user, :source_project def initialize(current_user: nil, source_project:) diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index d0687d28c21..068ae7f8c89 100644 --- a/app/finders/merge_requests_finder.rb +++ b/app/finders/merge_requests_finder.rb @@ -17,14 +17,42 @@ # sort: string # non_archived: boolean # my_reaction_emoji: string +# source_branch: string +# target_branch: string # class MergeRequestsFinder < IssuableFinder def klass MergeRequest end + def filter_items(_items) + items = by_source_branch(super) + + by_target_branch(items) + end + private + def source_branch + @source_branch ||= params[:source_branch].presence + end + + def by_source_branch(items) + return items unless source_branch + + items.where(source_branch: source_branch) + end + + def target_branch + @target_branch ||= params[:target_branch].presence + end + + def by_target_branch(items) + return items unless target_branch + + items.where(target_branch: target_branch) + end + def item_project_ids(items) items&.reorder(nil)&.select(:target_project_id) end diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb index b4605fca193..f5d2b9f253a 100644 --- a/app/finders/milestones_finder.rb +++ b/app/finders/milestones_finder.rb @@ -8,6 +8,8 @@ # state - filters by state. class MilestonesFinder + include FinderMethods + attr_reader :params, :project_ids, :group_ids def initialize(params = {}) diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 12157818bcd..33ee1e975b9 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -57,7 +57,7 @@ class NotesFinder types = %w(commit issue merge_request snippet) note_relations = types.map { |t| notes_for_type(t) } note_relations.map! { |notes| search(notes) } - UnionFinder.new.find_union(note_relations, Note) + UnionFinder.new.find_union(note_relations, Note.includes(:author)) end def noteables_for_type(noteable_type) diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index 4450766485f..a73c573736e 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -1,14 +1,30 @@ +# Snippets Finder +# +# Used to filter Snippets collections by a set of params +# +# Arguments. +# +# current_user - The current user, nil also can be used. +# params: +# visibility (integer) - Individual snippet visibility: Public(20), internal(10) or private(0). +# project (Project) - Project related. +# author (User) - Author related. +# +# params are optional class SnippetsFinder < UnionFinder - attr_accessor :current_user, :params + include Gitlab::Allowable + include FinderMethods + + attr_accessor :current_user, :project, :params def initialize(current_user, params = {}) @current_user = current_user @params = params + @project = params[:project] end def execute items = init_collection - items = by_project(items) items = by_author(items) items = by_visibility(items) @@ -18,25 +34,48 @@ class SnippetsFinder < UnionFinder private def init_collection - items = Snippet.all + if project.present? + authorized_snippets_from_project + else + authorized_snippets + end + end + + def authorized_snippets_from_project + if can?(current_user, :read_project_snippet, project) + if project.team.member?(current_user) + project.snippets + else + project.snippets.public_to_user(current_user) + end + else + Snippet.none + end + end - accessible(items) + def authorized_snippets + Snippet.where(feature_available_projects.or(not_project_related)) + .public_or_visible_to_user(current_user) end - def accessible(items) - segments = [] - segments << items.public_to_user(current_user) - segments << authorized_to_user(items) if current_user + def feature_available_projects + # Don't return any project related snippets if the user cannot read cross project + return table[:id].eq(nil) unless Ability.allowed?(current_user, :read_cross_project) + + projects = Project.public_or_visible_to_user(current_user, use_where_in: false) do |part| + part.with_feature_available_for_user(:snippets, current_user) + end.select(:id) + + arel_query = Arel::Nodes::SqlLiteral.new(projects.to_sql) + table[:project_id].in(arel_query) + end - find_union(segments, Snippet.includes(:author)) + def not_project_related + table[:project_id].eq(nil) end - def authorized_to_user(items) - items.where( - 'author_id = :author_id - OR project_id IN (:project_ids)', - author_id: current_user.id, - project_ids: current_user.authorized_projects.select(:id)) + def table + Snippet.arel_table end def by_visibility(items) @@ -53,12 +92,6 @@ class SnippetsFinder < UnionFinder items.where(author_id: params[:author].id) end - def by_project(items) - return items unless params[:project] - - items.where(project_id: params[:project].id) - end - def visibility_from_scope case params[:scope].to_s when 'are_private' diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 3502bf08971..edb17843002 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -13,6 +13,11 @@ # class TodosFinder + prepend FinderWithCrossProjectAccess + include FinderMethods + + requires_cross_project_access unless: -> { project? } + NONE = '0'.freeze attr_accessor :current_user, :params diff --git a/app/finders/user_recent_events_finder.rb b/app/finders/user_recent_events_finder.rb new file mode 100644 index 00000000000..6f7f7c30d92 --- /dev/null +++ b/app/finders/user_recent_events_finder.rb @@ -0,0 +1,33 @@ +# Get user activity feed for projects common for a user and a logged in user +# +# - current_user: The user viewing the events +# - user: The user for which to load the events +# - params: +# - offset: The page of events to return +class UserRecentEventsFinder + prepend FinderWithCrossProjectAccess + include FinderMethods + + requires_cross_project_access + + attr_reader :current_user, :target_user, :params + + def initialize(current_user, target_user, params = {}) + @current_user = current_user + @target_user = target_user + @params = params + end + + def execute + target_user + .recent_events + .merge(projects_for_current_user) + .references(:project) + .with_associations + .limit_recent(20, params[:offset]) + end + + def projects_for_current_user + ProjectsFinder.new(current_user: current_user).execute + end +end |