-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Can import / export credentials from built-in password manager #2548
base: master
Are you sure you want to change the base?
Can import / export credentials from built-in password manager #2548
Conversation
Might resolve #2114 |
We can now import / export from csv files that contain the columns After clicking import / export buttons there will first be a warning stating that the credentials will be in readable text (export) or existing passwords will be overwritten (import). |
The import warning is good! We should probably only show the warning if you have > 0 saved passwords before the import though. For the export warning, the Firefox one is good, but I don't want to copy it exactly :) How about "Your exported passwords will be saved in a file in plain text. When you're done using this file, you should delete it. Are you sure you want to continue?" What do you think about using an existing CSV package, like this? I remember CSV having various quirks to parsing it; it might be more robust to do that rather than doing it ourselves. |
js/passwordManager/keychain.js
Outdated
@@ -48,6 +48,53 @@ class Keychain { | |||
ipcRenderer.invoke('credentialStoreDeletePassword', { domain, username }) | |||
} | |||
|
|||
async importCredentials () { |
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.
I think it would be better if this took the CSV file contents as an argument, and the open dialog part was moved into passwordViewer. It's possible the CSV parsing would be better moved into there as well, but I'm not sure about that, so I'll leave that up to you.
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.
I was reading this comment and was like, "huh but this is already in the viewer right?" apparently not, thanks for catching :)
Yes parsing of the CSV was not the prettiest code I've written. I followed your suggestion of using a package that parses csv files and found Papaparse, this one returns the data as json/object-array with the header as a key for the value which is a huge + and it is lightweight. For the exporting I did not use the package since that's just 9 lines and very straightforward. |
Allows you to import / export credentials from the built-in password manager.
It will import / export using the same schema the built-in password manager uses to store the credentials which is the following:
Currently it will only let you import if you have 0 passwords and only export if you have more than 1 password. We might want to do some merge thing but I didn't want to do that yet since that could go a couple ways, e.g. override, merge.
Also refactored some "render to dom" functionality into separate functions.
Let me know what you think.