Merge branch 'release/17.2' into dev

app/forms/enterprise_tokens/token_form.rb

@@ -29,14 +29,23 @@
 #++
 module EnterpriseTokens
   class TokenForm < ApplicationForm
+    include Redmine::I18n
+
     form do |f|
       f.text_area(
         name: :encoded_token,
         label: I18n.t("admin.enterprise.create_dialog.type_token_text"),
         placeholder: I18n.t("admin.enterprise.create_dialog.token_placeholder"),
+        caption:,
         rows: 10,
         style: "resize: none"
       )
     end
+
+    def caption
+      link_translate("admin.enterprise.create_dialog.token_caption",
+                     links: { docs_url: %i[enterprise_guide on_premises activate] },
+                     external: true)
+    end
   end
 end

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

config/locales/en.yml

@@ -92,8 +92,9 @@ en:
         confirmation: "Are you sure you want to delete this Enterprise edition support token?"
       create_dialog:
         title: "Add Enterprise token"
-        type_token_text: "Type support token text"
+        type_token_text: "Your Enterprise token text"
         token_placeholder: "Paste your Enterprise edition support token here"
+        token_caption: "To learn more about how to activate Enterprise edition check our [documentation](docs_url)."
       add_token: "Upload an Enterprise edition support token"
       replace_token: "Replace your current support token"
       order: "Order Enterprise on-premises edition"

config/static_links.yml

@@ -68,6 +68,10 @@ enterprise_features:
     href: https://www.openproject.org/docs/system-admin-guide/manage-work-packages/work-package-status/#create-a-new-work-package-status
   work_package_subject_generation:
     href: https://www.openproject.org/docs/system-admin-guide/manage-work-packages/work-package-types/automatic-subjects/
+enterprise_guide:
+  on_premises:
+    activate:
+      href: https://www.openproject.org/docs/enterprise-guide/enterprise-on-premises-guide/activate-enterprise-on-premises/
 enterprise_support:
   href: https://www.openproject.org/docs/enterprise-guide/support/
   label: :label_enterprise_support

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

@@ -14,7 +14,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

modules/storages/app/controllers/storages/admin/project_storages_controller.rb

@@ -32,11 +32,12 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController
   include Storages::OAuthAccessGrantable
   include OpTurbo::ComponentStream
 
+  before_action :find_project_by_project_id
   before_action :find_project_storage, only: %i[oauth_access_grant edit update destroy destroy_info]
   menu_item :settings_project_storages
 
   def external_file_storages
-    @project_storages = Storages::ProjectStorage.where(project: @project).includes(:storage)
+    @project_storages = @project.project_storages.includes(:storage)
     render "/storages/project_settings/external_file_storages"
   end
 
@@ -132,7 +133,7 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController
   private
 
   def find_project_storage
-    @project_storage = Storages::ProjectStorage.find(params[:id])
+    @project_storage = @project.project_storages.find(params[:id])
   end
 
   def permitted_storage_settings_params

modules/storages/app/services/storages/project_storages/delete_service.rb

@@ -33,10 +33,10 @@ module Storages
     # Performs the deletion in the superclass. Associated FileLinks are deleted
     # by the model before_destroy hook.
     class DeleteService < ::BaseServices::Delete
-      def before_perform(*)
+      def after_validate(service_call)
         delete_project_folder if model.project_folder_automatic?
 
-        super
+        service_call
       end
 
       # "persist" is a callback from BaseContracted.perform

modules/storages/spec/services/storages/project_storages/delete_service_spec.rb

@@ -63,6 +63,19 @@ RSpec.describe Storages::ProjectStorages::DeleteService, :webmock, type: :model
       end
     end
 
+    context "when the user is not permitted to manage files in the project" do
+      let(:role) { create(:project_role, permissions: []) }
+      let(:project_storage) do
+        create(:project_storage, project:, storage:, project_folder_id: "1337", project_folder_mode: "automatic")
+      end
+
+      it "must not try to delete project folders" do
+        described_class.new(model: project_storage, user:).call
+
+        expect(command_double).not_to have_received(:call)
+      end
+    end
+
     context "if project folder mode is set to manual" do
       let(:project_storage) do
         create(:project_storage, project:, storage:, project_folder_id: "1337", project_folder_mode: "manual")

spec/features/admin/enterprise/enterprise_spec.rb

@@ -50,7 +50,7 @@ RSpec.describe "Enterprise token", :js do
       click_button "Add Enterprise token"
 
       expect(page).to have_dialog("Add Enterprise token")
-      expect(page).to have_field("Type support token text", type: "textarea")
+      expect(page).to have_field("Your Enterprise token text", type: "textarea")
     end
 
     context "with invalid input" do
@@ -135,7 +135,7 @@ RSpec.describe "Enterprise token", :js do
         enterprise_tokens_page.expect_add_token_validation_error("This token has already been added.")
 
         # Try importing with blank spaces and newlines before and after
-        fill_in "Type support token text", with: " \nfoobar \n"
+        fill_in "Your Enterprise token text", with: " \nfoobar \n"
         click_button "Add"
 
         # The dialog is still open with an error message on token field

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

spec/support/pages/admin/enterprise_tokens/index.rb

@@ -41,7 +41,7 @@ module Pages
         def add_enterprise_token(token_text)
           click_button "Add Enterprise token"
           modals.expect_modal("Add Enterprise token")
-          fill_in "Type support token text", with: token_text
+          fill_in "Your Enterprise token text", with: token_text
           click_button "Add"
         end
 
@@ -53,7 +53,7 @@ module Pages
 
         def expect_add_token_validation_error(message)
           expect(page).to have_dialog("Add Enterprise token")
-          expect(page).to have_field("Type support token text", validation_error: message)
+          expect(page).to have_field("Your Enterprise token text", validation_error: message)
         end
 
         private