feat: block auth/config when external credential provider is active#627
Open
MaxHuang22 wants to merge 6 commits intomainfrom
Open
feat: block auth/config when external credential provider is active#627MaxHuang22 wants to merge 6 commits intomainfrom
MaxHuang22 wants to merge 6 commits intomainfrom
Conversation
…providers Change-Id: Ie17a4b714e5eca17ae574ac188d570721790107d
…l credential providers Change-Id: I8f2ea0af6fe6506b29beb69264b04c21c0f75da1
…rovider is active Change-Id: If215cb8f0a53cc92d623dd3d842e4465124af2be
…der is active Change-Id: Ia61184fb2daeb6a7a38d122c647b7cb67eaf8b1f
…mmand behaviour Change-Id: I6d4b3c7d9d9c7b10fc2482fdc80252bf051771ee
github-actions
Bot
added
the
size/L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #627 +/- ##
==========================================
+ Coverage 59.94% 62.17% +2.22%
==========================================
Files 405 407 +2
Lines 42673 35839 -6834
==========================================
- Hits 25580 22282 -3298
+ Misses 15084 11526 -3558
- Partials 2009 2031 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds runtime checks that block interactive Changes
Sequence DiagramsequenceDiagram
participant User
participant Cmd as Auth/Config<br/>Command
participant PreRun as PersistentPreRunE<br/>Hook
participant Factory as cmdutil.Factory
participant Cred as CredentialProvider
participant Ext as ExternalProvider
User->>Cmd: run subcommand
Cmd->>PreRun: invoke PersistentPreRunE
PreRun->>Factory: RequireBuiltinCredentialProvider(ctx, "auth"/"config")
Factory->>Cred: ActiveExtensionProviderName(ctx)
Cred->>Ext: ResolveAccount(ctx)
alt external provider engaged (account or BlockError)
Ext-->>Cred: account or BlockError
Cred-->>Factory: provider name (non-empty)
Factory-->>PreRun: ExitValidation error (Detail.Type="external_provider")
PreRun-->>Cmd: return error (SilenceUsage set on matched subcommand)
Cmd-->>User: execution blocked with validation error
else no external provider engaged
Ext-->>Cred: nil (no account) or no engagement
Cred-->>Factory: "" (no provider)
Factory-->>PreRun: nil
PreRun-->>Cmd: continue
Cmd-->>User: command proceeds
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@61facfc531f55de70aa9b806c3051e23c6153336🧩 Skill updatenpx skills add larksuite/cli#feat/auth-login-block -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/auth_test.go`:
- Around line 351-354: The test currently pre-sets cmd.SilenceUsage = true which
masks the new PersistentPreRunE behavior in NewCmdAuth; remove or stop setting
cmd.SilenceUsage (leave it false) in the test, optionally discard command stderr
using io.Discard (add io to imports) so test output stays clean, run the command
with cmd.SetArgs(tt.args) and then assert that cmd.SilenceUsage (or the side
effect set by PersistentPreRunE) is true after execution to verify the new
behavior; keep cmd.SilenceErrors handling as needed but do not preconfigure
SilenceUsage before exercising PersistentPreRunE.
In `@cmd/config/config_test.go`:
- Around line 383-386: The test currently pre-sets cmd.SilenceUsage masking the
PersistentPreRunE guard; remove the line that sets SilenceUsage before running
NewCmdConfig/Execute, add io to imports and call cmd.SetErr(io.Discard) to keep
stderr quiet, then call cmd.Execute() and assert that cmd.SilenceUsage is true
(and keep SilenceErrors and cmd.SetArgs as-is); this verifies the new
PersistentPreRunE behavior rather than hiding it.
In `@internal/cmdutil/factory.go`:
- Around line 214-217: Add a unit test that verifies
RequireBuiltinCredentialProvider propagates provider errors: create a sentinel
error and a stub provider (e.g., stubExtProvider with name "env" that returns
the sentinel), build a credential provider set
(credential.NewCredentialProvider) and assign it to f.Credential from
TestFactory, then call f.RequireBuiltinCredentialProvider(ctx, "auth") and
assert the returned error is the sentinel using errors.Is; this ensures the
error produced by f.Credential.ActiveExtensionProviderName (and the
unexpected-error path) is not swallowed.
In `@internal/credential/credential_provider.go`:
- Around line 348-355: When returning the active provider string from the
credential check, ensure it is never empty: inside the errors.As branch for
extcred.BlockError (and in the acct != nil branch returning prov.Name()), if
blockErr.Provider or prov.Name() is empty, fall back first to prov.Name() (when
available) and then to a stable sentinel constant (e.g., "external-provider" or
"unknown-external-provider") so RequireBuiltinCredentialProvider cannot mistake
an engaged external provider for none; update the BlockError.Provider return
path and the prov.Name() return path to apply this fallback.
🪄 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: 4b66655f-9821-4aed-8f8d-bdb054b53e56
📒 Files selected for processing (8)
cmd/auth/auth.gocmd/auth/auth_test.gocmd/config/config.gocmd/config/config_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_test.gointernal/credential/credential_provider.gointernal/credential/credential_provider_test.go
- Use cmd.Find() to assert SilenceUsage on matched subcommand (not parent) - Add TestRequireBuiltinCredentialProvider_PropagatesProviderError for error path - Add 'external' fallback sentinel in ActiveExtensionProviderName Change-Id: Iba35779ad2ed9807556264ba23db7096541e2bf3
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cmdutil/factory_test.go (1)
407-414: Optional: make the "allows built-in" assertion explicit.
TestRequireBuiltinCredentialProvider_AllowsBuiltinProviderdepends on whateverTestFactorywires up forf.Credential. IfTestFactoryever returns aCredentialwith extension providers configured by default, this test silently becomes a no-op vs. the separateNilCredentialcase. Consider explicitly constructing acredential.NewCredentialProvider(nil, ...)with no extension providers to make the "built-in path" intent unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/factory_test.go` around lines 407 - 414, TestRequireBuiltinCredentialProvider_AllowsBuiltinProvider currently relies on TestFactory to supply f.Credential which could include extension providers; explicitly construct a credential provider with no extension providers (e.g., call credential.NewCredentialProvider(nil, <other-args>) or use the NilCredential variant) and pass it into TestFactory or assign it to f.Credential before calling f.RequireBuiltinCredentialProvider to ensure the test exercises the built-in path unambiguously; update the test so it references TestRequireBuiltinCredentialProvider_AllowsBuiltinProvider, TestFactory, RequireBuiltinCredentialProvider, and credential.NewCredentialProvider (or NilCredential) when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cmdutil/factory_test.go`:
- Around line 407-414:
TestRequireBuiltinCredentialProvider_AllowsBuiltinProvider currently relies on
TestFactory to supply f.Credential which could include extension providers;
explicitly construct a credential provider with no extension providers (e.g.,
call credential.NewCredentialProvider(nil, <other-args>) or use the
NilCredential variant) and pass it into TestFactory or assign it to f.Credential
before calling f.RequireBuiltinCredentialProvider to ensure the test exercises
the built-in path unambiguously; update the test so it references
TestRequireBuiltinCredentialProvider_AllowsBuiltinProvider, TestFactory,
RequireBuiltinCredentialProvider, and credential.NewCredentialProvider (or
NilCredential) when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d707ecb-365c-4ab8-aaf7-c2b5f7b2603f
📒 Files selected for processing (4)
cmd/auth/auth_test.gocmd/config/config_test.gointernal/cmdutil/factory_test.gointernal/credential/credential_provider.go
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
1 participant
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
When an external credential provider (env vars or sidecar) is active, the built-in
authandconfigcommand families are meaningless and potentially misleading — credentials are injected externally and the interactive management flow cannot function. This PR adds a guard that fails all subcommands under both families immediately with a structured error (exit 2, typeexternal_provider), directing the user or calling agent toward the appropriate authorization channel instead.Changes
internal/credential/credential_provider.go: addActiveExtensionProviderNameto probe extension providers directly (independent of the sync.Once cache)internal/cmdutil/factory.go: addRequireBuiltinCredentialProviderguard helper returning a structuredErrWithHinterrorcmd/auth/auth.go: wirePersistentPreRunEon theauthparent command; addcmd.SilenceUsage = trueto replicate root-level behaviourcmd/config/config.go: same guard wired on theconfigparent commandcredential_provider_test.go, 3 infactory_test.go, 6-subtestTestAuthBlockedByExternalProviderinauth_test.go, 5-subtestTestConfigBlockedByExternalProviderinconfig_test.go, 15 E2E scenarios intests_e2e/auth/Test Plan
make unit-testpassedmake validatepassedLARKSUITE_CLI_APP_ID=fake lark-cli auth login→ exit 2,{"ok":false,"error":{"type":"external_provider",...}}Related Issues
N/A
Summary by CodeRabbit
Bug Fixes
Tests