Repository navigation
fix(slack): organization Slack integrations are managed by owners and admins - #8236
Conversation
…anges - The organization Slack OAuth connect entry point and callback require an owner or admin, matching the authority other organization-scoped integration changes use. - Organization Slack disconnect and model changes require the same authority. Members keep a read-only card that still runs the connection test. - upsertSlackInstallation takes an explicit permission argument so the bound workspace only changes from a flow that verified organization management authority. - Access denials from the shared OAuth connect handler redirect with a specific error code rather than a generic initialization failure. Tests cover a member, an owner/admin, a repeated install for the same workspace, a personal connection, and the service-level permission check.
iscekic
added
drain
|
bot: Verified the role-aware organization Slack card and the owner/admin policy on the local dev stack (real Postgres, fake-account test data: one organization with an owner, an admin, and a member, plus one Slack integration for a test workspace). Observed:
Captured from
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change makes the purpose-less OAuth state fixture real by distinguishing Files Reviewed (1 file)
Previous Review Summaries (3 snapshots, latest commit cf925c9)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cf925c9)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe single incremental change adds a regression test whose fixture still carries an install purpose, so it duplicates the adjacent case instead of covering the no-purpose branch. Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit 5ec2f91)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental changes close all three earlier findings: the org rebind guard now falls back to OverviewThe rebind guard now compares Files Reviewed (3 files)
Previous review (commit 1073790)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryThe owner/admin gating on the Slack connect, callback, router, and UI paths is sound; the remaining gap is that the new service-level rebind guard skips legacy organization rows whose workspace id lives only in Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Reviewed by deepseek-v4.1-flash-1017 · Input: 38.5K · Output: 2.7K · Cached: 291.2K Review guidance: REVIEW.md from base branch |
- The organization workspace check compares the installation id with the account id fallback, so rows that record their workspace only in the account id are covered too. - Add a regression case for that legacy shape and make the unbound fixture a row with no workspace identity at all. - The member notice mentions the connection test only when the test control is rendered.
The organization role check runs before the install-purpose and attempt checks, so the same authority applies to states created without that purpose. Pin it with a callback case.
The helper defaulted a passed `undefined` back to the install purpose, so the new case re-ran the purpose-bearing state. Take `null` for "no purpose" and assert the signed state carries none.
iscekic
added
the
intent:ok
iscekic
added
merge:blocked:judge-unavailable
Evidence
Evidence: demonstrated — local dev stack with real Postgres and owned fake accounts: verified the member and admin organization Slack views and controls in the app, exercised the Slack callback and service suites (31 tests), and confirmed the web typecheck and touched-file lint and format checks pass.
Summary
Organization Slack integrations are managed by organization owners and admins. Members see the connection state and can run the connection test, and the card explains who manages it.
apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts,apps/web/src/lib/integrations/oauth/routes.ts, and the organization Slack reinstall route: organization installs are an owner/admin action, usingORGANIZATION_MANAGE_ROLESlike other organization-scoped integration routes.apps/web/src/lib/integrations/oauth/common.ts: the shared stateful connect handler redirects witherror=permission_required/error=organization_access_requiredwhen the caller is not allowed, instead of the generic initialization error.apps/web/src/lib/integrations/slack-service.ts:upsertSlackInstallationtakesallowWorkspaceRebind, provided by the callback after the role check.apps/web/src/routers/slack-router.ts:slack.uninstallAppandslack.updateModelare owner/admin actions for organizations.apps/web/src/components/integrations/SlackIntegrationDetails.tsxandIntegrationDetailPage.tsx: members get a read-only card with a short explanation; owners and admins keep the full set of controls.This does not conflict with the in-flight
upsertSlackInstallationcredential-store work.Verification
Local dev stack, real Postgres, fake-account test data: an organization with an owner, an admin, and a member.
slack.uninstallAppandslack.updateModelanswerUNAUTHORIZED;slack.testConnectionstill works.jest src/lib/integrations/slack-service.test.ts src/lib/integrations/oauth/platforms/slack-callback.test.ts: 31 passed (member, owner/admin, a completion without an install purpose, repeated install for the same workspace, personal connection, and the service permission check).tsgo --noEmitforapps/web: exit 0.oxlintandoxfmt --check: clean on touched files.Screenshots (fake accounts, local non-production stack) are attached in the verification comment.
Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Every org completion (owner/admin only)
Confidence: 0.70 · Precedent: Fix the root cause and the class of the issue: session 7f3470e0ba6f, "fix class of issue preventing progress". The work item requires owner/admin for every org Slack OAuth completion.
Answer: Gate connect and callback
Confidence: 0.65 · Precedent: Fix the class of the issue on every affected surface: session 75f8833fe8c1, "fix this for all ... on all surfaces". Also: if you see a gap, fix it: session 89824a222897, "if you saw a gap, fix it".
Answer: Allow with an explicit authorized flag
Confidence: 0.55 · Precedent: Trust the work item. It says a change to a different team id "requires that same role", so an admin may still change it in one flow. Keep the flag compatible with PR fix(slack): read bot credentials from the credential store #8028's upsertSlackInstallation edits.
Answer: Role-aware buttons
Confidence: 0.50 · Precedent: Hold all UI work to best-in-class standard: session 00eb2a1d3e4b, "design and UX MUST BE BEST-IN-CLASS". Do not show members buttons that are certain to fail.
Answer: Install + disconnect + model read-only
Confidence: 0.65 · Precedent: Autonomy: 'Fix the root cause and the class of the issue ... on every affected surface' (session 00eb2a1d3e4b, 'fix the class'); 'If you see a gap or an error, fix it' (session 89824a222897, 'if you saw a gap, fix it')
Answer: Map denial codes generically
Confidence: 0.70 · Precedent: Autonomy: fix the class on every affected surface (session 75f8833fe8c1, 'fix this for all shared services model pickers on all surfaces'); Engineering: extract shared code (session 5d8113f51c48)