feat(auth): support multiple users by upserting on login#232
feat(auth): support multiple users by upserting on login#232mio4kon wants to merge 2 commits intolarksuite:mainfrom
Conversation
- Replace overwrite logic with upsertUser helper: existing users with the same UserOpenId are updated in place, others are preserved. - Add resolveActiveUser in config: respects LARKSUITE_CLI_USER_OPEN_ID env var to select the active user; falls back to first user if unset.
github-actions
bot
added
the
size/L
📝 WalkthroughWalkthroughReplaced destructive user-list updates with an insert-or-update ("upsert") approach during login, added environment-variable-based active-user resolution and a user-removal helper in config, and adjusted logout to operate on the resolved active user only. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR replaces the single-user overwrite pattern on login with an Confidence Score: 5/5Safe to merge; only one P2 UX concern remains about a misleading error message in logout when the env var targets an unknown user. All three files are well-implemented. The upsertUser and RemoveUser helpers are correct, resolveActiveUser properly avoids fallback when the env var is set, and the token-cleanup removal is intentional. The sole remaining finding is a P2 UX issue (misleading message), which does not affect correctness or data integrity. cmd/auth/logout.go — misleading "Not logged in." message when LARKSUITE_CLI_USER_OPEN_ID is set but unmatched. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[auth login] --> B[upsertUser]
B --> C{UserOpenId exists in Users?}
C -- Yes --> D[Update in place]
C -- No --> E[Append new user]
D & E --> F[SaveMultiAppConfig]
G[auth logout] --> H[ResolveActiveUser]
H --> I{LARKSUITE_CLI_USER_OPEN_ID set?}
I -- Yes --> J{Match found?}
J -- Yes --> K[Return matched user]
J -- No --> L[Return nil - Not logged in]
I -- No --> M[Return Users 0]
K & M --> N[RemoveStoredToken]
N --> O[RemoveUser from slice]
O --> P[SaveMultiAppConfig]
Q[RequireConfig / RequireAuth] --> R[resolveActiveUser]
R --> I
Reviews (2): Last reviewed commit: "fix(auth): logout only removes the activ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return &users[i] | ||
| } | ||
| } | ||
| return nil // env var set but user not found — do not fall back |
There was a problem hiding this comment.
When LARKSUITE_CLI_USER_OPEN_ID is set but doesn't match any stored user, resolveActiveUser returns nil. RequireConfig propagates this as an empty UserOpenId, and RequireAuth surfaces the generic "not logged in" hint — with no indication that the env var itself is the problem. A user who sets the wrong open ID will be told to run lark-cli auth login, which won't help them.
Consider returning a specific error from RequireConfig when the env var is set but unmatched:
if u := resolveActiveUser(app.Users); u != nil {
cfg.UserOpenId = u.UserOpenId
cfg.UserName = u.UserName
} else if id := os.Getenv("LARKSUITE_CLI_USER_OPEN_ID"); id != "" {
return nil, &ConfigError{Code: 3, Type: "auth",
Message: fmt.Sprintf("user %q (from LARKSUITE_CLI_USER_OPEN_ID) not found in config", id)}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/login.go (1)
306-314:⚠️ Potential issue | 🟠 MajorLogout command clears all users, defeating multi-user support.
The login flow now correctly preserves existing users via
upsertUser, butcmd/auth/logout.go:60still wipes the entire user list withapp.Users = []core.AppUser{}. This means logging out removes all stored users, not just the active one.The function properly removes stored tokens for all users (lines 55–59), but then indiscriminately clears the user list from the config. The command's help text ("Log out (clear token)") and the presence of
LARKSUITE_CLI_USER_OPEN_IDenvironment variable for user selection suggest logout should remove only the active user's credentials.To complete multi-user support, logout should:
- Determine the active user (via
LARKSUITE_CLI_USER_OPEN_IDenv var if set, otherwise default to first user)- Remove only that user's token and entry from
app.Users- Preserve other users in the list
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 306 - 314, The logout command currently clears the entire app.Users slice; instead, determine the active user by reading LARKSUITE_CLI_USER_OPEN_ID (fall back to the first user if unset), keep the existing token-removal logic but target only that user's token, remove only that user's core.AppUser entry from app.Users (preserving others), and then persist changes via core.SaveMultiAppConfig; reference the logout handler in cmd/auth/logout.go, the env var LARKSUITE_CLI_USER_OPEN_ID, and the config helpers core.LoadMultiAppConfig/core.SaveMultiAppConfig and upsertUser-like behavior to implement per-user removal.
🧹 Nitpick comments (1)
cmd/auth/login.go (1)
381-389: Consider extracting duplicate config update logic.This block is nearly identical to lines 306-314. Both load the config, upsert the user, and save. Consider extracting a helper to reduce duplication:
func saveUserToConfig(openId, userName string) error { multi, _ := core.LoadMultiAppConfig() if multi == nil || len(multi.Apps) == 0 { return nil } app := &multi.Apps[0] app.Users = upsertUser(app.Users, core.AppUser{UserOpenId: openId, UserName: userName}) return core.SaveMultiAppConfig(multi) }Then both call sites become:
if err := saveUserToConfig(openId, userName); err != nil { return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 381 - 389, Extract the duplicated config update logic into a helper (e.g., saveUserToConfig) so both call sites reuse it: move the core.LoadMultiAppConfig(), nil/empty check, acquiring app := &multi.Apps[0], app.Users = upsertUser(... core.AppUser{UserOpenId: openId, UserName: userName}), and core.SaveMultiAppConfig(multi) into that function which returns an error; then replace both original blocks with a single call if err := saveUserToConfig(openId, userName); err != nil { return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) }.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 306-314: The logout command currently clears the entire app.Users
slice; instead, determine the active user by reading LARKSUITE_CLI_USER_OPEN_ID
(fall back to the first user if unset), keep the existing token-removal logic
but target only that user's token, remove only that user's core.AppUser entry
from app.Users (preserving others), and then persist changes via
core.SaveMultiAppConfig; reference the logout handler in cmd/auth/logout.go, the
env var LARKSUITE_CLI_USER_OPEN_ID, and the config helpers
core.LoadMultiAppConfig/core.SaveMultiAppConfig and upsertUser-like behavior to
implement per-user removal.
---
Nitpick comments:
In `@cmd/auth/login.go`:
- Around line 381-389: Extract the duplicated config update logic into a helper
(e.g., saveUserToConfig) so both call sites reuse it: move the
core.LoadMultiAppConfig(), nil/empty check, acquiring app := &multi.Apps[0],
app.Users = upsertUser(... core.AppUser{UserOpenId: openId, UserName:
userName}), and core.SaveMultiAppConfig(multi) into that function which returns
an error; then replace both original blocks with a single call if err :=
saveUserToConfig(openId, userName); err != nil { return
output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err)
}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9220e6ab-9832-451d-85b9-caa3beb9d562
📒 Files selected for processing (2)
cmd/auth/login.gointernal/core/config.go
|
|
- Export ResolveActiveUser and add RemoveUser helper in core package so logout can reuse the same active-user resolution logic. - authLogoutRun now removes token and config entry only for the active user (LARKSUITE_CLI_USER_OPEN_ID env var or first user), leaving other users intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/auth/logout.go (1)
61-63: Avoid printing user identifiers in warning output.Line 62 logs
active.UserOpenId. Prefer a generic message to reduce identifier exposure in shared logs.🔒 Minimal change
- fmt.Fprintf(f.IOStreams.ErrOut, "Warning: failed to remove token for %s: %v\n", active.UserOpenId, err) + fmt.Fprintf(f.IOStreams.ErrOut, "Warning: failed to remove active user token: %v\n", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/logout.go` around lines 61 - 63, The warning currently prints a user identifier via active.UserOpenId; change the fmt.Fprintf call in the larkauth.RemoveStoredToken error branch so it does not include the user ID. Use a generic message like "Warning: failed to remove stored token: %v\n" (still including err) and keep writing to f.IOStreams.ErrOut; update the call referencing larkauth.RemoveStoredToken and f.IOStreams.ErrOut accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/config.go`:
- Around line 158-169: The function resolveActiveUser currently returns nil for
both "no users exist" and "env-selected user not found", making callers unable
to distinguish the cases; change resolveActiveUser to return (*AppUser, error)
(or (*AppUser, bool, error)) and introduce distinct sentinel errors (e.g.,
ErrNoUsers and ErrSelectedUserNotFound) so callers can emit precise messages;
update the resolveActiveUser signature and its callers, populate ErrNoUsers when
len(users)==0 and return ErrSelectedUserNotFound when LARKSUITE_CLI_USER_OPEN_ID
is set but no matching AppUser (still return nil user), and ensure any new error
types are exported where needed.
---
Nitpick comments:
In `@cmd/auth/logout.go`:
- Around line 61-63: The warning currently prints a user identifier via
active.UserOpenId; change the fmt.Fprintf call in the larkauth.RemoveStoredToken
error branch so it does not include the user ID. Use a generic message like
"Warning: failed to remove stored token: %v\n" (still including err) and keep
writing to f.IOStreams.ErrOut; update the call referencing
larkauth.RemoveStoredToken and f.IOStreams.ErrOut accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21511091-ed0f-4210-8448-c8af191e4e31
📒 Files selected for processing (2)
cmd/auth/logout.gointernal/core/config.go
| func resolveActiveUser(users []AppUser) *AppUser { | ||
| if len(users) == 0 { | ||
| return nil | ||
| } | ||
| if id := os.Getenv("LARKSUITE_CLI_USER_OPEN_ID"); id != "" { | ||
| for i := range users { | ||
| if users[i].UserOpenId == id { | ||
| return &users[i] | ||
| } | ||
| } | ||
| return nil // env var set but user not found — do not fall back | ||
| } |
There was a problem hiding this comment.
Differentiate “no users” from “selected user not found.”
On Line 159 and Line 168, nil is returned for two different states. Callers can’t tell whether no one is logged in or LARKSUITE_CLI_USER_OPEN_ID is invalid, so they can only emit a generic message.
💡 Suggested refactor
+var ErrActiveUserNotFound = errors.New("active user not found")
+
-func ResolveActiveUser(users []AppUser) *AppUser {
- return resolveActiveUser(users)
+func ResolveActiveUser(users []AppUser) (*AppUser, error) {
+ return resolveActiveUser(users)
}
-func resolveActiveUser(users []AppUser) *AppUser {
+func resolveActiveUser(users []AppUser) (*AppUser, error) {
if len(users) == 0 {
- return nil
+ return nil, nil
}
if id := os.Getenv("LARKSUITE_CLI_USER_OPEN_ID"); id != "" {
for i := range users {
if users[i].UserOpenId == id {
- return &users[i]
+ return &users[i], nil
}
}
- return nil // env var set but user not found — do not fall back
+ return nil, ErrActiveUserNotFound
}
- return &users[0]
+ return &users[0], nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config.go` around lines 158 - 169, The function
resolveActiveUser currently returns nil for both "no users exist" and
"env-selected user not found", making callers unable to distinguish the cases;
change resolveActiveUser to return (*AppUser, error) (or (*AppUser, bool,
error)) and introduce distinct sentinel errors (e.g., ErrNoUsers and
ErrSelectedUserNotFound) so callers can emit precise messages; update the
resolveActiveUser signature and its callers, populate ErrNoUsers when
len(users)==0 and return ErrSelectedUserNotFound when LARKSUITE_CLI_USER_OPEN_ID
is set but no matching AppUser (still return nil user), and ensure any new error
types are exported where needed.
2 participants
#204
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Changes