Skip to content

feat: store authentication token in secret storage#632

Merged
congminh1254 merged 9 commits intoadd_pkce_supportfrom
improve-keychain-handling
Mar 9, 2026
Merged

feat: store authentication token in secret storage#632
congminh1254 merged 9 commits intoadd_pkce_supportfrom
improve-keychain-handling

Conversation

@congminh1254
Copy link
Member

No description provided.

@congminh1254 congminh1254 requested a review from a team March 4, 2026 08:11
@congminh1254 congminh1254 marked this pull request as draft March 4, 2026 08:12
@coveralls
Copy link

coveralls commented Mar 4, 2026

Pull Request Test Coverage Report for Build 22837876813

Details

  • 85 of 135 (62.96%) changed or added relevant lines in 5 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.3%) to 85.312%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/login-helper.js 11 12 91.67%
src/box-command.js 12 15 80.0%
src/token-cache.js 47 58 81.03%
src/commands/login.js 5 40 12.5%
Files with Coverage Reduction New Missed Lines %
src/commands/files/zip.js 1 81.03%
src/commands/login.js 1 13.69%
Totals Coverage Status
Change from base Build 21941871068: -0.3%
Covered Lines: 4827
Relevant Lines: 5443

💛 - Coveralls

@congminh1254 congminh1254 changed the title feat: Unify keychain and keytar dependencies feat: Unify keychain and keytar dependencies Mar 6, 2026
@congminh1254 congminh1254 changed the base branch from main to add_pkce_support March 6, 2026 10:30
@congminh1254 congminh1254 changed the base branch from add_pkce_support to main March 9, 2026 03:59
@congminh1254 congminh1254 marked this pull request as ready for review March 9, 2026 03:59
@congminh1254 congminh1254 changed the base branch from main to add_pkce_support March 9, 2026 04:11
@congminh1254 congminh1254 changed the title feat: Unify keychain and keytar dependencies feat: store authentication token in secret storage Mar 9, 2026
try {
keytar = require('keytar');
} catch {
// keytar cannot be imported because the library is not provided for this operating system / architecture
Copy link
Collaborator

Choose a reason for hiding this comment

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

should/can we add some log here to the use

Copy link
Member Author

Choose a reason for hiding this comment

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

We are doing some logging when we try to use this lib, but the lib is undefined. But if we do it here, user will see the message everytime they run any command.

})
);

Promise.all(promises)
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this promisses affects one another ?

| **Windows** | Credential Manager | Built-in |
| **Linux** | Secret Service (libsecret) | May require installation |

### Linux Installation
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link

@KwiatkowskiML KwiatkowskiML left a comment

Choose a reason for hiding this comment

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

LGTM but I'm not fully familiar with this codebase, so I would suggest to wait for one more review

@congminh1254 congminh1254 merged commit b4b4e08 into add_pkce_support Mar 9, 2026
3 checks passed
@congminh1254 congminh1254 deleted the improve-keychain-handling branch March 9, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants