-
Notifications
You must be signed in to change notification settings - Fork 560
Fix org member identity mapping for sync and SSO #404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,7 @@ async def sync_org_structure(self, db: AsyncSession) -> dict[str, Any]: | |
| user_count = 0 | ||
| profile_count = 0 | ||
| sync_start = datetime.now() | ||
| partial_failure = False | ||
|
|
||
| # Ensure provider exists | ||
| provider = await self._ensure_provider(db) | ||
|
|
@@ -149,6 +150,7 @@ async def sync_org_structure(self, db: AsyncSession) -> dict[str, Any]: | |
| await self._upsert_department(db, provider, dept) | ||
| dept_count += 1 | ||
| except Exception as e: | ||
| partial_failure = True | ||
| errors.append(f"Department {dept.external_id}: {str(e)}") | ||
| logger.error(f"[OrgSync] Failed to sync department {dept.external_id}: {e}") | ||
|
|
||
|
|
@@ -157,6 +159,7 @@ async def sync_org_structure(self, db: AsyncSession) -> dict[str, Any]: | |
| try: | ||
| users = await self.fetch_users(dept.external_id) | ||
| except Exception as e: | ||
| partial_failure = True | ||
| logger.error(f"[OrgSync] Failed to fetch users in department {dept.external_id}: {e}") | ||
| errors.append(f"Fetch users in dept {dept.external_id}: {str(e)}") | ||
| continue | ||
|
|
@@ -171,6 +174,7 @@ async def sync_org_structure(self, db: AsyncSession) -> dict[str, Any]: | |
| profile_count += 1 | ||
| member_count += 1 | ||
| except Exception as e: | ||
| partial_failure = True | ||
| logger.error(f"[OrgSync] Failed to sync member {user.external_id} ({user.name}): {e}") | ||
| errors.append(f"Member {user.external_id}: {str(e)}") | ||
|
|
||
|
|
@@ -181,9 +185,15 @@ async def sync_org_structure(self, db: AsyncSession) -> dict[str, Any]: | |
| self.provider.config = config | ||
| await db.flush() | ||
|
|
||
| # Reconciliation: mark records not updated in this sync as deleted | ||
| await self._reconcile(db, provider.id, sync_start) | ||
| await db.flush() | ||
| if partial_failure: | ||
| logger.warning( | ||
| f"[OrgSync] Skipping reconcile for provider {provider.id} because this sync had partial failures" | ||
| ) | ||
| errors.append("Reconcile skipped due to partial sync failures") | ||
| else: | ||
| # Reconciliation: mark records not updated in this sync as deleted | ||
| await self._reconcile(db, provider.id, sync_start) | ||
| await db.flush() | ||
|
|
||
| # Recalculate member counts for all departments (crucial for DingTalk/WeCom) | ||
| await self._update_member_counts(db, provider.id) | ||
|
|
@@ -386,6 +396,7 @@ async def _upsert_member( | |
| ) -> dict[str, Any]: | ||
| """Insert or update a member, platform user, and identity.""" | ||
| stats = {"user_created": False, "profile_synced": False} | ||
| self._validate_member_identifiers(provider, user) | ||
|
|
||
| # Find department using user's actual department list. | ||
| # DingTalk's dept_id_list last item is the most specific (leaf) department. | ||
|
|
@@ -413,23 +424,7 @@ async def _upsert_member( | |
| ) | ||
| department = dept_result.scalars().first() | ||
|
|
||
| # Check if exists by unionid or external_id or open_id (any matches), and provider | ||
| conditions = [] | ||
| if user.unionid: | ||
| conditions.append(OrgMember.unionid == user.unionid) | ||
| if user.external_id: | ||
| conditions.append(OrgMember.external_id == user.external_id) | ||
| if user.open_id: | ||
| conditions.append(OrgMember.open_id == user.open_id) | ||
|
|
||
| if conditions: | ||
| result = await db.execute( | ||
| select(OrgMember).where( | ||
| OrgMember.provider_id == provider.id, | ||
| or_(*conditions) | ||
| ) | ||
| ) | ||
| existing_member = result.scalars().first() | ||
| existing_member = await self._find_existing_member(db, provider, user) | ||
|
|
||
| now = datetime.now() | ||
|
|
||
|
|
@@ -535,6 +530,70 @@ async def _upsert_member( | |
| await db.flush() | ||
| return stats | ||
|
|
||
| def _provider_requires_unionid(self, provider: IdentityProvider) -> bool: | ||
| provider_type = (provider.provider_type or self.provider_type or "").lower() | ||
| return provider_type in {"feishu", "dingtalk"} | ||
|
|
||
| def _validate_member_identifiers(self, provider: IdentityProvider, user: ExternalUser) -> None: | ||
| user.unionid = (user.unionid or "").strip() | ||
| user.external_id = (user.external_id or "").strip() | ||
| user.open_id = (user.open_id or "").strip() | ||
|
|
||
| if self._provider_requires_unionid(provider) and not user.unionid: | ||
| raise ValueError( | ||
| f"unionid is required for {provider.provider_type} org sync user {user.external_id or user.name}" | ||
| ) | ||
|
|
||
| if user.unionid and user.external_id and user.unionid == user.external_id: | ||
| raise ValueError( | ||
| f"invalid unionid for org sync user {user.external_id or user.name}: unionid must not equal external_id" | ||
| ) | ||
|
|
||
| async def _find_existing_member( | ||
| self, | ||
| db: AsyncSession, | ||
| provider: IdentityProvider, | ||
| user: ExternalUser, | ||
| ) -> OrgMember | None: | ||
| if user.unionid: | ||
| result = await db.execute( | ||
| select(OrgMember).where( | ||
| OrgMember.provider_id == provider.id, | ||
| OrgMember.unionid == user.unionid, | ||
| ) | ||
| ) | ||
| existing_member = result.scalars().first() | ||
| if existing_member: | ||
| return existing_member | ||
|
|
||
| fallback_conditions = [] | ||
| if user.external_id: | ||
| fallback_conditions.append(OrgMember.external_id == user.external_id) | ||
| if user.open_id: | ||
| fallback_conditions.append(OrgMember.open_id == user.open_id) | ||
|
|
||
| if not fallback_conditions: | ||
| return None | ||
|
|
||
| fallback_query = select(OrgMember).where( | ||
| OrgMember.provider_id == provider.id, | ||
| or_(*fallback_conditions), | ||
| ) | ||
|
|
||
| # When unionid is required, only allow external/open id fallback to attach | ||
| # shell records that do not have a conflicting unionid yet. | ||
| if self._provider_requires_unionid(provider) and user.unionid: | ||
| fallback_query = fallback_query.where( | ||
| or_( | ||
| OrgMember.unionid.is_(None), | ||
| OrgMember.unionid == "", | ||
| OrgMember.unionid == user.unionid, | ||
| ) | ||
|
Comment on lines
+585
to
+591
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new fallback filter only allows Useful? React with 👍 / 👎. |
||
| ) | ||
|
|
||
| result = await db.execute(fallback_query) | ||
| return result.scalars().first() | ||
|
|
||
| async def _resolve_platform_user(self, db: AsyncSession, user: ExternalUser) -> User | None: | ||
| """Resolve platform user from external user info.""" | ||
| # 1. Try by Email matching (primary way now) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raising on missing
unionidhere causes_upsert_memberto fail for that user, butsync_org_structurecontinues and then runs reconciliation, which marks non-updated members asdeleted. That means a transient provider response/permission issue that omitsunionidcan soft-delete previously valid Feishu/DingTalk members in bulk on a single sync run, which is a data-loss regression introduced by this strict validation path.Useful? React with 👍 / 👎.