Update IdP Group Sync#478
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Group Sync guide now covers IdP-based workspace role provisioning, project access, and private collection access, with updated provider support, configuration options, mapping workflows, synchronization behavior, revocation rules, safeguards, and use cases. ChangesIdP Group Sync
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/authentication/group-sync.md`:
- Line 142: Update the Contractor offboarding documentation to remove the
promise of immediate access revocation and state that revocation occurs at the
next SAML login or OIDC/LDAP scheduled sync.
- Line 57: Update both the Auto remove entry and its corresponding later rule in
group-sync documentation to state that removal revokes access granted by each
group mapping when that mapping no longer matches, including workspace, project,
and private-collection access. Ensure the wording no longer implies removal
happens only when no group matches.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6855590d-4d34-41e8-966a-230b669933dd
📒 Files selected for processing (1)
docs/authentication/group-sync.md
| | --- | --- | --- | | ||
| | **Sync on login** | Update the user's group membership and access when they sign in | Enabled | | ||
| | **Offline sync** | Run a sync every six hours automatically, without waiting for users to log in (OIDC and LDAP only) | Disabled | | ||
| | **Auto remove** | Remove users from the workspace and projects when no group matches | Disabled | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify Auto remove scope and semantics.
The description omits private collection access and says removal occurs only when “no group matches,” while the later rules revoke group-synced access when any mapping no longer matches. Update both entries to describe per-mapping revocation across workspace, project, and private-collection access.
Also applies to: 109-109
🤖 Prompt for 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.
In `@docs/authentication/group-sync.md` at line 57, Update both the Auto remove
entry and its corresponding later rule in group-sync documentation to state that
removal revokes access granted by each group mapping when that mapping no longer
matches, including workspace, project, and private-collection access. Ensure the
wording no longer implies removal happens only when no group matches.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/authentication/group-sync.md (1)
8-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify why IdP Group Sync is tagged as Enterprise Grid.
The availability notes state this is supported on self-hosted Commercial and Airgapped Editions, but the badge labels it as “Enterprise Grid”. Align the badge text with the availability text or add a short clarification if Enterprise Grid is a prerequisite/scope, not the edition.
🤖 Prompt for 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. In `@docs/authentication/group-sync.md` around lines 8 - 12, Update the IdP Group Sync heading badge and availability note so they describe the same supported scope; either change the “Enterprise Grid” badge to match the self-hosted Commercial and Airgapped Editions, or add a concise clarification explaining Enterprise Grid’s prerequisite or scope.
🤖 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.
Outside diff comments:
In `@docs/authentication/group-sync.md`:
- Around line 8-12: Update the IdP Group Sync heading badge and availability
note so they describe the same supported scope; either change the “Enterprise
Grid” badge to match the self-hosted Commercial and Airgapped Editions, or add a
concise clarification explaining Enterprise Grid’s prerequisite or scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d13a7586-0985-493c-a5c4-a80fc5ab4b8e
📒 Files selected for processing (1)
docs/authentication/group-sync.md
|
Reviewed the rewrite against the current backend implementation ( A few things to correct before merge: 1. Offline sync interval is every 2 hours, not "every six hours" (appears in the Offline-sync settings-table row and the "What happens on sync" text). The actual beat schedule is every 2 hours: # apps/api/plane/celery.py:169-173
# IdP group sync - runs every 2 hours
"sync-idp-groups-offline": {
"task": "...group_sync_task.sync_idp_groups_offline",
"schedule": crontab(hour="*/2", minute=10), # Every 2 hours at minute 10
},Heads-up: the "6 hours" almost certainly came from a stale code comment — 2. Revocation order is stated backwards. The doc says access is revoked "in this order: private collections, projects, and finally the workspace", but the code revokes projects → private collections → workspace: # apps/api/plane/authentication/group_sync/service.py:208-217
for project_id in current_synced: # projects first
...
for collection_id in current_synced_collections: # then collections
...
if self._remove_from_workspace(...): # workspace lastWorkspace-last is correct; collections and projects are swapped. It's within a single atomic transaction so the outcome is identical, but the stated order is wrong — suggest "projects, then private collections, then the workspace" (or just "…removing workspace membership last"). 3. Minor / for the backend team, not necessarily this PR: the line "A workspace guest cannot be granted a project Admin role through a mapping" is true on the initial-add path (the guest ceiling is enforced at Everything else looks accurate. Nice cleanup overall. |
2 participants
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit