Skip to content

feat: refactor state, store it securely #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 10, 2025
Merged

feat: refactor state, store it securely #48

merged 2 commits into from
Apr 10, 2025

Conversation

nirinchev
Copy link
Collaborator

@nirinchev nirinchev commented Apr 10, 2025

Uses @napi-rs/keyring to store the credentials from the state in the OS secure storage location.

Additionally, reactors State to have a persistent field which gets stored and session field where we can add global state that should not persist across launches. For now, this is only the NodeDriverServiceProvider, which was previously in a now-deleted MongoDBToolState.

The node library we're using is a wrapper around https://github.com/open-source-cooperative/keyring-rs, which is a rust-based implementation wrapping the native credential stores for most desktop operating systems.

Fixes #20

@nirinchev nirinchev requested review from fmenezes and gagik April 10, 2025 14:23
Copy link
Collaborator

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Could be good to turn a State into a class with an enclosed entry if we ever want to support multiple credentials but for our current scope this looks good

@fmenezes
Copy link
Collaborator

LGTM, personally I would separate session and credentials and remove state from the picture

@nirinchev nirinchev requested review from fmenezes and gagik April 10, 2025 16:43
@nirinchev
Copy link
Collaborator Author

@gagik @fmenezes I used your feedback as inspiration and tried to do something in the middle 😅 I'm keeping the credentials and the rest of the state combined, but they're now encapsulated in a class responsible for saving/loading of data. I decided to keep them together until we decide what we want to do with the auth flows and whether we want to persist data at all, but should be fairly trivial to rip out if necessary.

Copy link
Collaborator

@blva blva left a comment

Choose a reason for hiding this comment

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

nice, LGTM 🚀

@nirinchev nirinchev merged commit b1fbd3f into main Apr 10, 2025
1 check passed
@nirinchev nirinchev deleted the ni/state branch April 10, 2025 16:55
nirinchev added a commit that referenced this pull request Apr 10, 2025
* main:
  feat: refactor state, store it securely (#48)
  feat: add unit test framework (#13)
  chore: update readme (#51)

# Conflicts:
#	src/tools/mongodb/connect.ts
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.

Investigate secure storage options
4 participants