Switch the macOS certificate viewer to a Mojo call.

The sheet is currently being drawn on a transparent overlay when using
remote Cocoa (e.g. PWA windows), which results in a number of UI issues.
This was exacerbated by
https://chromium-review.googlesource.com/c/chromium/src/+/2324112.

Instead, add a ShowCertificateViewer() Mojo call and move the
implementation to NativeWidgetNSWindowBridge. Along the way, simplify
the implementation. We can now largely rely on Cocoa's built-in lifetime
management for the SFCertificatePanel.

Bug: 916815, 1130490
Change-Id: Ia33102417510d7c0fc6fb09cf55d9fa61efdfdff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2421633Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809861}

chrome/browser/ui/views/certificate_viewer_mac_views.mm

@@ -2,124 +2,27 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#import <Cocoa/Cocoa.h>
-#import <SecurityInterface/SFCertificatePanel.h>
-
-#include "base/mac/foundation_util.h"
-#include "base/mac/scoped_cftyperef.h"
-#import "base/mac/scoped_nsobject.h"
-#include "base/notreached.h"
 #include "chrome/browser/certificate_viewer.h"
-#include "components/remote_cocoa/browser/window.h"
-#include "net/cert/x509_certificate.h"
-#include "net/cert/x509_util_ios_and_mac.h"
-#include "net/cert/x509_util_mac.h"
-
-@interface SSLCertificateViewerMac : NSObject
-// Initializes |certificates_| with the certificate chain for a given
-// certificate.
-- (instancetype)initWithCertificate:(net::X509Certificate*)certificate;
-
-// Shows the certificate viewer as a Cocoa sheet.
-- (void)showCertificateSheet:(NSWindow*)window;
-
-// Called when the certificate viewer Cocoa sheet is closed.
-- (void)sheetDidEnd:(NSWindow*)parent
-         returnCode:(NSInteger)returnCode
-            context:(void*)context;
-@end
-
-@implementation SSLCertificateViewerMac {
-  base::scoped_nsobject<NSArray> _certificates;
-  base::scoped_nsobject<SFCertificatePanel> _panel;
-  NSWindow* _remoteViewsCloneWindow;
-}
-
-- (instancetype)initWithCertificate:(net::X509Certificate*)certificate {
-  if ((self = [super init])) {
-    base::ScopedCFTypeRef<CFArrayRef> certChain(
-        net::x509_util::CreateSecCertificateArrayForX509Certificate(
-            certificate));
-    if (!certChain)
-      return self;
-    NSArray* certificates = base::mac::CFToNSCast(certChain.get());
-    _certificates.reset([certificates retain]);
-  }
-
-  // Explicitly disable revocation checking, regardless of user preferences
-  // or system settings. The behaviour of SFCertificatePanel is to call
-  // SecTrustEvaluate on the certificate(s) supplied, effectively
-  // duplicating the behaviour of net::X509Certificate::Verify(). However,
-  // this call stalls the UI if revocation checking is enabled in the
-  // Keychain preferences or if the cert may be an EV cert. By disabling
-  // revocation checking, the stall is limited to the time taken for path
-  // building and verification, which should be minimized due to the path
-  // being provided in |certificates|. This does not affect normal
-  // revocation checking from happening, which is controlled by
-  // net::X509Certificate::Verify() and user preferences, but will prevent
-  // the certificate viewer UI from displaying which certificate is revoked.
-  // This is acceptable, as certificate revocation will still be shown in
-  // the page info bubble if a certificate in the chain is actually revoked.
-  base::ScopedCFTypeRef<CFMutableArrayRef> policies(
-      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
-  if (!policies.get()) {
-    NOTREACHED();
-    return self;
-  }
-  // Add a basic X.509 policy, in order to match the behaviour of
-  // SFCertificatePanel when no policies are specified.
-  base::ScopedCFTypeRef<SecPolicyRef> basicPolicy;
-  OSStatus status =
-      net::x509_util::CreateBasicX509Policy(basicPolicy.InitializeInto());
-  if (status != noErr) {
-    NOTREACHED();
-    return self;
-  }
-  CFArrayAppendValue(policies, basicPolicy.get());
 
-  status = net::x509_util::CreateRevocationPolicies(false, policies);
-  if (status != noErr) {
-    NOTREACHED();
-    return self;
-  }
-
-  _panel.reset([[SFCertificatePanel alloc] init]);
-  [_panel setPolicies:base::mac::CFToNSCast(policies.get())];
-  return self;
-}
-
-- (void)showCertificateSheet:(NSWindow*)window {
-  // TODO(https://crbug.com/913303): The certificate viewer's interface to
-  // Cocoa should be wrapped in a mojo interface in order to allow
-  // instantiating across processes. As a temporary solution, create a
-  // transparent in-process window to the front.
-  if (remote_cocoa::IsWindowRemote(window)) {
-    _remoteViewsCloneWindow =
-        remote_cocoa::CreateInProcessTransparentClone(window);
-    window = _remoteViewsCloneWindow;
-  }
-  [_panel beginSheetForWindow:window
-                modalDelegate:self
-               didEndSelector:@selector(sheetDidEnd:returnCode:context:)
-                  contextInfo:nil
-                 certificates:_certificates
-                    showGroup:YES];
-}
-
-- (void)sheetDidEnd:(NSWindow*)parent
-         returnCode:(NSInteger)returnCode
-            context:(void*)context {
-  [_remoteViewsCloneWindow close];
-  _remoteViewsCloneWindow = nil;
-  _panel.reset();
-}
-
-@end
+#include "base/logging.h"
+#include "components/remote_cocoa/browser/window.h"
+#include "components/remote_cocoa/common/native_widget_ns_window.mojom.h"
 
 void ShowCertificateViewer(content::WebContents* web_contents,
                            gfx::NativeWindow parent,
                            net::X509Certificate* cert) {
-  base::scoped_nsobject<SSLCertificateViewerMac> viewer(
-      [[SSLCertificateViewerMac alloc] initWithCertificate:cert]);
-  [viewer showCertificateSheet:parent.GetNativeNSWindow()];
+  // The certificate viewer on macOS uses the OS viewer rather than the Views
+  // implementation (see https://crbug.com/953425), so go through a Mojo
+  // interface. This calls the platform APIs from the right process in PWAs.
+  // See https://crbug.com/916815. If this dialog is switched to Views, the Mojo
+  // call will no longer be needed.
+  remote_cocoa::mojom::NativeWidgetNSWindow* mojo_window =
+      remote_cocoa::GetWindowMojoInterface(parent);
+  if (!mojo_window) {
+    // Every WebContents window should have a Mojo interface.
+    LOG(ERROR) << "Could not get window Mojo interface";
+    return;
+  }
+
+  mojo_window->ShowCertificateViewer(cert);
 }

components/remote_cocoa/DEPS

@@ -3,6 +3,8 @@ include_rules = [
   "+components/crash/core/common/crash_key.h",
   "+components/viz/common",
   "+mojo/public/cpp/bindings",
+  "+net/cert",
+  "+services/network/public/mojom/network_param.mojom",
   "+skia/ext",
   "+ui/accelerated_widget_mac",
   "+ui/base",

components/remote_cocoa/app_shim/BUILD.gn

@@ -26,6 +26,8 @@ component("app_shim") {
     "bridged_content_view_touch_bar.mm",
     "browser_native_widget_window_mac.h",
     "browser_native_widget_window_mac.mm",
+    "certificate_viewer.h",
+    "certificate_viewer.mm",
     "color_panel_bridge.h",
     "color_panel_bridge.mm",
     "drag_drop_client.h",
@@ -59,6 +61,7 @@ component("app_shim") {
     "//components/crash/core/common",
     "//components/remote_cocoa/common:mojo",
     "//mojo/public/cpp/bindings",
+    "//net",
     "//ui/accelerated_widget_mac",
     "//ui/base",
     "//ui/base/ime:ime",
@@ -69,5 +72,6 @@ component("app_shim") {
   frameworks = [
     "Cocoa.framework",
     "QuartzCore.framework",
+    "SecurityInterface.framework",
   ]
 }

components/remote_cocoa/app_shim/certificate_viewer.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_REMOTE_COCOA_APP_SHIM_CERTIFICATE_VIEWER_H_
+#define COMPONENTS_REMOTE_COCOA_APP_SHIM_CERTIFICATE_VIEWER_H_
+
+#import <Cocoa/Cocoa.h>
+
+namespace net {
+class X509Certificate;
+}
+
+namespace remote_cocoa {
+
+// Shows the platform certificate viewer, displaying |certificate| and parented
+// to |owning_window|.
+void ShowCertificateViewerForWindow(NSWindow* owning_window,
+                                    net::X509Certificate* ceriticate);
+
+}  // namespace remote_cocoa
+
+#endif  // COMPONENTS_REMOTE_COCOA_APP_SHIM_CERTIFICATE_VIEWER_H_

components/remote_cocoa/app_shim/certificate_viewer.mm

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "components/remote_cocoa/app_shim/certificate_viewer.h"
+
+#include <CoreFoundation/CFArray.h>
+#include <Security/Security.h>
+#import <SecurityInterface/SFCertificatePanel.h>
+
+#include "base/mac/foundation_util.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/notreached.h"
+#include "net/cert/x509_util_ios_and_mac.h"
+#include "net/cert/x509_util_mac.h"
+
+namespace remote_cocoa {
+
+void ShowCertificateViewerForWindow(NSWindow* owning_window,
+                                    net::X509Certificate* certificate) {
+  base::ScopedCFTypeRef<CFArrayRef> cert_chain(
+      net::x509_util::CreateSecCertificateArrayForX509Certificate(certificate));
+  if (!cert_chain)
+    return;
+
+  // Explicitly disable revocation checking, regardless of user preferences
+  // or system settings. The behaviour of SFCertificatePanel is to call
+  // SecTrustEvaluate on the certificate(s) supplied, effectively
+  // duplicating the behaviour of net::X509Certificate::Verify(). However,
+  // this call stalls the UI if revocation checking is enabled in the
+  // Keychain preferences or if the cert may be an EV cert. By disabling
+  // revocation checking, the stall is limited to the time taken for path
+  // building and verification, which should be minimized due to the path
+  // being provided in |certificates|. This does not affect normal
+  // revocation checking from happening, which is controlled by
+  // net::X509Certificate::Verify() and user preferences, but will prevent
+  // the certificate viewer UI from displaying which certificate is revoked.
+  // This is acceptable, as certificate revocation will still be shown in
+  // the page info bubble if a certificate in the chain is actually revoked.
+  base::ScopedCFTypeRef<CFMutableArrayRef> policies(
+      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
+  if (!policies.get()) {
+    NOTREACHED();
+    return;
+  }
+  // Add a basic X.509 policy, in order to match the behaviour of
+  // SFCertificatePanel when no policies are specified.
+  base::ScopedCFTypeRef<SecPolicyRef> basic_policy;
+  OSStatus status =
+      net::x509_util::CreateBasicX509Policy(basic_policy.InitializeInto());
+  if (status != noErr) {
+    NOTREACHED();
+    return;
+  }
+  CFArrayAppendValue(policies, basic_policy.get());
+
+  status = net::x509_util::CreateRevocationPolicies(false, policies);
+  if (status != noErr) {
+    NOTREACHED();
+    return;
+  }
+
+  SFCertificatePanel* panel = [[SFCertificatePanel alloc] init];
+  [panel setPolicies:base::mac::CFToNSCast(policies.get())];
+  [panel beginSheetForWindow:owning_window
+               modalDelegate:nil
+              didEndSelector:nil
+                 contextInfo:nil
+                certificates:base::mac::CFToNSCast(cert_chain.get())
+                   showGroup:YES];
+  // beginSheetForWindow: internally retains an extra reference to |panel| and
+  // releases it when the sheet closes. Release the original reference so the
+  // sheet is destroyed.
+  [panel autorelease];
+}
+
+}  // namespace remote_cocoa

components/remote_cocoa/app_shim/native_widget_ns_window_bridge.h

@@ -201,6 +201,8 @@ class REMOTE_COCOA_APP_SHIM_EXPORT NativeWidgetNSWindowBridge
   void SetParent(uint64_t parent_id) override;
   void CreateSelectFileDialog(
       mojo::PendingReceiver<mojom::SelectFileDialog> receiver) override;
+  void ShowCertificateViewer(
+      const scoped_refptr<net::X509Certificate>& certificate) override;
   void StackAbove(uint64_t sibling_id) override;
   void StackAtTop() override;
   void ShowEmojiPanel() override;

components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm

@@ -19,6 +19,7 @@
 #include "base/strings/sys_string_conversions.h"
 #import "components/remote_cocoa/app_shim/bridged_content_view.h"
 #import "components/remote_cocoa/app_shim/browser_native_widget_window_mac.h"
+#import "components/remote_cocoa/app_shim/certificate_viewer.h"
 #import "components/remote_cocoa/app_shim/mouse_capture.h"
 #import "components/remote_cocoa/app_shim/native_widget_mac_frameless_nswindow.h"
 #import "components/remote_cocoa/app_shim/native_widget_mac_nswindow.h"
@@ -390,6 +391,11 @@ void NativeWidgetNSWindowBridge::CreateSelectFileDialog(
       std::move(receiver));
 }
 
+void NativeWidgetNSWindowBridge::ShowCertificateViewer(
+    const scoped_refptr<net::X509Certificate>& certificate) {
+  ShowCertificateViewerForWindow(window_, certificate.get());
+}
+
 void NativeWidgetNSWindowBridge::StackAbove(uint64_t sibling_id) {
   NativeWidgetNSWindowBridge* sibling_bridge =
       NativeWidgetNSWindowBridge::GetFromId(sibling_id);

components/remote_cocoa/browser/window.h

@@ -55,12 +55,6 @@ GetWindowMojoInterface(gfx::NativeWindow window);
 // being viewed in a remote process.
 bool REMOTE_COCOA_BROWSER_EXPORT IsWindowRemote(gfx::NativeWindow window);
 
-// Create a transparent NSWindow that is in the same position as |window|,
-// but is at the ModalPanel window level, so that it will appear over all
-// other window.
-NSWindow* REMOTE_COCOA_BROWSER_EXPORT
-CreateInProcessTransparentClone(gfx::NativeWindow window);
-
 }  // namespace remote_cocoa
 
 #endif  // COMPONENTS_REMOTE_COCOA_BROWSER_WINDOW_H_

components/remote_cocoa/browser/window.mm

@@ -66,17 +66,4 @@ mojom::NativeWidgetNSWindow* GetWindowMojoInterface(
   return nullptr;
 }
 
-NSWindow* CreateInProcessTransparentClone(gfx::NativeWindow remote_window) {
-  DCHECK(IsWindowRemote(remote_window));
-  NSWindow* window = [[NSWindow alloc]
-      initWithContentRect:[remote_window.GetNativeNSWindow() frame]
-                styleMask:NSWindowStyleMaskBorderless
-                  backing:NSBackingStoreBuffered
-                    defer:NO];
-  [window setAlphaValue:0];
-  [window makeKeyAndOrderFront:nil];
-  [window setLevel:NSModalPanelWindowLevel];
-  return window;
-}
-
 }  // namespace remote_cocoa

components/remote_cocoa/common/BUILD.gn

@@ -19,6 +19,7 @@ mojom("mojo") {
 
   public_deps = [
     "//mojo/public/mojom/base",
+    "//services/network/public/mojom",
     "//ui/base/accelerators/mojom",
     "//ui/base/mojom",
     "//ui/display/mojom",

components/remote_cocoa/common/native_widget_ns_window.mojom

@@ -6,6 +6,7 @@ module remote_cocoa.mojom;
 
 import "components/remote_cocoa/common/select_file_dialog.mojom";
 import "mojo/public/mojom/base/string16.mojom";
+import "services/network/public/mojom/network_param.mojom";
 import "ui/base/mojom/ui_base_types.mojom";
 import "ui/gfx/geometry/mojom/geometry.mojom";
 import "ui/gfx/mojom/ca_layer_params.mojom";
@@ -91,6 +92,9 @@ interface NativeWidgetNSWindow {
   // Open a panel for file selection or creation.
   CreateSelectFileDialog(pending_receiver<SelectFileDialog> dialog);
 
+  // Open a panel to display the specified certificate.
+  ShowCertificateViewer(network.mojom.X509Certificate certificate);
+
   // Places this NativeWidgetNSWindow in front of the NativeWidgetNSWindow
   // indicated by |sibling_id| in z-order.
   StackAbove(uint64 sibling_id);