fix: keycloak migration advisory locks

[yuvrajjsingh0]

Summary by CodeRabbit

• New Features

  • Improved migration handling with safer, concurrent-aware processing to reduce the risk of duplicate or conflicting permission imports.
  • Large account and group imports now run more reliably by loading data in smaller batches.

• Bug Fixes

  • Permission imports now replace outdated or duplicate entries more consistently, keeping access rules aligned with the latest configuration.
  • Prevents multiple instances from running the same migration at the same time when applying changes.

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
airborne_server/src/provider/authz/migration.rs	60% smaller
airborne_server/src/utils/advisory_lock.rs	52% smaller
airborne_server/src/provider/authz/casbin.rs	16% smaller

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 03196255-7003-4fb9-b5cb-e85e56b86ee0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

Casbin policy import logic was changed to perform idempotent upserts keyed on subject, scope, organisation, and application, replacing removed/mismatched rules rather than relying on add-policy return counts. Keycloak-to-Casbin migration now acquires a per-realm advisory lock and paginates user and group fetches; a new advisory lock namespace variant was added.

Changes

Authz Import and Migration Hardening

Layer / File(s)	Summary
Idempotent policy upsert airborne_server/src/provider/authz/casbin.rs	import_policy_entries now upserts policies keyed on (subject, scope, organisation, application), skipping only exact single matches and otherwise removing and re-adding via the enforcer guard.
Advisory-locked, paginated migration airborne_server/src/provider/authz/migration.rs, airborne_server/src/utils/advisory_lock.rs	A new KeycloakToCasbinMigration lock namespace is added and used to guard concurrent migrations per realm; user and group fetching from Keycloak is switched to offset-based pagination using KEYCLOAK_PAGE_SIZE.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Migration as import_keycloak_authz_to_casbin
  participant Lock as AdvisoryLock
  participant Keycloak
  participant Casbin

  Migration->>Lock: acquire lock (KeycloakToCasbinMigration, realm)
  alt lock unavailable
    Lock-->>Migration: not acquired
    Migration-->>Migration: log and return early
  else lock acquired
    loop paginated users
      Migration->>Keycloak: fetch users page (offset, KEYCLOAK_PAGE_SIZE)
      loop paginated groups per user
        Migration->>Keycloak: fetch groups page (offset, KEYCLOAK_PAGE_SIZE)
      end
      Migration->>Migration: map groups to PolicyEntry
    end
    Migration->>Casbin: import_policy_entries(policies)
    Casbin->>Casbin: upsert each policy by key, remove+add if changed
  end

Loading

Poem

A rabbit hops through locks and pages,
One realm at a time, through migration stages,
No more duplicate rules to confuse,
Just clean upserts, the kind we choose,
Thump thump — the policies now align! 🐇🔒

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main change: adding advisory locking around the Keycloak migration.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/keycloak-migration-advisory-lock

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@airborne_server/src/provider/authz/casbin.rs`:
- Around line 187-191: The policy import path currently updates Casbin via
remove_policies_for_filter_in_guard and guard.add_policy, but it never refreshes
authz_memberships, so imported roles can stay stale. After each successful
upsert in this import flow, trigger the membership cache refresh used by this
provider, and make sure to release the enforcer guard before doing so to avoid
self-deadlock. Use the existing import/policy upsert logic in casbin.rs as the
place to hook this in.
- Around line 187-190: The policy replacement in import_policy_entries() is not
atomic because remove_policies_for_filter_in_guard() persists before
add_policy(), so a failed insert can delete the existing grant. Update this flow
so removal and insertion happen in one transactional/rollback-safe unit, using
import_policy_entries(), remove_policies_for_filter_in_guard(), and add_policy()
as the main touchpoints. Also make sure authz_memberships is updated in the same
import path, since the current Casbin-only change can leave membership-based
reads stale after a successful import.

In `@airborne_server/src/provider/authz/migration.rs`:
- Around line 252-254: The error mapping in migration.rs currently includes the
user subject in the fetch failure message, which can leak PII into logs or API
responses. Update the `map_err` in the groups fetch path to return a generic
non-identifying message without interpolating `subject`, while keeping the error
context tied to the same fetch operation in `migration.rs`.
- Around line 179-190: The advisory lock handling in the migration flow is
unsafe because `AdvisoryLockGuard` only retains `DbPool`, so `release()`/`Drop`
may unlock through a different pooled session than the one that acquired the
lock. Update `AdvisoryLockGuard` and the `try_acquire_lock`/`release` path in
`migration.rs` to keep the checked-out connection (or use a transaction-scoped
advisory lock) bound to the same session for the full lifetime of the guard,
ensuring the lock is released on the original connection.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c61ba192-3b21-4ba5-8c34-89fae80d614d

📥 Commits

Reviewing files that changed from the base of the PR and between 4ca949f and 80cd25d.

📒 Files selected for processing (3)

• airborne_server/src/provider/authz/casbin.rs
• airborne_server/src/provider/authz/migration.rs
• airborne_server/src/utils/advisory_lock.rs