Merged
Conversation
Assisted-by: claude-code:claude-sonnet-4-6
Idempotent user+identity creation for auto-provisioning channel users. Handles concurrent races via unique-constraint re-read, derives usernames from email local-part or feishu-<uid[:8]> fallback with -N suffix on collision. Assisted-by: claude-code:claude-sonnet-4-6
Defines pkgchannel.Provisioner as an opt-in interface for channels that want to auto-provision users. Coordinator implements it, guarding against zero-admin deployments. Compile-time assertions catch interface drift. Assisted-by: claude-code:claude-sonnet-4-6
Adds tenant_key and auto_provision fields to FeishuConfig and the bot Config struct. validateConfig rejects auto_provision without tenant_key. Both NewChannel closures (plugin.go and runtime.go) propagate the fields. Assisted-by: claude-code:claude-sonnet-4-6
Adds fetchTenantProfile (contact.v3.user.get) and maybeAutoProvision. Cache is keyed on union_id (canonical) after profile fetch; falls back to open_id before the profile is available. Provisioning skipped silently on any error so message flow is never blocked. Assisted-by: claude-code:claude-sonnet-4-6
Provisioning runs after dedup+group-eligibility checks so duplicate events and bots in disabled groups never trigger it. /link messages are skipped so the manual link-code flow stays authoritative. tenantKey from the event is checked explicitly in onMessage; onReaction relies on the contact API as an implicit tenant filter. Assisted-by: claude-code:claude-sonnet-4-6
Adds auto-provisioning section covering how it works, required app scopes, how to find the tenant key, example config, and warnings about external guests and the zero-admin guard. Updates config reference tables with the two new fields. Assisted-by: claude-code:claude-sonnet-4-6
![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: c777fc91d3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if err := provisioner.ProvisionUser(ctx, pkgchannel.ProvisionRequest{ | ||
| Platform: pkgchannel.PlatformFeishu, | ||
| ExternalID: unionID, |
There was a problem hiding this comment.
This call forwards unionID directly as ExternalID even when no union_id is present in either the event or Contact API response. In that case, provisioning creates/looks up the identity key (feishu, ""), so every subsequent user missing union_id can be resolved to the same Anna account (because ProvisionIdentityUser is idempotent on (platform, externalID)). This is a cross-user identity mix-up; guard against empty IDs (or fall back to open_id) before provisioning.
Useful? React with 👍 / 👎.
Comment on lines
+89
to
+91
| _, notFound := store.GetUserByUsername(ctx, candidate) | ||
| if notFound != nil { | ||
| return candidate, nil //nolint:nilerr |
There was a problem hiding this comment.
Here any non-nil error from GetUserByUsername is treated as “username available.” If the datastore returns a real failure (e.g., lock/timeout), provisioning may pick an actually-taken username and then fail later with a confusing create error, or hide the real read failure entirely. Only sql.ErrNoRows should mark availability; other errors should be propagated.
Useful? React with 👍 / 👎.
📊 Coverage ReportTotal coverage: 46.4% Per-package breakdown |
P1: guard against empty union_id in maybeAutoProvision — without this, every user missing union_id would be stored as (feishu, "") and resolve to the same Anna account due to idempotent lookup on (platform, externalID). P2: distinguish sql.ErrNoRows from real DB failures in deriveUsername — only treat ErrNoRows as "username available"; propagate other errors so transient DB failures don't silently cause mis-keyed account creation. Assisted-by: claude-code:claude-sonnet-4-6
Exposes the two auto-provisioning config fields (tenant_key text input and auto_provision toggle) in the admin channels form so operators can configure them without hand-editing the database. Assisted-by: Claude Code:claude-sonnet-4-6
When auto_provision=true and tenant_key is not configured, the bot now calls /open-apis/tenant/v2/tenant/query on startup to detect its home tenant key. This avoids the first-event race condition and removes the requirement to manually look up and enter the tenant key. - Add fetchBotTenantKey() using client.Tenant.Tenant.Query() - Add effectiveTenantKey() combining configured + detected values - Drop validateConfig check requiring tenant_key when auto_provision=true - Update schema description and UI placeholder to reflect optional field Assisted-by: Claude Code:claude-sonnet-4-6
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d4100d891
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+30
to
+31
| existing, err := store.GetIdentityByPlatform(ctx, req.Platform, req.ExternalID) | ||
| if err == nil { |
There was a problem hiding this comment.
ProvisionIdentityUser currently treats every GetIdentityByPlatform error as a cache miss and proceeds to create a user. If that lookup fails for reasons other than not-found (for example transient DB/context failures), this path can perform unintended writes and then surface misleading downstream errors from create/rollback attempts. The provisioning flow should only continue on sql.ErrNoRows and immediately return other lookup errors.
Useful? React with 👍 / 👎.
Comment on lines
+140
to
+142
| provCtx, provCancel := b.apiContext() | ||
| b.maybeAutoProvision(provCtx, openID, unionID, tenantKey) | ||
| provCancel() |
There was a problem hiding this comment.
This call runs inline on the message handler hot path, and maybeAutoProvision performs a remote Contact API request on cache misses. Because the provisioning context uses a 30-second timeout, slow Feishu API responses can delay normal message processing for first-contact users by up to that timeout, despite the intent that provisioning failures should not block chat flow. Consider moving provisioning off the synchronous path (or otherwise decoupling first-message handling from profile fetch latency).
Useful? React with 👍 / 👎.
P1 (internal/auth/provision.go): GetIdentityByPlatform error was treated as "not found" for any error, not just sql.ErrNoRows. Transient DB errors now propagate immediately instead of proceeding to unintended user creation. P2 (handler.go): maybeAutoProvision ran synchronously on the message hot path, blocking delivery for first-contact users by up to 30s on Contact API latency. Both onMessage and onReaction now launch provisioning in a goroutine. Assisted-by: Claude Code:claude-sonnet-4-6
- Use platform field instead of hardcoded "feishu-" in deriveUsername - Wrap discarded error in Coordinator.ProvisionUser count check - Replace slog.Debug with logger() for consistent log routing - Extract isCachedProvision helper to deduplicate lock/check pattern Assisted-by: claude-code:claude-sonnet-4-6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/linkstep neededtenant_keyin config is checked against the event; external guests in shared groups are denied per-message as beforeDesign
internal/auth/provision.go— idempotentProvisionIdentityUser: fast-path identity lookup, username derivation (email local-part →feishu-<uid[:8]>fallback →-Nsuffix), race-safe via unique-constraint re-read on concurrent provisionspkg/channel.Provisioner— opt-in interface so only Feishu needs to implement it;Coordinator.ProvisionUserimplements it with the zero-admin guardplugins/channels/feishu/provision.go—fetchTenantProfile(contact.v3.user.get) +maybeAutoProvisionwith union_id-keyed 1h cache/linkmessages are skipped so the manual link-code flow stays authoritativeruntime.goandplugin.goboth propagate the newTenantKey/AutoProvisionfields;validateConfigrejectsauto_provision=truewithouttenant_keyTest plan
go test ./internal/auth/...— provision idempotency, username collision, empty-email fallbackgo test ./plugins/channels/feishu/...— validateConfig, maybeAutoProvision disabled/wrong-tenant/cache-hit/nil-profile casesgo test ./internal/channel/...— Coordinator compile-time assertionsauto_provision=true+ validtenant_key, send a message from a tenant user, verify Anna account + identity appear in DB; send from external guest, verify denial🤖 Generated with Claude Code