feat: Add --token options to caib login#305
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds saved-token persistence/normalization, a login ChangesToken Persistence and Login Caching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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 |
|
@ambient-code please review |
|
I am not sure why do we need to match jmp's options? |
It's not really a match, more of a general consolidation. I'd view it as a caib login improvement rather than something that needs to be mapped directly to jumpstarter. I was mainly referring to the issue description I linked. |
|
I think the only one of these that makes sense is --token, the other two don't add much, especially --nointeractive, machine users aren't expected to login... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/caib/config/config.go (1)
226-238: 💤 Low valueConsider simplifying bearer prefix stripping.
The current implementation uses
strings.Fieldsandstrings.Join, which could theoretically alter tokens with embedded spaces (though real JWTs and opaque tokens don't contain spaces). A simpler approach would be:func normalizeToken(token string) string { trimmed := strings.TrimSpace(token) if trimmed == "" { return "" } // Case-insensitive bearer prefix removal if len(trimmed) > 7 && strings.EqualFold(trimmed[:7], "bearer ") { return strings.TrimSpace(trimmed[7:]) } return trimmed }This avoids splitting on all whitespace and is clearer about intent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/config/config.go` around lines 226 - 238, The normalizeToken function should avoid splitting the token on all whitespace; instead detect and strip a case-insensitive "bearer " prefix directly to preserve any internal spacing and make intent clearer: in normalizeToken, after trimming (strings.TrimSpace), check the trimmed string length and use strings.EqualFold on the first 7 characters to match "bearer " and then return the substring trimmed of leading/trailing space; otherwise return the trimmed input. Update the logic in normalizeToken to use this prefix-slice approach instead of strings.Fields/strings.Join.cmd/caib/common/api_client.go (1)
239-252: 💤 Low valueCode duplication to avoid circular import is reasonable here.
The
sanitizeTokenhelper duplicatesconfig.normalizeToken(as noted in the comment) to avoid a circular dependency between thecommonandconfigpackages. This is a pragmatic tradeoff.Alternative for future consideration: If this logic needs updates, consider extracting both functions to a shared internal
tokenutilpackage to maintain a single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/common/api_client.go` around lines 239 - 252, Keep the local sanitizeToken function but update its comment to note the duplication with config.normalizeToken and add a clear TODO to extract both to a shared internal package (e.g., internal/tokenutil) if the logic changes; reference the symbols sanitizeToken and config.normalizeToken in the comment so future maintainers know they must be kept in sync and where to consolidate them to avoid the circular import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/caib/auth/oidc.go`:
- Around line 466-508: The exported SavePreObtainedToken currently has no
non-test call sites and unsafely stores the token issuer without validation;
either make it unexported (savePreObtainedToken) or hook it into the CLI/--token
code path so it is actually used, and add issuer validation by accepting an
expectedIssuer parameter (or reading configured issuer) and comparing it to the
parsed claims["iss"] before writing TokenCache.Issuer; update references to
TokenCache, tokenCachePath, and any CLI/token handling functions to pass the
expected issuer or call the unexported helper accordingly, and return an error
when the issuers do not match.
In `@cmd/caib/login.go`:
- Line 48: The flag help mentions an environment variable but the code never
reads it; update runLogin to support CAIB_TOKEN by checking loginToken and, if
empty, assigning strings.TrimSpace(os.Getenv("CAIB_TOKEN")) so the CLI honours
the env var; ensure the flag is still registered via
cmd.Flags().StringVar(&loginToken, "token", ...) and reference loginToken and
runLogin when adding this env lookup (mirroring how config.DefaultServer() reads
CAIB_SERVER).
---
Nitpick comments:
In `@cmd/caib/common/api_client.go`:
- Around line 239-252: Keep the local sanitizeToken function but update its
comment to note the duplication with config.normalizeToken and add a clear TODO
to extract both to a shared internal package (e.g., internal/tokenutil) if the
logic changes; reference the symbols sanitizeToken and config.normalizeToken in
the comment so future maintainers know they must be kept in sync and where to
consolidate them to avoid the circular import.
In `@cmd/caib/config/config.go`:
- Around line 226-238: The normalizeToken function should avoid splitting the
token on all whitespace; instead detect and strip a case-insensitive "bearer "
prefix directly to preserve any internal spacing and make intent clearer: in
normalizeToken, after trimming (strings.TrimSpace), check the trimmed string
length and use strings.EqualFold on the first 7 characters to match "bearer "
and then return the substring trimmed of leading/trailing space; otherwise
return the trimmed input. Update the logic in normalizeToken to use this
prefix-slice approach instead of strings.Fields/strings.Join.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78b63d08-7a1a-4115-a6f0-9f80b82291a2
📒 Files selected for processing (13)
cmd/caib/auth/oidc.gocmd/caib/auth/wrapper.gocmd/caib/auth/wrapper_test.gocmd/caib/authcmd/authcmd.gocmd/caib/authcmd/authcmd_test.gocmd/caib/common/api_client.gocmd/caib/common/api_client_test.gocmd/caib/config/config.gocmd/caib/config/config_test.gocmd/caib/container/utilities.gocmd/caib/login.gocmd/caib/login_test.gocmd/caib/root.go
💤 Files with no reviewable changes (1)
- cmd/caib/root.go
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice work closing the feature gap between caib login and jmp login. The token caching, --nointeractive flag, and improved error messages are well thought-out — especially the guard in ExecuteWithReauth that prevents silently falling through to OIDC when an explicit token is rejected.
Test coverage is solid across all new paths. A few things to consider below.
| // If the pointer value exactly matches the env var, treat it as implicit | ||
| // and allow saved_token to take precedence for "subsequent commands" | ||
| // after `caib login --token`. | ||
| if tokenValue != "" && envToken != "" && tokenValue == envToken { |
There was a problem hiding this comment.
This heuristic is clever but fragile: if a user explicitly passes --token X where X happens to equal CAIB_TOKEN, the saved token silently wins over their explicit flag value.
A more robust approach would be to thread a tokenExplicit bool parameter (or use cmd.Flags().Changed("token") at the call site) so CreateBuildAPIClient can distinguish "user typed it" from "cobra set the default." That said, the current approach works for the common case — just worth a // CAVEAT: note for future maintainers.
we also have this issue open (though I'm not sure how relevant it still is): #78. If it's no longer needed, we can drop the nointeractive, also i guess with the comment you added, I think --token should cover that use case as well. I also think it's worth keeping --endpoint. I know it doesn't address a functional gap, but I think it improves readability and makes the command more explicit. when running something like this it's clearer to have the option to specify the endpoint directly if the user wants to:
|
i'll close the issue --token already exists, and is paired with --server rather than endpoint, endpoint is a jumpstarter concept |
So do you want to keep only the token option as part of the login or should I close the PR and related issues? |
let's keep --token |
bkhizgiy
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/caib/login_test.go (1)
14-56: 🏗️ Heavy liftConsider adding an integration test for the token-saving behavior.
The unit tests verify flag registration and argument validation, but the core functionality added in this PR—saving a token to config and bypassing OIDC—is not tested. An integration test should:
- Set up a temporary config directory (similar to
TestLoginSavesConfigWhenServerReachable)- Execute
runLoginwith a non-emptyloginToken- Verify the token is saved to
cli.jsonviaconfig.LoadSavedToken()- Confirm the early return prevents OIDC authentication
This would ensure the critical path introduced by the
--tokenflag works correctly across config persistence and command flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/login_test.go` around lines 14 - 56, Add an integration test that verifies the --token flow: create a temp config dir like TestLoginSavesConfigWhenServerReachable, call runLogin (or execute newLoginCmd with the token flag set) with a non-empty loginToken, ensure config.LoadSavedToken() reads the saved token from cli.json, and assert that runLogin returns early (bypassing OIDC) by checking no OIDC flow was invoked (e.g., no auth server started or mock OIDC not called); use the existing TestLoginSavesConfigWhenServerReachable setup as reference for temp dir and file assertions and reference runLogin, newLoginCmd, loginToken, and config.LoadSavedToken in your test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/caib/login_test.go`:
- Around line 14-56: Add an integration test that verifies the --token flow:
create a temp config dir like TestLoginSavesConfigWhenServerReachable, call
runLogin (or execute newLoginCmd with the token flag set) with a non-empty
loginToken, ensure config.LoadSavedToken() reads the saved token from cli.json,
and assert that runLogin returns early (bypassing OIDC) by checking no OIDC flow
was invoked (e.g., no auth server started or mock OIDC not called); use the
existing TestLoginSavesConfigWhenServerReachable setup as reference for temp dir
and file assertions and reference runLogin, newLoginCmd, loginToken, and
config.LoadSavedToken in your test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1619e0ae-e643-4b3d-acc0-b685a37e5a6d
📒 Files selected for processing (9)
cmd/caib/authcmd/authcmd.gocmd/caib/authcmd/authcmd_test.gocmd/caib/common/api_client.gocmd/caib/common/api_client_test.gocmd/caib/config/config.gocmd/caib/config/config_test.gocmd/caib/login.gocmd/caib/login_test.gocmd/caib/root.go
💤 Files with no reviewable changes (1)
- cmd/caib/root.go
✅ Files skipped from review due to trivial changes (2)
- cmd/caib/authcmd/authcmd_test.go
- cmd/caib/authcmd/authcmd.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/caib/config/config_test.go
- cmd/caib/common/api_client_test.go
- cmd/caib/common/api_client.go
- cmd/caib/config/config.go
maboras-rh
left a comment
maboras-rh
left a comment
There was a problem hiding this comment.
Looks good overall, with a few minor comments - check if any of them are worth addressing.
- in
ExecuteWithReauth-currentTokencondition check - Duplicated
sanitizeToken/normalizeToken- https://github.com/centos-automotive-suite/automotive-dev-operator/pull/305/changes#r3394997983 - token env var equality check - https://github.com/centos-automotive-suite/automotive-dev-operator/pull/305/changes#r3394997986
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: claude-sonnet-4.6
3 participants
Summary
Add new option to caib login:
--tokenflag saves the token to ~/.config/caib/cli.json so all subsequent commands use it supports oc whoami -t opaque tokens and JWTsRelated Issues
#160
#161
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
New Features
Bug Fixes
Documentation