[PM-16227] Move import to sdk and enable it in browser/web #12479

quexten · 2024-12-19T15:01:41Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-16227

📔 Objective

Makes use of the sdk in order to handle import on desktop web and browser. Drops importer code from desktop_native since it's not needed there anymore.

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

github-actions · 2024-12-19T15:10:34Z

Checkmarx One – Scan Summary & Details – a0b00d16-eb57-4cc5-829c-859d2b8324ad

No New Or Fixed Issues Found

codecov · 2024-12-19T15:12:14Z

Codecov Report

Attention: Patch coverage is 13.18681% with 79 lines in your changes missing coverage. Please review.

Project coverage is 33.73%. Comparing base (196c1e1) to head (e25d940).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...angular/src/vault/components/add-edit.component.ts	0.00%	35 Missing ⚠️
...ponents/sshkey-section/sshkey-section.component.ts	16.21%	27 Missing and 4 partials ⚠️
libs/common/src/models/export/ssh-key.export.ts	0.00%	5 Missing ⚠️
...vault/popup/components/vault/add-edit.component.ts	0.00%	3 Missing ⚠️
.../desktop/src/vault/app/vault/add-edit.component.ts	0.00%	2 Missing ⚠️
...access/view/emergency-add-edit-cipher.component.ts	66.66%	0 Missing and 1 partial ⚠️
...c/app/vault/individual-vault/add-edit.component.ts	66.66%	0 Missing and 1 partial ⚠️
.../web/src/app/vault/org-vault/add-edit.component.ts	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12479      +/-   ##
==========================================
- Coverage   33.74%   33.73%   -0.01%     
==========================================
  Files        2918     2918              
  Lines       90994    91047      +53     
  Branches    17200    17215      +15     
==========================================
+ Hits        30703    30712       +9     
- Misses      57891    57924      +33     
- Partials     2400     2411      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

audreyality

Requested changes marked with ⚠️.
Review-blocking concerns that may not require changes marked with 🤔.
Everything else is commentary.

audreyality · 2024-12-27T13:41:55Z

libs/common/src/models/export/ssh-key.export.ts

-    domain.privateKey = req.privateKey != null ? new EncString(req.privateKey) : null;
-    domain.publicKey = req.publicKey != null ? new EncString(req.publicKey) : null;
-    domain.keyFingerprint = req.keyFingerprint != null ? new EncString(req.keyFingerprint) : null;
+    const parsedKey = import_ssh_key(req.privateKey);


🤔 Seems like there should be a null check here. You have one when importing from the clipboard, in case import_ssh_key returns null.

Ok, so thinking this through, this should only occur when a user hand-modified the export to be broken (or a bug caused the export to be in a broken state), since SSH keys do not have an arbitrary format.

Since we only ever want consistent, and correct keys in the vault, the intended behavior would be to skip the entire cipher. Currently, just from the code, this seems like it would throw and thus throw the entire import.

What is tools recommendation on this?

I see several ways to filter out these broken ciphers in the import later on (i.e in f.e paresDecrypted as a "post-processing" step on the result), but I don't know whether that's what tools intends?

@djsmith85 - I think this is a place where we'll need your importer expertise.

libs/angular/src/vault/components/add-edit.component.ts

audreyality · 2024-12-27T13:49:04Z

libs/angular/src/vault/components/add-edit.component.ts

+          } else {
+            password = await this.getSshKeyPassword();
+            await this.importSshKeyFromClipboard(password);
+          }


♻️ It's generally bad form to use try/catch for control flow. Consider signalling the expected case of a missing or incorrect password using the return code instead of an exception.

I wish there was a better way to represent results in ts/js, but the way this is done in the PR is the default way to hand over results from rust(wasm) to js/ts: https://rustwasm.github.io/docs/wasm-bindgen/reference/types/result.html

This doesn't need to be solved now--♻️ identifies tech debt. If you're interested in it, though, you might consider embedding an enum in the Ok block of the result.

enum SshImport { Imported(value), MissingPassword, // expected failure case InvalidPassword, // expected failure case } fn method(...) : Result<SshImport, Exception> { // ... }

There may be better ways to handle this; the main idea is that you have "process failures" (which relate to things like validation) and "runtime failures" (which relate to circumstances outside of the user's control).

Wasm-bindgen does not support enums with values at the moment (otherwise Result would also be trivial to represent), but only C-style enums: rustwasm/wasm-bindgen#2407. So for passing to typescript, this would need to be converted to some struct, similar to what was done in the desktop-only implementation.

libs/angular/src/vault/components/add-edit.component.ts

audreyality · 2024-12-27T14:19:55Z

libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.ts

@@ -79,4 +90,71 @@ export class SshKeySectionComponent implements OnInit {
      keyFingerprint,
    });
  }
