Skip to content

fix: inconsistent error messages when credentials fail to save#2737

Open
Maanvi212006 wants to merge 4 commits into
zowe:masterfrom
Maanvi212006:fix/inconsistent-secure-save-error-messages
Open

fix: inconsistent error messages when credentials fail to save#2737
Maanvi212006 wants to merge 4 commits into
zowe:masterfrom
Maanvi212006:fix/inconsistent-secure-save-error-messages

Conversation

@Maanvi212006

@Maanvi212006 Maanvi212006 commented Jun 9, 2026

Copy link
Copy Markdown

fixes #1907

What It Does

  • Found two call sites where config.save() was called without using the secureSaveError helper
    ConfigAutoStore.ts and ConvertV1Profiles.ts
    Users were seeing the raw unhelpful messageFailed to load Keytar moduleinstead of the consistent, user-friendlyUnable to securely save credentials message` with possible solutions
  • Wrapped both config.save() calls in try/catch blocks that throw ConfigUtils.secureSaveError() on failure

How to Test

  • Set up an environment where keytar fails to load (e.g. missing @zowe/secrets-for-zowe-sdk module)
  • Trigger a credential save operation via zowe config secure or any command that auto-stores credentials

Before fix: error message shows Failed to load Keytar module: ...
After fix: error message shows Unable to securely save credentials. Possible Solutions: ...

Review Checklist
I certify that I have:

  • updated the changelog
  • manually tested my changes
  • added/updated automated unit/integration tests
  • created/ran system tests (provide build number if applicable)
  • followed the contribution guidelines

Additional Comments
ConfigUtils import was also added to ConvertV1Profiles.ts as it was not previously imported there.

Signed-off-by: Maanvi Chetwani <maanvichetwani21@gmail.com>
@github-project-automation github-project-automation Bot moved this to New Issues in Zowe CLI Squad Jun 9, 2026
@zowe-robot zowe-robot moved this from New Issues to Review/QA in Zowe CLI Squad Jun 9, 2026
@zFernand0 zFernand0 self-requested a review June 9, 2026 10:53
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.79%. Comparing base (4fda279) to head (52bd5c5).

Files with missing lines Patch % Lines
...kages/imperative/src/config/src/ConfigAutoStore.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2737      +/-   ##
==========================================
- Coverage   91.92%   91.79%   -0.13%     
==========================================
  Files         650      650              
  Lines       19893    19898       +5     
  Branches     4351     4474     +123     
==========================================
- Hits        18286    18266      -20     
- Misses       1605     1630      +25     
  Partials        2        2              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Maanvi212006 and others added 2 commits June 12, 2026 19:51
Signed-off-by: Maanvi Chetwani <maanvichetwani21@gmail.com>

@zFernand0 zFernand0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM! 😋

I believe we are missing a few more instances of config.save where we could leverage this same try/catch + throw ConfigUtils.secureSaveError();

Image

This PR addresses the first two instances of the picture above 🙏

👀 @t1m0thyj

@zFernand0 zFernand0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to request changes 🙏

@zFernand0 zFernand0 requested a review from t1m0thyj June 16, 2026 12:45
@Maanvi212006

Copy link
Copy Markdown
Author

Changes LGTM! 😋

I believe we are missing a few more instances of config.save where we could leverage this same try/catch + throw ConfigUtils.secureSaveError();

Image This PR addresses the first two instances of the picture above 🙏

👀 @t1m0thyj

@zFernand0 Thanks for the review!
I too dug into this. BaseAuthHandler.ts has 2 more unguarded config.save() calls. There's also the same pattern in BaseAutoInitHandler.ts (2), init.handler.ts (1), secure.handler.ts (1), and set.handler.ts (1) — none of them wrap save() in try/catch + secureSaveError().
Happy to extend this PR to cover all of them if that's preferred

@t1m0thyj t1m0thyj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @Maanvi212006! Please resolve failing unit tests:

Errors thrown in packages/imperative/src/config/__tests__/ConfigAutoStore.unit.test.ts
    ● Test suite failed to run
  
      packages/imperative/src/config/__tests__/ConfigAutoStore.unit.test.ts:577:53 - error TS2304: Cannot find name 'handlerParams'.
  
      577             await ConfigAutoStore.storeSessCfgProps(handlerParams as any, {
                                                              ~~~~~~~~~~~~~

Signed-off-by: Maanvi Chetwani <maanvichetwani21@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Review/QA

Development

Successfully merging this pull request may close these issues.

Inconsistent error messages when credentials fail to save error

4 participants