[#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

@@ -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)

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

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

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

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

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

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

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

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)

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
 

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