From e02e72f52d00f6acefaeee63ee98725c9239b73c Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Wed, 2 Jul 2025 15:52:36 +0200 Subject: [PATCH] Add enterprise checks for SCIM API Including a banner for the UI (currently using a placeholder image) and a check for SCIM API calls. While it's theoretically possible to setup a client without an enterprise token (new/edit/create/update are not guarded), there is no click path to create a client and eventually the API requests would be blocked. --- Gemfile | 2 +- Gemfile.lock | 6 +++--- .../scim_v2/scim_controller_mixins.rb | 11 ++++++++-- app/views/admin/scim_clients/index.html.erb | 9 +++++++-- config/initializers/feature_decisions.rb | 3 ++- config/initializers/menus.rb | 3 ++- config/locales/en.yml | 4 ++++ .../admin/scim_clients/create_spec.rb | 6 +++--- .../features/admin/scim_clients/index_spec.rb | 18 +++++++++++++++-- .../admin/scim_clients/update_spec.rb | 6 +++--- spec/requests/scim_v2/authentication_spec.rb | 20 +++++++++++++++++-- spec/requests/scim_v2/groups_spec.rb | 2 +- spec/requests/scim_v2/schemas_spec.rb | 2 +- .../scim_v2/service_provider_config_spec.rb | 2 +- spec/requests/scim_v2/users_spec.rb | 2 +- 15 files changed, 72 insertions(+), 24 deletions(-) diff --git a/Gemfile b/Gemfile index 51912bf05a4..de1d063606d 100644 --- a/Gemfile +++ b/Gemfile @@ -203,7 +203,7 @@ gem "aws-sdk-core", "~> 3.107" # File upload via fog + screenshots on travis gem "aws-sdk-s3", "~> 1.91" -gem "openproject-token", "~> 7.2.0" +gem "openproject-token", "~> 7.3.0" gem "plaintext", "~> 0.3.2" diff --git a/Gemfile.lock b/Gemfile.lock index 95522f7485c..7ca8bb789f9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -866,7 +866,7 @@ GEM activesupport (>= 7.1.0) openproject-octicons (>= 19.25.0) view_component (>= 3.1, < 4.0) - openproject-token (7.2.2) + openproject-token (7.3.0) activemodel openssl (3.3.0) openssl-signature_algorithm (1.3.0) @@ -1458,7 +1458,7 @@ DEPENDENCIES openproject-reporting! openproject-storages! openproject-team_planner! - openproject-token (~> 7.2.0) + openproject-token (~> 7.3.0) openproject-two_factor_authentication! openproject-webhooks! openproject-xls_export! @@ -1832,7 +1832,7 @@ CHECKSUMS openproject-reporting (1.0.0) openproject-storages (1.0.0) openproject-team_planner (1.0.0) - openproject-token (7.2.2) sha256=16062c6206d46c28924f273d4fb708022830c8bbe961c604c917b386a52e8740 + openproject-token (7.3.0) sha256=20d004e21b9b3f76c524210c6ece6fe274ec40dbb8c0de8683fd9ce41278cac9 openproject-two_factor_authentication (1.0.0) openproject-webhooks (1.0.0) openproject-xls_export (1.0.0) diff --git a/app/controllers/scim_v2/scim_controller_mixins.rb b/app/controllers/scim_v2/scim_controller_mixins.rb index d44942eaf02..274ce52e7c7 100644 --- a/app/controllers/scim_v2/scim_controller_mixins.rb +++ b/app/controllers/scim_v2/scim_controller_mixins.rb @@ -37,9 +37,10 @@ module ScimV2 module Overwrites # Completely overwriting authenticate method of Scimitar def authenticate - return handle_scim_error(Scimitar::AuthenticationError.new) unless OpenProject::FeatureDecisions.scim_api_active? + if !EnterpriseToken.allows_to?(:scim_api) || !OpenProject::FeatureDecisions.scim_api_active? + return handle_scim_error(Scimitar::AuthenticationError.new) + end - warden = request.env["warden"] User.current = warden.authenticate!(scope: :scim_v2) # Only a ServiceAccount associated with a ScimClient can use SCIM Server API @@ -47,6 +48,12 @@ module ScimV2 handle_scim_error(Scimitar::AuthenticationError.new) end end + + private + + def warden + request.env["warden"] + end end end end diff --git a/app/views/admin/scim_clients/index.html.erb b/app/views/admin/scim_clients/index.html.erb index e294e1766f2..6aac8cc9ad1 100644 --- a/app/views/admin/scim_clients/index.html.erb +++ b/app/views/admin/scim_clients/index.html.erb @@ -51,5 +51,10 @@ See COPYRIGHT and LICENSE files for more details. end %> - -<%= render(Admin::ScimClients::TableComponent.new(rows: @scim_clients)) %> +<%= with_enterprise_banner_guard( + :scim_api, + variant: :medium, + image: "enterprise/automatically-generated-subjects.png" # TODO: exchange for correct image +) do + render(Admin::ScimClients::TableComponent.new(rows: @scim_clients)) +end %> diff --git a/config/initializers/feature_decisions.rb b/config/initializers/feature_decisions.rb index ec9259afa9c..0ed017623b8 100644 --- a/config/initializers/feature_decisions.rb +++ b/config/initializers/feature_decisions.rb @@ -48,7 +48,8 @@ OpenProject::FeatureDecisions.add :calculated_value_project_attribute, description: "Allows the use of calculated values as a project attribute." OpenProject::FeatureDecisions.add :scim_api, - description: "Enables SCIM API." + description: "Enables SCIM API.", + force_active: true OpenProject::FeatureDecisions.add :block_note_editor, description: "Enables the block note editor for rich text fields where available." diff --git a/config/initializers/menus.rb b/config/initializers/menus.rb index e075b44873b..ebf39bc9343 100644 --- a/config/initializers/menus.rb +++ b/config/initializers/menus.rb @@ -528,7 +528,8 @@ Redmine::MenuManager.map :admin_menu do |menu| { controller: "/admin/scim_clients", action: "index" }, if: ->(_) { User.current.admin? && OpenProject::FeatureDecisions.scim_api_active? }, parent: :authentication, - caption: ScimClient.model_name.human(count: 2) + caption: ScimClient.model_name.human(count: 2), + enterprise_feature: "scim_api" menu.push :announcements, { controller: "/announcements", action: "edit" }, diff --git a/config/locales/en.yml b/config/locales/en.yml index 688ae4b4331..68b5971eb03 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2211,6 +2211,7 @@ en: placeholder_users: Placeholder Users project_list_sharing: Project List Sharing readonly_work_packages: Readonly Work Packages + scim_api: SCIM server API sso_auth_providers: Single sign-on team_planner_view: Team Planner View virus_scanning: Antivirus Scanning @@ -2279,6 +2280,9 @@ en: nextcloud_sso: title: "Single Sign-On for Nextcloud Storage" description: "Enable seamless and secure authentication for your Nextcloud storage with Single Sign-On. Simplify access management and enhance user convenience." + scim_api: + title: "SCIM clients" + description: "Automate user management in OpenProject by seamlessly integrating external identity services like Microsoft Entra or Keycloak through our SCIM Server API. Available starting with the Enterprise corporate plan." virus_scanning: description: "Ensure uploaded files in OpenProject are scanned for viruses before being accessible by other users." placeholder_users: diff --git a/spec/features/admin/scim_clients/create_spec.rb b/spec/features/admin/scim_clients/create_spec.rb index 234b28d58d2..44326b8585a 100644 --- a/spec/features/admin/scim_clients/create_spec.rb +++ b/spec/features/admin/scim_clients/create_spec.rb @@ -36,7 +36,7 @@ RSpec.describe "Creating a SCIM client", :js, :selenium, driver: :firefox_de do current_user { admin } - it "can create a SCIM client authenticating through JWT", :aggregate_failures do + it "can create a SCIM client authenticating through JWT", :aggregate_failures, with_ee: [:scim_api] do visit new_admin_scim_client_path expect(page).to be_axe_clean.within("#content") @@ -64,7 +64,7 @@ RSpec.describe "Creating a SCIM client", :js, :selenium, driver: :firefox_de do expect(created_client.auth_provider_link&.external_id).to eq("123-abc-456-def") end - it "can create a SCIM client authenticating through client credentials" do + it "can create a SCIM client authenticating through client credentials", with_ee: [:scim_api] do visit new_admin_scim_client_path fill_in "Name", with: "My SCIM Client" @@ -83,7 +83,7 @@ RSpec.describe "Creating a SCIM client", :js, :selenium, driver: :firefox_de do end end - it "can create a SCIM client authenticating through a static access token" do + it "can create a SCIM client authenticating through a static access token", with_ee: [:scim_api] do visit new_admin_scim_client_path fill_in "Name", with: "My SCIM Client" diff --git a/spec/features/admin/scim_clients/index_spec.rb b/spec/features/admin/scim_clients/index_spec.rb index a0382ea4259..82fb3f8278d 100644 --- a/spec/features/admin/scim_clients/index_spec.rb +++ b/spec/features/admin/scim_clients/index_spec.rb @@ -35,12 +35,25 @@ RSpec.describe "Listing SCIM clients", :js, :selenium, driver: :firefox_de do current_user { admin } - context "when there are no SCIM clients" do + context "when using an insufficient enterprise token" do + it "renders an enterprise banner and no table" do + visit admin_scim_clients_path + + expect(page).to be_axe_clean.within("#content") + .skipping("color-contrast") # https://community.openproject.org/wp/65507 + + expect(page).to have_enterprise_banner(:corporate) + expect(page).to have_no_test_selector("Admin::ScimClients::TableComponent") + end + end + + context "when there are no SCIM clients", with_ee: [:scim_api] do it "renders a proper blank slate" do visit admin_scim_clients_path expect(page).to be_axe_clean.within "#content" + expect(page).not_to have_enterprise_banner within_test_selector("Admin::ScimClients::TableComponent") do expect(page).to have_content("No SCIM clients configured yet") expect(page).to have_content("Add clients to see them here") @@ -54,7 +67,7 @@ RSpec.describe "Listing SCIM clients", :js, :selenium, driver: :firefox_de do end end - context "when there are SCIM clients" do + context "when there are SCIM clients", with_ee: [:scim_api] do let!(:sso_client) { create(:scim_client) } it "renders a proper clients table" do @@ -62,6 +75,7 @@ RSpec.describe "Listing SCIM clients", :js, :selenium, driver: :firefox_de do expect(page).to be_axe_clean.within "#content" + expect(page).not_to have_enterprise_banner within_test_selector("Admin::ScimClients::TableComponent") do within(".name") { expect(page).to have_content(sso_client.name) } within(".authentication_method") { expect(page).to have_content("JWT from identity provider") } diff --git a/spec/features/admin/scim_clients/update_spec.rb b/spec/features/admin/scim_clients/update_spec.rb index 83b035f6f9b..b7d5f1004e9 100644 --- a/spec/features/admin/scim_clients/update_spec.rb +++ b/spec/features/admin/scim_clients/update_spec.rb @@ -48,7 +48,7 @@ RSpec.describe "Updating a SCIM client", :js, :selenium, driver: :firefox_de do current_user { admin } - it "can update a SCIM client authenticating through JWT", :aggregate_failures do + it "can update a SCIM client authenticating through JWT", :aggregate_failures, with_ee: [:scim_api] do visit edit_admin_scim_client_path(sso_scim_client) expect(page).to be_axe_clean.within("#content") .skipping("link-in-text-block") # https://community.openproject.org/wp/65252 @@ -78,7 +78,7 @@ RSpec.describe "Updating a SCIM client", :js, :selenium, driver: :firefox_de do expect(ScimClient.where(id: sso_scim_client.id)).to be_empty end - it "can update a SCIM client authenticating through client credentials", :aggregate_failures do + it "can update a SCIM client authenticating through client credentials", :aggregate_failures, with_ee: [:scim_api] do visit edit_admin_scim_client_path(oauth_client_scim_client) expect(page).to be_axe_clean.within("#content") .skipping("link-in-text-block") # https://community.openproject.org/wp/65252 @@ -108,7 +108,7 @@ RSpec.describe "Updating a SCIM client", :js, :selenium, driver: :firefox_de do expect(ScimClient.where(id: oauth_client_scim_client.id)).to be_empty end - it "can update a SCIM client authenticating through a static access token", :aggregate_failures do + it "can update a SCIM client authenticating through a static access token", :aggregate_failures, with_ee: [:scim_api] do visit edit_admin_scim_client_path(token_scim_client) expect(page).to be_axe_clean.within("#content") .skipping("link-in-text-block") # https://community.openproject.org/wp/65252 diff --git a/spec/requests/scim_v2/authentication_spec.rb b/spec/requests/scim_v2/authentication_spec.rb index fe45d51854b..6592a02334f 100644 --- a/spec/requests/scim_v2/authentication_spec.rb +++ b/spec/requests/scim_v2/authentication_spec.rb @@ -38,7 +38,7 @@ RSpec.describe "SCIM API Authentication" do let(:scim_client) { create(:scim_client, authentication_method: :oauth2_token, auth_provider_id: oidc_provider.id) } describe "GET /scim_v2/ServiceProviderConfig" do - context "with the feature flag enabled", with_flag: { scim_api: true } do + context "with the feature flag and enterprise enabled", with_ee: [:scim_api], with_flag: { scim_api: true } do context "with static token" do let(:oauth_access_token) { create(:oauth_access_token, resource_owner: service_account, scopes: ["scim_v2"]) } let!(:token) { oauth_access_token.plaintext_token } @@ -148,7 +148,23 @@ RSpec.describe "SCIM API Authentication" do end end - context "with the feature flag disabled", with_flag: { scim_api: false } do + context "with the feature flag disabled", with_ee: [:scim_api], with_flag: { scim_api: false } do + let(:oauth_access_token) { create(:oauth_access_token, resource_owner: service_account, scopes: ["scim_v2"]) } + let!(:token) { oauth_access_token.plaintext_token } + + it do + get "/scim_v2/ServiceProviderConfig", {}, headers + + response_body = JSON.parse(last_response.body) + expect(response_body).to eq( + { "detail" => "Requires authentication", "schemas" => ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status" => "401" } + ) + expect(last_response).to have_http_status(401) + end + end + + context "with the enterprise feature missing", with_flag: { scim_api: true } do let(:oauth_access_token) { create(:oauth_access_token, resource_owner: service_account, scopes: ["scim_v2"]) } let!(:token) { oauth_access_token.plaintext_token } diff --git a/spec/requests/scim_v2/groups_spec.rb b/spec/requests/scim_v2/groups_spec.rb index 7f48ca00f37..88703e8f78d 100644 --- a/spec/requests/scim_v2/groups_spec.rb +++ b/spec/requests/scim_v2/groups_spec.rb @@ -30,7 +30,7 @@ require "spec_helper" -RSpec.describe "SCIM API Groups" do +RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do let(:external_user_id) { "idp_user_id_123asdqwe12345" } let(:external_group_id) { "idp_group_id_123asdqwe12345" } let(:external_admin_id) { "idp_admin_id_123asdqwe12345" } diff --git a/spec/requests/scim_v2/schemas_spec.rb b/spec/requests/scim_v2/schemas_spec.rb index 3d37867a7ba..f2b205176a6 100644 --- a/spec/requests/scim_v2/schemas_spec.rb +++ b/spec/requests/scim_v2/schemas_spec.rb @@ -30,7 +30,7 @@ require "spec_helper" -RSpec.describe "SCIM API Schemas" do +RSpec.describe "SCIM API Schemas", with_ee: [:scim_api] do let(:oidc_provider_slug) { "keycloak" } let(:oidc_provider) { create(:oidc_provider, slug: oidc_provider_slug) } let(:headers) { { "CONTENT_TYPE" => "application/scim+json", "HTTP_AUTHORIZATION" => "Bearer #{token.plaintext_token}" } } diff --git a/spec/requests/scim_v2/service_provider_config_spec.rb b/spec/requests/scim_v2/service_provider_config_spec.rb index 3f61ea29cbe..ccdc54439cb 100644 --- a/spec/requests/scim_v2/service_provider_config_spec.rb +++ b/spec/requests/scim_v2/service_provider_config_spec.rb @@ -30,7 +30,7 @@ require "spec_helper" -RSpec.describe "SCIM API ServiceProviderConfig" do +RSpec.describe "SCIM API ServiceProviderConfig", with_ee: [:scim_api] do let(:oidc_provider_slug) { "keycloak" } let(:oidc_provider) { create(:oidc_provider, slug: oidc_provider_slug) } let(:headers) { { "CONTENT_TYPE" => "application/scim+json", "HTTP_AUTHORIZATION" => "Bearer #{token.plaintext_token}" } } diff --git a/spec/requests/scim_v2/users_spec.rb b/spec/requests/scim_v2/users_spec.rb index 3ea25b14b4f..821ae06a175 100644 --- a/spec/requests/scim_v2/users_spec.rb +++ b/spec/requests/scim_v2/users_spec.rb @@ -30,7 +30,7 @@ require "spec_helper" -RSpec.describe "SCIM API Users" do +RSpec.describe "SCIM API Users", with_ee: [:scim_api] do let(:external_user_id) { "idp_user_id_123asdqwe12345" } let(:external_group_id) { "idp_group_id_123asdqwe12345" } let(:external_admin_id) { "idp_admin_id_123asdqwe12345" }