+
+  async importSshKeyFromClipboard(password: string = "") {


♻️ This function is replicated with minor alterations across several components. Consider extracting the decision making into one or more shared functions.

To be honest, after rewriting it, it is so much shorter that moving this out to a service, just for the two consumers, seems like it would lead to less clarity of the code.

To be honest, after rewriting it, it is so much shorter that moving this out to a service, just for the two consumers, seems like it would lead to less clarity of the code.

It can just be a plain old function. There's no need to embed it in a service.

That said, this is also an optional thing.

…rden/clients into km/pm-16227/ssh-import-web-browser

audreyality · 2025-01-06T15:26:01Z

libs/angular/src/vault/components/add-edit.component.ts

+          return;
+        }
+        continue;
+      }
+      break;


⛏️ Mixing 3 different control flow keywords here leads me to believe there's a better way to state this.

audreyality · 2025-01-06T15:33:41Z

libs/angular/src/vault/components/add-edit.component.ts

+          } else {
+            password = await this.getSshKeyPassword();
+            await this.importSshKeyFromClipboard(password);
+          }


This doesn't need to be solved now--♻️ identifies tech debt. If you're interested in it, though, you might consider embedding an enum in the Ok block of the result.

enum SshImport { Imported(value), MissingPassword, // expected failure case InvalidPassword, // expected failure case } fn method(...) : Result<SshImport, Exception> { // ... }

There may be better ways to handle this; the main idea is that you have "process failures" (which relate to things like validation) and "runtime failures" (which relate to circumstances outside of the user's control).

Move import to sdk and enable it in browser/web

646f245

quexten added 5 commits December 19, 2024 17:34

Add uncomitted files

b57c5a9

Update package lock

4a1b674

Fix prettier formatting

34a60d0

Merge branch 'main' into km/pm-16227/ssh-import-web-browser

b42aed6

Merge branch 'main' into km/pm-16227/ssh-import-web-browser

74f1033

quexten changed the title ~~Move import to sdk and enable it in browser/web~~ [PM-16227] Move import to sdk and enable it in browser/web Dec 23, 2024

quexten added 2 commits December 23, 2024 11:57

Fix build

a2b7cd9

Merge branch 'main' into km/pm-16227/ssh-import-web-browser

2886cbe

quexten marked this pull request as ready for review December 24, 2024 14:24

quexten requested review from a team as code owners December 24, 2024 14:24

quexten requested review from addisonbeck, gbubemismith and Patrick-Pimentel-Bitwarden December 24, 2024 14:24

audreyality requested changes Dec 27, 2024

View reviewed changes

quexten added 8 commits January 6, 2025 12:21

Merge branch 'main' into km/pm-16227/ssh-import-web-browser

85498cb

Merge branch 'km/pm-16227/ssh-import-web-browser' of github.com:bitwa…

bbc59a2

…rden/clients into km/pm-16227/ssh-import-web-browser

Merge branch 'main' into km/pm-16227/ssh-import-web-browser

cc37c6e

Rewrite import logic

e67bc60

Update ssh import logic for cipher form component

51ce107

Fix build on browser

7c6b44b

Break early in retry logic

d39672e

Fix build

e25d940

audreyality reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-16227] Move import to sdk and enable it in browser/web #12479

[PM-16227] Move import to sdk and enable it in browser/web #12479

quexten commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

audreyality left a comment

audreyality Dec 27, 2024

quexten Jan 6, 2025 •

edited

Loading

audreyality Jan 6, 2025

audreyality Dec 27, 2024 •

edited

Loading

quexten Jan 6, 2025

audreyality Jan 6, 2025 •

edited

Loading

quexten Jan 6, 2025

audreyality Dec 27, 2024

quexten Jan 6, 2025

audreyality Jan 6, 2025

audreyality Jan 6, 2025

audreyality Jan 6, 2025 •

edited

Loading

[PM-16227] Move import to sdk and enable it in browser/web #12479

Are you sure you want to change the base?

[PM-16227] Move import to sdk and enable it in browser/web #12479

Conversation

quexten commented Dec 19, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Dec 19, 2024 • edited Loading

No New Or Fixed Issues Found

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

audreyality left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quexten Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audreyality Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audreyality Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audreyality Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

quexten commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

quexten Jan 6, 2025 •

edited

Loading

audreyality Dec 27, 2024 •

edited

Loading

audreyality Jan 6, 2025 •

edited

Loading

audreyality Jan 6, 2025 •

edited

Loading