-
Notifications
You must be signed in to change notification settings - Fork 760
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
Secrets are now managed by native OS credential store. #1618
Conversation
This looks good. I think we should consider code that removes the store bot secrets from the @cwhitten - thoughts? |
@justinwilaby bot secrets are currently stored in |
I think we need to do that before we can submit for the security & privacy reviews. @tonyanziano what do you think? |
I thought that removing the property would be sufficient, and that further writes to I can add some code that purges any secrets from |
e7e3f68
to
76acf69
Compare
return botsJson.bots; | ||
return botsJson.bots.map(bot => { | ||
delete bot.secret; | ||
return bot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code purges the secret
property from any BotInfo
objects when we first load bots from disk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified working on Mac! Nice work
Resolves #1464
Please try to break this :)
Bot secrets are now stored by your OS in your native credential store:
Windows: Credential Manager
Mac: Keychain
Linux: Secret Service API / libsecret
NOTE #1: Due to the fact that we now use
keytar
which is a native node addon, it has to be built properly for Electron when launching and running the app, and must be built for Node when running the tests (Jest environment).I couldn't figure out a way to have it easily built for both environments, so if you decide to flip back and forth between running tests and launching the app, then you will need to use the scripts in the
root/package.json
file:rebuild:keytar:electron
: use when you want to run the apprebuild:keytar:node
: use when you want to run the testsLet me know if you think this process could be improved or better documented.
NOTE #2: The library used to rebuild
keytar
for Electron,electron-rebuild
, does not play nicely with Lerna monorepos, and sokeytar
had to be added as a dependency to the rootpackage.json
file. This should not have any adverse effects other than there being an extra line in the file. I opened an issue againstelectron-rebuild
and it can be found here.