Issue metadata
Sign in to add a comment
|
Move the list of fatal cert errors from SSLManager to SSLErrorHandler |
||||||||||||||||||||||||
Issue descriptionThe main idea is to remove |overridable| from the options_mask in SSLManager and [Chrome]ContentBrowserClient, and calculate it in SSLErrorHandler. This way, the calculation of |overridable| can be shared for committed interstitials in Issue 752370 . Here's what I'm doing in my CL. Note that we currently have two similar-but-not-compatible options masks: - SSLManager::OnCertErrorInternalOptionsMask - SSLErrorUI::SSLErrorOptionsMask 1) Instead of options_mask, pass two booleans (|strict_enforcement| and |expired_previous_decision|) into SSLManager::OnCertErrorInternal() and SSLErrorHandler::HandleSSLError(). - Remove the SSLManager::OnCertErrorInternalOptionsMask enum. 2) Remove the overridable calculation from the switch statement in SSLManager::OnCertError() and create SSLErrorHandler::IsCertErrorFatal(). 3) Calculate an options_mask (based on SSLErrorUI::SSLErrorOptionsMask) in SSLErrorHandler::HandleSSLError and pass it into SSLErrorHandlerDelegateImpl(). - This uses SSLErrorHandler::IsCertErrorFatal(). 4) Remove the |overridable| argument from ContentBrowserClient::AllowCertificateError() and all implementations. Step 4 touches a lot of files with trivial changes, and I'm thinking of doing it in a separate CL. (The first CL would just make the |overridable| argument useless.)
,
Oct 4 2017
,
Oct 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d954eef1154df2ea1ed2fb9ab80b463852b124a9 commit d954eef1154df2ea1ed2fb9ab80b463852b124a9 Author: Lucas Garron <lgarron@chromium.org> Date: Wed Oct 04 19:57:56 2017 Move the calculation of fatal cert errors from SSLManager to SSLErrorHandler. content::SSLManager previously contained some logic for computing whether/how an interstitial should be bypassable, which it then passed into ContentBrowserClient::AllowCertificateError(), though only ChromeContentBrowserClient's implementation was using it. This presents a problem for the interstitial refactor because this logic is needed for interstitial display, but SSLManager/ChromeContentBrowserClient::AllowCertificateError() is no longer going to be the point that triggers interstitial display. We're moving this logic into //chrome's SSLErrorHandler, which is a more convenient place for the logic to live because both the old and new interstitial display codepaths pass through that point. This change moves the calculation of the following into ssl_error_handler.cc: 1. The mapping of cert_error -> fatal (opposite of "soft overridable"). - Moves from SSLManager::OnCertError() and into an anonymous namespace in `ssl_error_handler.cc`. 2. The calculation of an SSLErrorUI::SSLErrorOptionsMask mask for SSLBlockingPage::Create(). - Moves from ChromeContentBrowserClient::AllowCertificateError(). In addition, this change removes the SSLManager::OnCertErrorInternalOptionsMask enum, since its use would be reduced to a passing a single bool from SSLManager::OnCertError() to SSLManager::OnCertErrorInternal(). (Also, it was confusingly similar to SSLErrorUI::SSLErrorOptionsMask.) This change leaves the `overridable` parameter in ContentBrowserClient::AllowCertificateError() unused. The followup CL at crrev.com/c/680114 will remove it. Bug: 768105 Change-Id: Iffbd56d944fc96427cb41a691aecd04236bb4cb6 Reviewed-on: https://chromium-review.googlesource.com/679937 Commit-Queue: Lucas Garron <lgarron@chromium.org> Reviewed-by: Emily Stark <estark@chromium.org> Cr-Commit-Position: refs/heads/master@{#506490} [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/chrome/browser/ssl/ssl_blocking_page.cc [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/chrome/browser/ssl/ssl_blocking_page.h [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/chrome/browser/ssl/ssl_browser_tests.cc [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/chrome/browser/ssl/ssl_error_handler.cc [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/chrome/browser/ssl/ssl_error_handler.h [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/chrome/browser/ssl/ssl_error_handler_unittest.cc [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/components/security_interstitials/core/ssl_error_ui.h [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/content/browser/ssl/ssl_manager.cc [modify] https://crrev.com/d954eef1154df2ea1ed2fb9ab80b463852b124a9/content/browser/ssl/ssl_manager.h
,
Oct 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e commit 9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e Author: Lucas Garron <lgarron@chromium.org> Date: Thu Oct 05 03:56:41 2017 Remove `overridable` parameter from ContentBrowserClient::AllowCertificateError() and its overrides. content::SSLManager previously contained some logic for computing whether/how an interstitial should be bypassable, which it then passed into ContentBrowserClient::AllowCertificateError(), though only ChromeContentBrowserClient's implementation was using it. This presented a problem for the interstitial refactor because this logic is needed for interstitial display, but SSLManager/ChromeContentBrowserClient::AllowCertificateError() is no longer going to be the point that triggers interstitial display. crrev.com/c/679937 moved this logic into //chrome's SSLErrorHandler, which is a more convenient place for the logic to live because both the old and new interstitial display codepaths pass through that point. In order to simplify reviews, that CL kept the interface of `ContentBrowserClient::AllowCertificateError()` intact, even though the `overridable` calculation is now done in SSLErrorHandler. This follow-up CL modifies `AllowCertificateError` to remove the defunct `overridable` parameter. Note that this change does not update the similar parameter in WebClient::AllowCertificateError() on iOS. TBR=jochen@chromium.org Bug: 768105 Change-Id: I8214a89b6497108dea3dfe636735c865041a0673 Reviewed-on: https://chromium-review.googlesource.com/680114 Commit-Queue: Lucas Garron <lgarron@chromium.org> Reviewed-by: Emily Stark <estark@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#506638} [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/android_webview/browser/aw_content_browser_client.cc [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/android_webview/browser/aw_content_browser_client.h [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/chromecast/browser/cast_content_browser_client.cc [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/chromecast/browser/cast_content_browser_client.h [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/content/browser/ssl/ssl_manager.cc [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/content/public/browser/content_browser_client.cc [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/content/public/browser/content_browser_client.h [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/headless/lib/browser/headless_content_browser_client.cc [modify] https://crrev.com/9cc660cd8f17beef93a1d8fb7a3c2dd7e87ce55e/headless/lib/browser/headless_content_browser_client.h
,
Oct 9 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by lgar...@chromium.org
, Sep 23 2017