Merge branch 'release/17.0' into release/17.1

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

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

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

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) {

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

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

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

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