Skip to content
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

feat: promisify dialog.showCertificateTrustDialog() #17181

Merged
merged 2 commits into from Mar 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 5 additions & 7 deletions atom/browser/ui/certificate_trust.h
Expand Up @@ -7,7 +7,7 @@

#include <string>

#include "base/callback_forward.h"
#include "atom/common/promise_util.h"
#include "base/memory/ref_counted.h"
#include "net/cert/x509_certificate.h"

Expand All @@ -17,12 +17,10 @@ class NativeWindow;

namespace certificate_trust {

typedef base::Callback<void(void)> ShowTrustCallback;

void ShowCertificateTrust(atom::NativeWindow* parent_window,
const scoped_refptr<net::X509Certificate>& cert,
const std::string& message,
const ShowTrustCallback& callback);
v8::Local<v8::Promise> ShowCertificateTrust(
atom::NativeWindow* parent_window,
const scoped_refptr<net::X509Certificate>& cert,
const std::string& message);

} // namespace certificate_trust

Expand Down
57 changes: 31 additions & 26 deletions atom/browser/ui/certificate_trust_mac.mm
Expand Up @@ -15,20 +15,20 @@

@interface TrustDelegate : NSObject {
@private
certificate_trust::ShowTrustCallback callback_;
std::unique_ptr<atom::util::Promise> promise_;
SFCertificateTrustPanel* panel_;
scoped_refptr<net::X509Certificate> cert_;
SecTrustRef trust_;
CFArrayRef cert_chain_;
SecPolicyRef sec_policy_;
}

- (id)initWithCallback:(const certificate_trust::ShowTrustCallback&)callback
panel:(SFCertificateTrustPanel*)panel
cert:(const scoped_refptr<net::X509Certificate>&)cert
trust:(SecTrustRef)trust
certChain:(CFArrayRef)certChain
secPolicy:(SecPolicyRef)secPolicy;
- (id)initWithPromise:(atom::util::Promise)promise
panel:(SFCertificateTrustPanel*)panel
cert:(const scoped_refptr<net::X509Certificate>&)cert
trust:(SecTrustRef)trust
certChain:(CFArrayRef)certChain
secPolicy:(SecPolicyRef)secPolicy;

- (void)panelDidEnd:(NSWindow*)sheet
returnCode:(int)returnCode
Expand All @@ -47,14 +47,14 @@ - (void)dealloc {
[super dealloc];
}

- (id)initWithCallback:(const certificate_trust::ShowTrustCallback&)callback
panel:(SFCertificateTrustPanel*)panel
cert:(const scoped_refptr<net::X509Certificate>&)cert
trust:(SecTrustRef)trust
certChain:(CFArrayRef)certChain
secPolicy:(SecPolicyRef)secPolicy {
- (id)initWithPromise:(atom::util::Promise)promise
panel:(SFCertificateTrustPanel*)panel
cert:(const scoped_refptr<net::X509Certificate>&)cert
trust:(SecTrustRef)trust
certChain:(CFArrayRef)certChain
secPolicy:(SecPolicyRef)secPolicy {
if ((self = [super init])) {
callback_ = callback;
promise_.reset(new atom::util::Promise(std::move(promise)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need keeping promise_ in a std::unique_ptr. It is only needed when the stored promise may be empty, but in this class since the promise is guaranteed to be always available, it can just be stored as normal value.

@interface TrustDelegate : NSObject {
 @private
  atom::util::Promise promise_;
}

promise_ = std::move(promise);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized there is no constructor in objective-c classes so std::unique_ptr is the only way to go. This PR should be ready to go!

panel_ = panel;
cert_ = cert;
trust_ = trust;
Expand All @@ -73,19 +73,22 @@ - (void)panelDidEnd:(NSWindow*)sheet
// now.
cert_db->NotifyObserversCertDBChanged();

callback_.Run();

promise_->Resolve();
[self autorelease];
}

@end

namespace certificate_trust {

void ShowCertificateTrust(atom::NativeWindow* parent_window,
const scoped_refptr<net::X509Certificate>& cert,
const std::string& message,
const ShowTrustCallback& callback) {
v8::Local<v8::Promise> ShowCertificateTrust(
atom::NativeWindow* parent_window,
const scoped_refptr<net::X509Certificate>& cert,
const std::string& message) {
v8::Isolate* isolate = v8::Isolate::GetCurrent();
atom::util::Promise promise(isolate);
v8::Local<v8::Promise> handle = promise.GetHandle();

auto* sec_policy = SecPolicyCreateBasicX509();
auto cert_chain =
net::x509_util::CreateSecCertificateArrayForX509Certificate(cert.get());
Expand All @@ -98,18 +101,20 @@ void ShowCertificateTrust(atom::NativeWindow* parent_window,
auto msg = base::SysUTF8ToNSString(message);

auto panel = [[SFCertificateTrustPanel alloc] init];
auto delegate = [[TrustDelegate alloc] initWithCallback:callback
panel:panel
cert:cert
trust:trust
certChain:cert_chain
secPolicy:sec_policy];
auto delegate = [[TrustDelegate alloc] initWithPromise:std::move(promise)
panel:panel
cert:cert
trust:trust
certChain:cert_chain
secPolicy:sec_policy];
[panel beginSheetForWindow:window
modalDelegate:delegate
didEndSelector:@selector(panelDidEnd:returnCode:contextInfo:)
contextInfo:nil
trust:trust
message:msg];

return handle;
}

} // namespace certificate_trust
18 changes: 10 additions & 8 deletions atom/browser/ui/certificate_trust_win.cc
Expand Up @@ -8,7 +8,6 @@

#include <wincrypt.h>

#include "base/callback.h"
#include "net/cert/cert_database.h"
#include "net/cert/x509_util_win.h"

Expand Down Expand Up @@ -57,14 +56,16 @@ CERT_CHAIN_PARA GetCertificateChainParameters() {
return params;
}

void ShowCertificateTrust(atom::NativeWindow* parent_window,
const scoped_refptr<net::X509Certificate>& cert,
const std::string& message,
const ShowTrustCallback& callback) {
PCCERT_CHAIN_CONTEXT chain_context;
v8::Local<v8::Promise> ShowCertificateTrust(
atom::NativeWindow* parent_window,
const scoped_refptr<net::X509Certificate>& cert,
const std::string& message) {
v8::Isolate* isolate = v8::Isolate::GetCurrent();
atom::util::Promise promise(isolate);
v8::Local<v8::Promise> handle = promise.GetHandle();

PCCERT_CHAIN_CONTEXT chain_context;
auto cert_context = net::x509_util::CreateCertContextWithChain(cert.get());

auto params = GetCertificateChainParameters();

if (CertGetCertificateChain(NULL, cert_context.get(), NULL, NULL, &params,
Expand All @@ -79,7 +80,8 @@ void ShowCertificateTrust(atom::NativeWindow* parent_window,
CertFreeCertificateChain(chain_context);
}

callback.Run();
promise.Resolve();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method is synchronous / blocking on windows? Might be worth documenting / possibly making async somehow 🤔

return handle;
}

} // namespace certificate_trust
23 changes: 23 additions & 0 deletions docs/api/dialog.md
Expand Up @@ -330,6 +330,29 @@ attached to the parent window, making it modal.

On Windows the options are more limited, due to the Win32 APIs used:

* The `message` argument is not used, as the OS provides its own confirmation
dialog.
* The `browserWindow` argument is ignored since it is not possible to make
this confirmation dialog modal.

**[Deprecated Soon](promisification.md)**

### `dialog.showCertificateTrustDialog([browserWindow, ]options)` _macOS_ _Windows_

* `browserWindow` [BrowserWindow](browser-window.md) (optional)
* `options` Object
* `certificate` [Certificate](structures/certificate.md) - The certificate to trust/import.
* `message` String - The message to display to the user.

Returns `Promise<void>` - resolves when the certificate trust dialog is shown.

On macOS, this displays a modal dialog that shows a message and certificate
information, and gives the user the option of trusting/importing the
certificate. If you provide a `browserWindow` argument the dialog will be
attached to the parent window, making it modal.

On Windows the options are more limited, due to the Win32 APIs used:

* The `message` argument is not used, as the OS provides its own confirmation
dialog.
* The `browserWindow` argument is ignored since it is not possible to make
Expand Down
4 changes: 2 additions & 2 deletions docs/api/promisification.md
Expand Up @@ -9,8 +9,6 @@ When a majority of affected functions are migrated, this flag will be enabled by
### Candidate Functions

- [app.importCertificate(options, callback)](https://github.com/electron/electron/blob/master/docs/api/app.md#importCertificate)
- [dialog.showMessageBox([browserWindow, ]options[, callback])](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showMessageBox)
- [dialog.showCertificateTrustDialog([browserWindow, ]options, callback)](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showCertificateTrustDialog)
- [contents.print([options], [callback])](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#print)

### Converted Functions
Expand All @@ -34,6 +32,8 @@ When a majority of affected functions are migrated, this flag will be enabled by
- [dialog.showSaveDialog([browserWindow, ]options[, callback])](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showSaveDialog)
- [inAppPurchase.purchaseProduct(productID, quantity, callback)](https://github.com/electron/electron/blob/master/docs/api/in-app-purchase.md#purchaseProduct)
- [inAppPurchase.getProducts(productIDs, callback)](https://github.com/electron/electron/blob/master/docs/api/in-app-purchase.md#getProducts)
- [dialog.showMessageBox([browserWindow, ]options[, callback])](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showMessageBox)
- [dialog.showCertificateTrustDialog([browserWindow, ]options, callback)](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showCertificateTrustDialog)
- [netLog.stopLogging([callback])](https://github.com/electron/electron/blob/master/docs/api/net-log.md#stopLogging)
- [protocol.isProtocolHandled(scheme, callback)](https://github.com/electron/electron/blob/master/docs/api/protocol.md#isProtocolHandled)
- [ses.clearHostResolverCache([callback])](https://github.com/electron/electron/blob/master/docs/api/session.md#clearHostResolverCache)
Expand Down
16 changes: 6 additions & 10 deletions lib/browser/api/dialog.js
Expand Up @@ -223,31 +223,27 @@ module.exports = {
return binding.showErrorBox(...args)
},

showCertificateTrustDialog: function (...args) {
const [window, options, callback] = parseArgs(...args)

showCertificateTrustDialog: function (window, options) {
if (window && window.constructor !== BrowserWindow) options = window
if (options == null || typeof options !== 'object') {
throw new TypeError('options must be an object')
}

let { certificate, message } = options
const { certificate, message = '' } = options
if (certificate == null || typeof certificate !== 'object') {
throw new TypeError('certificate must be an object')
}

if (message == null) {
message = ''
} else if (typeof message !== 'string') {
throw new TypeError('message must be a string')
}
if (typeof message !== 'string') throw new TypeError('message must be a string')

return binding.showCertificateTrustDialog(window, certificate, message, callback)
return binding.showCertificateTrustDialog(window, certificate, message)
}
}

module.exports.showMessageBox = deprecate.promisify(module.exports.showMessageBox)
module.exports.showOpenDialog = deprecate.promisify(module.exports.showOpenDialog)
module.exports.showSaveDialog = deprecate.promisify(module.exports.showSaveDialog)
module.exports.showCertificateTrustDialog = deprecate.promisify(module.exports.showCertificateTrustDialog)

// Mark standard asynchronous functions.
v8Util.setHiddenValue(module.exports.showMessageBox, 'asynchronous', true)
Expand Down