Fix org member identity mapping for sync and SSO#404
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d30a2e4897
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| lookup_provider_user_id = user_info.provider_union_id or user_info.provider_user_id | ||
| existing_user = await sso_service.check_duplicate_identity(db, provider, lookup_provider_user_id) |
There was a problem hiding this comment.
The duplicate check in bind_identity now uses only provider_union_id (lookup_provider_user_id), but link_identity still matches existing OrgMember rows by open_id as well. For legacy rows that are keyed only by open_id (common from earlier bindings), check_duplicate_identity will miss the conflict and the later link_identity call can reassign member.user_id to the current account, effectively stealing an already-linked identity during bind.
Useful? React with 👍 / 👎.
| 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}" | ||
| ) |
There was a problem hiding this comment.
Raising on missing unionid here causes _upsert_member to fail for that user, but sync_org_structure continues and then runs reconciliation, which marks non-updated members as deleted. That means a transient provider response/permission issue that omits unionid can 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 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4101cfbd03
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| member = member_result.scalar_one_or_none() | ||
| if member: | ||
| return member |
There was a problem hiding this comment.
The new _find_identity_member helper returns on the first identifier hit, even when that OrgMember is just an unlinked shell (user_id is null). In resolve_user_identity/check_duplicate_identity, this short-circuit means a later open_id/external_id match that is already linked to another user is never checked, so bind can incorrectly pass conflict checks and then link_identity links the wrong record. Fresh evidence here is the new early return member in the lookup loop, which did not exist in the previous single-query path.
Useful? React with 👍 / 👎.
| 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, | ||
| ) |
There was a problem hiding this comment.
The new fallback filter only allows external_id/open_id matches when existing unionid is null/empty or already equal to the incoming value. That blocks updates to legacy rows with incorrect non-empty unionid values (a known historical state), so _upsert_member creates a new row instead of updating the existing linked one; if email/mobile is absent, the replacement row loses user_id, and a later reconcile can delete the previously linked row. This introduces account-link breakage during normal sync migration.
Useful? React with 👍 / 👎.
1 participant
Summary
unionidduring Feishu/DingTalk org syncunionid == external_idunion_id/open_id/external_iduser_idis no longer written intounionidVerification
PYTHONPYCACHEPREFIX=/tmp/codex-pycache python3 -m py_compile app/services/sso_service.py app/services/channel_user_service.py app/services/registration_service.py app/api/auth.py app/api/feishu.py tests/test_identity_id_mapping.py tests/test_org_sync_adapter.pypytestnot run in this environment because the module is not installed