Skip to content

GitLab

T
tangled
Commit 9740f4b3 authored by David Benjamin's avatar David Benjamin Committed by Commit Bot
Browse files
Options

Simplify SSL client certificate selector logic 

We currently have a C++ views object owning an Cocoa object which then owns a
bridge back into some other C++ interfaces. We can simplify this by having the
top-level C++ object handle those interfaces and leave the Cocoa object to
purely manage the SFChooseIdentityPanel.

This doesn't fix the https://crbug.com/987744 but is some cleanup along the
way. Before we add more cases to that logic, reduce the number of places we
spread that logic out.

This does result in some RunUntilIdle() calls in the tests no longer quite
waiting long enough, I suspect because the Cocoa event loop is involved. I've
switched them to specifically wait for the relevant object to be destroyed,
which seems more robust in general. It seems that was also the cause of the
lifetime mismatch in https://crbug.com/1041175 so now we don't need a WeakPtr.

Unfortunately the diff here is kind of hard to read. I had to reorder some of
the classes so they could refer to each other. (Note the old bridge object was
similarly interleaved between the @interface and @implementation.)

Bug: 987744, 1041175
Change-Id: Icccc9e18cdfc64444a6496748dc106be6233896a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978946
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: default avatarRyan Sleevi <rsleevi@chromium.org>
Reviewed-by: default avatarElly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734237}
parent e06316c0
Hide whitespace changes
Inline Side-by-side
Showing with 262 additions and 284 deletions
chrome/browser/ssl/ssl_client_auth_observer.cc
View file @ 9740f4b3
...@@ -21,9 +21,17 @@ SSLClientAuthObserver::SSLClientAuthObserver( ...@@ -21,9 +21,17 @@ SSLClientAuthObserver::SSLClientAuthObserver(
std::unique_ptr<content::ClientCertificateDelegate> delegate) std::unique_ptr<content::ClientCertificateDelegate> delegate)
: browser_context_(browser_context), : browser_context_(browser_context),
cert_request_info_(cert_request_info), cert_request_info_(cert_request_info),
delegate_(std::move(delegate)) {} delegate_(std::move(delegate)) {
DCHECK(delegate_);
}
SSLClientAuthObserver::~SSLClientAuthObserver() { SSLClientAuthObserver::~SSLClientAuthObserver() {
// The caller is required to explicitly stop observing, but call
// StopObserving() anyway to avoid a dangling pointer. (StopObserving() is
// idempotent, so it may be called multiple times.)
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK_EQ(0u, GetActiveObservers().count(this));
StopObserving();
} }
void SSLClientAuthObserver::CertificateSelected( void SSLClientAuthObserver::CertificateSelected(
......
chrome/browser/ssl/ssl_client_auth_observer.h
View file @ 9740f4b3
...@@ -46,10 +46,13 @@ class SSLClientAuthObserver { ...@@ -46,10 +46,13 @@ class SSLClientAuthObserver {
// Cancels the certificate selection and aborts the request. // Cancels the certificate selection and aborts the request.
void CancelCertificateSelection(); void CancelCertificateSelection();
// Begins observing notifications from other SSLClientAuthHandler instances. // Begins observing notifications from other SSLClientAuthObserver instances.
// If another instance chooses a cert for a matching SSLCertRequestInfo, we // If another instance chooses a cert for a matching SSLCertRequestInfo, we
// will also use the same cert and OnCertSelectedByNotification will be called // will also use the same cert and OnCertSelectedByNotification will be called
// so that the cert selection UI can be closed. // so that the cert selection UI can be closed.
//
// The caller must call CertificateSelected(), CancelCertificateSelection(),
// or StopObserving() before the SSLClientAuthObserver is destroyed.
void StartObserving(); void StartObserving();
// Stops observing notifications. We will no longer act on client auth // Stops observing notifications. We will no longer act on client auth
......
chrome/browser/ui/views/ssl_client_certificate_selector_mac.mm
View file @ 9740f4b3
...@@ -34,7 +34,6 @@ ...@@ -34,7 +34,6 @@
#include "net/ssl/ssl_platform_key_mac.h" #include "net/ssl/ssl_platform_key_mac.h"
#include "ui/base/buildflags.h" #include "ui/base/buildflags.h"
#include "ui/base/l10n/l10n_util_mac.h" #include "ui/base/l10n/l10n_util_mac.h"
#include "ui/views/widget/widget_observer.h"
@interface SFChooseIdentityPanel (SystemPrivate) @interface SFChooseIdentityPanel (SystemPrivate)
// A system-private interface that dismisses a panel whose sheet was started by // A system-private interface that dismisses a panel whose sheet was started by
...@@ -44,69 +43,10 @@ ...@@ -44,69 +43,10 @@
- (void)_dismissWithCode:(NSInteger)code; - (void)_dismissWithCode:(NSInteger)code;
@end @end
// This is the main class that runs the certificate selector panel. It's in
// Objective-C mainly because the only way to get a result out of that panel is
// a callback of a target/selector pair.
@interface SSLClientCertificateSelectorMac : NSObject
- (instancetype)
initWithBrowserContext:(const content::BrowserContext*)browserContext
certRequestInfo:(net::SSLCertRequestInfo*)certRequestInfo
delegate:
(std::unique_ptr<content::ClientCertificateDelegate>)delegate;
- (void)createForWebContents:(content::WebContents*)webContents
clientCerts:(net::ClientCertIdentityList)inputClientCerts;
- (void)setOverlayWindow:(views::Widget*)overlayWindow;
- (void)closeSelectorSheetWithCode:(NSModalResponse)response;
@end
// A bridge to the C++ world. It performs the two tasks of being a
// SSLClientAuthObserver and bridging to the SSL authentication system, and
// being a WidgetObserver for the overlay window so that if it is closed the
// cert selector is shut down.
class SSLClientAuthObserverCocoaBridge : public SSLClientAuthObserver,
public views::WidgetObserver {
public:
SSLClientAuthObserverCocoaBridge(
const content::BrowserContext* browser_context,
net::SSLCertRequestInfo* cert_request_info,
std::unique_ptr<content::ClientCertificateDelegate> delegate,
SSLClientCertificateSelectorMac* controller)
: SSLClientAuthObserver(browser_context,
cert_request_info,
std::move(delegate)),
controller_(controller) {}
void SetOverlayWindow(views::Widget* overlay_window) {
overlay_window->AddObserver(this);
}
// SSLClientAuthObserver implementation:
void OnCertSelectedByNotification() override {
[controller_ closeSelectorSheetWithCode:NSModalResponseStop];
}
// WidgetObserver:
void OnWidgetClosing(views::Widget* widget) override {
// Note that the SFChooseIdentityPanel takes a reference to its delegate in
// its -beginSheetForWindow:... method (bad SFChooseIdentityPanel!) so break
// the retain cycle by explicitly canceling the dialog.
[controller_ closeSelectorSheetWithCode:NSModalResponseAbort];
}
void OnWidgetDestroying(views::Widget* widget) override {
widget->RemoveObserver(this);
}
private:
SSLClientCertificateSelectorMac* controller_; // weak, owns us
};
namespace { namespace {
class SSLClientCertificateSelectorDelegate;
// These Clear[Window]TableViewDataSources... functions help work around a bug // These Clear[Window]TableViewDataSources... functions help work around a bug
// in macOS where SFChooseIdentityPanel leaks a window and some views, including // in macOS where SFChooseIdentityPanel leaks a window and some views, including
// an NSTableView. Future events may make cause the table view to query its // an NSTableView. Future events may make cause the table view to query its
...@@ -138,153 +78,19 @@ void ClearWindowTableViewDataSources(NSWindow* window) { ...@@ -138,153 +78,19 @@ void ClearWindowTableViewDataSources(NSWindow* window) {
} // namespace } // namespace
@implementation SSLClientCertificateSelectorMac { // This is the main class that runs the certificate selector panel. It's in
// The list of SecIdentityRefs offered to the user. // Objective-C mainly because the only way to get a result out of that panel is
base::scoped_nsobject<NSMutableArray> _sec_identities; // a callback of a target/selector pair.
@interface SSLClientCertificateSelectorMac : NSObject
// The corresponding list of ClientCertIdentities.
net::ClientCertIdentityList _cert_identities;
// A C++ object to bridge SSLClientAuthObserver notifications to us.
std::unique_ptr<SSLClientAuthObserverCocoaBridge> _observer;
base::scoped_nsobject<SFChooseIdentityPanel> _panel;
// Invisible overlay window used to block interaction with the tab underneath.
views::Widget* _overlayWindow;
}
- (instancetype) - (instancetype)
initWithBrowserContext:(const content::BrowserContext*)browserContext initWithClientCerts:(net::ClientCertIdentityList)clientCerts
certRequestInfo:(net::SSLCertRequestInfo*)certRequestInfo delegate:(base::WeakPtr<SSLClientCertificateSelectorDelegate>)
delegate:(std::unique_ptr<content::ClientCertificateDelegate>) delegate;
delegate {
DCHECK(browserContext);
DCHECK(certRequestInfo);
if ((self = [super init])) {
_observer = std::make_unique<SSLClientAuthObserverCocoaBridge>(
browserContext, certRequestInfo, std::move(delegate), self);
}
return self;
}
// The selector sheet ended. There are four possibilities for the return code.
//
// These two return codes are actually generated by the SFChooseIdentityPanel,
// although for testing purposes the OkAndCancelableForTesting implementation
// will also generate them to simulate the user clicking buttons.
//
// - NSModalResponseOK/Cancel: The user clicked the "OK" or "Cancel" button; the
// SSL auth system needs to be told of this choice.
//
// These two return codes are generated by the SSLClientAuthObserverCocoaBridge
// to force the SFChooseIdentityPanel to be closed for various reasons.
//
// - NSModalResponseAbort: The user closed the owning tab; the SSL auth system
// needs to be told of this cancellation.
// - NSModalResponseStop: The SSL auth system already has an answer; just tear
// down the dialog.
//
// Note that there is a disagreement between the docs and the SDK header file as
// to the type of the return code. It has empirically been determined to be an
// int, not an NSInteger. rdar://45344010
- (void)sheetDidEnd:(NSWindow*)sheet
returnCode:(int)returnCode
context:(void*)context {
if (returnCode == NSModalResponseAbort) {
_observer->CancelCertificateSelection();
} else if (returnCode == NSModalResponseOK ||
returnCode == NSModalResponseCancel) {
net::ClientCertIdentity* cert = nullptr;
if (returnCode == NSModalResponseOK) {
NSUInteger index = [_sec_identities indexOfObject:(id)[_panel identity]];
if (index != NSNotFound)
cert = _cert_identities[index].get();
}
if (cert) {
_observer->CertificateSelected(
cert->certificate(),
CreateSSLPrivateKeyForSecIdentity(cert->certificate(),
cert->sec_identity_ref())
.get());
} else {
_observer->CertificateSelected(nullptr, nullptr);
}
} else {
DCHECK_EQ(NSModalResponseStop, returnCode);
// Do nothing else; do not call back to the SSL auth system.
}
// Stop observing the SSL authentication system. In theory this isn't needed
// as the CertificateSelected() and CancelCertificateSelection() calls both
// implicitly call StopObserving() and the SSL auth system calls
// StopObserving() before making the OnCertSelectedByNotification() callback.
// However, StopObserving() is idempotent so call it out of a deep paranoia
// born of many a dangling pointer.
_observer->StopObserving();
// See comment at definition; this works around a bug.
ClearWindowTableViewDataSources(sheet);
// Do not release SFChooseIdentityPanel here. Its -_okClicked: method, after
// calling out to this method, keeps accessing its ivars, and if panel_ is the
// last reference keeping it alive, it will crash.
_panel.autorelease();
_overlayWindow->Close(); // Asynchronously releases |self|.
}
- (void)createForWebContents:(content::WebContents*)webContents
clientCerts:(net::ClientCertIdentityList)inputClientCerts {
_cert_identities = std::move(inputClientCerts);
_sec_identities.reset([[NSMutableArray alloc] init]);
for (const auto& cert : _cert_identities) {
DCHECK(cert->sec_identity_ref());
[_sec_identities addObject:(id)cert->sec_identity_ref()];
}
// Get the message to display:
NSString* message = l10n_util::GetNSStringF(
IDS_CLIENT_CERT_DIALOG_TEXT,
base::ASCIIToUTF16(
_observer->cert_request_info()->host_and_port.ToString()));
// Create and set up a system choose-identity panel. - (void)showSheetForWindow:(NSWindow*)window;
_panel.reset([[SFChooseIdentityPanel alloc] init]);
[_panel setInformativeText:message];
[_panel setDefaultButtonTitle:l10n_util::GetNSString(IDS_OK)];
[_panel setAlternateButtonTitle:l10n_util::GetNSString(IDS_CANCEL)];
base::ScopedCFTypeRef<SecPolicyRef> sslPolicy;
if (net::x509_util::CreateSSLClientPolicy(sslPolicy.InitializeInto()) ==
noErr) {
[_panel setPolicies:(id)sslPolicy.get()];
}
}
- (void)closeSelectorSheetWithCode:(NSModalResponse)response { - (void)closeSelectorSheetWithCode:(NSModalResponse)response;
// Closing the sheet using -[NSApp endSheet:] doesn't work, so use the private
// method. If the sheet is already closed then this is a message send to nil
// and thus a no-op.
[_panel _dismissWithCode:response];
}
- (void)showSheetForWindow:(NSWindow*)window {
NSString* title = l10n_util::GetNSString(IDS_CLIENT_CERT_DIALOG_TITLE);
[_panel beginSheetForWindow:window
modalDelegate:self
didEndSelector:@selector(sheetDidEnd:returnCode:context:)
contextInfo:nil
identities:_sec_identities
message:title];
_observer->StartObserving();
}
- (void)setOverlayWindow:(views::Widget*)overlayWindow {
_overlayWindow = overlayWindow;
_observer->SetOverlayWindow(_overlayWindow);
}
@end @end
...@@ -321,25 +127,32 @@ namespace { ...@@ -321,25 +127,32 @@ namespace {
// OS-provided client certificate selector. // OS-provided client certificate selector.
class SSLClientCertificateSelectorDelegate class SSLClientCertificateSelectorDelegate
: public views::WidgetDelegateView, : public views::WidgetDelegateView,
public chrome::OkAndCancelableForTesting { public chrome::OkAndCancelableForTesting,
public SSLClientAuthObserver {
public: public:
SSLClientCertificateSelectorDelegate( SSLClientCertificateSelectorDelegate(
content::WebContents* contents, content::WebContents* contents,
net::SSLCertRequestInfo* cert_request_info, net::SSLCertRequestInfo* cert_request_info,
net::ClientCertIdentityList client_certs, net::ClientCertIdentityList client_certs,
std::unique_ptr<content::ClientCertificateDelegate> delegate) std::unique_ptr<content::ClientCertificateDelegate> delegate)
: certificate_selector_([[SSLClientCertificateSelectorMac alloc] : SSLClientAuthObserver(contents->GetBrowserContext(),
initWithBrowserContext:contents->GetBrowserContext() cert_request_info,
certRequestInfo:cert_request_info std::move(delegate)) {
delegate:std::move(delegate)]), certificate_selector_.reset([[SSLClientCertificateSelectorMac alloc]
weak_factory_(this) { initWithClientCerts:std::move(client_certs)
delegate:weak_factory_.GetWeakPtr()]);
views::Widget* overlay_window = views::Widget* overlay_window =
constrained_window::ShowWebModalDialogWithOverlayViews(this, contents); constrained_window::ShowWebModalDialogWithOverlayViews(this, contents);
[certificate_selector_ setOverlayWindow:overlay_window];
[certificate_selector_ createForWebContents:contents
clientCerts:std::move(client_certs)];
[certificate_selector_ showSheetForWindow:overlay_window->GetNativeWindow() [certificate_selector_ showSheetForWindow:overlay_window->GetNativeWindow()
.GetNativeNSWindow()]; .GetNativeNSWindow()];
StartObserving();
}
~SSLClientCertificateSelectorDelegate() override {
// Note that the SFChooseIdentityPanel takes a reference to its delegate
// (|certificate_selector_|) in its -beginSheetForWindow:... method. Break
// the retain cycle by explicitly canceling the dialog.
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseAbort];
} }
// WidgetDelegate: // WidgetDelegate:
...@@ -354,6 +167,11 @@ class SSLClientCertificateSelectorDelegate ...@@ -354,6 +167,11 @@ class SSLClientCertificateSelectorDelegate
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseCancel]; [certificate_selector_ closeSelectorSheetWithCode:NSModalResponseCancel];
} }
// SSLClientAuthObserver implementation:
void OnCertSelectedByNotification() override {
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseStop];
}
void SetDeallocClosureForTesting(base::OnceClosure dealloc_closure) { void SetDeallocClosureForTesting(base::OnceClosure dealloc_closure) {
DeallocClosureCaller* caller = [[DeallocClosureCaller alloc] DeallocClosureCaller* caller = [[DeallocClosureCaller alloc]
initWithDeallocClosure:std::move(dealloc_closure)]; initWithDeallocClosure:std::move(dealloc_closure)];
...@@ -369,6 +187,10 @@ class SSLClientCertificateSelectorDelegate ...@@ -369,6 +187,10 @@ class SSLClientCertificateSelectorDelegate
weak_factory_.GetWeakPtr()); weak_factory_.GetWeakPtr());
} }
void CloseWidgetWithReason(views::Widget::ClosedReason reason) {
GetWidget()->CloseWithReason(reason);
}
private: private:
void CloseSelector() { void CloseSelector() {
[certificate_selector_ closeSelectorSheetWithCode:NSModalResponseStop]; [certificate_selector_ closeSelectorSheetWithCode:NSModalResponseStop];
...@@ -376,13 +198,155 @@ class SSLClientCertificateSelectorDelegate ...@@ -376,13 +198,155 @@ class SSLClientCertificateSelectorDelegate
base::scoped_nsobject<SSLClientCertificateSelectorMac> certificate_selector_; base::scoped_nsobject<SSLClientCertificateSelectorMac> certificate_selector_;
base::WeakPtrFactory<SSLClientCertificateSelectorDelegate> weak_factory_; base::WeakPtrFactory<SSLClientCertificateSelectorDelegate> weak_factory_{
this};
DISALLOW_COPY_AND_ASSIGN(SSLClientCertificateSelectorDelegate); DISALLOW_COPY_AND_ASSIGN(SSLClientCertificateSelectorDelegate);
}; };
} // namespace } // namespace
@implementation SSLClientCertificateSelectorMac {
// The list of SecIdentityRefs offered to the user.
base::scoped_nsobject<NSMutableArray> _secIdentities;
// The corresponding list of ClientCertIdentities.
net::ClientCertIdentityList _certIdentities;
// A C++ object to report the client certificate selection to.
base::WeakPtr<SSLClientCertificateSelectorDelegate> _delegate;
base::scoped_nsobject<SFChooseIdentityPanel> _panel;
}
- (instancetype)
initWithClientCerts:(net::ClientCertIdentityList)clientCerts
delegate:(base::WeakPtr<SSLClientCertificateSelectorDelegate>)
delegate {
if ((self = [super init])) {
_delegate = delegate;
_certIdentities = std::move(clientCerts);
_secIdentities.reset([[NSMutableArray alloc] init]);
for (const auto& cert : _certIdentities) {
DCHECK(cert->sec_identity_ref());
[_secIdentities addObject:(id)cert->sec_identity_ref()];
}
}
return self;
}
// The selector sheet ended. There are four possibilities for the return code.
//
// These two return codes are actually generated by the SFChooseIdentityPanel,
// although for testing purposes the OkAndCancelableForTesting implementation
// will also generate them to simulate the user clicking buttons.
//
// - NSModalResponseOK/Cancel: The user clicked the "OK" or "Cancel" button; the
// SSL auth system needs to be told of this choice.
//
// These two return codes are generated by the
// SSLClientCertificateSelectorDelegate to force the SFChooseIdentityPanel to be
// closed for various reasons.
//
// - NSModalResponseAbort: The user closed the owning tab; the SSL auth system
// needs to be told of this cancellation.
// - NSModalResponseStop: The SSL auth system already has an answer; just tear
// down the dialog.
//
// Note that there is a disagreement between the docs and the SDK header file as
// to the type of the return code. It has empirically been determined to be an
// int, not an NSInteger. rdar://45344010
- (void)sheetDidEnd:(NSWindow*)sheet
returnCode:(int)returnCode
context:(void*)context {
views::Widget::ClosedReason closedReason =
views::Widget::ClosedReason::kUnspecified;
if (_delegate) {
if (returnCode == NSModalResponseAbort) {
_delegate->CancelCertificateSelection();
} else if (returnCode == NSModalResponseOK ||
returnCode == NSModalResponseCancel) {
net::ClientCertIdentity* cert = nullptr;
if (returnCode == NSModalResponseOK) {
NSUInteger index = [_secIdentities indexOfObject:(id)[_panel identity]];
if (index != NSNotFound)
cert = _certIdentities[index].get();
closedReason = views::Widget::ClosedReason::kAcceptButtonClicked;
} else {
closedReason = views::Widget::ClosedReason::kCancelButtonClicked;
}
if (cert) {
_delegate->CertificateSelected(
cert->certificate(),
CreateSSLPrivateKeyForSecIdentity(cert->certificate(),
cert->sec_identity_ref())
.get());
} else {
_delegate->CertificateSelected(nullptr, nullptr);
}
} else {
DCHECK_EQ(NSModalResponseStop, returnCode);
_delegate->StopObserving();
}
} else {
// This should be impossible, assuming _dismissWithCode: synchronously calls
// this method. (SSLClientCertificateSelectorDelegate calls
// closeSelectorSheetWithCode: on destruction.)
NOTREACHED();
}
// See comment at definition; this works around a bug.
ClearWindowTableViewDataSources(sheet);
// Do not release SFChooseIdentityPanel here. Its -_okClicked: method, after
// calling out to this method, keeps accessing its ivars, and if panel_ is the
// last reference keeping it alive, it will crash.
_panel.autorelease();
if (_delegate) {
// This asynchronously releases |self|.
_delegate->CloseWidgetWithReason(closedReason);
}
}
- (void)showSheetForWindow:(NSWindow*)window {
// Get the message to display:
NSString* message = l10n_util::GetNSStringF(
IDS_CLIENT_CERT_DIALOG_TEXT,
base::ASCIIToUTF16(
_delegate->cert_request_info()->host_and_port.ToString()));
// Create and set up a system choose-identity panel.
_panel.reset([[SFChooseIdentityPanel alloc] init]);
[_panel setInformativeText:message];
[_panel setDefaultButtonTitle:l10n_util::GetNSString(IDS_OK)];
[_panel setAlternateButtonTitle:l10n_util::GetNSString(IDS_CANCEL)];
base::ScopedCFTypeRef<SecPolicyRef> sslPolicy;
if (net::x509_util::CreateSSLClientPolicy(sslPolicy.InitializeInto()) ==
noErr) {
[_panel setPolicies:(id)sslPolicy.get()];
}
NSString* title = l10n_util::GetNSString(IDS_CLIENT_CERT_DIALOG_TITLE);
[_panel beginSheetForWindow:window
modalDelegate:self
didEndSelector:@selector(sheetDidEnd:returnCode:context:)
contextInfo:nil
identities:_secIdentities
message:title];
}
- (void)closeSelectorSheetWithCode:(NSModalResponse)response {
// Closing the sheet using -[NSApp endSheet:] doesn't work, so use the private
// method. If the sheet is already closed then this is a message send to nil
// and thus a no-op.
[_panel _dismissWithCode:response];
}
@end
namespace chrome { namespace chrome {
base::OnceClosure ShowSSLClientCertificateSelector( base::OnceClosure ShowSSLClientCertificateSelector(
......
chrome/browser/ui/views/ssl_client_certificate_selector_mac_browsertest.mm
View file @ 9740f4b3
...@@ -11,7 +11,6 @@ ...@@ -11,7 +11,6 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/bind_helpers.h" #include "base/bind_helpers.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/test/metrics/histogram_tester.h" #include "base/test/metrics/histogram_tester.h"
#include "chrome/browser/ssl/ssl_client_auth_metrics.h" #include "chrome/browser/ssl/ssl_client_auth_metrics.h"
#include "chrome/browser/ssl/ssl_client_certificate_selector.h" #include "chrome/browser/ssl/ssl_client_certificate_selector.h"
...@@ -37,46 +36,63 @@ using web_modal::WebContentsModalDialogManager; ...@@ -37,46 +36,63 @@ using web_modal::WebContentsModalDialogManager;
namespace { namespace {
struct TestClientCertificateDelegateResults class TestClientCertificateDelegateResults {
: public base::SupportsWeakPtr<TestClientCertificateDelegateResults> { public:
bool destroyed = false; bool destroyed() const { return destroyed_; }
bool continue_with_certificate_called = false; bool continue_with_certificate_called() const {
scoped_refptr<net::X509Certificate> cert; return continue_with_certificate_called_;
scoped_refptr<net::SSLPrivateKey> key; }
net::X509Certificate* cert() const { return cert_.get(); }
net::SSLPrivateKey* key() const { return key_.get(); }
void WaitForDelegateDestroyed() {
if (!destroyed_)
run_loop_.Run();
EXPECT_TRUE(destroyed_);
}
void OnDelegateDestroyed() {
destroyed_ = true;
run_loop_.Quit();
}
void ContinueWithCertificate(scoped_refptr<net::X509Certificate> cert,
scoped_refptr<net::SSLPrivateKey> key) {
EXPECT_FALSE(continue_with_certificate_called_);
cert_ = std::move(cert);
key_ = std::move(key);
continue_with_certificate_called_ = true;
}
private:
bool destroyed_ = false;
bool continue_with_certificate_called_ = false;
scoped_refptr<net::X509Certificate> cert_;
scoped_refptr<net::SSLPrivateKey> key_;
base::RunLoop run_loop_;
}; };
class TestClientCertificateDelegate class TestClientCertificateDelegate
: public content::ClientCertificateDelegate { : public content::ClientCertificateDelegate {
public: public:
// Creates a ClientCertificateDelegate that sets |results->destroyed| to true // Creates a ClientCertificateDelegate that sets |results->destroyed| to true
// on destruction. The |results| parameter is a WeakPtr so we avoid retaining // on destruction. The delegate must not outlive |results|.
// a dangling pointer to a stack object that has since been deallocated.
explicit TestClientCertificateDelegate( explicit TestClientCertificateDelegate(
base::WeakPtr<TestClientCertificateDelegateResults> results) TestClientCertificateDelegateResults* results)
: results_(results) {} : results_(results) {}
~TestClientCertificateDelegate() override { ~TestClientCertificateDelegate() override { results_->OnDelegateDestroyed(); }
if (!results_) {
return;
}
results_->destroyed = true;
}
// content::ClientCertificateDelegate. // content::ClientCertificateDelegate.
void ContinueWithCertificate(scoped_refptr<net::X509Certificate> cert, void ContinueWithCertificate(scoped_refptr<net::X509Certificate> cert,
scoped_refptr<net::SSLPrivateKey> key) override { scoped_refptr<net::SSLPrivateKey> key) override {
if (!results_) { results_->ContinueWithCertificate(std::move(cert), std::move(key));
return;
}
EXPECT_FALSE(results_->continue_with_certificate_called);
results_->cert = cert;
results_->key = key;
results_->continue_with_certificate_called = true;
// TODO(mattm): Add a test of selecting the 2nd certificate (if possible). // TODO(mattm): Add a test of selecting the 2nd certificate (if possible).
} }
private: private:
base::WeakPtr<TestClientCertificateDelegateResults> results_; TestClientCertificateDelegateResults* results_;
DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate); DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate);
}; };
...@@ -154,21 +170,19 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Basic) { ...@@ -154,21 +170,19 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Basic) {
chrome::ShowSSLClientCertificateSelector( chrome::ShowSSLClientCertificateSelector(
web_contents, auth_requestor_->cert_request_info_.get(), web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(results.AsWeakPtr())); std::make_unique<TestClientCertificateDelegate>(&results));
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
WebContentsModalDialogManager::TestApi test_api( WebContentsModalDialogManager::TestApi test_api(
web_contents_modal_dialog_manager); web_contents_modal_dialog_manager);
test_api.CloseAllDialogs(); test_api.CloseAllDialogs();
base::RunLoop().RunUntilIdle(); results.WaitForDelegateDestroyed();
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
histograms.ExpectUniqueSample(kClientCertSelectHistogramName, histograms.ExpectUniqueSample(kClientCertSelectHistogramName,
ClientCertSelectionResult::kUserCloseTab, 1); ClientCertSelectionResult::kUserCloseTab, 1);
EXPECT_FALSE(results.continue_with_certificate_called());
EXPECT_TRUE(results.destroyed);
EXPECT_FALSE(results.continue_with_certificate_called);
} }
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
...@@ -185,35 +199,21 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, ...@@ -185,35 +199,21 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
chrome::ShowSSLClientCertificateSelector( chrome::ShowSSLClientCertificateSelector(
web_contents, auth_requestor_->cert_request_info_.get(), web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(results.AsWeakPtr())); std::make_unique<TestClientCertificateDelegate>(&results));
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
// Cancel the dialog without selecting a certificate. // Cancel the dialog without selecting a certificate.
std::move(cancellation_callback).Run(); std::move(cancellation_callback).Run();
base::RunLoop().RunUntilIdle(); results.WaitForDelegateDestroyed();
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
// The user did not close the tab, so there should be zero samples reported // The user did not close the tab, so there should be zero samples reported
// for ClientCertSelectionResult::kUserCloseTab. // for ClientCertSelectionResult::kUserCloseTab.
histograms.ExpectTotalCount(kClientCertSelectHistogramName, 0);
// A note on object lifetimes: EXPECT_FALSE(results.continue_with_certificate_called());
//
// The earlier call to ShowSSLClientCertificateSelector() created a
// self-owning (and self-deleting) SSLClientAuthObserver, which also owns the
// TestClientCertificateDelegate in this test.
//
// At this point, the TestClientCertificateDelegate's destructor has not
// executed because no SSLClientAuthObserver methods that trigger
// self-deletion have been called, e.g. ContinueWithCertificate().
//
// Since ~TestClientCertificateDelegate() must write to |results| to indicate
// destruction occurred, it must have some way to know whether |results|
// still exists. This is why we are using a WeakPtr to |results|.
EXPECT_FALSE(results.destroyed);
EXPECT_FALSE(results.continue_with_certificate_called);
} }
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Cancel) { IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Cancel) {
...@@ -229,23 +229,22 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Cancel) { ...@@ -229,23 +229,22 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Cancel) {
chrome::ShowSSLClientCertificateSelectorMacForTesting( chrome::ShowSSLClientCertificateSelectorMacForTesting(
web_contents, auth_requestor_->cert_request_info_.get(), web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(results.AsWeakPtr()), std::make_unique<TestClientCertificateDelegate>(&results),
base::DoNothing()); base::DoNothing());
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
ok_and_cancelable->ClickCancelButton(); ok_and_cancelable->ClickCancelButton();
base::RunLoop().RunUntilIdle(); results.WaitForDelegateDestroyed();
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
histograms.ExpectUniqueSample(kClientCertSelectHistogramName, histograms.ExpectUniqueSample(kClientCertSelectHistogramName,
ClientCertSelectionResult::kUserCancel, 1); ClientCertSelectionResult::kUserCancel, 1);
// ContinueWithCertificate(nullptr, nullptr) should have been called. // ContinueWithCertificate(nullptr, nullptr) should have been called.
EXPECT_TRUE(results.destroyed); EXPECT_TRUE(results.continue_with_certificate_called());
EXPECT_TRUE(results.continue_with_certificate_called); EXPECT_FALSE(results.cert());
EXPECT_FALSE(results.cert); EXPECT_FALSE(results.key());
EXPECT_FALSE(results.key);
} }
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Accept) { IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Accept) {
...@@ -261,28 +260,27 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Accept) { ...@@ -261,28 +260,27 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, Accept) {
chrome::ShowSSLClientCertificateSelectorMacForTesting( chrome::ShowSSLClientCertificateSelectorMacForTesting(
web_contents, auth_requestor_->cert_request_info_.get(), web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(results.AsWeakPtr()), std::make_unique<TestClientCertificateDelegate>(&results),
base::DoNothing()); base::DoNothing());
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
ok_and_cancelable->ClickOkButton(); ok_and_cancelable->ClickOkButton();
base::RunLoop().RunUntilIdle(); results.WaitForDelegateDestroyed();
EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive());
// The first cert in the list should have been selected. // The first cert in the list should have been selected.
EXPECT_TRUE(results.destroyed); EXPECT_TRUE(results.continue_with_certificate_called());
EXPECT_TRUE(results.continue_with_certificate_called); EXPECT_EQ(client_cert1_, results.cert());
EXPECT_EQ(client_cert1_, results.cert); ASSERT_TRUE(results.key());
ASSERT_TRUE(results.key);
histograms.ExpectUniqueSample(kClientCertSelectHistogramName, histograms.ExpectUniqueSample(kClientCertSelectHistogramName,
ClientCertSelectionResult::kUserSelect, 1); ClientCertSelectionResult::kUserSelect, 1);
// The test keys are RSA keys. // The test keys are RSA keys.
EXPECT_EQ(net::SSLPrivateKey::DefaultAlgorithmPreferences(EVP_PKEY_RSA, true), EXPECT_EQ(net::SSLPrivateKey::DefaultAlgorithmPreferences(EVP_PKEY_RSA, true),
results.key->GetAlgorithmPreferences()); results.key()->GetAlgorithmPreferences());
TestSSLPrivateKeyMatches(results.key.get(), pkcs8_key1_); TestSSLPrivateKeyMatches(results.key(), pkcs8_key1_);
} }
// Test that switching to another tab correctly hides the sheet. // Test that switching to another tab correctly hides the sheet.
...@@ -304,7 +302,7 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, HideShow) { ...@@ -304,7 +302,7 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, HideShow) {
chrome::ShowSSLClientCertificateSelector( chrome::ShowSSLClientCertificateSelector(
web_contents, auth_requestor_->cert_request_info_.get(), web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(), GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(results.AsWeakPtr())); std::make_unique<TestClientCertificateDelegate>(&results));
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive()); EXPECT_TRUE(web_contents_modal_dialog_manager->IsDialogActive());
...@@ -327,19 +325,20 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, HideShow) { ...@@ -327,19 +325,20 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, HideShow) {
EXPECT_EQ(1.0, [sheet_window alphaValue]); EXPECT_EQ(1.0, [sheet_window alphaValue]);
EXPECT_FALSE([overlay_window ignoresMouseEvents]); EXPECT_FALSE([overlay_window ignoresMouseEvents]);
EXPECT_FALSE(results.destroyed); EXPECT_FALSE(results.destroyed());
EXPECT_FALSE(results.continue_with_certificate_called); EXPECT_FALSE(results.continue_with_certificate_called());
// Close the tab. Delegate should be destroyed without continuing. // Close the tab. Delegate should be destroyed without continuing.
chrome::CloseTab(browser()); chrome::CloseTab(browser());
EXPECT_TRUE(results.destroyed); results.WaitForDelegateDestroyed();
EXPECT_FALSE(results.continue_with_certificate_called); EXPECT_FALSE(results.continue_with_certificate_called());
} }
// Test that we can't trigger the crash from https://crbug.com/653093 // Test that we can't trigger the crash from https://crbug.com/653093
IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
WorkaroundTableViewCrash) { WorkaroundTableViewCrash) {
base::RunLoop run_loop; base::RunLoop run_loop;
TestClientCertificateDelegateResults results;
@autoreleasepool { @autoreleasepool {
content::WebContents* web_contents = content::WebContents* web_contents =
...@@ -347,7 +346,9 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, ...@@ -347,7 +346,9 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
chrome::OkAndCancelableForTesting* ok_and_cancelable = chrome::OkAndCancelableForTesting* ok_and_cancelable =
chrome::ShowSSLClientCertificateSelectorMacForTesting( chrome::ShowSSLClientCertificateSelectorMacForTesting(
web_contents, auth_requestor_->cert_request_info_.get(), web_contents, auth_requestor_->cert_request_info_.get(),
GetTestCertificateList(), nullptr, run_loop.QuitClosure()); GetTestCertificateList(),
std::make_unique<TestClientCertificateDelegate>(&results),
run_loop.QuitClosure());
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
ok_and_cancelable->ClickOkButton(); ok_and_cancelable->ClickOkButton();
...@@ -370,4 +371,6 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest, ...@@ -370,4 +371,6 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMacTest,
userInfo:@{ userInfo:@{
@"NSScrollerStyle" : @(NSScrollerStyleOverlay) @"NSScrollerStyle" : @(NSScrollerStyleOverlay)
}]; }];
results.WaitForDelegateDestroyed();
} }
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment