Issue 26 - Add importer for LastPass CSV files

palant / pfp

A simple and secure browser extension to be used with KeePass databases.

https://pfp.works/

Mozilla Public License 2.0

113 stars 14 forks source link

Issue 26 - Add importer for LastPass CSV files #67

Closed kzar closed 6 years ago

kzar commented 6 years ago

This is a rebased version of #53, there are still some FIXME comments which need to be dealt with but it's working.

Questions:

I throw away the name of the LastPass entry (except in the special case of a secure note with no associated username or URL). That seems like a shame but I don't know what else we can do.
I ignore the case of there being multiple entries with the same domain + username, hoping that setPassword is clever enough to thrown an error / do something smart. But if that's not the case we'll be silently losing entries! I've run out of time for today to test / investigate further.

kzar commented 6 years ago

Oh, one thing I noticed was the file selection dialog is expecting the user to pick a .json file, so I had to manually change that before I was allowed to select my .csv file.

palant commented 6 years ago

As of https://github.com/palant/easypasswords/commit/27a494831fe1bf08dbd0015e342f4b672a254d61 stored passwords can have revisions as well. So it should now be possible to address your concerns.

kzar commented 6 years ago

As discussed in IRC I've addressed what I can for now. There are still some problems / things to do:

It seems that Node 6 doesn't include the URL API, which is why one of the Travis CI checks just failed.
I've not replaced the character by character parsing with your regexp based suggestion, since it seemed to have a bug causing empty values not to be appended to the currentEntry. I've not had time to figure out that out.
I'm still not logging / displaying the various error messages correctly, I've left the FIXME comments in for those.
I'm not handling duplicate entries properly using revisions like you suggested.
I've left a couple of FIXME comments in the tests where I wasn't sure if the current behaviour was correct.

palant commented 6 years ago

This has been merged with some additional changes.