From 272ff67619f6a4b61a67505b12bdf82db6c4ccee Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Thu, 19 Feb 2026 13:20:35 +0100 Subject: [PATCH] Introduce visible scope for types and statuses Both models are only supposed to be visible to users that have some basic permissions in at least one project. While the desired scoping is not very fine grained (you either see all or nothing), it still makes sense for all models to have such a scope for consistency purposes. --- app/models/status.rb | 2 ++ app/models/type.rb | 7 ++++ app/services/mcp_resources/status.rb | 2 +- app/services/mcp_resources/status_list.rb | 6 +++- app/services/mcp_resources/type.rb | 2 +- app/services/mcp_resources/type_list.rb | 6 +++- lib/api/v3/statuses/statuses_api.rb | 4 +-- lib/api/v3/types/types_api.rb | 4 +-- spec/models/status_spec.rb | 24 +++++++++++++ spec/models/type_spec.rb | 34 ++++++++++++++++++- .../mcp/mcp_resources/status_list_spec.rb | 17 +++++++++- .../requests/mcp/mcp_resources/status_spec.rb | 10 +++++- .../mcp/mcp_resources/type_list_spec.rb | 17 +++++++++- spec/requests/mcp/mcp_resources/type_spec.rb | 10 +++++- .../mcp/mcp_tools/list_statuses_spec.rb | 11 ++++++ .../requests/mcp/mcp_tools/list_types_spec.rb | 11 ++++++ 16 files changed, 154 insertions(+), 13 deletions(-) diff --git a/app/models/status.rb b/app/models/status.rb index 9ea8ffda25d..93d849c55e3 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -49,6 +49,8 @@ class Status < ApplicationRecord after_save :unmark_old_default_value, if: :is_default? + scope :visible, ->(user = User.current) { user.allowed_in_any_project?(:view_work_packages) ? all : none } + def unmark_old_default_value Status.where.not(id:).update_all(is_default: false) end diff --git a/app/models/type.rb b/app/models/type.rb index d7d5e097974..1655b654776 100644 --- a/app/models/type.rb +++ b/app/models/type.rb @@ -72,6 +72,13 @@ class Type < ApplicationRecord scope :without_standard, -> { where(is_standard: false).order(:position) } scope :default, -> { where(is_default: true) } + scope :visible, ->(user = User.current) { + if user.allowed_in_any_project?(:view_work_packages) || user.allowed_in_any_project?(:manage_types) + all + else + none + end + } delegate :to_s, to: :name diff --git a/app/services/mcp_resources/status.rb b/app/services/mcp_resources/status.rb index 28b9316fb3c..ea941cfe7af 100644 --- a/app/services/mcp_resources/status.rb +++ b/app/services/mcp_resources/status.rb @@ -37,7 +37,7 @@ module McpResources default_description "Access work package statuses of this OpenProject instance." def read(id:) - status = ::Status.find_by(id:) + status = ::Status.visible(current_user).find_by(id:) return nil if status.nil? API::V3::Statuses::StatusRepresenter.new(status, current_user:) diff --git a/app/services/mcp_resources/status_list.rb b/app/services/mcp_resources/status_list.rb index be8c6e19826..76ed65d3836 100644 --- a/app/services/mcp_resources/status_list.rb +++ b/app/services/mcp_resources/status_list.rb @@ -37,7 +37,11 @@ module McpResources default_description "A list of all work package statuses configured in this OpenProject instance." def read - API::V3::Statuses::StatusCollectionRepresenter.new(::Status.all, self_link: api_v3_paths.statuses, current_user:) + API::V3::Statuses::StatusCollectionRepresenter.new( + ::Status.visible(current_user), + self_link: api_v3_paths.statuses, + current_user: + ) end end end diff --git a/app/services/mcp_resources/type.rb b/app/services/mcp_resources/type.rb index 2e7f6f5c8f4..f3ab341a8e5 100644 --- a/app/services/mcp_resources/type.rb +++ b/app/services/mcp_resources/type.rb @@ -37,7 +37,7 @@ module McpResources default_description "Access work package types of this OpenProject instance." def read(id:) - type = ::Type.find_by(id:) + type = ::Type.visible.find_by(id:) return nil if type.nil? API::V3::Types::TypeRepresenter.new(type, current_user:) diff --git a/app/services/mcp_resources/type_list.rb b/app/services/mcp_resources/type_list.rb index cbe4a1693d1..d8c02013c53 100644 --- a/app/services/mcp_resources/type_list.rb +++ b/app/services/mcp_resources/type_list.rb @@ -37,7 +37,11 @@ module McpResources default_description "A list of all work package types configured in this OpenProject instance." def read - API::V3::Types::TypeCollectionRepresenter.new(::Type.includes(:color).all, self_link: api_v3_paths.types, current_user:) + API::V3::Types::TypeCollectionRepresenter.new( + ::Type.includes(:color).visible(current_user), + self_link: api_v3_paths.types, + current_user: + ) end end end diff --git a/lib/api/v3/statuses/statuses_api.rb b/lib/api/v3/statuses/statuses_api.rb index 2948ff66f9b..dc1f01ab07e 100644 --- a/lib/api/v3/statuses/statuses_api.rb +++ b/lib/api/v3/statuses/statuses_api.rb @@ -39,7 +39,7 @@ module API end get do - StatusCollectionRepresenter.new(Status.all, + StatusCollectionRepresenter.new(Status.visible, self_link: api_v3_paths.statuses, current_user:) end @@ -49,7 +49,7 @@ module API # Note that naming the method #status or having # a variable named @status colides with grape. def work_package_status - Status.find(params[:id]) + Status.visible.find(params[:id]) end end diff --git a/lib/api/v3/types/types_api.rb b/lib/api/v3/types/types_api.rb index c1f4e83901d..3fc78471a4d 100644 --- a/lib/api/v3/types/types_api.rb +++ b/lib/api/v3/types/types_api.rb @@ -39,7 +39,7 @@ module API end get do - types = Type.includes(:color).all + types = Type.includes(:color).visible TypeCollectionRepresenter .new(types, self_link: api_v3_paths.types, @@ -48,7 +48,7 @@ module API route_param :id, type: Integer, desc: "Type ID" do after_validation do - type = Type.find(params[:id]) + type = Type.visible.find(params[:id]) @representer = TypeRepresenter.new(type, current_user:) end diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index d061c478510..70899648d4e 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -66,6 +66,30 @@ RSpec.describe Status do end end + describe ".visible" do + subject { described_class.visible(user) } + + let!(:status) { create(:status) } + let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } + + before do + create(:member, user:, roles: [create(:project_role, permissions: permissions)]) + end + + it "returns the same statuses as all" do + expect(subject.to_a).to match_array(described_class.all.to_a) + end + + context "when the user has the wrong permission" do + let(:permissions) { %i[view_wikis] } + + it "returns no statuses" do + expect(subject.to_a).to be_empty + end + end + end + describe "#is_readonly" do let!(:status) { build(:status, is_readonly: true) } diff --git a/spec/models/type_spec.rb b/spec/models/type_spec.rb index 14e464cc7fc..583be22b8d9 100644 --- a/spec/models/type_spec.rb +++ b/spec/models/type_spec.rb @@ -48,7 +48,39 @@ RSpec.describe Type do end end - describe ".statuses" do + describe ".visible" do + subject { described_class.visible(user) } + + let!(:type) { create(:status) } + let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } + + before do + create(:member, user:, roles: [create(:project_role, permissions: permissions)]) + end + + it "returns the same types as all" do + expect(subject.to_a).to match_array(described_class.all.to_a) + end + + context "when the user has the manage_types permission in a project" do + let(:permissions) { %i[manage_types] } + + it "returns the same types as all" do + expect(subject.to_a).to match_array(described_class.all.to_a) + end + end + + context "when the user has the wrong permission" do + let(:permissions) { %i[view_wikis] } + + it "returns no types" do + expect(subject.to_a).to be_empty + end + end + end + + describe "#statuses" do subject { type.statuses } context "when new" do diff --git a/spec/requests/mcp/mcp_resources/status_list_spec.rb b/spec/requests/mcp/mcp_resources/status_list_spec.rb index 94ea2697b14..0afd144f2b0 100644 --- a/spec/requests/mcp/mcp_resources/status_list_spec.rb +++ b/spec/requests/mcp/mcp_resources/status_list_spec.rb @@ -39,7 +39,8 @@ RSpec.describe McpResources::StatusList, with_flag: { mcp_server: true } do end let(:access_token) { create(:oauth_access_token, scopes: "mcp", resource_owner: user) } - let(:user) { create(:admin) } # using an admin, to ensure visibility of everything + let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } let(:request_body) do { jsonrpc: "2.0", @@ -59,6 +60,7 @@ RSpec.describe McpResources::StatusList, with_flag: { mcp_server: true } do let(:resource_config) { create(:mcp_configuration, identifier: described_class.qualified_name) } before do + create(:member, user:, roles: [create(:project_role, permissions: permissions)]) server_config.save! resource_config.save! end @@ -78,6 +80,19 @@ RSpec.describe McpResources::StatusList, with_flag: { mcp_server: true } do it_behaves_like "MCP empty resource response" end + + context "when lacking permission to see statuses" do + let(:permissions) { [] } + + it_behaves_like "MCP text resource response" + + it "responds with an empty list" do + subject + text_content = parsed_results.fetch("contents").first + types_collection = JSON.parse(text_content.fetch("text")) + expect(types_collection.dig("_embedded", "elements")).to be_empty + end + end end context "when the mcp_server enterprise feature is disabled" do diff --git a/spec/requests/mcp/mcp_resources/status_spec.rb b/spec/requests/mcp/mcp_resources/status_spec.rb index 22661eb6f8f..b767f4738ca 100644 --- a/spec/requests/mcp/mcp_resources/status_spec.rb +++ b/spec/requests/mcp/mcp_resources/status_spec.rb @@ -39,7 +39,8 @@ RSpec.describe McpResources::Status, with_flag: { mcp_server: true } do end let(:access_token) { create(:oauth_access_token, scopes: "mcp", resource_owner: user) } - let(:user) { create(:admin) } # using an admin, to ensure visibility of everything + let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } let(:request_body) do { jsonrpc: "2.0", @@ -58,6 +59,7 @@ RSpec.describe McpResources::Status, with_flag: { mcp_server: true } do let(:resource_config) { create(:mcp_configuration, identifier: described_class.qualified_name) } before do + create(:member, user:, roles: [create(:project_role, permissions: permissions)]) server_config.save! resource_config.save! end @@ -83,6 +85,12 @@ RSpec.describe McpResources::Status, with_flag: { mcp_server: true } do it_behaves_like "MCP empty resource response" end + + context "when requesting a status not visible to the user" do + let(:permissions) { [] } + + it_behaves_like "MCP empty resource response" + end end context "when the mcp_server enterprise feature is disabled" do diff --git a/spec/requests/mcp/mcp_resources/type_list_spec.rb b/spec/requests/mcp/mcp_resources/type_list_spec.rb index 64daa452140..ae16318d49b 100644 --- a/spec/requests/mcp/mcp_resources/type_list_spec.rb +++ b/spec/requests/mcp/mcp_resources/type_list_spec.rb @@ -39,7 +39,8 @@ RSpec.describe McpResources::TypeList, with_flag: { mcp_server: true } do end let(:access_token) { create(:oauth_access_token, scopes: "mcp", resource_owner: user) } - let(:user) { create(:admin) } # using an admin, to ensure visibility of everything + let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } let(:request_body) do { jsonrpc: "2.0", @@ -59,6 +60,7 @@ RSpec.describe McpResources::TypeList, with_flag: { mcp_server: true } do let(:resource_config) { create(:mcp_configuration, identifier: described_class.qualified_name) } before do + create(:member, user:, roles: [create(:project_role, permissions: permissions)]) server_config.save! resource_config.save! end @@ -78,6 +80,19 @@ RSpec.describe McpResources::TypeList, with_flag: { mcp_server: true } do it_behaves_like "MCP empty resource response" end + + context "when lacking permission to see types" do + let(:permissions) { [] } + + it_behaves_like "MCP text resource response" + + it "responds with an empty list" do + subject + text_content = parsed_results.fetch("contents").first + types_collection = JSON.parse(text_content.fetch("text")) + expect(types_collection.dig("_embedded", "elements")).to be_empty + end + end end context "when the mcp_server enterprise feature is disabled" do diff --git a/spec/requests/mcp/mcp_resources/type_spec.rb b/spec/requests/mcp/mcp_resources/type_spec.rb index c99038c3cdd..01353854a6f 100644 --- a/spec/requests/mcp/mcp_resources/type_spec.rb +++ b/spec/requests/mcp/mcp_resources/type_spec.rb @@ -39,7 +39,8 @@ RSpec.describe McpResources::Type, with_flag: { mcp_server: true } do end let(:access_token) { create(:oauth_access_token, scopes: "mcp", resource_owner: user) } - let(:user) { create(:admin) } # using an admin, to ensure visibility of everything + let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } let(:request_body) do { jsonrpc: "2.0", @@ -58,6 +59,7 @@ RSpec.describe McpResources::Type, with_flag: { mcp_server: true } do let(:resource_config) { create(:mcp_configuration, identifier: described_class.qualified_name) } before do + create(:member, user:, roles: [create(:project_role, permissions: permissions)]) server_config.save! resource_config.save! end @@ -83,6 +85,12 @@ RSpec.describe McpResources::Type, with_flag: { mcp_server: true } do it_behaves_like "MCP empty resource response" end + + context "when requesting a type not visible to the user" do + let(:permissions) { [] } + + it_behaves_like "MCP empty resource response" + end end context "when the mcp_server enterprise feature is disabled" do diff --git a/spec/requests/mcp/mcp_tools/list_statuses_spec.rb b/spec/requests/mcp/mcp_tools/list_statuses_spec.rb index 182fb9fe7ff..e67faaac7a2 100644 --- a/spec/requests/mcp/mcp_tools/list_statuses_spec.rb +++ b/spec/requests/mcp/mcp_tools/list_statuses_spec.rb @@ -40,6 +40,7 @@ RSpec.describe McpTools::ListStatuses, with_flag: { mcp_server: true } do let(:access_token) { create(:oauth_access_token, scopes: "mcp", resource_owner: user) } let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } let(:request_body) do { jsonrpc: "2.0", @@ -61,6 +62,7 @@ RSpec.describe McpTools::ListStatuses, with_flag: { mcp_server: true } do let(:tool_config) { create(:mcp_configuration, identifier: described_class.qualified_name) } before do + create(:member, user:, roles: [create(:project_role, permissions: permissions)]) server_config.save! tool_config.save! end @@ -83,6 +85,15 @@ RSpec.describe McpTools::ListStatuses, with_flag: { mcp_server: true } do it_behaves_like "MCP error response" end + + context "when lacking permission to see statuses" do + let(:permissions) { [] } + + it "finds no statuses" do + subject + expect(parsed_results.dig("structuredContent", "count")).to eq(0) + end + end end context "when the mcp_server enterprise feature is disabled" do diff --git a/spec/requests/mcp/mcp_tools/list_types_spec.rb b/spec/requests/mcp/mcp_tools/list_types_spec.rb index d7fea3943b8..b74204aee9d 100644 --- a/spec/requests/mcp/mcp_tools/list_types_spec.rb +++ b/spec/requests/mcp/mcp_tools/list_types_spec.rb @@ -40,6 +40,7 @@ RSpec.describe McpTools::ListTypes, with_flag: { mcp_server: true } do let(:access_token) { create(:oauth_access_token, scopes: "mcp", resource_owner: user) } let(:user) { create(:user) } + let(:permissions) { %i[view_work_packages] } let(:request_body) do { jsonrpc: "2.0", @@ -61,6 +62,7 @@ RSpec.describe McpTools::ListTypes, with_flag: { mcp_server: true } do let(:tool_config) { create(:mcp_configuration, identifier: described_class.qualified_name) } before do + create(:member, project: create(:project, no_types: true), user:, roles: [create(:project_role, permissions: permissions)]) server_config.save! tool_config.save! end @@ -83,6 +85,15 @@ RSpec.describe McpTools::ListTypes, with_flag: { mcp_server: true } do it_behaves_like "MCP error response" end + + context "when lacking permission to see types" do + let(:permissions) { [] } + + it "finds no types" do + subject + expect(parsed_results.dig("structuredContent", "count")).to eq(0) + end + end end context "when the mcp_server enterprise feature is disabled" do