feat: slack user authentication gating and login flow#1719
feat: slack user authentication gating and login flow#1719
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🚀 Preview Environment (PR #1719)Preview URL: https://pr-1719.dev.getgram.ai
Gram Preview Bot |
qstearns
force-pushed
the
slack-apps-auth/3-user-linking
branch
from
4280b27 to
6778787
Compare
qstearns
force-pushed
the
slack-apps-auth/3-user-linking
branch
from
6778787 to
40dd965
Compare
qstearns
changed the title
|
|
||||||||||||||||
qstearns
force-pushed
the
slack-apps-auth/3-user-linking
branch
from
edbdc0b to
aa0469f
Compare
qstearns
force-pushed
the
slack-apps-auth/3-user-linking
branch
from
e0bab5c to
efcc9e2
Compare
qstearns
force-pushed
the
slack-apps-auth/3-user-linking
branch
from
efcc9e2 to
c50eb84
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🟡 Accessing pgtype.Text .String without checking .Valid causes decrypt of empty string
In SlackAppOAuthCallback and SlackAppEventHandler, nullable database fields are accessed via .String without first checking .Valid, which will attempt to decrypt an empty string if the app is unconfigured.
Detailed Explanation
At server/internal/thirdparty/slack/impl.go:133, app.SlackClientSecret.String is passed to s.enc.Decrypt() without checking app.SlackClientSecret.Valid. If the Slack app is in "unconfigured" status (i.e., ConfigureSlackApp hasn't been called yet), SlackClientSecret will have Valid=false and String="". The Decrypt call on an empty string will produce a confusing error rather than a clear "app not configured" message.
The same issue occurs at:
- Line 138:
app.SlackClientID.Stringused without validity check - Line 188:
app.SlackSigningSecret.Stringused without validity check - Line 293:
app.SlackBotToken.Stringused without validity check
Impact: If someone hits the OAuth callback URL or events URL for an unconfigured app, they'll get a confusing 500 error instead of a clear 400/404 error. For the events endpoint, this could happen if Slack sends events before the app is fully configured.
(Refers to lines 133-138)
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| if processEvent { | ||
| slackUserID := event.Event.User | ||
| if slackUserID != "" { | ||
| _, err := s.repo.GetSlackUserMapping(ctx, repo.GetSlackUserMappingParams{ | ||
| SlackAppID: appID, | ||
| SlackUserID: slackUserID, | ||
| }) | ||
| if err != nil { | ||
| if !errors.Is(err, pgx.ErrNoRows) { | ||
| return oops.E(oops.CodeUnexpected, err, "check slack user mapping").Log(ctx, s.logger) | ||
| } | ||
|
|
||
| // No mapping — create pending auth and send ephemeral login link | ||
| tokenBytes := make([]byte, 16) | ||
| if _, err := rand.Read(tokenBytes); err != nil { | ||
| return oops.E(oops.CodeUnexpected, err, "generate auth token").Log(ctx, s.logger) | ||
| } | ||
| token := hex.EncodeToString(tokenBytes) | ||
|
|
||
| if _, err := s.repo.CreatePendingAuth(ctx, repo.CreatePendingAuthParams{ | ||
| SlackAppID: appID, | ||
| SlackUserID: slackUserID, | ||
| Token: token, | ||
| ChannelID: event.Event.Channel, | ||
| }); err != nil { | ||
| return oops.E(oops.CodeUnexpected, err, "create pending auth").Log(ctx, s.logger) | ||
| } | ||
|
|
||
| loginURL := fmt.Sprintf("%s/slack/login/%s", s.cfg.GramServerURL, token) | ||
|
|
||
| decryptedBotToken, err := s.enc.Decrypt(app.SlackBotToken.String) | ||
| if err != nil { | ||
| return oops.E(oops.CodeUnexpected, err, "decrypt bot token for ephemeral").Log(ctx, s.logger) | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 Auth gating bypassed when Slack event has empty User field
When event.Event.User is empty, the user authentication check is skipped entirely, but the event is still processed by the workflow.
Root Cause
At server/internal/thirdparty/slack/impl.go:263-316, the auth gating logic is wrapped in if slackUserID != "". When slackUserID is empty (which can happen for certain Slack event subtypes like bot messages or system messages that pass through the processEvent filter), execution falls through to line 312 where ExecuteProcessSlackEventWorkflow is called without any user mapping check.
The intended behavior per the PR description is: "unmapped Slack users receive an ephemeral login link; only linked users trigger tool calls." But events with an empty user ID bypass this gate entirely.
Impact: Events without a user ID will be processed without authentication, defeating the purpose of the user authentication gating feature.
(Refers to lines 263-310)
Prompt for agents
In server/internal/thirdparty/slack/impl.go, around lines 263-316 in SlackAppEventHandler, restructure the auth gating so that events with an empty User field are not processed by the workflow. The simplest fix is to require a non-empty slackUserID for event processing:
Change the logic from:
if processEvent {
slackUserID := event.Event.User
if slackUserID != "" {
// ... auth check that returns early if unmapped ...
}
// falls through to execute workflow
}
To:
if processEvent {
slackUserID := event.Event.User
if slackUserID == "" {
// Skip events without a user ID
w.WriteHeader(http.StatusOK)
return nil
}
_, err := s.repo.GetSlackUserMapping(...)
if err != nil {
// ... unmapped user handling (ephemeral link) ...
w.WriteHeader(http.StatusOK)
return nil
}
// User is mapped, proceed
if _, err := background.ExecuteProcessSlackEventWorkflow(...); err != nil {
...
}
}
Was this helpful? React with 👍 or 👎 to provide feedback.
| return oops.E(oops.CodeUnauthorized, fmt.Errorf("no auth context"), "authentication required").Log(ctx, s.logger) | ||
| } | ||
|
|
||
| gramUserID, err := uuid.Parse(authCtx.UserID) | ||
| if err != nil { | ||
| return oops.E(oops.CodeUnexpected, err, "parse user ID").Log(ctx, s.logger) | ||
| } | ||
|
|
||
| pendingAuth, err := s.repo.GetPendingAuthByToken(ctx, token) | ||
| if err != nil { | ||
| return oops.E(oops.CodeNotFound, err, "pending auth not found").Log(ctx, s.logger) |
There was a problem hiding this comment.
🟡 Concurrent auth completion can overwrite Slack user mapping
CompleteSlackAuth reads a pending token, upserts slack_user_mappings, then marks the token completed in separate statements without transactional locking. Two near-simultaneous requests for the same token can both pass the pending check and both execute the upsert, allowing the second request to overwrite gram_user_id.
Root Cause
In server/internal/thirdparty/slack/impl.go:394-409, the flow is:
GetPendingAuthByToken(... status='pending')CreateSlackUserMapping(... ON CONFLICT ... DO UPDATE SET gram_user_id=...)CompletePendingAuth(...)
Because these are not wrapped in a transaction with row locking, duplicate submits race. The mapping upsert in server/internal/thirdparty/slack/queries.sql:131-135 makes the overwrite explicit.
Actual: duplicate/racing completion attempts can rebind a Slack user to a different Gram user.
Expected: first successful completion should win; subsequent attempts should be rejected idempotently and must not change existing mapping.
Impact: edge-case account-link integrity issue (token replay/race window) and inconsistent behavior under duplicate link attempts.
Prompt for agents
In server/internal/thirdparty/slack/impl.go, make CompleteSlackAuth atomic. Wrap token lookup, mapping creation, and completion update in a single DB transaction and lock the pending-auth row (`SELECT ... FOR UPDATE`) before mutating. Change the behavior so once a pending token is completed, subsequent attempts are treated as already-used and do not update existing mappings. Also adjust server/internal/thirdparty/slack/queries.sql if needed to add a transactional query for pending-auth lock/read and a create-mapping variant that does not overwrite an existing mapping during duplicate completion attempts.
Was this helpful? React with 👍 or 👎 to provide feedback.
qstearns
force-pushed
the
slack-apps-auth/3-user-linking
branch
from
c50eb84 to
bf86e58
Compare
Drop slack_app_connections and add two new tables for user-level authentication gating of Slack bot interactions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Gate Slack event processing behind user mapping — unmapped Slack users receive an ephemeral login link instead of triggering tool calls. Adds auth info/complete endpoints and a browser-based SlackLogin page. Co-Authored-By: Claude Opus 4.6 <[email protected]>
qstearns
force-pushed
the
slack-apps-auth/3-user-linking
branch
from
bf86e58 to
23e0327
Compare
|
|
||||||||||||||||
- Fix agent_client.go tool call bug: assistant messages with tool calls were being treated as final responses, skipping tool execution - Add GramAppID to SlackEvent so activities look up apps by UUID instead of team ID (supports multiple apps per workspace) - Add GetAppAuthInfoByID to slack client for UUID-based app lookup - Wire system_prompt from slack_apps through to chat completion - Add GetSlackRegistrationWithUser SQL query (joins to users table) - Replace unique index on slack_team_id with non-unique index - Add empty chat response guard in workflow - Add SlackRegister page, remove old SlackLogin page - Add SLUG_EXEMPT_PATHS for /slack/register in auth handler - Add sequence diagrams for provisioning and event user auth flows - Strip debug console.logs from Auth.tsx Co-Authored-By: Claude Opus 4.6 <[email protected]>
| console.log( | ||
| "[AuthHandler] REDIRECT: no projectSlug, going to default project →", | ||
| preferredProject, | ||
| ); |
There was a problem hiding this comment.
🟡 Debug console.log left in production code
A console.log statement was added to the AuthHandler component that will execute in production every time a user without a projectSlug in the URL is redirected to the default project. This logs the preferred project slug to the browser console on every such navigation.
| console.log( | |
| "[AuthHandler] REDIRECT: no projectSlug, going to default project →", | |
| preferredProject, | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const location = useLocation(); | ||
|
|
||
| if (session.session === "" && !import.meta.env.DEV) { | ||
| if (session.session === "" && !__GRAM_DEV_AUTH_BYPASS__) { |
| setState("registering"); | ||
|
|
||
| try { | ||
| const res = await fetch(`${getServerURL()}/rpc/slack-apps.register`, { |
There was a problem hiding this comment.
Should this use the sdk?
| CONSTRAINT slack_app_toolsets_slack_app_id_toolset_id_key UNIQUE (slack_app_id, toolset_id) | ||
| ); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS slack_registrations ( |
There was a problem hiding this comment.
does this represent slack apps or slack user <-> app connections?
|
|
||
| func (s *PostSlackMessage) Do(ctx context.Context, input PostSlackMessageInput) error { | ||
| authInfo, err := s.slackClient.GetAppAuthInfo(ctx, input.Event.TeamID) | ||
| gramAppID, err := uuid.Parse(input.Event.GramAppID) |
| httpMethod: ptr("GET"), | ||
| httpStatus: ptr(int32(200)), | ||
| httpRoute: ptr("/api/users"), | ||
| httpMethod: new("GET"), |
There was a problem hiding this comment.
This is the file that the pre commit hook breaks
You might have to disable it just to get this to not stop the build
|
|
||
| // GramAppID is the Gram-internal slack_apps.id, set by the event handler | ||
| // before dispatching to Temporal. Not part of the Slack JSON payload. | ||
| GramAppID string `json:"gram_app_id,omitempty"` |
There was a problem hiding this comment.
OKay i see this comment now, but i still don't fully get it
3 participants
Summary
slack_user_mappingsandslack_pending_authstables, auth info/complete endpoints, and a browser-based SlackLogin page.Test plan
🤖 Generated with Claude Code