Skip to content

[OP-19185] Extend LDAP sync to build org structure (department sync)#23848

Merged
klaustopher merged 41 commits into
devfrom
OP-19185-department-sync
Jun 23, 2026
Merged

[OP-19185] Extend LDAP sync to build org structure (department sync)#23848
klaustopher merged 41 commits into
devfrom
OP-19185-department-sync

Conversation

@klaustopher

Copy link
Copy Markdown
Contributor

Implements OP-19185.

What this does

Adds LDAP organizational unit → Department synchronization, analogous to the existing flat LDAP group sync. Given a configured base DN, OpenProject walks the OU subtree beneath it, mirrors it into the Department hierarchy, and assigns each user to the Department of the OU it resides in (derived from the user's DN). This realizes the ticket's two acceptance criteria — sync can produce departments (alongside the existing groups path) and the result is nested.

CN=John Doe,OU=Frontend,OU=Development,OU=IT,DC=openproject,DC=com
            └─ John lands in the "Frontend" department (Frontend ⊂ Development ⊂ IT)

New module: modules/ldap_departments

Mirrors modules/ldap_groups. Enterprise-gated behind the existing :ldap_groups feature; admin UI additionally behind the departments_active? runtime flag.

  • ModelsSynchronizedTree (base-DN config: base DN, OU structure filter, OU-name attribute, optional stable-ID attribute, optional user filter, sync_users), SynchronizedDepartment (OU↔Department mapping), Membership (sync-managed user↔department).
  • Services
    • SynchronizeTreeService: discovers OUs, builds the Department tree parents-first, matches by stable GUID with DN fallback (so OU renames/moves don't orphan), prunes vanished OUs (keeps the Department, drops the mapping).
    • SynchronizeMembersService: places users by parsing each entry's parent container DN, LDAP-authoritative single-department moves, removes memberships the sync no longer sees, optional user creation.
    • SynchronizationService orchestrator + SynchronizationJob (cron */30, separate from group sync).
  • Admin UISynchronizedTreesController (CRUD + on-demand synchronize), SynchronizedDepartmentsController (unmanage a mapping), views, i18n.
  • Configldap_departments_disable_sync_job setting; module registered in Gemfile.modules.

Core changes (main app)

  • Sibling-scoped Department name uniqueness — migration narrows the users[lastname,type] unique index to placeholder users; Group#uniqueness_of_name is now OU-aware (global for regular groups, per-parent for organizational units). Real OU trees repeat names (Support under both IT and HR), which the previous global constraint forbade.
  • Read-only lock on LDAP-managed departmentsGroup#ldap_managed? (a registration hook so core stays module-agnostic) gates Groups::BaseContract/DeleteContract; the sync uses the permissive Groups::SyncUpdateContract. The admin UI hides rename/re-parent/add/remove/delete actions and shows a "Managed by LDAP synchronization" badge. Departments become editable again only once the OU disappears and the mapping is dropped.

Tests & dev tooling

  • Unit specs (stubbed LDAP, fast/deterministic): tree building, duplicate names, removal, GUID rename, member placement/move/removal/creation, model validations, contract + UI locking, request flow.
  • Real-LDAP integration spec via Ladle/ApacheDS — full end-to-end sync against a live server, including real subtree scoping.
  • Extended spec/fixtures/ldap/users.ldif with a nested OU tree (ou=org) + users — additive, existing ldap_groups specs still pass.
  • ldap_departments:development:ldap_server rake task boots a dev LDAP server, configures a tree, runs the sync, and prints the resulting structure. The shared fixture means the existing ldap_groups:development:ldap_server now also exposes the OUs.

Decisions worth reviewer attention

  • Removed OU / removed config → keep the Department, only drop the mapping + sync-managed memberships (never cascade-delete a department sub-tree on a transient LDAP hiccup).
  • Multiple, non-overlapping trees per LDAP connection (overlap rejected by validation).
  • The core uniqueness relaxation and read-only locking touch the shared Departments feature — coordinate with the Departments/Sirius owners.
  • Membership currently goes per-user through the services (correctness first); revisit batching if very large OUs prove slow.

Manual verification

  1. ./bin/rails ldap_departments:development:ldap_server
  2. Administration → Authentication → LDAP department synchronization → confirm the tree + Synchronize
  3. Administration → Departments → verify the hierarchy, user placement, and that managed departments are read-only.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
1 Warning
⚠️ Attention developer & reviewer: Files with potential user references found:

  • modules/ldap_departments/db/migrate/20260622150055_create_ldap_departments_tables.rb (migration with user reference)

Please make sure:

  1. You've added proper relationships (has_many, belongs_to, …) and their inverse in your models
  2. You deal with model destruction dependencies: dependent: :destroy or dependent: :delete_all
  3. You add behavior and tests for the Principal::DeleteJob (app/workers/principals/delete_job.rb)
  4. You replace references to users with deleted user in Principal::ReplaceReferencesService (app/services/principals/replace_references_service.rb) by adding to the replacements initailizer (config/initializers/replace_references_service.rb) for the core, or using the replace_principal_references helper in modules/plugins.
  5. You test the above behaviors with an integration test (e.g., like this one to confirm deletion of users is possible.

This helps prevent dangling database objects when users are deleted and resulting bugs.

Generated by 🚫 Danger

@github-actions

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]
🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23848, linked for reference only):

- `rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]`

Treat this as a standalone task, unrelated to PR #23848. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23848 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @klaustopher to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @klaustopher, and request a review from @klaustopher.
On every commit, set @klaustopher as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Comment thread modules/ldap_groups/lib/tasks/ldap_groups.rake Outdated
@oliverguenther

Copy link
Copy Markdown
Member
2026-06-23_11-46-41@2x

we may want to use the same enterprise key, but a different locale key to be able to make this banner less confusing as the page is not about LDAP groups

@klaustopher klaustopher force-pushed the OP-19185-department-sync branch from e454f4a to 89eba77 Compare June 23, 2026 10:10
@oliverguenther

Copy link
Copy Markdown
Member
2026-06-23_12-03-30@2x

The delete confirmation should probably inform the user what happens. I would first have expected that it deletes all departments, then read up the description here to learn it only deletes the connection.

Ideally we would let the user choose in the dialog whether they want to delete both, because arguably in many cases you'd want that. At least we should add some info about what happens

@oliverguenther oliverguenther left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • If I manually add a department, the system lets me try to move from a managed department to that manual department. It ends up in an endless loop of the dialog.
Image

Comment thread modules/ldap_departments/app/models/ldap_departments/synchronized_department.rb Outdated
@klaustopher klaustopher merged commit 7ec5071 into dev Jun 23, 2026
15 of 16 checks passed
@klaustopher klaustopher deleted the OP-19185-department-sync branch June 23, 2026 12:44
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants