diff --git a/app/policies/query_policy.rb b/app/policies/query_policy.rb index e193e9e886f..e2c23b2f4c9 100644 --- a/app/policies/query_policy.rb +++ b/app/policies/query_policy.rb @@ -36,7 +36,7 @@ class QueryPolicy < BasePolicy hash[cached_query] = { show: viewable?(cached_query), update: persisted_and_own_or_public?(cached_query), - destroy: persisted_and_own_or_public?(cached_query), + destroy: destroy_allowed?(cached_query), create: create_allowed?(cached_query), create_new: create_new_allowed?(cached_query), publicize: publicize_allowed?(cached_query), @@ -126,4 +126,9 @@ class QueryPolicy < BasePolicy user.allowed_in_any_project?(:share_calendars) end end + + def destroy_allowed?(query) + (query.persisted? && !query.project && query.user == user && user.logged?) || + persisted_and_own_or_public?(query) + end end diff --git a/modules/boards/app/models/boards/grid.rb b/modules/boards/app/models/boards/grid.rb index 2192b1d0ae4..5201fcdb860 100644 --- a/modules/boards/app/models/boards/grid.rb +++ b/modules/boards/app/models/boards/grid.rb @@ -29,9 +29,9 @@ module Boards class Grid < ::Grids::Grid belongs_to :project - validates_presence_of :name + validates :name, presence: true - before_destroy :delete_queries + before_destroy :delete_queries, prepend: true set_acts_as_attachable_options view_permission: :show_board_views, delete_permission: :manage_board_views, @@ -62,7 +62,11 @@ module Boards private def delete_queries - contained_queries.delete_all + policy = QueryPolicy.new(User.current) + + contained_queries + .select { |q| policy.allowed?(q, :destroy) } + .each(&:delete) end def contained_query_ids diff --git a/modules/boards/spec/models/boards/grid_spec.rb b/modules/boards/spec/models/boards/grid_spec.rb index 2709e4d7b30..e90a3f9ac0b 100644 --- a/modules/boards/spec/models/boards/grid_spec.rb +++ b/modules/boards/spec/models/boards/grid_spec.rb @@ -83,4 +83,48 @@ RSpec.describe Boards::Grid do end end end + + describe "#destroy" do + context "with an associated query" do + let(:project) { create(:project) } + let(:instance) { described_class.new name: "foo", row_count: 2, column_count: 2, project: } + let(:query) do + create(:query, + public: true, + project: project) + end + + current_user { build_stubbed(:user) } + + before do + widget = Grids::Widget.new(identifier: "work_package_query", + start_row: 1, + end_row: 2, + start_column: 1, + end_column: 2, + options: { "queryId" => query.id }) + + instance.widgets = [widget] + instance.save! + end + + context "when the user has the permissions to manage queries" do + before do + mock_permissions_for(current_user) do |mock| + mock.allow_in_project :manage_public_queries, project: + end + end + + it "deletes the query" do + expect { instance.destroy }.to change(Query, :count).by(-1) + end + end + + context "when the user lacks the permissions to manage queries" do + it "keeps the query" do + expect { instance.destroy }.not_to change(Query, :count) + end + end + end + end end diff --git a/modules/grids/lib/grids/configuration/in_project_base_registration.rb b/modules/grids/lib/grids/configuration/in_project_base_registration.rb index 675d3abdcdb..bebb5d7b4c0 100644 --- a/modules/grids/lib/grids/configuration/in_project_base_registration.rb +++ b/modules/grids/lib/grids/configuration/in_project_base_registration.rb @@ -15,7 +15,11 @@ module Grids::Configuration "custom_text" remove_query_lambda = -> { - ::Query.find_by(id: options[:queryId])&.destroy + @query = ::Query.find_by(id: options["queryId"]) + + next unless @query && QueryPolicy.new(User.current).allowed?(@query, :destroy) + + @query.destroy! } save_or_manage_queries_lambda = ->(user, project) { diff --git a/modules/my_page/lib/my_page/grid_registration.rb b/modules/my_page/lib/my_page/grid_registration.rb index 783fa6d6012..3594078b7b0 100644 --- a/modules/my_page/lib/my_page/grid_registration.rb +++ b/modules/my_page/lib/my_page/grid_registration.rb @@ -16,7 +16,13 @@ module MyPage "news" wp_table_strategy_proc = Proc.new do - after_destroy -> { ::Query.find_by(id: options[:queryId])&.destroy } + after_destroy -> { + @query = ::Query.find_by(id: options["queryId"]) + + next unless @query && QueryPolicy.new(User.current).allowed?(@query, :destroy) + + @query.destroy! + } allowed ->(user, _project) { user.allowed_in_any_project?(:save_queries) } @@ -26,7 +32,13 @@ module MyPage # Allow users without save_queries permission to access the widgets # but they are not allowed to update the underlying query wp_static_table_strategy_proc = Proc.new do - after_destroy -> { ::Query.find_by(id: options[:queryId])&.destroy } + after_destroy -> { + @query = ::Query.find_by(id: options["queryId"]) + + next unless @query && QueryPolicy.new(User.current).allowed?(@query, :destroy) + + @query.destroy! + } options_representer "::API::V3::Grids::Widgets::QueryOptionsRepresenter" end diff --git a/modules/my_page/spec/models/grids/my_page_spec.rb b/modules/my_page/spec/models/grids/my_page_spec.rb index 1f40e0dcc58..5f4950b711c 100644 --- a/modules/my_page/spec/models/grids/my_page_spec.rb +++ b/modules/my_page/spec/models/grids/my_page_spec.rb @@ -45,31 +45,53 @@ RSpec.describe Grids::MyPage do end context "altering widgets" do - context "when removing a work_packages_table widget" do - let(:user) { create(:user) } + shared_examples_for "removing a query widget" do |identifier| + let(:query_user) { create(:user) } let(:query) do create(:query, - user:) + user: query_user, + project: nil) end before do - widget = Grids::Widget.new(identifier: "work_packages_table", + widget = Grids::Widget.new(identifier:, start_row: 1, end_row: 2, start_column: 1, end_column: 2, - options: { queryId: query.id }) + options: { "queryId" => query.id }) instance.widgets = [widget] instance.save! end - it "removes the widget's query" do - instance.widgets = [] + context "when the query is owned by the user" do + current_user { query_user } - expect(Query.find_by(id: query.id)) - .to be_nil + it "removes the widget's query" do + instance.widgets = [] + + expect(Query.find_by(id: query.id)) + .to be_nil + end + end + + context "when the query is not owned by the user" do + current_user { create(:user) } + + it "removes the widget but keeps the query" do + instance.widgets = [] + + expect(Query.find_by(id: query.id)) + .to eql query + end end end + + it_behaves_like "removing a query widget", "work_packages_table" + it_behaves_like "removing a query widget", "work_packages_assigned" + it_behaves_like "removing a query widget", "work_packages_accountable" + it_behaves_like "removing a query widget", "work_packages_watched" + it_behaves_like "removing a query widget", "work_packages_created" end end diff --git a/modules/overviews/spec/models/grids/overview_spec.rb b/modules/overviews/spec/models/grids/overview_spec.rb new file mode 100644 index 00000000000..b1ba921f888 --- /dev/null +++ b/modules/overviews/spec/models/grids/overview_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +# -- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +# ++ + +require "spec_helper" + +RSpec.describe Grids::Overview do + let(:instance) { described_class.new(row_count: 5, column_count: 5) } + + context "when altering widgets" do + shared_examples_for "removing a query widget" do |identifier| + let(:project) { create(:project) } + let(:query) do + create(:query, + public: true, + project:) + end + + current_user { build_stubbed(:user) } + + before do + widget = Grids::Widget.new(identifier:, + start_row: 1, + end_row: 2, + start_column: 1, + end_column: 2, + options: { "queryId" => query.id }) + + instance.widgets = [widget] + instance.save! + end + + context "when the current user has the permission to manage public queries" do + before do + mock_permissions_for(current_user) do |mock| + mock.allow_in_project :manage_public_queries, project: + end + end + + it "removes the widget's query" do + instance.widgets = [] + + expect(Query.find_by(id: query.id)) + .to be_nil + end + end + + context "when the current user lacks the permission to manage public queries" do + current_user { create(:user) } + + it "removes the widget but keeps the query" do + instance.widgets = [] + + expect(Query.find_by(id: query.id)) + .to eql query + end + end + end + + it_behaves_like "removing a query widget", "work_packages_table" + it_behaves_like "removing a query widget", "work_packages_graph" + end +end diff --git a/spec/policies/query_policy_spec.rb b/spec/policies/query_policy_spec.rb index 6490215a7e3..2cb6becb094 100644 --- a/spec/policies/query_policy_spec.rb +++ b/spec/policies/query_policy_spec.rb @@ -78,17 +78,12 @@ RSpec.describe QueryPolicy, type: :controller do end end - shared_examples "action on persisted" do |action, global| + shared_examples "action on persisted" do |action, global:| context "for #{action} #{global ? 'in global context' : 'in project context'}" do if global let(:project) { nil } end - before do - allow(query).to receive(:new_record?).and_return false - allow(query).to receive(:persisted?).and_return true - end - it "is false if the user has no permission in the project" do mock_permissions_for(user, &:forbid_everything) @@ -170,7 +165,7 @@ RSpec.describe QueryPolicy, type: :controller do end end - shared_examples "action on unpersisted" do |action, global| + shared_examples "action on unpersisted" do |action, global:| context "for #{action} #{global ? 'in global context' : 'in project context'}" do if global let(:project) { nil } @@ -210,7 +205,7 @@ RSpec.describe QueryPolicy, type: :controller do end end - shared_examples "publicize" do |global| + shared_examples "publicize" do |global:| context (global ? "in global context" : "in project context").to_s do if global let(:project) { nil } @@ -247,7 +242,7 @@ RSpec.describe QueryPolicy, type: :controller do end end - shared_examples "depublicize" do |global| + shared_examples "depublicize" do |global:| context (global ? "in global context" : "in project context").to_s do if global let(:project) { nil } @@ -286,7 +281,7 @@ RSpec.describe QueryPolicy, type: :controller do end end - shared_examples "star" do |global| + shared_examples "star" do |global:| context (global ? "in global context" : "in project context").to_s do if global let(:project) { nil } @@ -302,7 +297,7 @@ RSpec.describe QueryPolicy, type: :controller do end end - shared_examples "update ordered_work_packages" do |global| + shared_examples "update ordered_work_packages" do |global:| context (global ? "in global context" : "in project context").to_s do if global let(:project) { nil } @@ -367,7 +362,7 @@ RSpec.describe QueryPolicy, type: :controller do end end - shared_examples "share via ical" do |global| + shared_examples "share via ical" do |global:| context (global ? "in global context" : "in project context").to_s do if global let(:project) { nil } @@ -391,9 +386,127 @@ RSpec.describe QueryPolicy, type: :controller do end end + shared_examples "action on destroy in global context" do + let(:project) { nil } + + context "if the user has no permission " \ + "AND it is a global query " \ + "AND it is the user's query " \ + "AND the user is logged in" \ + "AND the query is not public" do + it "is true" do + mock_permissions_for(user) { it&.forbid_everything } + + query.user = user + query.public = false + + expect(subject) + .to be_allowed(query, :destroy) + end + end + + context "if the user has no permission AND it is a global query AND it is another user's query" do + it "is false" do + mock_permissions_for(user) { it&.forbid_everything } + + query.user = build_stubbed(:user) + query.public = false + + expect(subject) + .not_to be_allowed(query, :destroy) + end + end + + context "if the user has no permission " \ + "AND it is a project query " \ + "AND it is the user's query " \ + "AND the user is logged in" \ + "AND the query is not public" do + it "is false" do + mock_permissions_for(user) { it&.forbid_everything } + + query.user = user + query.project = build_stubbed(:project) + query.public = false + + expect(subject) + .not_to be_allowed(query, :destroy) + end + end + + context "if the user has no permission " \ + "AND it is a global query " \ + "AND it is the user's query " \ + "AND the user is not logged in " \ + "AND the query is not public" do + # This case shouldn't happen as anonymous users cannot create queries + let(:user) { build_stubbed(:anonymous) } + + it "is false" do + mock_permissions_for(user) { it&.forbid_everything } + query.user = user + query.public = false + + expect(subject) + .not_to be_allowed(query, :destroy) + end + end + + context "if the user has no permission " \ + "AND it is a global query " \ + "AND it is the user's query " \ + "AND the query is not persisted" do + let(:query) { Query.new(attributes_for(:query, project:, user:)) } + + it "is false" do + mock_permissions_for(user) { it&.forbid_everything } + query.user = user + query.public = false + allow(query).to receive(:persisted?).and_return false + + expect(subject) + .not_to be_allowed(query, :destroy) + end + end + + context "if the user has the permission to manage_public_queries " \ + "AND it is a global query " \ + "AND it is another user's query " \ + "AND it is a public query" do + it "is true" do + mock_permissions_for(user) do |mock| + mock.allow_in_project :manage_public_queries, project: build_stubbed(:project) + end + + query.user = build_stubbed(:user) + query.public = true + + expect(subject) + .to be_allowed(query, :destroy) + end + end + + context "if the user has the permission to manage_public_queries " \ + "AND it is a global query " \ + "AND it is another user's query " \ + "AND it isn't a public query" do + it "is false" do + mock_permissions_for(user) do |mock| + mock.allow_in_project :manage_public_queries, project: build_stubbed(:project) + end + + query.user = build_stubbed(:user) + query.public = false + + expect(subject) + .not_to be_allowed(query, :destroy) + end + end + end + it_behaves_like "action on persisted", :update, global: true it_behaves_like "action on persisted", :update, global: false - it_behaves_like "action on persisted", :destroy, global: true + it_behaves_like "action on destroy in global context" it_behaves_like "action on persisted", :destroy, global: false it_behaves_like "action on unpersisted", :create, global: true it_behaves_like "action on unpersisted", :create, global: false