From 78833fa836167bbcbe6bcc9da3a69b7b5aa2db5a Mon Sep 17 00:00:00 2001 From: Pavel Balashou Date: Wed, 8 Apr 2026 14:20:19 +0200 Subject: [PATCH] [#73707] User does not meet password requirements during Jira Import. https://community.openproject.org/wp/73707 --- app/models/import/jira_import.rb | 90 +++++ app/models/import/jira_user.rb | 2 +- .../jira_fetch_and_import_projects_job.rb | 88 +---- spec/models/import/jira_import_spec.rb | 331 +++++++++++++++++- 4 files changed, 422 insertions(+), 89 deletions(-) diff --git a/app/models/import/jira_import.rb b/app/models/import/jira_import.rb index 438dbe6ccd7..10ce786c761 100644 --- a/app/models/import/jira_import.rb +++ b/app/models/import/jira_import.rb @@ -30,6 +30,8 @@ module Import class JiraImport < ApplicationRecord + include Import::JiraOpenProjectReferenceCreation + self.table_name = "jira_imports" belongs_to :jira, class_name: "Import::Jira" @@ -81,5 +83,93 @@ module Import Import::JiraUser.where(jira_import_id: id).destroy_all end # rubocop:enable Metrics/AbcSize + + def import_users + Import::JiraUser.where(jira_import_id: id).find_each do |jira_user| + import_user(jira_user) + end + end + + private + + # rubocop:disable Metrics/PerceivedComplexity + # rubocop:disable Metrics/AbcSize + def import_user(jira_user) + call = Users::CreateService + .new(user: User.system, contract_class: EmptyContract) + .call(jira_user.to_op_attributes) + + call.on_success do |_result| + create_reference!( + op_leg: call.result, + jira_leg: jira_user, + jira_import: self, + uses_existing: false + ) + end + call.on_failure do |_result| + if call.errors.find { |error| error.type == :taken }.present? + user = jira_user.try_to_find_existing_op_users.first + if user.present? + create_reference!( + op_leg: user, + jira_leg: jira_user, + jira_import: self, + uses_existing: true + ) + else + raise "Existing User is expected to be found, because there was an email " \ + "or login collision. See attributes: #{jira_user.to_op_attributes}" + end + else + raise call.message + end + end + + jira_user_groups = jira_user.payload["groups"]["items"].pluck("name") + + jira_user_groups.each do |group_name| + call = Groups::CreateService + .new(user: User.system, contract_class: EmptyContract) + .call(name: group_name) + call.on_success do |result| + group = result.result + create_reference!( + op_leg: group, + jira_leg: nil, + jira_import: self, + uses_existing: false + ) + end + call.on_failure do |_result| + if call.errors.find { |error| error.type == :taken }.present? + group = Group.where(name: group_name).first + if group.present? + create_reference!( + op_leg: group, + jira_leg: nil, + jira_import: self, + uses_existing: true + ) + else + raise "Existing Group is expected to be found. Group name: #{group_name}" + end + else + raise call.message + end + end + member_id = Import::JiraOpenProjectReference.where( + jira_import_id: id, + jira_entity_id: jira_user.id, + jira_entity_class: jira_user.class.to_s + ).pick(:op_entity_id) + group = Group.find_by!(name: group_name) + Groups::AddUsersService + .new(group, current_user: User.system) + .call(ids: [member_id], send_notifications: false) + end + end + # rubocop:enable Metrics/PerceivedComplexity + # rubocop:enable Metrics/AbcSize end end diff --git a/app/models/import/jira_user.rb b/app/models/import/jira_user.rb index da9d09f47a8..83a3f61dc98 100644 --- a/app/models/import/jira_user.rb +++ b/app/models/import/jira_user.rb @@ -43,7 +43,7 @@ module Import firstname, lastname = split_display_name(payload["displayName"]) { login: payload["name"], - password: SecureRandom.uuid, + password: OpenProject::Passwords::Generator.random_password, firstname:, lastname:, mail: payload["emailAddress"], diff --git a/app/workers/import/jira_fetch_and_import_projects_job.rb b/app/workers/import/jira_fetch_and_import_projects_job.rb index a93e2f39447..7d68aaf8c27 100644 --- a/app/workers/import/jira_fetch_and_import_projects_job.rb +++ b/app/workers/import/jira_fetch_and_import_projects_job.rb @@ -39,7 +39,7 @@ module Import fetch_and_save_users_data(jira_import) Journal::NotificationConfiguration.with(false) do - import_users(jira_import) + jira_import.import_users Import::JiraImportProjectsJob.perform_now(jira_import_id) end @@ -163,91 +163,5 @@ module Import raise "Error fetching user data for user key #{jira_user_key}: #{e.message}" end end - - def import_users(jira_import) - Import::JiraUser.where(jira_import_id: jira_import.id).find_each do |jira_user| - import_user(jira_user, jira_import) - end - end - - # rubocop:disable Metrics/PerceivedComplexity - # rubocop:disable Metrics/AbcSize - def import_user(jira_user, jira_import) - call = Users::CreateService - .new(user: User.system, contract_class: EmptyContract) - .call(jira_user.to_op_attributes) - - call.on_success do |_result| - create_reference!( - op_leg: call.result, - jira_leg: jira_user, - jira_import:, - uses_existing: false - ) - end - call.on_failure do |_result| - if call.errors.find { |error| error.type == :taken }.present? - user = jira_user.try_to_find_existing_op_users.first - if user.present? - create_reference!( - op_leg: user, - jira_leg: jira_user, - jira_import:, - uses_existing: true - ) - else - raise "Existing User is expected to be found, because there was an email " \ - "or login collision. See attributes: #{jira_user.to_op_attributes}" - end - else - raise call.message - end - end - - jira_user_groups = jira_user.payload["groups"]["items"].pluck("name") - - jira_user_groups.each do |group_name| - call = Groups::CreateService - .new(user: User.system, contract_class: EmptyContract) - .call(name: group_name) - call.on_success do |result| - group = result.result - create_reference!( - op_leg: group, - jira_leg: nil, - jira_import:, - uses_existing: false - ) - end - call.on_failure do |_result| - if call.errors.find { |error| error.type == :taken }.present? - group = Group.where(name: group_name).first - if group.present? - create_reference!( - op_leg: group, - jira_leg: nil, - jira_import:, - uses_existing: true - ) - else - raise "Existing Group is expected to be found. Group name: #{group_name}" - end - else - raise call.message - end - end - member_id = Import::JiraOpenProjectReference.where( - jira_import_id: jira_import.id, - jira_entity_id: jira_user.id, - jira_entity_class: jira_user.class.to_s - ).pick(:op_entity_id) - group = Group.find_by!(name: group_name) - Groups::AddUsersService - .new(group, current_user: User.system) - .call(ids: [member_id], send_notifications: false) - end - end - # rubocop:enable Metrics/PerceivedComplexity - # rubocop:enable Metrics/AbcSize end end diff --git a/spec/models/import/jira_import_spec.rb b/spec/models/import/jira_import_spec.rb index ebe66e48053..c0453639631 100644 --- a/spec/models/import/jira_import_spec.rb +++ b/spec/models/import/jira_import_spec.rb @@ -32,7 +32,8 @@ require "spec_helper" RSpec.describe Import::JiraImport do let(:jira) { create(:jira) } - let(:author) { create(:user) } + let(:author_password) { OpenProject::Passwords::Generator.random_password } + let(:author) { create(:user, password: author_password, password_confirmation: author_password) } subject(:jira_import) { create(:jira_import, jira:, author:) } @@ -132,4 +133,332 @@ RSpec.describe Import::JiraImport do expect(Import::JiraField.exists?(other_field.id)).to be true end end + + describe "#import_users", with_settings: { + password_active_rules: %w(lowercase uppercase numeric special), + password_min_length: 4, + password_min_adhered_rules: 4 + } do + def jira_user_payload(name:, display_name:, email:, groups: [], key: "JIRAUSER10000", active: true) + { + "key" => key, + "name" => name, + "self" => "https://jira-dc.openproject.org/rest/api/2/user?username=#{name}", + "active" => active, + "expand" => "groups,applicationRoles", + "groups" => { + "size" => groups.size, + "items" => groups.map { |g| { "name" => g, "self" => "https://jira-dc.openproject.org/rest/api/2/group?groupname=#{g}" } } + }, + "locale" => "en_US", + "deleted" => false, + "timeZone" => "Europe/Berlin", + "avatarUrls" => { + "16x16" => "https://www.gravatar.com/avatar/abc?d=mm&s=16", + "24x24" => "https://www.gravatar.com/avatar/abc?d=mm&s=24", + "32x32" => "https://www.gravatar.com/avatar/abc?d=mm&s=32", + "48x48" => "https://www.gravatar.com/avatar/abc?d=mm&s=48" + }, + "displayName" => display_name, + "emailAddress" => email, + "lastLoginTime" => "2026-03-26T08:49:31+0000", + "applicationRoles" => { "size" => 1, "items" => [] } + } + end + + let(:email) { "jdoe@example.com" } + let(:existing_user_password) { OpenProject::Passwords::Generator.random_password } + + # creates system user proactively. so, next coming User.count change cases don't count + # this one as created during #import_users call. + before { User.system } + + context "when importing a new user without groups" do + let!(:jira_user) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10100", + name: "jdoe@example.com", + display_name: "John Doe", + email:, + groups: [] + )) + end + + it "creates a new OpenProject user" do + expect { jira_import.import_users }.to change(User, :count).by(1) + end + + it "creates the user with correct attributes" do + jira_import.import_users + + user = User.find_by(login: email) + expect(user).to have_attributes( + firstname: "John", + lastname: "Doe", + mail: email, + status: "locked" + ) + end + + it "creates a reference between Jira user and OpenProject user" do + expect { jira_import.import_users }.to change(Import::JiraOpenProjectReference, :count).by(1) + + reference = Import::JiraOpenProjectReference.last + expect(reference).to have_attributes( + jira_entity_id: jira_user.id.to_s, + jira_entity_class: "Import::JiraUser", + op_entity_class: "User", + uses_existing: false + ) + end + end + + context "when importing a user that already exists by email" do + let!(:existing_user) do + create(:user, + mail: email, + password: existing_user_password, + password_confirmation: existing_user_password, + login: "login") + end + let!(:jira_user) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10101", + name: "jdoe@example.com", + display_name: "John Doe", + email:, + groups: [] + )) + end + + it "does not create a new user" do + expect { jira_import.import_users }.not_to change(User, :count) + end + + it "creates a reference to the existing user with uses_existing flag" do + jira_import.import_users + + reference = Import::JiraOpenProjectReference.find_by(jira_entity_id: jira_user.id) + expect(reference).to have_attributes( + jira_entity_id: jira_user.id.to_s, + jira_entity_class: "Import::JiraUser", + op_entity_id: existing_user.id.to_s, + op_entity_class: "User", + uses_existing: true + ) + end + end + + context "when importing a user that already exists by login" do + let(:login) { "login" } + let!(:existing_user) do + create(:user, + mail: "other@example.com", + password_confirmation: existing_user_password, + password: existing_user_password, + login:) + end + let!(:jira_user) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10102", + name: login, + display_name: "John Doe", + email:, + groups: [] + )) + end + + it "does not create a new user" do + expect { jira_import.import_users }.not_to change(User, :count) + end + + it "creates a reference to the existing user with uses_existing flag" do + jira_import.import_users + + reference = Import::JiraOpenProjectReference.find_by(jira_entity_id: jira_user.id) + expect(reference).to have_attributes( + jira_entity_id: jira_user.id.to_s, + jira_entity_class: "Import::JiraUser", + op_entity_id: existing_user.id.to_s, + op_entity_class: "User", + uses_existing: true + ) + end + end + + context "when importing a user with groups" do + let!(:jira_user) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10103", + name: "j.roth@openproject.com", + display_name: "Judith Roth", + email: "j.roth@openproject.com", + groups: ["jira-administrators", "jira-software-users"] + )) + end + + it "creates the groups" do + expect { jira_import.import_users }.to change(Group, :count).by(2) + + expect(Group.exists?(name: "jira-administrators")).to be true + expect(Group.exists?(name: "jira-software-users")).to be true + end + + it "adds the user to the groups" do + jira_import.import_users + + user = User.find_by(login: "j.roth@openproject.com") + expect(user.groups.pluck(:name)).to contain_exactly("jira-administrators", "jira-software-users") + end + + it "creates references for the groups" do + jira_import.import_users + + group_references = Import::JiraOpenProjectReference.where(op_entity_class: "Group") + expect(group_references.count).to eq(2) + expect(group_references.pluck(:uses_existing)).to all(be false) + end + end + + context "when importing a user with an existing group" do + let!(:existing_group) { create(:group, name: "jira-administrators") } + let!(:jira_user) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10104", + name: "j.roth@openproject.com", + display_name: "Judith Roth", + email: "j.roth@openproject.com", + groups: ["jira-administrators"] + )) + end + + it "does not create a duplicate group" do + expect { jira_import.import_users }.not_to change(Group, :count) + end + + it "adds the user to the existing group" do + jira_import.import_users + + user = User.find_by(login: "j.roth@openproject.com") + expect(user.groups).to include(existing_group) + end + + it "creates a reference with uses_existing flag for the group" do + jira_import.import_users + + group_reference = Import::JiraOpenProjectReference.find_by( + op_entity_class: "Group", + op_entity_id: existing_group.id + ) + expect(group_reference.uses_existing).to be true + end + end + + context "when importing multiple users" do + let!(:jira_user1) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10105", + name: "jdoe@example.com", + display_name: "John Doe", + email: "jdoe@example.com", + groups: ["jira-software-users"] + )) + end + let!(:jira_user2) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10106", + name: "jsmith@example.com", + display_name: "Jane Smith", + email: "jsmith@example.com", + groups: ["jira-software-users"] + )) + end + + it "creates all users" do + expect { jira_import.import_users }.to change(User, :count).by(2) + end + + it "creates the shared group only once" do + expect { jira_import.import_users }.to change(Group, :count).by(1) + end + + it "adds both users to the shared group" do + jira_import.import_users + + group = Group.find_by(name: "jira-software-users") + expect(group.users.pluck(:login)).to contain_exactly("jdoe@example.com", "jsmith@example.com") + end + end + + context "when user has a single-word display name" do + let!(:jira_user) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10108", + name: "admin@example.com", + display_name: "Administrator", + email: "admin@example.com", + groups: [] + )) + end + + it "uses the name for both firstname and lastname" do + jira_import.import_users + + user = User.find_by(login: "admin@example.com") + expect(user).to have_attributes( + firstname: "Administrator", + lastname: "Administrator" + ) + end + end + + context "when user has a multi-part display name" do + let!(:jira_user) do + create(:jira_user, + jira:, + jira_import:, + payload: jira_user_payload( + key: "JIRAUSER10109", + name: "jvd@example.com", + display_name: "Jean Van Der Berg", + email: "jvd@example.com", + groups: [] + )) + end + + it "uses all but last word as firstname and last word as lastname" do + jira_import.import_users + + user = User.find_by(login: "jvd@example.com") + expect(user).to have_attributes( + firstname: "Jean Van Der", + lastname: "Berg" + ) + end + end + end end