fix(slack): prevent linking multiple workspaces to a single tenant#2286
fix(slack): prevent linking multiple workspaces to a single tenant#2286amikofalvy merged 2 commits intomainfrom
Conversation
Only one Slack workspace per Inkeep tenant is supported today. This adds a guard in the OAuth callback to reject installations when the tenant already has a different workspace connected, and removes the "Add Workspace" button from the manage UI. Re-installing the same workspace (re-auth) is still allowed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
🦋 Changeset detectedLatest commit: f7dc7fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
oauth.ts:295-311Database query failure in workspace limit check produces generic error instead of specific actionable feedback
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
slack-dashboard.tsx:39Terminology mismatch ("Remove" vs "Uninstall") between error message and UI action
💭 Consider (3) 💭
💭 1) oauth.ts:295-311 TOCTOU race condition in workspace limit enforcement
Issue: The guard checking for existing workspaces and the workspace creation are not atomic. Two simultaneous OAuth flows for different workspaces could both pass the check.
Why: Low exploitability — requires simultaneous OAuth flows through external Slack redirects with precise timing. The UI removal of "Add Workspace" reduces attack surface to direct URL manipulation. The existing DB constraint on (tenant_id, slack_team_id) prevents same-workspace duplicates but not different workspaces.
Fix: If strict single-workspace enforcement is required, consider a DB-level unique constraint on tenantId alone, or wrap check+insert in a serializable transaction. Given the PR description states this is app-layer enforcement to be easily lifted for multi-workspace support, the current approach is acceptable.
💭 2) oauth.test.ts Missing test coverage for workspace limit guard
Issue: The new workspace limit check logic is not covered by existing tests. oauth.test.ts only tests state signing/verification, not the route handler.
Why: Test coverage would ensure the behavior is preserved during future refactoring.
Fix: Add integration tests mocking listWorkAppSlackWorkspacesByTenant to verify: (1) installation succeeds with no existing workspaces, (2) same workspace re-auth succeeds, (3) different workspace installation fails with workspace_limit_reached.
💭 3) docs Documentation gap for single-workspace limit
Issue: The installation docs (agents-docs/content/talk-to-your-agents/slack/installation.mdx) don't mention the single-workspace-per-tenant limitation.
Why: Documenting constraints upfront improves first-contact clarity and reduces support friction.
Fix: Consider adding a note explaining: (1) only one Slack workspace per organization, (2) to switch, first uninstall via the dashboard.
✅ What's Done Well
- Guard placement is correct: The check happens in
/oauth_redirect(authenticated callback), not/install(noAuth), avoiding information leakage about whether a tenant has a workspace. - Idempotency preserved: Re-installing the same workspace (token refresh) works correctly due to the
slackTeamId !== workspaceData.teamIdcheck. - Error code conventions followed:
workspace_limit_reachedmatches the existing snake_case pattern used throughout the file. - Toast handling follows established UI patterns: Uses
toast.error()appropriately for error conditions. - No DB migration needed: App-layer enforcement is intentional for easy lift when multi-workspace ships.
📋 Changeset Required
The changeset-bot flagged this PR has no changeset. Per AGENTS.md guidelines, this change affects @inkeep/agents-work-apps and @inkeep/agents-manage-ui with user-facing behavior changes and should have a changeset:
pnpm bump patch --pkg agents-work-apps --pkg agents-manage-ui "Enforce single Slack workspace per tenant and improve error handling"🚫 REQUEST CHANGES
Summary: Clean implementation with good design decisions (guard placement, idempotency). The Major issue around error handling for DB query failures should be addressed — when the workspace check query fails, users should get a specific, retryable error message rather than a generic callback_error. The terminology mismatch is a quick fix. Consider items are optional improvements.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
oauth.ts |
Error code naming | Validated as following existing conventions — not an issue |
slack-dashboard.tsx |
Toast pattern | Validated as following existing conventions — not an issue |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-errors |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-product |
2 | 0 | 1 | 0 | 1 | 0 | 0 |
pr-review-consistency |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-security-iam |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 3 | 0 | 2 | 0 | 2 |
| } else if (error === 'workspace_limit_reached') { | ||
| toast.error( | ||
| 'Only one Slack workspace can be connected per organization. Remove the existing workspace to connect a different one.' | ||
| ); |
There was a problem hiding this comment.
🟡 Minor: Terminology mismatch between error message and UI action
Issue: The error message uses "Remove" but the UI action in workspace-hero.tsx is labeled "Uninstall".
Why: Users following the error guidance may look for a "Remove" button that doesn't exist, causing friction in the recovery flow.
Fix: (1-click apply)
| ); | |
| toast.error( | |
| 'Only one Slack workspace can be connected per organization. Uninstall the existing workspace to connect a different one.' | |
| ); |
Refs:
| if (tenantId && workspaceData.teamId) { | ||
| const existingWorkspaces = await listWorkAppSlackWorkspacesByTenant(runDbClient)(tenantId); | ||
| const hasOtherWorkspace = existingWorkspaces.some( | ||
| (w) => w.slackTeamId !== workspaceData.teamId | ||
| ); | ||
| if (hasOtherWorkspace) { | ||
| logger.warn( | ||
| { | ||
| tenantId, | ||
| newTeamId: workspaceData.teamId, | ||
| existingTeamIds: existingWorkspaces.map((w) => w.slackTeamId), | ||
| }, | ||
| 'Tenant already has a different Slack workspace, rejecting installation' | ||
| ); | ||
| return c.redirect(`${dashboardUrl}?error=workspace_limit_reached`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 Major: Database query failure produces generic error
Issue: The listWorkAppSlackWorkspacesByTenant call is not wrapped in its own try-catch. If this query fails (DB timeout, connection pool exhaustion), the outer catch on line 434 produces a generic callback_error rather than a specific error.
Why: At this point in the flow, the Slack OAuth has already succeeded (tokens granted). A transient DB failure here means the user sees "Slack installation failed: callback_error" with no indication the issue was specifically with the workspace check. On retry, it might succeed, but the user has no way to know this.
Fix: Wrap the workspace check in its own try-catch with a distinct error code:
if (tenantId && workspaceData.teamId) {
let existingWorkspaces: WorkAppSlackWorkspaceSelect[];
try {
existingWorkspaces = await listWorkAppSlackWorkspacesByTenant(runDbClient)(tenantId);
} catch (err) {
logger.error({ err, tenantId }, 'Failed to check existing workspaces');
return c.redirect(`${dashboardUrl}?error=workspace_check_failed`);
}
const hasOtherWorkspace = existingWorkspaces.some(
(w) => w.slackTeamId !== workspaceData.teamId
);
if (hasOtherWorkspace) {
logger.warn(
{
tenantId,
newTeamId: workspaceData.teamId,
existingTeamIds: existingWorkspaces.map((w) => w.slackTeamId),
},
'Tenant already has a different Slack workspace, rejecting installation'
);
return c.redirect(`${dashboardUrl}?error=workspace_limit_reached`);
}
}Then add corresponding UI handling for workspace_check_failed with a retryable message.
Refs:
- Wrap workspace limit check in try-catch with specific `workspace_check_failed` error code for transient DB failures - Fix terminology: "Remove" → "Uninstall" to match UI action label - Add changeset for agents-work-apps and agents-manage-ui Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Previous Feedback Addressed
All issues from the prior review have been resolved in commit f7dc7fa3a:
| Prior Issue | Status |
|---|---|
| 🟠 Major: DB query failure produces generic error | ✅ Fixed — wrapped in try-catch with workspace_check_failed error code |
| 🟡 Minor: Terminology mismatch ("Remove" vs "Uninstall") | ✅ Fixed — message now says "Uninstall" |
| 📋 Missing changeset | ✅ Added — wealthy-lavender-donkey.md |
✅ Delta Review
The new commit cleanly addresses the feedback:
-
Error handling (
oauth.ts:295-319): ThelistWorkAppSlackWorkspacesByTenantcall is now properly wrapped in its own try-catch, returningworkspace_check_failedon failure with appropriate logging. -
UI error handling (
slack-dashboard.tsx:40-41): Added handler forworkspace_check_failedwith a user-friendly, retryable message: "Could not verify workspace status. Please try again." -
Terminology fix (
slack-dashboard.tsx:37-39): Error message now correctly references "Uninstall" to match the UI action. -
Changeset: Properly scoped to
@inkeep/agents-work-appsand@inkeep/agents-manage-uiwith clear description.
✅ APPROVE
Summary: Clean implementation addressing all prior feedback. The workspace limit guard is well-structured with proper error handling, logging, and user-facing messages. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Delta re-review | — | — | — | — | — | — | — |
Note: This was a scoped delta re-review. No sub-reviewers dispatched since changes were limited to addressing prior feedback.
…lation guide Reflects PR #2286 which prevents linking multiple Slack workspaces to a single Inkeep tenant.
* docs: add single workspace per tenant limitation note to Slack overview * docs: add single workspace per tenant limitation note to Slack installation guide Reflects PR #2286 which prevents linking multiple Slack workspaces to a single Inkeep tenant. * docs: add single workspace limitation note to Slack configuration page --------- Co-authored-by: inkeep[bot] <257615677+inkeep[bot]@users.noreply.github.com>
Summary
/oauth_redirect) that rejects installations when the tenant already has a different Slack workspace connectedworkspace_limit_reachederror code in the UI with a clear toast messageContext
The Slack work app doesn't support multiple workspaces per tenant today. The data model technically allowed it (unique constraint is on
(tenant_id, slack_team_id), nottenant_idalone), and the UI had an "Add Workspace" button. This could lead to undefined behavior since the dashboard only renders the first workspace.Design decisions
/oauth_redirect, not/install— the/installroute isnoAuth(), so checking there would leak whether a tenant has a workspace installed. The callback redirects errors to the authenticated manage UI.slackTeamId !== workspaceData.teamId, so re-authorizing the existing workspace still worksTest plan
workspace_limit_reachederror and display toast/install?tenant_id=Xstill starts OAuth flow (no info leak)🤖 Generated with Claude Code