Add default CLI app keys management support in credentials#1350
Merged
PasanT9 merged 4 commits intowso2:masterfrom Mar 26, 2026
Merged
Add default CLI app keys management support in credentials#1350PasanT9 merged 4 commits intowso2:masterfrom
PasanT9 merged 4 commits intowso2:masterfrom
Conversation
manodyaSenevirathne
requested review from
CrowleyRajapakse,
ashera96 and
npamudika
as code owners
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded persistent storage for a default CLI application's consumer credentials: new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (getKeys)"
participant Store as "JsonStore (credentials)"
participant Keygen as "Key Generation API"
participant Token as "Token Generator"
CLI->>Store: GetDefaultAppKeys(env)
alt keys found
Store-->>CLI: consumerKey, consumerSecret
CLI->>Token: getNewToken(using consumerKey/secret)
else keys missing
CLI->>Keygen: createAppOrRegenerate()
Keygen-->>CLI: ConsumerKey, ConsumerSecret
CLI->>Store: SetDefaultAppKeys(env, consumerKey, consumerSecret)
Store-->>CLI: persist OK
CLI->>Token: getNewToken(using new consumerKey/secret)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@import-export-cli/credentials/credentials.go`:
- Around line 65-68: When EraseAPIM clears APIM credentials it must also remove
any APIM-scoped DefaultApp; update the EraseAPIM implementation in
import-export-cli/credentials/jsonstore.go so that after setting
environment.APIM to an empty/zero value it also sets environment.DefaultApp =
nil (or deletes the DefaultApp entry) on the Environment struct, ensuring
DefaultApp is cleared when APIM is erased.
In `@import-export-cli/credentials/jsonstore.go`:
- Around line 214-221: The SetDefaultAppKeys method currently persists a
long-lived consumer secret without notifying the user; update
JsonStore.SetDefaultAppKeys to emit the same PlainTextWarnMessage that
SetAPIMCredentials and SetMICredentials print (e.g., call/print
PlainTextWarnMessage) when writing the DefaultApp keys so users are warned
before s.persist() is invoked; keep the existing Base64Encode usage and
placement of s.persist() unchanged.
In `@import-export-cli/impl/getKeys.go`:
- Around line 133-135: The credential cache write is currently ignoring errors
from credentials.GetDefaultCredentialStore and store.SetDefaultAppKeys (seen
around the use of keyGenEnv and keygenResponse.ConsumerKey/ConsumerSecret), so
change the code to check and handle both errors: capture the error returned by
credentials.GetDefaultCredentialStore, log/return it if non-nil, then call
store.SetDefaultAppKeys and check its error; if SetDefaultAppKeys fails, log the
failure and propagate/return the error (or at minimum surface it as a warning so
callers know persistence failed) so the run does not silently drop the stored
keys.
- Around line 106-115: When using appKeys.List[0] and
credentials.GetDefaultCredentialStore()/store.GetDefaultAppKeys(keyGenEnv),
detect the case where GetDefaultAppKeys returns missing/empty or masked secrets
(e.g., empty string or mask like "*****") and do not silently fall back to the
masked values from getApplicationKeys(); instead either invoke the key
rotation/regeneration flow (implement and call a clear function such as
rotateOrGenerateAppKeys(...) that performs generation and persists the real
consumerKey/consumerSecret into the credential store) before calling
getNewToken(appKey, scopes), or return an explicit error that tells the user to
regenerate/rotate CLI app keys; update the branch around appKeys.List[0],
store.GetDefaultAppKeys, and the call to getNewToken to enforce this check and
remediation path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8b28bf8-9a3f-4b83-b3be-a96b700c9924
📒 Files selected for processing (4)
import-export-cli/credentials/credentials.goimport-export-cli/credentials/jsonstore.goimport-export-cli/credentials/store.goimport-export-cli/impl/getKeys.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
import-export-cli/credentials/jsonstore.go (1)
252-260: Consider addingdefaultAppExistscheck for consistency withEraseAPIM.
EraseMIcurrently deletes the environment if APIM credentials don't exist, but doesn't check forDefaultApp. While the current design tiesDefaultAppto APIM flows (making this unlikely to cause issues), adding the check would provide defensive consistency:- if !apimCredentialsExists(environment.APIM) { + if !apimCredentialsExists(environment.APIM) && !defaultAppExists(environment.DefaultApp) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@import-export-cli/credentials/jsonstore.go` around lines 252 - 260, EraseMI currently deletes the environment if apimCredentialsExists(environment.APIM) returns false, but it doesn't check defaultAppExists(environment.DefaultApp) like EraseAPIM does; update the EraseMI logic (in the EraseMI function in jsonstore.go) to only delete s.credentials.Environments[env] when both apimCredentialsExists(environment.APIM) and defaultAppExists(environment.DefaultApp) are false, otherwise clear environment.MI = MiCredential{} and write back s.credentials.Environments[env]; keep the final return s.persist() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@import-export-cli/credentials/jsonstore.go`:
- Around line 226-227: There is an extra closing brace '}' at the end of the
file (stray brace on line 227) that causes a syntax error; remove the stray '}'
so the surrounding function/block in jsonstore.go correctly ends where it should
(ensuring the braces for the function/method in this file are balanced and the
function that currently ends at line 226 remains the final closing brace).
---
Nitpick comments:
In `@import-export-cli/credentials/jsonstore.go`:
- Around line 252-260: EraseMI currently deletes the environment if
apimCredentialsExists(environment.APIM) returns false, but it doesn't check
defaultAppExists(environment.DefaultApp) like EraseAPIM does; update the EraseMI
logic (in the EraseMI function in jsonstore.go) to only delete
s.credentials.Environments[env] when both
apimCredentialsExists(environment.APIM) and
defaultAppExists(environment.DefaultApp) are false, otherwise clear
environment.MI = MiCredential{} and write back s.credentials.Environments[env];
keep the final return s.persist() unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd6276e5-daf3-40ab-a401-64079f576ac5
📒 Files selected for processing (1)
import-export-cli/credentials/jsonstore.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@import-export-cli/credentials/jsonstore.go`:
- Around line 224-227: The code contains duplicated unreachable statements after
a return in jsonstore.go: remove the duplicate fmt.Printf(PlainTextWarnMessage,
s.Path) and the second return nil that follow the earlier return nil (these are
merge artifacts); keep a single fmt.Printf(PlainTextWarnMessage, s.Path) before
the return nil (or remove the print entirely if that was already executed
earlier) so that the function that references s.Path and PlainTextWarnMessage
has no dead code and compiles cleanly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 624b21f8-c3cc-42bc-9e4b-da9bfb21c50c
📒 Files selected for processing (1)
import-export-cli/credentials/jsonstore.go
manodyaSenevirathne
force-pushed
the
fix/get-keys-command
branch
from
ff1f59f to
6fd400f
Compare
PasanT9
approved these changes
Mar 26, 2026
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.
Purpose
Adds support for storing and retrieving the default CLI application's consumer key and secret per environment. These changes ensure that the CLI can persist and later retrieve these credentials, as those credentials can view only once and backend masks them in subsequent API responses.
Summary by CodeRabbit
New Features
Bug Fixes