From 5bf981c64bdd1ba1130e544c32dddc34385574da Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 23 Feb 2026 17:50:12 +0100 Subject: [PATCH 01/11] Backport removal of `find_model_object` and properly scope project storages --- .../admin/project_storages_controller.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/modules/storages/app/controllers/storages/admin/project_storages_controller.rb b/modules/storages/app/controllers/storages/admin/project_storages_controller.rb index e26bacbcad7..5faadd6c83c 100644 --- a/modules/storages/app/controllers/storages/admin/project_storages_controller.rb +++ b/modules/storages/app/controllers/storages/admin/project_storages_controller.rb @@ -32,13 +32,12 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController include Storages::OAuthAccessGrantable include OpTurbo::ComponentStream - model_object Storages::ProjectStorage - - before_action :find_model_object, only: %i[oauth_access_grant edit update destroy destroy_info] + 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 @@ -61,7 +60,6 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController end def edit - @project_storage = @object @project_storage.project_folder_mode = project_folder_mode_from_params if project_folder_mode_from_params.present? @last_project_folders = Storages::LastProjectFolder @@ -88,7 +86,6 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController end def oauth_access_grant - @project_storage = @object storage = @project_storage.storage auth_state = ::Storages::Adapters::Authentication.authorization_state(storage:, user: current_user) @@ -105,7 +102,7 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController def update service_result = ::Storages::ProjectStorages::UpdateService - .new(user: current_user, model: @object) + .new(user: current_user, model: @project_storage) .call(permitted_storage_settings_params) if service_result.success? @@ -113,14 +110,13 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController flash[:notice] = I18n.t(:notice_successful_update) redirect_to_project_storages_path_with_oauth_access_grant_confirmation(@project_storage.storage) else - @project_storage = @object render "/storages/project_settings/edit" end end def destroy Storages::ProjectStorages::DeleteService - .new(user: current_user, model: @object) + .new(user: current_user, model: @project_storage) .call .on_failure { |service_result| flash[:error] = service_result.errors.full_messages } @@ -129,13 +125,17 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController def destroy_info respond_with_dialog Storages::ProjectStorages::DestroyConfirmationDialogComponent.new( - project_storage: @object, + project_storage: @project_storage, target: :project ) end private + def find_project_storage + @project_storage = @project.project_storages.find(params[:id]) + end + def permitted_storage_settings_params params .require(:storages_project_storage) From 334ae53bccffb8a232523cb6050b5b326c235a1b Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 23 Feb 2026 17:53:02 +0100 Subject: [PATCH 02/11] move `delete_project_folder` after the validation --- .../app/services/storages/project_storages/delete_service.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/storages/app/services/storages/project_storages/delete_service.rb b/modules/storages/app/services/storages/project_storages/delete_service.rb index a518c730bb4..fda80008cf6 100644 --- a/modules/storages/app/services/storages/project_storages/delete_service.rb +++ b/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 From fc88c1640d07783c2917d569c0add47eb694c40c Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 24 Feb 2026 11:56:42 +0100 Subject: [PATCH 03/11] Fix specs --- .../storages/app/views/storages/project_settings/edit.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/storages/app/views/storages/project_settings/edit.html.erb b/modules/storages/app/views/storages/project_settings/edit.html.erb index 8db2cfd68f8..a13486010eb 100644 --- a/modules/storages/app/views/storages/project_settings/edit.html.erb +++ b/modules/storages/app/views/storages/project_settings/edit.html.erb @@ -27,7 +27,7 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<% html_title t(:label_administration), t("project_module_storages"), t("label_edit_x", x: @object.storage.name) %> +<% html_title t(:label_administration), t("project_module_storages"), t("label_edit_x", x: @project_storage.storage.name) %> <%= render Primer::OpenProject::PageHeader.new do |header| From 53c1fb4059422ad200a81cd643ed26e9af562e4f Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 24 Feb 2026 13:24:33 +0100 Subject: [PATCH 04/11] Fix project storages not being properly loaded through the project --- .../admin/project_storages_controller.rb | 20 +++++++++---------- .../project_storages/delete_service.rb | 4 ++-- .../storages/project_settings/edit.html.erb | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/modules/storages/app/controllers/storages/admin/project_storages_controller.rb b/modules/storages/app/controllers/storages/admin/project_storages_controller.rb index 96a4cc13d6c..e64cc47575c 100644 --- a/modules/storages/app/controllers/storages/admin/project_storages_controller.rb +++ b/modules/storages/app/controllers/storages/admin/project_storages_controller.rb @@ -32,13 +32,12 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController include Storages::OAuthAccessGrantable include OpTurbo::ComponentStream - model_object Storages::ProjectStorage - - before_action :find_model_object, only: %i[oauth_access_grant edit update destroy destroy_info] + 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 @@ -61,7 +60,6 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController end def edit - @project_storage = @object @project_storage.project_folder_mode = project_folder_mode_from_params if project_folder_mode_from_params.present? @last_project_folders = Storages::LastProjectFolder @@ -88,7 +86,6 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController end def oauth_access_grant - @project_storage = @object storage = @project_storage.storage auth_state = ::Storages::Adapters::Authentication.authorization_state(storage:, user: current_user) @@ -105,7 +102,7 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController def update service_result = ::Storages::ProjectStorages::UpdateService - .new(user: current_user, model: @object) + .new(user: current_user, model: @project_storage) .call(permitted_storage_settings_params) if service_result.success? @@ -113,14 +110,13 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController flash[:notice] = I18n.t(:notice_successful_update) redirect_to_project_storages_path_with_oauth_access_grant_confirmation(@project_storage.storage) else - @project_storage = @object render "/storages/project_settings/edit" end end def destroy Storages::ProjectStorages::DeleteService - .new(user: current_user, model: @object) + .new(user: current_user, model: @project_storage) .call .on_failure { |service_result| flash[:error] = service_result.errors.full_messages } @@ -128,11 +124,15 @@ class Storages::Admin::ProjectStoragesController < Projects::SettingsController end def destroy_info - respond_with_dialog Storages::ProjectStorages::Projects::DestroyConfirmationDialogComponent.new(storage: @object) + respond_with_dialog Storages::ProjectStorages::Projects::DestroyConfirmationDialogComponent.new(storage: @project_storage) end private + def find_project_storage + @project_storage = @project.project_storages.find(params[:id]) + end + def permitted_storage_settings_params params .require(:storages_project_storage) diff --git a/modules/storages/app/services/storages/project_storages/delete_service.rb b/modules/storages/app/services/storages/project_storages/delete_service.rb index a518c730bb4..fda80008cf6 100644 --- a/modules/storages/app/services/storages/project_storages/delete_service.rb +++ b/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 diff --git a/modules/storages/app/views/storages/project_settings/edit.html.erb b/modules/storages/app/views/storages/project_settings/edit.html.erb index 8db2cfd68f8..a13486010eb 100644 --- a/modules/storages/app/views/storages/project_settings/edit.html.erb +++ b/modules/storages/app/views/storages/project_settings/edit.html.erb @@ -27,7 +27,7 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<% html_title t(:label_administration), t("project_module_storages"), t("label_edit_x", x: @object.storage.name) %> +<% html_title t(:label_administration), t("project_module_storages"), t("label_edit_x", x: @project_storage.storage.name) %> <%= render Primer::OpenProject::PageHeader.new do |header| From 3681c006f7df44c058997227b11bd8f768d9b0b7 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 24 Feb 2026 13:31:50 +0100 Subject: [PATCH 05/11] Add spec for DeleteService behavior --- .../project_storages/delete_service_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb b/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb index 33138ac0233..f2830dfa207 100644 --- a/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb +++ b/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") From f77d391b787c97bbf2df2bb7a61ecd2b25e684e3 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 24 Feb 2026 13:31:50 +0100 Subject: [PATCH 06/11] Add spec for DeleteService behavior --- .../project_storages/delete_service_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb b/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb index 33138ac0233..f2830dfa207 100644 --- a/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb +++ b/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") From 58217b51eb78fa5df6bbda841f4003149ec84d39 Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Wed, 25 Feb 2026 09:37:54 +0100 Subject: [PATCH 07/11] Update texts and caption in EE token dialog --- app/forms/enterprise_tokens/token_form.rb | 9 +++++++++ config/locales/en.yml | 3 ++- config/static_links.yml | 4 ++++ spec/features/admin/enterprise/enterprise_spec.rb | 4 ++-- spec/support/pages/admin/enterprise_tokens/index.rb | 4 ++-- 5 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app/forms/enterprise_tokens/token_form.rb b/app/forms/enterprise_tokens/token_form.rb index 36e650bb3a4..e2473111146 100644 --- a/app/forms/enterprise_tokens/token_form.rb +++ b/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 diff --git a/config/locales/en.yml b/config/locales/en.yml index 6267dd4593d..c072cbf78ba 100644 --- a/config/locales/en.yml +++ b/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" diff --git a/config/static_links.yml b/config/static_links.yml index 25ace21aa98..7708b89c816 100644 --- a/config/static_links.yml +++ b/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 diff --git a/spec/features/admin/enterprise/enterprise_spec.rb b/spec/features/admin/enterprise/enterprise_spec.rb index dfe2840e247..013f5028d1b 100644 --- a/spec/features/admin/enterprise/enterprise_spec.rb +++ b/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 diff --git a/spec/support/pages/admin/enterprise_tokens/index.rb b/spec/support/pages/admin/enterprise_tokens/index.rb index 81b742ff61c..74fc30693d5 100644 --- a/spec/support/pages/admin/enterprise_tokens/index.rb +++ b/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 From 1096e71cd8493954ddcad0010eb8fb2e551db8de Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 25 Feb 2026 11:43:03 +0100 Subject: [PATCH 08/11] global query deletion of own queries simplified --- app/policies/query_policy.rb | 7 +- spec/policies/query_policy_spec.rb | 139 ++++++++++++++++++++++++++--- 2 files changed, 132 insertions(+), 14 deletions(-) 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/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 From 0a2378979d720bf4f8cd60829c1d6ef2852ae6e6 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 25 Feb 2026 11:49:47 +0100 Subject: [PATCH 09/11] =?UTF-8?q?prevent=20deletion=20of=20other=20users?= =?UTF-8?q?=CB=9A=20queries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../my_page/lib/my_page/grid_registration.rb | 16 +++++++- .../my_page/spec/models/grids/my_page_spec.rb | 40 ++++++++++++++----- 2 files changed, 45 insertions(+), 11 deletions(-) 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 From f900dd54f594a3837461b5bd90ecab2e10f1323c Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 25 Feb 2026 12:52:10 +0100 Subject: [PATCH 10/11] =?UTF-8?q?prevent=20deletion=20of=20other=20users?= =?UTF-8?q?=CB=9A=20queries=20via=20boards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- modules/boards/app/models/boards/grid.rb | 10 +++-- .../boards/spec/models/boards/grid_spec.rb | 44 +++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) 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 From 34e577421d0a60d44e9ed4c9ebfee05eaed57f5c Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 25 Feb 2026 13:13:39 +0100 Subject: [PATCH 11/11] =?UTF-8?q?prevent=20deletion=20of=20other=20users?= =?UTF-8?q?=CB=9A=20queries=20via=20overview?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../in_project_base_registration.rb | 6 +- .../spec/models/grids/overview_spec.rb | 89 +++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 modules/overviews/spec/models/grids/overview_spec.rb 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/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