feat(auth): align login with set-token; stop persisting client credentials#37
Conversation
Document the advanced rollout where one Slack app serves a whole team: each member authorizes it and gets their own per-user token. Covers the owner's one-time setup (scopes ceiling, redirect URL, public distribution, client credentials), the per-teammate `slk auth login`, the all-or-nothing consent screen, and treating the client secret as a shared credential. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the authentication flow to ensure that OAuth client_id and client_secret are used transiently and not persisted in the configuration file. It introduces a unified promptCtx for interactive field resolution across auth set-token and auth login commands, adds a --non-interactive flag to the login command, and updates documentation and tests accordingly. Feedback was provided regarding the derivation of file descriptors for terminal-dependent operations to ensure correct behavior when input streams are redirected.
| fd := int(os.Stdin.Fd()) | ||
| if f, ok := in.(*os.File); ok { | ||
| fd = int(f.Fd()) | ||
| } | ||
| return promptCtx{ | ||
| in: in, | ||
| errw: cmd.ErrOrStderr(), | ||
| fd: fd, | ||
| interactive: !nonInteractive && isTerminal(fd), | ||
| } |
There was a problem hiding this comment.
According to the general rules, terminal-dependent operations should derive the file descriptor from the reader itself if it is an *os.File, rather than defaulting to os.Stdin.Fd(). This ensures correct behavior when input streams are redirected. Additionally, interactivity should only be enabled if the input source is actually a terminal and a valid *os.File descriptor is available.
var fd int
f, ok := in.(*os.File)
if ok {
fd = int(f.Fd())
} else {
fd = -1
}
return promptCtx{
in: in,
errw: cmd.ErrOrStderr(),
fd: fd,
interactive: !nonInteractive && ok && isTerminal(fd),
}References
- When performing terminal-dependent operations (e.g., checking if a reader is a terminal), derive the file descriptor from the reader itself if it is an
*os.File, rather than defaulting toos.Stdin.Fd(). This ensures correct behavior when input streams are redirected.
There was a problem hiding this comment.
Addressed in the amended commit. The fd now falls back to -1 (not os.Stdin.Fd()) for a non-*os.File reader, so a redirected/in-memory input can't borrow os.Stdin's terminal status — isTerminal(-1) is false. I did not add the explicit ok && guard on interactive: the tests stub isTerminal over an in-memory reader to exercise the prompt path, and an ok guard would force interactive to false there regardless of the stub. With fd = -1 the isTerminal(fd) check already yields false for a non-file reader in production, so the guard is redundant for real usage (and cmd.InOrStdin() is os.Stdin, an *os.File, in production anyway).
…tials Bring `auth login` to the same interactive model as `set-token` via a shared promptCtx (internal/commands/auth.go + prompt.go): when a field is missing and stdin is a TTY, login prompts for profile / workspace / client id / client secret (the secret entered hidden); flags remain the unchanged agent/script path. Add --non-interactive; non-interactive defaults the profile to "default" and prints a hint, and errors clearly when client id/secret are missing. Stop persisting the OAuth client id/secret: login used them only for the token exchange yet wrote them to config.toml. Remove the ClientID/ClientSecret profile fields (and their encrypt/decrypt + status handling); existing stored values are dropped on the next config write. Only user_token and bot_token are stored, encrypted at rest. Regenerate the slk-auth skill (login gains --non-interactive); sync README Credential storage + SPEC. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0336a17 to
c0b650e
Compare
Highlights since 0.5.1: - feat(auth): align `auth login` with `set-token`'s interactive model (shared promptCtx, --non-interactive) and stop persisting the OAuth client_id/client_secret (#37) - docs: Team setup guide; release-recovery runbook (#35, #37) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two commits, two concerns (merge with a merge commit to keep both on
main):feat(auth): alignauth loginwithset-token+ stop persisting client credentialsauth loginnow shares one interactive model withset-tokenvia a sharedpromptCtx: when a field is missing and stdin is a TTY, it prompts for profile / workspace / client id / client secret (secret entered hidden). Flags remain the unchanged agent/script path.--non-interactive; non-interactive defaults the profile todefault+ prints a hint, and errors clearly (exit 1) when client id/secret are missing — never blocks headless callers.client_id/client_secret— they were written toconfig.tomlbut only ever used transiently for the token exchange. Removed the profile fields (+ encrypt/decrypt + status handling); existing stored values drop on the next config write. Onlyuser_token/bot_tokenare stored, encrypted at rest.slk-authskill; syncs README "Credential storage" + SPEC.docs: Team setup guideDocuments sharing one Slack app across a team (one app, per-user tokens, owner one-time setup, per-teammate
auth login, all-or-nothing consent, secret-as-shared-credential).Verification
go vet, full uncachedgo test ./..., build all pass.🤖 Generated with Claude Code