fix(auth): explain invalid scopes in auth login#427
Open
cookier wants to merge 2 commits intolarksuite:mainfrom
Open
fix(auth): explain invalid scopes in auth login#427cookier wants to merge 2 commits intolarksuite:mainfrom
cookier wants to merge 2 commits intolarksuite:mainfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds scope-diagnostic logic and tests that detect unknown or app-disabled OAuth scopes during device authorization errors and returns a detailed, user-facing auth error instead of the prior generic message. Changes
Sequence DiagramsequenceDiagram
participant LoginCmd as Login Command
participant ScopeValidator as Scope Validator
participant AppInfo as App Info Loader
participant Registry as Scope Registry
LoginCmd->>LoginCmd: RequestDeviceAuthorization(requestedScopes)
LoginCmd-->>LoginCmd: receives error ("invalid or malformed scope")
LoginCmd->>ScopeValidator: explainScopeRequestError(err, requestedScopes, config)
alt error matches scope-validation pattern
ScopeValidator->>AppInfo: loadAppInfo(config.AppID)
AppInfo-->>ScopeValidator: enabled scopes or error
ScopeValidator->>Registry: CollectAllScopesFromMeta("user")
Registry-->>ScopeValidator: known scopes
ScopeValidator->>ScopeValidator: diagnoseRequestedScopes(requestedScopes, known, enabled)
ScopeValidator->>ScopeValidator: suggestScopes(...) and format message
ScopeValidator-->>LoginCmd: *output.ExitError (auth) with diagnostics*
else other error
ScopeValidator-->>LoginCmd: nil (no special handling)
end
LoginCmd->>LoginCmd: return formatted auth error or fallback error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
the
size/L
Greptile SummaryThis PR adds scope-error diagnostics to
Confidence Score: 5/5Safe to merge; all remaining findings are P2 quality improvements that do not affect correctness. The core logic for detecting and explaining invalid scopes is correct. The login.go change is minimal. Both P2 findings (dropping original error in fallback message, test coupling to live registry) are improvement suggestions that do not cause wrong behavior in production scenarios. No P0/P1 issues were found. cmd/auth/login_scope_validation.go (fallback path drops original error); cmd/auth/login_scope_validation_test.go (three tests couple to live registry data) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["RequestDeviceAuthorization fails"] --> B["explainScopeRequestError called"]
B --> C{"isInvalidScopeError?"}
C -- No --> D["return nil\n(caller shows generic ErrAuth)"]
C -- Yes --> E["loadAppInfo to get enabled scopes"]
E --> F{"Info returned OK?"}
F -- Yes --> G["enabled = app's user scopes map"]
F -- No --> H["appInfoErr = err\nenabled = nil"]
G --> I["diagnoseRequestedScopes\n(against known registry + enabled)"]
H --> I
I --> J{"Unknown or\nNotEnabled > 0?"}
J -- Yes --> K["Build per-scope error lines\n(with suggestions)\nreturn ErrAuth"]
J -- No --> L{"appInfoErr != nil?"}
L -- Yes --> M["return ErrAuth:\n'could not be fully diagnosed'\n(original requestErr dropped)"]
L -- No --> N["return nil\n(caller shows generic ErrAuth)"]
Reviews (3): Last reviewed commit: "fix(auth): tighten invalid-scope diagnos..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/auth/login_scope_validation_test.go (1)
46-71: Add one test that exercises the app-enabled-scope branch.Current assertions validate unknown/suggestion formatting, but there’s no case proving
NotEnableddiagnostics when app info fetch succeeds. Please add a mockedgetAppInfosuccess path and assert the “scope not enabled for current app” line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_scope_validation_test.go` around lines 46 - 71, Add a new unit test that covers the app-enabled-scope branch of explainScopeRequestError by mocking getAppInfo to return a successful app info (so diagnostics of type NotEnabled are produced) and asserting the returned error message contains the "scope not enabled for current app" / "not enabled for this app" hint; locate and update the test file with a new test (similar to TestExplainScopeRequestError_FormatsDetailedValidation) that calls explainScopeRequestError with a scope that will trigger NotEnabled diagnostics, inject a TestFactory or mock that overrides getAppInfo to succeed, and add assertions that the error string includes the NotEnabled message and any related "lark-cli auth scopes" hint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/login_scope_validation.go`:
- Around line 33-44: The current logic skips disabled-scope detection when
getAppInfo(ctx, f, config.AppID) fails (enabled stays nil), which can cause
known-but-disabled scopes to be treated as valid; change the code so that when
getAppInfo returns an error or nil info you still initialize enabled as an empty
map (e.g., enabled = make(map[string]bool, 0)) before calling
diagnoseRequestedScopes(requestedScope, knownUserScopes(), enabled) so
diagnoseReportedScopes can correctly populate NotEnabled; optionally log the
getAppInfo error for diagnostics but do not let a lookup failure short-circuit
scope validation.
---
Nitpick comments:
In `@cmd/auth/login_scope_validation_test.go`:
- Around line 46-71: Add a new unit test that covers the app-enabled-scope
branch of explainScopeRequestError by mocking getAppInfo to return a successful
app info (so diagnostics of type NotEnabled are produced) and asserting the
returned error message contains the "scope not enabled for current app" / "not
enabled for this app" hint; locate and update the test file with a new test
(similar to TestExplainScopeRequestError_FormatsDetailedValidation) that calls
explainScopeRequestError with a scope that will trigger NotEnabled diagnostics,
inject a TestFactory or mock that overrides getAppInfo to succeed, and add
assertions that the error string includes the NotEnabled message and any related
"lark-cli auth scopes" hint.
🪄 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: 13d0b7aa-8497-4e0a-9ec6-d4f2cedee9a0
📒 Files selected for processing (3)
cmd/auth/login.gocmd/auth/login_scope_validation.gocmd/auth/login_scope_validation_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/auth/login_scope_validation.go (1)
37-44:⚠️ Potential issue | 🟡 MinorTreat a
nilapp-info result as an inspection failure.If
loadAppInforeturnsinfo == nilwitherr == nil, bothenabledandappInfoErrstay nil. In that state, disabled-scope inspection is skipped and this helper can still fall back to the generic auth error instead of the detailed scope diagnostics.💡 Proposed fix
if info, err := loadAppInfo(ctx, f, config.AppID); err == nil && info != nil { enabled = make(map[string]bool, len(info.UserScopes)) for _, scope := range info.UserScopes { enabled[scope] = true } - } else { + } else if err != nil { appInfoErr = err + } else { + appInfoErr = fmt.Errorf("enabled app scopes could not be inspected automatically") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_scope_validation.go` around lines 37 - 44, When calling loadAppInfo, handle the case where err==nil but info==nil by treating it as a failure: if info is nil set appInfoErr to a non-nil error (e.g., fmt.Errorf("loadAppInfo returned nil info for app %s", config.AppID)) so the later disabled-scope inspection runs; keep the existing population of enabled only when info != nil and err == nil, and ensure you reference loadAppInfo, info, enabled and appInfoErr in the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/auth/login_scope_validation.go`:
- Around line 37-44: When calling loadAppInfo, handle the case where err==nil
but info==nil by treating it as a failure: if info is nil set appInfoErr to a
non-nil error (e.g., fmt.Errorf("loadAppInfo returned nil info for app %s",
config.AppID)) so the later disabled-scope inspection runs; keep the existing
population of enabled only when info != nil and err == nil, and ensure you
reference loadAppInfo, info, enabled and appInfoErr in the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6203a25d-56d5-4629-9a2f-5581ce527cfc
📒 Files selected for processing (2)
cmd/auth/login_scope_validation.gocmd/auth/login_scope_validation_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/auth/login_scope_validation_test.go
cookier
force-pushed
the
fix/auth-login-invalid-scope-diagnostics
branch
from
1644069 to
ea800fe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improve lark-cli auth login --scope error reporting when device authorization fails because the requested scope list contains invalid entries.
Instead of only surfacing the upstream OAuth error, this change diagnoses the requested scope list locally and reports:
Changes
Test Plan
Note: the full ./cmd/auth package currently still hits a pre-existing failure in TestAuthLoginRun_MissingRequestedScopeAlignsWithLoginSuccess related to strict mode / log file cleanup on this machine. The new #416-focused tests pass.
Related Issues
Closes #416
Summary by CodeRabbit
New Features
Tests