summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/assets/javascripts/behaviors/markdown/render_mermaid.js1
-rw-r--r--app/controllers/dashboard/todos_controller.rb2
-rw-r--r--app/graphql/resolvers/todo_resolver.rb2
-rw-r--r--app/graphql/types/alert_management/alert_type.rb8
-rw-r--r--app/graphql/types/user_interface.rb7
-rw-r--r--app/models/alert_management/alert.rb4
-rw-r--r--app/models/application_record.rb8
-rw-r--r--app/models/commit.rb4
-rw-r--r--app/models/design_management/design.rb4
-rw-r--r--app/models/group.rb4
-rw-r--r--app/models/issue.rb38
-rw-r--r--app/models/note.rb10
-rw-r--r--app/models/project.rb4
-rw-r--r--app/policies/todo_policy.rb7
-rw-r--r--app/services/todos/allowed_target_filter_service.rb18
-rw-r--r--app/views/dashboard/todos/index.html.haml8
-rw-r--r--lib/api/todos.rb1
-rw-r--r--lib/gitlab/graphql/todos_project_permission_preloader/field_extension.rb26
-rw-r--r--spec/features/dashboard/todos/target_state_spec.rb20
-rw-r--r--spec/features/dashboard/todos/todos_filtering_spec.rb2
-rw-r--r--spec/features/dashboard/todos/todos_spec.rb38
-rw-r--r--spec/features/markdown/mermaid_spec.rb23
-rw-r--r--spec/graphql/mutations/todos/mark_done_spec.rb10
-rw-r--r--spec/graphql/mutations/todos/restore_spec.rb10
-rw-r--r--spec/graphql/resolvers/todo_resolver_spec.rb39
-rw-r--r--spec/models/diff_note_spec.rb6
-rw-r--r--spec/models/discussion_note_spec.rb11
-rw-r--r--spec/models/legacy_diff_note_spec.rb11
-rw-r--r--spec/models/synthetic_note_spec.rb11
-rw-r--r--spec/policies/todo_policy_spec.rb24
-rw-r--r--spec/requests/api/graphql/current_user/todos_query_spec.rb45
-rw-r--r--spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb12
-rw-r--r--spec/requests/api/graphql/mutations/todos/mark_done_spec.rb10
-rw-r--r--spec/requests/api/graphql/mutations/todos/restore_many_spec.rb10
-rw-r--r--spec/requests/api/graphql/mutations/todos/restore_spec.rb10
-rw-r--r--spec/requests/api/todos_spec.rb71
-rw-r--r--spec/services/todos/allowed_target_filter_service_spec.rb59
37 files changed, 452 insertions, 126 deletions
diff --git a/app/assets/javascripts/behaviors/markdown/render_mermaid.js b/app/assets/javascripts/behaviors/markdown/render_mermaid.js
index 293fe9f4133..3f878949f9b 100644
--- a/app/assets/javascripts/behaviors/markdown/render_mermaid.js
+++ b/app/assets/javascripts/behaviors/markdown/render_mermaid.js
@@ -66,6 +66,7 @@ export function initMermaid(mermaid) {
useMaxWidth: true,
htmlLabels: true,
},
+ secure: ['secure', 'securityLevel', 'startOnLoad', 'maxTextSize', 'htmlLabels'],
securityLevel: 'strict',
});
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 25ac0af9731..21bbb4d0c98 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -16,6 +16,8 @@ class Dashboard::TodosController < Dashboard::ApplicationController
@todos = @todos.with_entity_associations
return if redirect_out_of_range(@todos, todos_page_count(@todos))
+
+ @allowed_todos = ::Todos::AllowedTargetFilterService.new(@todos, current_user).execute
end
def destroy
diff --git a/app/graphql/resolvers/todo_resolver.rb b/app/graphql/resolvers/todo_resolver.rb
index 8966285fccc..af48ceefd6f 100644
--- a/app/graphql/resolvers/todo_resolver.rb
+++ b/app/graphql/resolvers/todo_resolver.rb
@@ -34,7 +34,7 @@ module Resolvers
return Todo.none unless current_user.present? && target.present?
return Todo.none if target.is_a?(User) && target != current_user
- TodosFinder.new(current_user, todo_finder_params(args)).execute
+ TodosFinder.new(current_user, todo_finder_params(args)).execute.with_entity_associations
end
private
diff --git a/app/graphql/types/alert_management/alert_type.rb b/app/graphql/types/alert_management/alert_type.rb
index 0ff0775ca86..eb0d999554f 100644
--- a/app/graphql/types/alert_management/alert_type.rb
+++ b/app/graphql/types/alert_management/alert_type.rb
@@ -115,11 +115,9 @@ module Types
null: true,
description: 'Runbook for the alert as defined in alert details.'
- field :todos,
- Types::TodoType.connection_type,
- null: true,
- description: 'To-do items of the current user for the alert.',
- resolver: Resolvers::TodoResolver
+ field :todos, description: 'To-do items of the current user for the alert.', resolver: Resolvers::TodoResolver do
+ extension(::Gitlab::Graphql::TodosProjectPermissionPreloader::FieldExtension)
+ end
field :details_url,
GraphQL::STRING_TYPE,
diff --git a/app/graphql/types/user_interface.rb b/app/graphql/types/user_interface.rb
index e5abc033155..7d61b296eae 100644
--- a/app/graphql/types/user_interface.rb
+++ b/app/graphql/types/user_interface.rb
@@ -55,9 +55,6 @@ module Types
type: GraphQL::STRING_TYPE,
null: false,
description: 'Web path of the user.'
- field :todos,
- resolver: Resolvers::TodoResolver,
- description: 'To-do items of the user.'
field :group_memberships,
type: Types::GroupMemberType.connection_type,
null: true,
@@ -81,6 +78,10 @@ module Types
description: 'Projects starred by the user.',
resolver: Resolvers::UserStarredProjectsResolver
+ field :todos, resolver: Resolvers::TodoResolver, description: 'To-do items of the user.' do
+ extension(::Gitlab::Graphql::TodosProjectPermissionPreloader::FieldExtension)
+ end
+
# Merge request field: MRs can be authored, assigned, or assigned-for-review:
field :authored_merge_requests,
resolver: Resolvers::AuthoredMergeRequestsResolver,
diff --git a/app/models/alert_management/alert.rb b/app/models/alert_management/alert.rb
index 679406e68d7..d0e4163dcdb 100644
--- a/app/models/alert_management/alert.rb
+++ b/app/models/alert_management/alert.rb
@@ -266,6 +266,10 @@ module AlertManagement
end
end
+ def to_ability_name
+ 'alert_management_alert'
+ end
+
private
def hook_data
diff --git a/app/models/application_record.rb b/app/models/application_record.rb
index a93348a3b27..527b67712ee 100644
--- a/app/models/application_record.rb
+++ b/app/models/application_record.rb
@@ -86,4 +86,12 @@ class ApplicationRecord < ActiveRecord::Base
values = enum_mod.definition.transform_values { |v| v[:value] }
enum(enum_mod.key => values)
end
+
+ def readable_by?(user)
+ Ability.allowed?(user, "read_#{to_ability_name}".to_sym, self)
+ end
+
+ def to_ability_name
+ model_name.element
+ end
end
diff --git a/app/models/commit.rb b/app/models/commit.rb
index a1ed5eb9ab9..8e7f526c512 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -550,6 +550,10 @@ class Commit
expire_note_etag_cache_for_related_mrs
end
+ def readable_by?(user)
+ Ability.allowed?(user, :read_commit, self)
+ end
+
private
def expire_note_etag_cache_for_related_mrs
diff --git a/app/models/design_management/design.rb b/app/models/design_management/design.rb
index e2d10cc7e78..79f5a63bcb6 100644
--- a/app/models/design_management/design.rb
+++ b/app/models/design_management/design.rb
@@ -182,10 +182,6 @@ module DesignManagement
File.join(DesignManagement.designs_directory, "issue-#{issue.iid}", design.filename)
end
- def to_ability_name
- 'design'
- end
-
def description
''
end
diff --git a/app/models/group.rb b/app/models/group.rb
index eefb8d3d16a..1e7308499a0 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -713,10 +713,6 @@ class Group < Namespace
Gitlab::ServiceDesk.supported? && all_projects.service_desk_enabled.exists?
end
- def to_ability_name
- model_name.singular
- end
-
def activity_path
Gitlab::Routing.url_helpers.activity_group_path(self)
end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 00fcba5298a..d91d72e1fba 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -537,6 +537,25 @@ class Issue < ApplicationRecord
self.update_column(:upvotes_count, self.upvotes)
end
+ # Returns `true` if the given User can read the current Issue.
+ #
+ # This method duplicates the same check of issue_policy.rb
+ # for performance reasons, check commit: 002ad215818450d2cbbc5fa065850a953dc7ada8
+ # Make sure to sync this method with issue_policy.rb
+ def readable_by?(user)
+ if user.can_read_all_resources?
+ true
+ elsif project.owner == user
+ true
+ elsif confidential? && !assignee_or_author?(user)
+ project.team.member?(user, Gitlab::Access::REPORTER)
+ else
+ project.public? ||
+ project.internal? && !user.external? ||
+ project.team.member?(user)
+ end
+ end
+
private
def spammable_attribute_changed?
@@ -562,25 +581,6 @@ class Issue < ApplicationRecord
Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_created_action(author: author)
end
- # Returns `true` if the given User can read the current Issue.
- #
- # This method duplicates the same check of issue_policy.rb
- # for performance reasons, check commit: 002ad215818450d2cbbc5fa065850a953dc7ada8
- # Make sure to sync this method with issue_policy.rb
- def readable_by?(user)
- if user.can_read_all_resources?
- true
- elsif project.owner == user
- true
- elsif confidential? && !assignee_or_author?(user)
- project.team.member?(user, Gitlab::Access::REPORTER)
- else
- project.public? ||
- project.internal? && !user.external? ||
- project.team.member?(user)
- end
- end
-
# Returns `true` if this Issue is visible to everybody.
def publicly_visible?
project.public? && !confidential? && !::Gitlab::ExternalAuthorization.enabled?
diff --git a/app/models/note.rb b/app/models/note.rb
index ed341e58436..2ad6df85e5f 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -384,12 +384,6 @@ class Note < ApplicationRecord
super
end
- # This method is to be used for checking read permissions on a note instead of `system_note_with_references_visible_for?`
- def readable_by?(user)
- # note_policy accounts for #system_note_with_references_visible_for?(user) check when granting read access
- Ability.allowed?(user, :read_note, self)
- end
-
def award_emoji?
can_be_award_emoji? && contains_emoji_only?
end
@@ -406,10 +400,6 @@ class Note < ApplicationRecord
note =~ /\A#{Banzai::Filter::EmojiFilter.emoji_pattern}\s?\Z/
end
- def to_ability_name
- model_name.singular
- end
-
def noteable_ability_name
if for_snippet?
'snippet'
diff --git a/app/models/project.rb b/app/models/project.rb
index 9e6e29aadda..c5522737b87 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1485,10 +1485,6 @@ class Project < ApplicationRecord
end
end
- def to_ability_name
- model_name.singular
- end
-
# rubocop: disable CodeReuse/ServiceClass
def execute_hooks(data, hooks_scope = :push_hooks)
run_after_commit_or_now do
diff --git a/app/policies/todo_policy.rb b/app/policies/todo_policy.rb
index d01a046c343..6237fbc50fa 100644
--- a/app/policies/todo_policy.rb
+++ b/app/policies/todo_policy.rb
@@ -5,7 +5,10 @@ class TodoPolicy < BasePolicy
condition(:own_todo) do
@user && @subject.user_id == @user.id
end
+ condition(:can_read_target) do
+ @user && @subject.target&.readable_by?(@user)
+ end
- rule { own_todo }.enable :read_todo
- rule { own_todo }.enable :update_todo
+ rule { own_todo & can_read_target }.enable :read_todo
+ rule { own_todo & can_read_target }.enable :update_todo
end
diff --git a/app/services/todos/allowed_target_filter_service.rb b/app/services/todos/allowed_target_filter_service.rb
new file mode 100644
index 00000000000..dfed616710b
--- /dev/null
+++ b/app/services/todos/allowed_target_filter_service.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module Todos
+ class AllowedTargetFilterService
+ include Gitlab::Allowable
+
+ def initialize(todos, current_user)
+ @todos = todos
+ @current_user = current_user
+ end
+
+ def execute
+ Preloaders::UserMaxAccessLevelInProjectsPreloader.new(@todos.map(&:project).compact, @current_user).execute
+
+ @todos.select { |todo| can?(@current_user, :read_todo, todo) }
+ end
+ end
+end
diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml
index ca10861115b..58f817bf63b 100644
--- a/app/views/dashboard/todos/index.html.haml
+++ b/app/views/dashboard/todos/index.html.haml
@@ -25,7 +25,7 @@
= number_with_delimiter(todos_done_count)
.nav-controls
- - if @todos.any?(&:pending?)
+ - if @allowed_todos.any?(&:pending?)
.gl-mr-3
= link_to destroy_all_dashboard_todos_path(todos_filter_params), class: 'gl-button btn btn-default btn-loading align-items-center js-todos-mark-all', method: :delete, data: { href: destroy_all_dashboard_todos_path(todos_filter_params) } do
Mark all as done
@@ -82,11 +82,11 @@
= sort_title_oldest_created
.row.js-todos-all
- - if @todos.any?
+ - if @allowed_todos.any?
.col.js-todos-list-container{ data: { qa_selector: "todos_list_container" } }
- .js-todos-options{ data: { per_page: @todos.limit_value, current_page: @todos.current_page, total_pages: @todos.total_pages } }
+ .js-todos-options{ data: { per_page: @allowed_todos.count, current_page: @todos.current_page, total_pages: @todos.total_pages } }
%ul.content-list.todos-list
- = render @todos
+ = render @allowed_todos
= paginate @todos, theme: "gitlab"
.js-nothing-here-container.empty-state.hidden
.svg-content
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index a001313a11f..e0e5ca615ac 100644
--- a/lib/api/todos.rb
+++ b/lib/api/todos.rb
@@ -92,6 +92,7 @@ module API
end
get do
todos = paginate(find_todos.with_entity_associations)
+ todos = ::Todos::AllowedTargetFilterService.new(todos, current_user).execute
options = { with: Entities::Todo, current_user: current_user }
batch_load_issuable_metadata(todos, options)
diff --git a/lib/gitlab/graphql/todos_project_permission_preloader/field_extension.rb b/lib/gitlab/graphql/todos_project_permission_preloader/field_extension.rb
new file mode 100644
index 00000000000..77f3b1ac71a
--- /dev/null
+++ b/lib/gitlab/graphql/todos_project_permission_preloader/field_extension.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Graphql
+ module TodosProjectPermissionPreloader
+ class FieldExtension < ::GraphQL::Schema::FieldExtension
+ def after_resolve(value:, memo:, **rest)
+ todos = value.to_a
+
+ Preloaders::UserMaxAccessLevelInProjectsPreloader.new(
+ todos.map(&:project).compact,
+ current_user(rest)
+ ).execute
+
+ value
+ end
+
+ private
+
+ def current_user(options)
+ options.dig(:context, :current_user)
+ end
+ end
+ end
+ end
+end
diff --git a/spec/features/dashboard/todos/target_state_spec.rb b/spec/features/dashboard/todos/target_state_spec.rb
index 4c43948201c..b0aafdda59a 100644
--- a/spec/features/dashboard/todos/target_state_spec.rb
+++ b/spec/features/dashboard/todos/target_state_spec.rb
@@ -3,16 +3,20 @@
require 'spec_helper'
RSpec.describe 'Dashboard > Todo target states' do
- let(:user) { create(:user) }
- let(:author) { create(:user) }
- let(:project) { create(:project, :public) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:author) { create(:user) }
+ let_it_be(:project) { create(:project, :public) }
+
+ before_all do
+ project.add_developer(user)
+ end
before do
sign_in(user)
end
it 'on a closed issue todo has closed label' do
- issue_closed = create(:issue, state: 'closed')
+ issue_closed = create(:issue, state: 'closed', project: project)
create_todo issue_closed
visit dashboard_todos_path
@@ -22,7 +26,7 @@ RSpec.describe 'Dashboard > Todo target states' do
end
it 'on an open issue todo does not have an open label' do
- issue_open = create(:issue)
+ issue_open = create(:issue, project: project)
create_todo issue_open
visit dashboard_todos_path
@@ -32,7 +36,7 @@ RSpec.describe 'Dashboard > Todo target states' do
end
it 'on a merged merge request todo has merged label' do
- mr_merged = create(:merge_request, :simple, :merged, author: user)
+ mr_merged = create(:merge_request, :simple, :merged, author: user, source_project: project)
create_todo mr_merged
visit dashboard_todos_path
@@ -42,7 +46,7 @@ RSpec.describe 'Dashboard > Todo target states' do
end
it 'on a closed merge request todo has closed label' do
- mr_closed = create(:merge_request, :simple, :closed, author: user)
+ mr_closed = create(:merge_request, :simple, :closed, author: user, source_project: project)
create_todo mr_closed
visit dashboard_todos_path
@@ -52,7 +56,7 @@ RSpec.describe 'Dashboard > Todo target states' do
end
it 'on an open merge request todo does not have an open label' do
- mr_open = create(:merge_request, :simple, author: user)
+ mr_open = create(:merge_request, :simple, author: user, source_project: project)
create_todo mr_open
visit dashboard_todos_path
diff --git a/spec/features/dashboard/todos/todos_filtering_spec.rb b/spec/features/dashboard/todos/todos_filtering_spec.rb
index b1464af4194..53209db3107 100644
--- a/spec/features/dashboard/todos/todos_filtering_spec.rb
+++ b/spec/features/dashboard/todos/todos_filtering_spec.rb
@@ -128,7 +128,7 @@ RSpec.describe 'Dashboard > User filters todos', :js do
describe 'filter by action' do
before do
- create(:todo, :build_failed, user: user_1, author: user_2, project: project_1)
+ create(:todo, :build_failed, user: user_1, author: user_2, project: project_1, target: merge_request)
create(:todo, :marked, user: user_1, author: user_2, project: project_1, target: issue1)
create(:todo, :review_requested, user: user_1, author: user_2, project: project_1, target: issue1)
end
diff --git a/spec/features/dashboard/todos/todos_spec.rb b/spec/features/dashboard/todos/todos_spec.rb
index 0bc6cc9c017..7345bfa19e2 100644
--- a/spec/features/dashboard/todos/todos_spec.rb
+++ b/spec/features/dashboard/todos/todos_spec.rb
@@ -3,10 +3,16 @@
require 'spec_helper'
RSpec.describe 'Dashboard Todos' do
+ include DesignManagementTestHelpers
+
let_it_be(:user) { create(:user, username: 'john') }
let_it_be(:author) { create(:user) }
let_it_be(:project) { create(:project, :public) }
- let_it_be(:issue) { create(:issue, due_date: Date.today, title: "Fix bug") }
+ let_it_be(:issue) { create(:issue, project: project, due_date: Date.today, title: "Fix bug") }
+
+ before_all do
+ project.add_developer(user)
+ end
context 'User does not have todos' do
before do
@@ -21,8 +27,8 @@ RSpec.describe 'Dashboard Todos' do
context 'when the todo references a merge request' do
let(:referenced_mr) { create(:merge_request, source_project: project) }
- let(:note) { create(:note, project: project, note: "Check out #{referenced_mr.to_reference}") }
- let!(:todo) { create(:todo, :mentioned, user: user, project: project, author: author, note: note) }
+ let(:note) { create(:note, project: project, note: "Check out #{referenced_mr.to_reference}", noteable: create(:issue, project: project)) }
+ let!(:todo) { create(:todo, :mentioned, user: user, project: project, author: author, note: note, target: note.noteable) }
before do
sign_in(user)
@@ -39,9 +45,26 @@ RSpec.describe 'Dashboard Todos' do
end
end
- context 'User has a todo', :js do
+ context 'user has an unauthorized todo' do
before do
+ sign_in(user)
+ end
+
+ it 'does not render the todo' do
+ unauthorized_issue = create(:issue)
+ create(:todo, :mentioned, user: user, project: unauthorized_issue.project, target: unauthorized_issue, author: author)
create(:todo, :mentioned, user: user, project: project, target: issue, author: author)
+
+ visit dashboard_todos_path
+
+ expect(page).to have_selector('.todos-list .todo', count: 1)
+ end
+ end
+
+ context 'User has a todo', :js do
+ let_it_be(:user_todo) { create(:todo, :mentioned, user: user, project: project, target: issue, author: author) }
+
+ before do
sign_in(user)
visit dashboard_todos_path
@@ -183,7 +206,7 @@ RSpec.describe 'Dashboard Todos' do
end
context 'approval todo' do
- let(:merge_request) { create(:merge_request, title: "Fixes issue") }
+ let(:merge_request) { create(:merge_request, title: "Fixes issue", source_project: project) }
before do
create(:todo, :approval_required, user: user, project: project, target: merge_request, author: user)
@@ -199,7 +222,7 @@ RSpec.describe 'Dashboard Todos' do
end
context 'review request todo' do
- let(:merge_request) { create(:merge_request, title: "Fixes issue") }
+ let(:merge_request) { create(:merge_request, title: "Fixes issue", source_project: project) }
before do
create(:todo, :review_requested, user: user, project: project, target: merge_request, author: user)
@@ -355,7 +378,7 @@ RSpec.describe 'Dashboard Todos' do
end
context 'User has a Build Failed todo' do
- let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: author) }
+ let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: author, target: create(:merge_request, source_project: project)) }
before do
sign_in(user)
@@ -386,6 +409,7 @@ RSpec.describe 'Dashboard Todos' do
end
before do
+ enable_design_management
project.add_developer(user)
sign_in(user)
diff --git a/spec/features/markdown/mermaid_spec.rb b/spec/features/markdown/mermaid_spec.rb
index c4994838d26..e080c7ffb3f 100644
--- a/spec/features/markdown/mermaid_spec.rb
+++ b/spec/features/markdown/mermaid_spec.rb
@@ -260,8 +260,6 @@ RSpec.describe 'Mermaid rendering', :js do
description *= 51
- project = create(:project, :public)
-
wiki_page = build(:wiki_page, { container: project, content: description })
wiki_page.create message: 'mermaid test commit' # rubocop:disable Rails/SaveBang
wiki_page = project.wiki.find_page(wiki_page.slug)
@@ -277,6 +275,27 @@ RSpec.describe 'Mermaid rendering', :js do
expect(page).not_to have_selector('.js-lazy-render-mermaid-container')
end
end
+
+ it 'does not allow HTML injection' do
+ description = <<~MERMAID
+ ```mermaid
+ %%{init: {"flowchart": {"htmlLabels": "false"}} }%%
+ flowchart
+ A["<iframe></iframe>"]
+ ```
+ MERMAID
+
+ issue = create(:issue, project: project, description: description)
+
+ visit project_issue_path(project, issue)
+
+ wait_for_requests
+ wait_for_mermaid
+
+ page.within('.description') do
+ expect(page).not_to have_xpath("//iframe")
+ end
+ end
end
def wait_for_mermaid
diff --git a/spec/graphql/mutations/todos/mark_done_spec.rb b/spec/graphql/mutations/todos/mark_done_spec.rb
index b5f2ff5d044..9723ac8af42 100644
--- a/spec/graphql/mutations/todos/mark_done_spec.rb
+++ b/spec/graphql/mutations/todos/mark_done_spec.rb
@@ -5,17 +5,23 @@ require 'spec_helper'
RSpec.describe Mutations::Todos::MarkDone do
include GraphqlHelpers
+ let_it_be(:project) { create(:project) }
+ let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:other_user) { create(:user) }
- let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) }
- let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
+ let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
+ let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) }
let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
+ before_all do
+ project.add_developer(current_user)
+ end
+
specify { expect(described_class).to require_graphql_authorizations(:update_todo) }
describe '#resolve' do
diff --git a/spec/graphql/mutations/todos/restore_spec.rb b/spec/graphql/mutations/todos/restore_spec.rb
index 22fb1bba7a8..954bb3db668 100644
--- a/spec/graphql/mutations/todos/restore_spec.rb
+++ b/spec/graphql/mutations/todos/restore_spec.rb
@@ -5,17 +5,23 @@ require 'spec_helper'
RSpec.describe Mutations::Todos::Restore do
include GraphqlHelpers
+ let_it_be(:project) { create(:project) }
+ let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:other_user) { create(:user) }
- let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) }
- let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending) }
+ let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+ let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) }
let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
+ before_all do
+ project.add_developer(current_user)
+ end
+
specify { expect(described_class).to require_graphql_authorizations(:update_todo) }
describe '#resolve' do
diff --git a/spec/graphql/resolvers/todo_resolver_spec.rb b/spec/graphql/resolvers/todo_resolver_spec.rb
index ac14852b365..0760935a2fe 100644
--- a/spec/graphql/resolvers/todo_resolver_spec.rb
+++ b/spec/graphql/resolvers/todo_resolver_spec.rb
@@ -4,19 +4,28 @@ require 'spec_helper'
RSpec.describe Resolvers::TodoResolver do
include GraphqlHelpers
+ include DesignManagementTestHelpers
specify do
expect(described_class).to have_nullable_graphql_type(Types::TodoType.connection_type)
end
describe '#resolve' do
+ let_it_be(:project) { create(:project) }
let_it_be(:current_user) { create(:user) }
+ let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:author1) { create(:user) }
let_it_be(:author2) { create(:user) }
- let_it_be(:merge_request_todo_pending) { create(:todo, user: current_user, target_type: 'MergeRequest', state: :pending, action: Todo::MENTIONED, author: author1) }
- let_it_be(:issue_todo_done) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2) }
- let_it_be(:issue_todo_pending) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) }
+ let_it_be(:issue_todo_done) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2, target: issue) }
+ let_it_be(:issue_todo_pending) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue) }
+
+ let(:merge_request) { create(:merge_request, source_project: project) }
+ let!(:merge_request_todo_pending) { create(:todo, user: current_user, target: merge_request, state: :pending, action: Todo::MENTIONED, author: author1) }
+
+ before_all do
+ project.add_developer(current_user)
+ end
it 'calls TodosFinder' do
expect_next_instance_of(TodosFinder) do |finder|
@@ -40,7 +49,9 @@ RSpec.describe Resolvers::TodoResolver do
end
it 'returns the todos for multiple filters' do
- design_todo_pending = create(:todo, target_type: 'DesignManagement::Design', user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
+ enable_design_management
+ design = create(:design, issue: issue)
+ design_todo_pending = create(:todo, target: design, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
todos = resolve_todos(type: ['MergeRequest', 'DesignManagement::Design'])
@@ -59,11 +70,15 @@ RSpec.describe Resolvers::TodoResolver do
group3 = create(:group)
group1.add_developer(current_user)
+ issue1 = create(:issue, project: create(:project, group: group1))
group2.add_developer(current_user)
+ issue2 = create(:issue, project: create(:project, group: group2))
+ group3.add_developer(current_user)
+ issue3 = create(:issue, project: create(:project, group: group3))
- todo4 = create(:todo, group: group1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
- todo5 = create(:todo, group: group2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
- create(:todo, group: group3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
+ todo4 = create(:todo, group: group1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue1)
+ todo5 = create(:todo, group: group2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue2)
+ create(:todo, group: group3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue3)
todos = resolve_todos(group_id: [group2.id, group1.id])
@@ -93,9 +108,13 @@ RSpec.describe Resolvers::TodoResolver do
project2 = create(:project)
project3 = create(:project)
- todo4 = create(:todo, project: project1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
- todo5 = create(:todo, project: project2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
- create(:todo, project: project3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
+ project1.add_developer(current_user)
+ project2.add_developer(current_user)
+ project3.add_developer(current_user)
+
+ todo4 = create(:todo, project: project1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project1))
+ todo5 = create(:todo, project: project2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project2))
+ create(:todo, project: project3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project3))
todos = resolve_todos(project_id: [project2.id, project1.id])
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 2731eadecc0..11652d9841b 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -552,4 +552,10 @@ RSpec.describe DiffNote do
expect(subject.on_image?).to be_truthy
end
end
+
+ describe '#to_ability_name' do
+ subject { described_class.new.to_ability_name }
+
+ it { is_expected.to eq('note') }
+ end
end
diff --git a/spec/models/discussion_note_spec.rb b/spec/models/discussion_note_spec.rb
new file mode 100644
index 00000000000..6e1b39cc438
--- /dev/null
+++ b/spec/models/discussion_note_spec.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DiscussionNote do
+ describe '#to_ability_name' do
+ subject { described_class.new.to_ability_name }
+
+ it { is_expected.to eq('note') }
+ end
+end
diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb
new file mode 100644
index 00000000000..ee3bbf186b9
--- /dev/null
+++ b/spec/models/legacy_diff_note_spec.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe LegacyDiffNote do
+ describe '#to_ability_name' do
+ subject { described_class.new.to_ability_name }
+
+ it { is_expected.to eq('note') }
+ end
+end
diff --git a/spec/models/synthetic_note_spec.rb b/spec/models/synthetic_note_spec.rb
new file mode 100644
index 00000000000..55fa4f7c33d
--- /dev/null
+++ b/spec/models/synthetic_note_spec.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SyntheticNote do
+ describe '#to_ability_name' do
+ subject { described_class.new.to_ability_name }
+
+ it { is_expected.to eq('note') }
+ end
+end
diff --git a/spec/policies/todo_policy_spec.rb b/spec/policies/todo_policy_spec.rb
index b4876baa504..16435b21666 100644
--- a/spec/policies/todo_policy_spec.rb
+++ b/spec/policies/todo_policy_spec.rb
@@ -9,22 +9,28 @@ RSpec.describe TodoPolicy do
let_it_be(:user2) { create(:user) }
let_it_be(:user3) { create(:user) }
- let_it_be(:todo1) { create(:todo, author: author, user: user1) }
- let_it_be(:todo2) { create(:todo, author: author, user: user2) }
+ let_it_be(:project) { create(:project) }
+ let_it_be(:issue) { create(:issue, project: project) }
+
+ let_it_be(:todo1) { create(:todo, author: author, user: user1, issue: issue) }
+ let_it_be(:todo2) { create(:todo, author: author, user: user2, issue: issue) }
let_it_be(:todo3) { create(:todo, author: author, user: user2) }
- let_it_be(:todo4) { create(:todo, author: author, user: user3) }
+ let_it_be(:todo4) { create(:todo, author: author, user: user3, issue: issue) }
def permissions(user, todo)
described_class.new(user, todo)
end
+ before_all do
+ project.add_developer(user1)
+ project.add_developer(user2)
+ end
+
describe 'own_todo' do
- it 'allows owners to access their own todos' do
+ it 'allows owners to access their own todos if they can read todo target' do
[
[user1, todo1],
- [user2, todo2],
- [user2, todo3],
- [user3, todo4]
+ [user2, todo2]
].each do |user, todo|
expect(permissions(user, todo)).to be_allowed(:read_todo)
end
@@ -38,7 +44,9 @@ RSpec.describe TodoPolicy do
[user2, todo4],
[user3, todo1],
[user3, todo2],
- [user3, todo3]
+ [user3, todo3],
+ [user2, todo3],
+ [user3, todo4]
].each do |user, todo|
expect(permissions(user, todo)).to be_disallowed(:read_todo)
end
diff --git a/spec/requests/api/graphql/current_user/todos_query_spec.rb b/spec/requests/api/graphql/current_user/todos_query_spec.rb
index e298de0df01..981b10a7467 100644
--- a/spec/requests/api/graphql/current_user/todos_query_spec.rb
+++ b/spec/requests/api/graphql/current_user/todos_query_spec.rb
@@ -4,12 +4,17 @@ require 'spec_helper'
RSpec.describe 'Query current user todos' do
include GraphqlHelpers
+ include DesignManagementTestHelpers
let_it_be(:current_user) { create(:user) }
- let_it_be(:commit_todo) { create(:on_commit_todo, user: current_user, project: create(:project, :repository)) }
- let_it_be(:issue_todo) { create(:todo, user: current_user, target: create(:issue)) }
- let_it_be(:merge_request_todo) { create(:todo, user: current_user, target: create(:merge_request)) }
- let_it_be(:design_todo) { create(:todo, user: current_user, target: create(:design)) }
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:unauthorize_project) { create(:project) }
+ let_it_be(:commit_todo) { create(:on_commit_todo, user: current_user, project: project) }
+ let_it_be(:issue) { create(:issue, project: project) }
+ let_it_be(:issue_todo) { create(:todo, project: project, user: current_user, target: issue) }
+ let_it_be(:merge_request_todo) { create(:todo, project: project, user: current_user, target: create(:merge_request, source_project: project)) }
+ let_it_be(:design_todo) { create(:todo, project: project, user: current_user, target: create(:design, issue: issue)) }
+ let_it_be(:unauthorized_todo) { create(:todo, user: current_user, project: unauthorize_project, target: create(:issue, project: unauthorize_project)) }
let(:fields) do
<<~QUERY
@@ -23,16 +28,22 @@ RSpec.describe 'Query current user todos' do
graphql_query_for('currentUser', {}, query_graphql_field('todos', {}, fields))
end
+ before_all do
+ project.add_developer(current_user)
+ end
+
subject { graphql_data.dig('currentUser', 'todos', 'nodes') }
before do
+ enable_design_management
+
post_graphql(query, current_user: current_user)
end
it_behaves_like 'a working graphql query'
it 'contains the expected ids' do
- is_expected.to include(
+ is_expected.to contain_exactly(
a_hash_including('id' => commit_todo.to_global_id.to_s),
a_hash_including('id' => issue_todo.to_global_id.to_s),
a_hash_including('id' => merge_request_todo.to_global_id.to_s),
@@ -41,11 +52,33 @@ RSpec.describe 'Query current user todos' do
end
it 'returns Todos for all target types' do
- is_expected.to include(
+ is_expected.to contain_exactly(
a_hash_including('targetType' => 'COMMIT'),
a_hash_including('targetType' => 'ISSUE'),
a_hash_including('targetType' => 'MERGEREQUEST'),
a_hash_including('targetType' => 'DESIGN')
)
end
+
+ context 'when requesting a single field' do
+ let(:fields) do
+ <<~QUERY
+ nodes {
+ id
+ }
+ QUERY
+ end
+
+ it 'avoids N+1 queries', :request_store do
+ control = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) }
+
+ project2 = create(:project)
+ project2.add_developer(current_user)
+ issue2 = create(:issue, project: project2)
+ create(:todo, user: current_user, target: issue2, project: project2)
+
+ # An additional query is made for each different group that owns a todo through a project
+ expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(control).with_threshold(2)
+ end
+ end
end
diff --git a/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb b/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb
index 8f92105dc9c..9ac98db91e2 100644
--- a/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb
+++ b/spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb
@@ -5,14 +5,16 @@ require 'spec_helper'
RSpec.describe 'Marking all todos done' do
include GraphqlHelpers
+ let_it_be(:project) { create(:project) }
+ let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:other_user) { create(:user) }
let_it_be(:other_user2) { create(:user) }
- let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) }
- let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
- let_it_be(:todo3) { create(:todo, user: current_user, author: author, state: :pending) }
+ let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
+ let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+ let_it_be(:todo3) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) }
@@ -28,6 +30,10 @@ RSpec.describe 'Marking all todos done' do
)
end
+ before_all do
+ project.add_developer(current_user)
+ end
+
def mutation_response
graphql_mutation_response(:todos_mark_all_done)
end
diff --git a/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb b/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb
index 8a9a0b9e845..7f5ea71c760 100644
--- a/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb
+++ b/spec/requests/api/graphql/mutations/todos/mark_done_spec.rb
@@ -5,12 +5,14 @@ require 'spec_helper'
RSpec.describe 'Marking todos done' do
include GraphqlHelpers
+ let_it_be(:project) { create(:project) }
+ let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:other_user) { create(:user) }
- let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) }
- let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
+ let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
+ let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) }
@@ -29,6 +31,10 @@ RSpec.describe 'Marking todos done' do
)
end
+ before_all do
+ project.add_developer(current_user)
+ end
+
def mutation_response
graphql_mutation_response(:todo_mark_done)
end
diff --git a/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb b/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb
index e71a232ff7c..70e3cc7f5cd 100644
--- a/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb
+++ b/spec/requests/api/graphql/mutations/todos/restore_many_spec.rb
@@ -5,12 +5,14 @@ require 'spec_helper'
RSpec.describe 'Restoring many Todos' do
include GraphqlHelpers
+ let_it_be(:project) { create(:project) }
+ let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:other_user) { create(:user) }
- let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) }
- let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
+ let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+ let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) }
@@ -30,6 +32,10 @@ RSpec.describe 'Restoring many Todos' do
)
end
+ before_all do
+ project.add_developer(current_user)
+ end
+
def mutation_response
graphql_mutation_response(:todo_restore_many)
end
diff --git a/spec/requests/api/graphql/mutations/todos/restore_spec.rb b/spec/requests/api/graphql/mutations/todos/restore_spec.rb
index a58c7fc69fc..d995191c97e 100644
--- a/spec/requests/api/graphql/mutations/todos/restore_spec.rb
+++ b/spec/requests/api/graphql/mutations/todos/restore_spec.rb
@@ -5,12 +5,14 @@ require 'spec_helper'
RSpec.describe 'Restoring Todos' do
include GraphqlHelpers
+ let_it_be(:project) { create(:project) }
+ let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:other_user) { create(:user) }
- let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) }
- let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending) }
+ let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+ let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) }
@@ -29,6 +31,10 @@ RSpec.describe 'Restoring Todos' do
)
end
+ before_all do
+ project.add_developer(current_user)
+ end
+
def mutation_response
graphql_mutation_response(:todo_restore)
end
diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb
index 00de1ef5964..d31f571e636 100644
--- a/spec/requests/api/todos_spec.rb
+++ b/spec/requests/api/todos_spec.rb
@@ -3,18 +3,22 @@
require 'spec_helper'
RSpec.describe API::Todos do
+ include DesignManagementTestHelpers
+
let_it_be(:group) { create(:group) }
let_it_be(:project_1) { create(:project, :repository, group: group) }
let_it_be(:project_2) { create(:project) }
let_it_be(:author_1) { create(:user) }
let_it_be(:author_2) { create(:user) }
let_it_be(:john_doe) { create(:user, username: 'john_doe') }
+ let_it_be(:issue) { create(:issue, project: project_1) }
let_it_be(:merge_request) { create(:merge_request, source_project: project_1) }
let_it_be(:merge_request_todo) { create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request) }
- let_it_be(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe) }
- let_it_be(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe) }
+ let_it_be(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe, target: issue) }
+ let_it_be(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe, target: issue) }
let_it_be(:pending_3) { create(:on_commit_todo, project: project_1, author: author_2, user: john_doe) }
- let_it_be(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe) }
+ let_it_be(:pending_4) { create(:on_commit_todo, project: project_1, author: author_2, user: john_doe, commit_id: 'invalid_id') }
+ let_it_be(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe, target: issue) }
let_it_be(:award_emoji_1) { create(:award_emoji, awardable: merge_request, user: author_1, name: 'thumbsup') }
let_it_be(:award_emoji_2) { create(:award_emoji, awardable: pending_1.target, user: author_1, name: 'thumbsup') }
let_it_be(:award_emoji_3) { create(:award_emoji, awardable: pending_2.target, user: author_2, name: 'thumbsdown') }
@@ -77,13 +81,13 @@ RSpec.describe API::Todos do
expect(json_response[0]['target_type']).to eq('Commit')
expect(json_response[1]['target_type']).to eq('Issue')
- expect(json_response[1]['target']['upvotes']).to eq(0)
+ expect(json_response[1]['target']['upvotes']).to eq(1)
expect(json_response[1]['target']['downvotes']).to eq(1)
expect(json_response[1]['target']['merge_requests_count']).to eq(0)
expect(json_response[2]['target_type']).to eq('Issue')
expect(json_response[2]['target']['upvotes']).to eq(1)
- expect(json_response[2]['target']['downvotes']).to eq(0)
+ expect(json_response[2]['target']['downvotes']).to eq(1)
expect(json_response[2]['target']['merge_requests_count']).to eq(0)
expect(json_response[3]['target_type']).to eq('MergeRequest')
@@ -93,6 +97,19 @@ RSpec.describe API::Todos do
expect(json_response[3]['target']['downvotes']).to eq(0)
end
+ context "when current user does not have access to one of the TODO's target" do
+ it 'filters out unauthorized todos' do
+ no_access_project = create(:project, :repository, group: group)
+ no_access_merge_request = create(:merge_request, source_project: no_access_project)
+ no_access_todo = create(:todo, project: no_access_project, author: author_2, user: john_doe, target: no_access_merge_request)
+
+ get api('/todos', john_doe)
+
+ expect(json_response.count).to eq(4)
+ expect(json_response.map { |t| t['id'] }).not_to include(no_access_todo.id, pending_4.id)
+ end
+ end
+
context 'and using the author filter' do
it 'filters based on author_id param' do
get api('/todos', john_doe), params: { author_id: author_2.id }
@@ -163,23 +180,31 @@ RSpec.describe API::Todos do
end
it 'avoids N+1 queries', :request_store do
+ create_issue_todo_for(john_doe)
create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request)
get api('/todos', john_doe)
- control = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) }
+ control1 = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) }
+
+ create_issue_todo_for(john_doe)
+ create_mr_todo_for(john_doe, project_2)
+ create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe, target: merge_request)
+ new_todo = create_mr_todo_for(john_doe)
+ merge_request_3 = create(:merge_request, :jira_branch, source_project: new_todo.project)
+ create(:on_commit_todo, project: new_todo.project, author: author_1, user: john_doe, target: merge_request_3)
+ create(:todo, project: new_todo.project, author: author_2, user: john_doe, target: merge_request_3)
- merge_request_2 = create(:merge_request, source_project: project_2)
- create(:todo, project: project_2, author: author_2, user: john_doe, target: merge_request_2)
+ expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control1).with_threshold(4)
+ control2 = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) }
- project_3 = create(:project, :repository)
- project_3.add_developer(john_doe)
- merge_request_3 = create(:merge_request, source_project: project_3)
- create(:todo, project: project_3, author: author_2, user: john_doe, target: merge_request_3)
- create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe)
- create(:on_commit_todo, project: project_3, author: author_1, user: john_doe)
+ create_issue_todo_for(john_doe)
+ create_issue_todo_for(john_doe, project_1)
+ create_issue_todo_for(john_doe, project_1)
+
+ # Additional query only when target belongs to project from different group
+ expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control2).with_threshold(1)
- expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control)
expect(response).to have_gitlab_http_status(:ok)
end
@@ -201,6 +226,8 @@ RSpec.describe API::Todos do
end
before do
+ enable_design_management
+
api_request
end
@@ -222,6 +249,20 @@ RSpec.describe API::Todos do
)
end
end
+
+ def create_mr_todo_for(user, project = nil)
+ new_project = project || create(:project, group: create(:group))
+ new_project.add_developer(user) if project.blank?
+ new_merge_request = create(:merge_request, source_project: new_project)
+ create(:todo, project: new_project, author: user, user: user, target: new_merge_request)
+ end
+
+ def create_issue_todo_for(user, project = nil)
+ new_project = project || create(:project, group: create(:group))
+ new_project.group.add_developer(user) if project.blank?
+ issue = create(:issue, project: new_project)
+ create(:todo, project: new_project, target: issue, author: user, user: user)
+ end
end
describe 'POST /todos/:id/mark_as_done' do
diff --git a/spec/services/todos/allowed_target_filter_service_spec.rb b/spec/services/todos/allowed_target_filter_service_spec.rb
new file mode 100644
index 00000000000..707df8e8514
--- /dev/null
+++ b/spec/services/todos/allowed_target_filter_service_spec.rb
@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::AllowedTargetFilterService do
+ include DesignManagementTestHelpers
+
+ let_it_be(:authorized_group) { create(:group, :private) }
+ let_it_be(:authorized_project) { create(:project, group: authorized_group) }
+ let_it_be(:unauthorized_group) { create(:group, :private) }
+ let_it_be(:unauthorized_project) { create(:project, group: unauthorized_group) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:authorized_issue) { create(:issue, project: authorized_project) }
+ let_it_be(:authorized_issue_todo) { create(:todo, project: authorized_project, target: authorized_issue, user: user) }
+ let_it_be(:unauthorized_issue) { create(:issue, project: unauthorized_project) }
+ let_it_be(:unauthorized_issue_todo) { create(:todo, project: unauthorized_project, target: unauthorized_issue, user: user) }
+ let_it_be(:authorized_design) { create(:design, issue: authorized_issue) }
+ let_it_be(:authorized_design_todo) { create(:todo, project: authorized_project, target: authorized_design, user: user) }
+ let_it_be(:unauthorized_design) { create(:design, issue: unauthorized_issue) }
+ let_it_be(:unauthorized_design_todo) { create(:todo, project: unauthorized_project, target: unauthorized_design, user: user) }
+
+ # Cannot use let_it_be with MRs
+ let(:authorized_mr) { create(:merge_request, source_project: authorized_project) }
+ let(:authorized_mr_todo) { create(:todo, project: authorized_project, user: user, target: authorized_mr) }
+ let(:unauthorized_mr) { create(:merge_request, source_project: unauthorized_project) }
+ let(:unauthorized_mr_todo) { create(:todo, project: unauthorized_project, user: user, target: unauthorized_mr) }
+
+ before_all do
+ authorized_group.add_developer(user)
+ end
+
+ describe '#execute' do
+ subject(:execute_service) { described_class.new(all_todos, user).execute }
+
+ let!(:all_todos) { authorized_todos + unauthorized_todos }
+
+ let(:authorized_todos) do
+ [
+ authorized_mr_todo,
+ authorized_issue_todo,
+ authorized_design_todo
+ ]
+ end
+
+ let(:unauthorized_todos) do
+ [
+ unauthorized_mr_todo,
+ unauthorized_issue_todo,
+ unauthorized_design_todo
+ ]
+ end
+
+ before do
+ enable_design_management
+ end
+
+ it { is_expected.to match_array(authorized_todos) }
+ end
+end