[PM-18668] Create importer for xml files from Password Depot 17 #14760
Conversation
- Create importer - Create test data - Create unit tests
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #14760 +/- ##
==========================================
+ Coverage 36.82% 36.92% +0.10%
==========================================
Files 3201 3222 +21
Lines 92743 92913 +170
Branches 13929 13988 +59
==========================================
+ Hits 34149 34307 +158
- Misses 57187 57195 +8
- Partials 1407 1411 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job, no security vulnerabilities found in this Pull Request |
…s/pm-18668/create-xml-importer-for-password-depot-17
djsmith85
force-pushed
the
tools/pm-18668/create-xml-importer-for-password-depot-17
branch
from
6e347bc to
63a7592
Compare
… with the same name
|
| <ng-container *ngIf="format === 'passworddepot17xml'"> | ||
| On the desktop application, go to Tools → Export → Enter your master password | ||
| → Select XML Format (*.xml) as Export format → Click on next → Choose which | ||
| entries should be included in the export → Click on next to export into the location | ||
| previously chosen. | ||
| </ng-container> |
There was a problem hiding this comment.
⛏️ Shouldn't this text be in a messages.json file?
There was a problem hiding this comment.
🤔 I'm not a fan of isolating the test data from the test. We end up with magic strings in assertions that aren't easy to relate the test body.
💭 Importers are kind of special in that they have to deal with large amounts of data. Password depot's XML is particularly verbose, but it's not exceptional in that respect. Maybe this is a case where we should have one .spec.ts per case?
📝 The line-level feedback cites examples that apply broadly across this file.
| let customField = cipher.fields.find((f) => f.name === "IDS_PuTTyProtocol"); | ||
| expect(customField).toBeDefined(); | ||
| expect(customField.value).toEqual("0"); | ||
|
|
||
| customField = cipher.fields.find((f) => f.name === "IDS_PuTTyKeyFile"); | ||
| expect(customField).toBeDefined(); | ||
| expect(customField.value).toEqual("pathToKeyFile"); | ||
|
|
||
| customField = cipher.fields.find((f) => f.name === "IDS_PuTTyKeyPassword"); | ||
| expect(customField).toBeDefined(); | ||
| expect(customField.value).toEqual("passwordForKeyFile"); | ||
|
|
||
| customField = cipher.fields.find((f) => f.name === "IDS_PuTTyPort"); | ||
| expect(customField).toBeDefined(); | ||
| expect(customField.value).toEqual("8080"); |
There was a problem hiding this comment.
🎨 These cases should use asymmetric matchers, specifically expect.arrayContaining and expect.objectContaining.
// find an item that is structurally equal
expect(cipher.fields).toEqual(
expect.arrayContaining({ name: 'IDS_PuTTyProtocol', value: 0 })
);
// find an item with matching properties:
expect(cipher.fields).toEqual(
expect.arrayContaining(
expect.objectContaining({ name: 'IDS_PuTTyProtocol', value: 0 })
)
);| expect(cipher.login).not.toBeNull(); | ||
| expect(cipher.login.password).toBe("somePassword"); | ||
| expect(cipher.login.username).toBe("someUser"); | ||
| expect(cipher.login.uri).toBe("http://some-bank.com"); |
There was a problem hiding this comment.
🎨 These tests could be simplified using toMatchObject, unless there's a reason toBe is necessary:
| expect(cipher.login).not.toBeNull(); | |
| expect(cipher.login.password).toBe("somePassword"); | |
| expect(cipher.login.username).toBe("someUser"); | |
| expect(cipher.login.uri).toBe("http://some-bank.com"); | |
| expect(cipher.login?.password).toEqual({ | |
| password: "somePassword", | |
| username: "someUser", | |
| uri: "http://some-bank.com" | |
| ); |
|
|
||
| const cipher = result.ciphers.shift(); | ||
|
|
||
| expect(cipher.type).toBe(CipherType.SecureNote); |
There was a problem hiding this comment.
⛏️ Using a toBe check with an enum-like is almost certainly going to cause trouble as we implement ADR-0025.
There was a problem hiding this comment.
👍🏻 Already using the enum-like pattern! w00t!
| export type PasswordDepotCustomFieldType = | ||
| _PasswordDepotCustomFieldType[keyof _PasswordDepotCustomFieldType]; | ||
| export const PasswordDepotCustomFieldType: Readonly<{ | ||
| [K in keyof typeof _PasswordDepotCustomFieldType]: PasswordDepotCustomFieldType; | ||
| }> = Object.freeze(_PasswordDepotCustomFieldType); |
There was a problem hiding this comment.
📝 This also applies to the other enum-likes in sibling files.
| @@ -348,6 +349,8 @@ export class ImportService implements ImportServiceAbstraction { | |||
| return new PasswordXPCsvImporter(); | |||
| case "netwrixpasswordsecure": | |||
| return new NetwrixPasswordSecureCsvImporter(); | |||
| case "passworddepot17xml": | |||
There was a problem hiding this comment.
🎨 Seems like the strings here might be better represented as an enum-like.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18668
📔 Objective
Adding support for importing xml-exports from Password Depot 17
Item type mapping:
Logins
Credit Cards
Secure Note
Identity
Field mapping
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes