feat: Implement password delegate for NSS #41205

arenevier · 2024-02-01T03:36:27Z

Description of Change

Introduce an app event client-certificate-request-password. It allows the user to display a UI to prompt for the password.

An alternative would have been to implement a class similar to CryptoModulePasswordDialogView, to provide the UI. This might have been simpler for the user, comparing to letting them implement the UI. But it seems like electron does not have an i18n framework, so it's not possible to provide a locale aware UI.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation, tutorials, templates and examples are changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Add the event client-certificate-request-password to app. It is dispatched when a PIN is needed to unlock a cryptographic device.

welcome · 2024-02-01T03:36:30Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

arenevier · 2024-02-01T03:40:22Z

This event is only dispatched when NSS requires a certificate to unlock a certificate. This typically happens when the client certificate is stored on a smart card. So I really don't know how to add a test for that. If you think of a way to do, I will be happy to add some unit tests.

arenevier · 2024-02-16T06:16:25Z

Do you have feedback about that pull request?

shell/browser/crypto_module_delegate_nss.cc

docs/api/app.md

shell/browser/crypto_module_delegate_nss.cc

docs/api/app.md

arenevier · 2024-04-18T21:23:26Z

I had closed the pull request by accident. I'm still looking for reviews

filenames.gni

shell/browser/crypto_module_delegate_nss.cc

docs/api/app.md

shell/browser/electron_browser_client.cc

samuelmaddock

API LGTM

samuelmaddock · 2024-06-11T17:07:07Z

shell/browser/crypto_module_delegate_nss.cc

+      gin_helper::internal::Event::New(isolate);
+  v8::Local<v8::Object> event_object = event.ToV8().As<v8::Object>();
+  gin_helper::Dictionary dict(isolate, event_object);
+  dict.Set("hostName", server_.host());


Suggested change

dict.Set("hostName", server_.host());

dict.Set("hostname", server_.host());

Based on casing seen in other APIs (setCertificateVerifyProc)

samuelmaddock · 2024-06-11T20:25:28Z

docs/api/app.md

+const { app } = require('electron')
+
+async function passwordPromptUI (text) {
+  return new Promise((resolve, reject) => {
+    // display UI to prompt user for password
+    // ...
+    // ...
+    resolve('the password')
+  })
+}


Would be great if we had a working example here. It's too bad our dialog API doesn't support basic input prompting 🥲

samuelmaddock · 2024-06-11T20:29:46Z

@arenevier rebasing your PR might fix the build errors being seen

samuelmaddock · 2024-06-15T16:21:15Z

I'm adding a wip label on this PR until the requested changes can be implemented. Please tag @electron/wg-api once it's ready to be reviewed again and we'll take another look! 🙇

Introduce an app event client-certificate-request-password. It allows the user to display a UI to prompt for the password. An alternative would have been to implement a class similar to CryptoModulePasswordDialogView, to provide the UI. This might have been simpler for the user, comparing to letting them implement the UI. But it seems like electron does not have an i18n framework, so it's not possible to provide a locale aware UI.

* use a trampoline handler in JS. The api exposed is now app.setClientCertRequestPasswordHandler * use properties on the Event object instead of positional parameters * remove ChromeNSSCryptoModuleDelegate::OnPassword in favor of args->GetNext(&password_)

- backslash escape parametrized TypeScript - rename hostName param to hostname - use base::ScopedAllowBaseSyncPrimitivesForTesting - and then, rename ChromeNSSCryptoModuleDelegate to ElectronNSSCryptoModuleDelegate

arenevier · 2024-06-24T17:51:23Z

@electron/wg-api: I have updated the patch. I think I addressed all the comments.

arenevier · 2024-06-24T17:53:22Z

@electron/api-wg

arenevier · 2024-06-24T17:54:33Z

@samuelmaddock: I'm pinging you directly because I'm not sure if the ping to electron/wg-api worked or not

samuelmaddock

API LGTM

docs/api/app.md

Co-authored-by: Sam Maddock <[email protected]>

docs/api/app.md

Co-authored-by: Erick Zhao <[email protected]>

erickzhao

API LGTM

arenevier · 2024-07-09T18:50:21Z

@codebytere @ckerr : could one of you take a look?

samuelmaddock · 2024-07-12T22:02:10Z

@arenevier we've recently started deploying GitHub actions to run builds/tests for Electron. We're working through some issues with them running in PRs from forked repos. I'll try to get back to you next week once we've resolved the issue so we can get this PR back to a ✅ state and hopefully merged soon.

samuelmaddock · 2024-07-15T21:48:52Z

@arenevier would you mind rebasing to the latest changes on main? It should resolve the CI errors.

arenevier · 2024-07-15T23:35:49Z

@arenevier would you mind rebasing to the latest changes on main? It should resolve the CI errors.

Looks like there is still an issue.

welcome · 2024-07-17T13:48:05Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2024-07-17T13:48:06Z

Release Notes Persisted

Add the event client-certificate-request-password to app. It is dispatched when a PIN is needed to unlock a cryptographic device.

electron-cation bot added the new-pr 🌱 PR opened recently label Feb 1, 2024

codebytere changed the title ~~feat: Implement password delegate for NSS (#41188)~~ feat: Implement password delegate for NSS Feb 1, 2024

zcbenz requested a review from ckerr February 8, 2024 01:16

zcbenz added platform/linux semver/minor backwards-compatible functionality no-backport labels Feb 8, 2024

electron-cation bot added api-review/requested 🗳 and removed new-pr 🌱 PR opened recently labels Feb 8, 2024

zcbenz reviewed Mar 4, 2024

View reviewed changes

shell/browser/crypto_module_delegate_nss.cc Outdated Show resolved Hide resolved

shell/browser/crypto_module_delegate_nss.cc Outdated Show resolved Hide resolved

zcbenz requested a review from a team March 4, 2024 01:18

nornagon reviewed Mar 4, 2024

View reviewed changes

docs/api/app.md Outdated Show resolved Hide resolved

shell/browser/crypto_module_delegate_nss.cc Outdated Show resolved Hide resolved

docs/api/app.md Outdated Show resolved Hide resolved

arenevier closed this Mar 5, 2024

arenevier force-pushed the password_delegate branch from 861bbee to cc7e80c Compare March 5, 2024 22:41

arenevier reopened this Apr 18, 2024

codebytere reviewed Apr 19, 2024

View reviewed changes

filenames.gni Outdated Show resolved Hide resolved

shell/browser/crypto_module_delegate_nss.cc Outdated Show resolved Hide resolved

arenevier requested a review from codebytere April 19, 2024 17:58

samuelmaddock reviewed Jun 11, 2024

View reviewed changes

docs/api/app.md Outdated Show resolved Hide resolved

shell/browser/electron_browser_client.cc Show resolved Hide resolved

samuelmaddock reviewed Jun 11, 2024

View reviewed changes

samuelmaddock added the wip ⚒ label Jun 15, 2024

Arno Renevier added 4 commits June 24, 2024 10:49

fix lint:markdown error

42d7517

address review comments

42a2d2a

* use a trampoline handler in JS. The api exposed is now app.setClientCertRequestPasswordHandler * use properties on the Event object instead of positional parameters * remove ChromeNSSCryptoModuleDelegate::OnPassword in favor of args->GetNext(&password_)

address review comments second round

5dbb7bf

- backslash escape parametrized TypeScript - rename hostName param to hostname - use base::ScopedAllowBaseSyncPrimitivesForTesting - and then, rename ChromeNSSCryptoModuleDelegate to ElectronNSSCryptoModuleDelegate

arenevier force-pushed the password_delegate branch from 6ecab9d to 5dbb7bf Compare June 24, 2024 17:50

arenevier requested a review from samuelmaddock June 24, 2024 17:50

samuelmaddock removed the wip ⚒ label Jun 24, 2024

samuelmaddock approved these changes Jun 25, 2024

View reviewed changes

docs/api/app.md Outdated Show resolved Hide resolved

Update docs/api/app.md

fb84803

Co-authored-by: Sam Maddock <[email protected]>

erickzhao reviewed Jun 25, 2024

View reviewed changes

docs/api/app.md Outdated Show resolved Hide resolved

Update docs/api/app.md

c0f655b

Co-authored-by: Erick Zhao <[email protected]>

erickzhao approved these changes Jul 9, 2024

View reviewed changes

electron-cation bot added api-review/approved ✅ and removed api-review/requested 🗳 labels Jul 9, 2024

Merge branch 'electron:main' into password_delegate

df296f1

jkleinsc approved these changes Jul 17, 2024

View reviewed changes

jkleinsc merged commit 81bdba6 into electron:main Jul 17, 2024
49 checks passed

This comment was marked as spam.

Sign in to view

	dict.Set("hostName", server_.host());
	dict.Set("hostname", server_.host());

feat: Implement password delegate for NSS #41205

feat: Implement password delegate for NSS #41205

Uh oh!

Conversation

arenevier commented Feb 1, 2024 • edited by codebytere Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

welcome bot commented Feb 1, 2024

Uh oh!

arenevier commented Feb 1, 2024

Uh oh!

arenevier commented Feb 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arenevier commented Apr 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samuelmaddock left a comment

Choose a reason for hiding this comment

Uh oh!

samuelmaddock Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

samuelmaddock Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

samuelmaddock commented Jun 11, 2024

Uh oh!

samuelmaddock commented Jun 15, 2024

Uh oh!

arenevier commented Jun 24, 2024

Uh oh!

arenevier commented Jun 24, 2024

Uh oh!

arenevier commented Jun 24, 2024

Uh oh!

samuelmaddock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erickzhao left a comment

Choose a reason for hiding this comment

Uh oh!

arenevier commented Jul 9, 2024

Uh oh!

samuelmaddock commented Jul 12, 2024

Uh oh!

samuelmaddock commented Jul 15, 2024

Uh oh!

arenevier commented Jul 15, 2024

Uh oh!

Uh oh!

welcome bot commented Jul 17, 2024

Uh oh!

release-clerk bot commented Jul 17, 2024

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

arenevier commented Feb 1, 2024 •

edited by codebytere

Loading