From 601b054e044ce436338408d09e5766f91dd467e8 Mon Sep 17 00:00:00 2001 From: Pavel Balashou Date: Thu, 3 Jul 2025 02:44:51 +0200 Subject: [PATCH] [#62107] React on PR comments volume 2. - Fix some Rubocop complains. - Does not set user.firstname and user.lastname to stupid values. To avoid confusion. - Add inverse_of option to user_auth_provider_links -- provider association. - Extract ScimitarSchemaExtension module to a dedicated file. - Fix users/delete_service_spec.rb --- app/controllers/scim_v2/users_controller.rb | 2 -- app/models/principal.rb | 19 +++------- app/models/user.rb | 20 +++++++++-- app/models/user_auth_provider_link.rb | 2 +- config/initializers/scimitar.rb | 9 ----- lib/scimitar_schema_extension.rb | 38 ++++++++++++++++++++ spec/requests/scim_v2/authentication_spec.rb | 9 ++--- spec/requests/scim_v2/groups_spec.rb | 3 +- spec/requests/scim_v2/schemas_spec.rb | 2 +- spec/requests/scim_v2/users_spec.rb | 10 +++--- spec/services/users/delete_service_spec.rb | 4 +-- 11 files changed, 76 insertions(+), 42 deletions(-) create mode 100644 lib/scimitar_schema_extension.rb diff --git a/app/controllers/scim_v2/users_controller.rb b/app/controllers/scim_v2/users_controller.rb index e211be004b1..aaeb30ebb59 100644 --- a/app/controllers/scim_v2/users_controller.rb +++ b/app/controllers/scim_v2/users_controller.rb @@ -37,8 +37,6 @@ module ScimV2 storage_class.transaction do user = storage_class.new user.from_scim!(scim_hash: scim_resource.as_json) - user.firstname = "123" if user.firstname.blank? - user.lastname = "456" if user.lastname.blank? call = Users::CreateService .new(user: User.current, model: user) .call(user.attributes) diff --git a/app/models/principal.rb b/app/models/principal.rb index 5e10900f000..88dce60da38 100644 --- a/app/models/principal.rb +++ b/app/models/principal.rb @@ -67,7 +67,10 @@ class Principal < ApplicationRecord foreign_key: "user_id" has_many :projects, through: :memberships has_many :categories, foreign_key: "assigned_to_id", dependent: :nullify, inverse_of: :assigned_to - has_many :user_auth_provider_links, dependent: :destroy, foreign_key: :user_id + has_many :user_auth_provider_links, + dependent: :destroy, + foreign_key: :user_id, + inverse_of: :principal has_many :auth_providers, through: :user_auth_provider_links has_paper_trail @@ -210,20 +213,6 @@ class Principal < ApplicationRecord end end - def scim_active=(is_active) - if is_active - activate - true - else - lock if active? - false - end - end - - def scim_active - active? - end - def scim_external_id active_user_auth_provider_link&.external_id end diff --git a/app/models/user.rb b/app/models/user.rb index 1bfcd5b80fd..f3595c16f78 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -278,7 +278,7 @@ class User < Principal def self.try_to_autologin(key) token = Token::AutoLogin.find_by_plaintext_value(key) # rubocop:disable Rails/DynamicFindBy # Make sure there's only 1 token that matches the key - if token && ((token.created_at > Setting.autologin.to_i.day.ago) && token.user&.active?) + if token && (token.created_at > Setting.autologin.to_i.day.ago) && token.user&.active? token.user end end @@ -565,11 +565,27 @@ class User < Principal def scim_emails=(emails) email = (emails.find { |email| email.primary == true }) || (emails.find { |email| email.type == "work" }) || - emails.sort.first + emails.min self.mail = email&.value end + # rubocop:disable Naming/PredicateMethod + def scim_active=(is_active) + if is_active + activate + true + else + lock if active? + false + end + end + + def scim_active + active? + end + # rubocop:enable Naming/PredicateMethod + def self.scim_resource_type Scimitar::Resources::User end diff --git a/app/models/user_auth_provider_link.rb b/app/models/user_auth_provider_link.rb index de7925b22d7..f3fdc331c9e 100644 --- a/app/models/user_auth_provider_link.rb +++ b/app/models/user_auth_provider_link.rb @@ -29,7 +29,7 @@ #++ class UserAuthProviderLink < ApplicationRecord - belongs_to :principal, foreign_key: :user_id + belongs_to :principal, foreign_key: :user_id, inverse_of: :user_auth_provider_links belongs_to :auth_provider scope :with_identity_url, ->(identity_url) do diff --git a/config/initializers/scimitar.rb b/config/initializers/scimitar.rb index a50550655fb..266285e1c8b 100644 --- a/config/initializers/scimitar.rb +++ b/config/initializers/scimitar.rb @@ -37,15 +37,6 @@ Rails.application.config.to_prepare do application_controller_mixin: ScimV2::ScimControllerMixins ) - module ScimitarSchemaExtension - def scim_attributes - super + [Scimitar::Schema::Attribute.new(name: "externalId", - type: "string", - caseExact: true, - required: true)] - end - end - Scimitar::Schema::User.singleton_class.class_eval do prepend ScimitarSchemaExtension end diff --git a/lib/scimitar_schema_extension.rb b/lib/scimitar_schema_extension.rb new file mode 100644 index 00000000000..c94019ab766 --- /dev/null +++ b/lib/scimitar_schema_extension.rb @@ -0,0 +1,38 @@ +# 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. +#++ + +module ScimitarSchemaExtension + def scim_attributes + super + [Scimitar::Schema::Attribute.new(name: "externalId", + type: "string", + caseExact: true, + required: true)] + end +end diff --git a/spec/requests/scim_v2/authentication_spec.rb b/spec/requests/scim_v2/authentication_spec.rb index d765b3b3a85..fe45d51854b 100644 --- a/spec/requests/scim_v2/authentication_spec.rb +++ b/spec/requests/scim_v2/authentication_spec.rb @@ -117,7 +117,7 @@ RSpec.describe "SCIM API Authentication" do header "Authorization", "Bearer #{token}" end - it "" do + it do get "/scim_v2/ServiceProviderConfig", {}, headers expect(last_response).to have_http_status(200) @@ -126,18 +126,18 @@ RSpec.describe "SCIM API Authentication" do context "when scim_v2 scope is missing in token" do let(:token_scope) { "api_v3" } - it "" do + it do get "/scim_v2/ServiceProviderConfig", {}, headers - expect(last_response).to have_http_status(401) expect(last_response.body).to eq("insufficient_scope") expect(last_response.headers["WWW-Authenticate"]).to eq("Bearer realm=\"OpenProject API\", error=\"insufficient_scope\", error_description=\"Requires scope scim_v2 to access this resource.\"") + expect(last_response).to have_http_status(401) end end context "when token_sub does not match a service_account" do before { service_account.user_auth_provider_links.delete_all } - it "" do + it do get "/scim_v2/ServiceProviderConfig", {}, headers expect(last_response).to have_http_status(401) @@ -160,6 +160,7 @@ RSpec.describe "SCIM API Authentication" do { "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 end diff --git a/spec/requests/scim_v2/groups_spec.rb b/spec/requests/scim_v2/groups_spec.rb index f34015b096f..7f48ca00f37 100644 --- a/spec/requests/scim_v2/groups_spec.rb +++ b/spec/requests/scim_v2/groups_spec.rb @@ -304,8 +304,7 @@ RSpec.describe "SCIM API Groups" do "members" => [ { "value" => user.id.to_s }, { "value" => admin.id.to_s } - ], - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"] + ] } put "/scim_v2/Groups/#{group.id}", request_body.to_json, headers diff --git a/spec/requests/scim_v2/schemas_spec.rb b/spec/requests/scim_v2/schemas_spec.rb index fd9bbe27afd..618e05d7b0f 100644 --- a/spec/requests/scim_v2/schemas_spec.rb +++ b/spec/requests/scim_v2/schemas_spec.rb @@ -42,7 +42,7 @@ RSpec.describe "SCIM API Schemas" do describe "GET /scim_v2/Schemas" do context "with the feature flag enabled", with_flag: { scim_api: true } do - it do + it "responds with supported schemas " do get "/scim_v2/Schemas", {}, headers response_body = JSON.parse(last_response.body) diff --git a/spec/requests/scim_v2/users_spec.rb b/spec/requests/scim_v2/users_spec.rb index 87fac55e617..3ea25b14b4f 100644 --- a/spec/requests/scim_v2/users_spec.rb +++ b/spec/requests/scim_v2/users_spec.rb @@ -246,10 +246,12 @@ RSpec.describe "SCIM API Users" do expect(last_response).to have_http_status(409) response_body = JSON.parse(last_response.body) - expect(response_body).to eq({ "schemas" => ["urn:ietf:params:scim:api:messages:2.0:Error"], - "detail" => "Operation failed due to a uniqueness constraint: Username has already been taken.", - "status" => "409", - "scimType" => "uniqueness" }) + expect(response_body).to eq( + { "schemas" => ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail" => "Operation failed due to a uniqueness constraint: Username has already been taken.", + "status" => "409", + "scimType" => "uniqueness" } + ) end end diff --git a/spec/services/users/delete_service_spec.rb b/spec/services/users/delete_service_spec.rb index 9a05b189bdb..2b566cdd2d4 100644 --- a/spec/services/users/delete_service_spec.rb +++ b/spec/services/users/delete_service_spec.rb @@ -70,7 +70,7 @@ RSpec.describe Users::DeleteService, type: :model do allow(actor).to receive(:admin?).and_return false end - it_behaves_like "deletes the user" + it_behaves_like "does not delete the user" end context "with privileged system user" do @@ -90,7 +90,7 @@ RSpec.describe Users::DeleteService, type: :model do context "with system user" do let(:actor) { User.system } - it_behaves_like "deletes the user" + it_behaves_like "does not delete the user" end end end