|
|
Created:
4 years, 5 months ago by Eugene But (OOO till 7-30) Modified:
4 years, 5 months ago CC:
chromium-reviews, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Implemented iOS analogue of SSLErrorHandler class.
Currently on iOS SSL interstitials are presented in donwstream's Tab.
This CL allows presenting them from WebClient::AllowCertificateError to be
consistent with other platforms (where interstitials are presented from
ContentBrowserClient::AllowCertificateError).
Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than
Tab code because the latter used to present Safe Browsing interstitials
which can not be supported by WKWebView.
BUG=602298
Committed: https://crrev.com/fbff955ad6b6f78b26582e896e5a79d0aaf4b2b8
Cr-Commit-Position: refs/heads/master@{#403288}
Patch Set 1 #Patch Set 2 : Disabled gn test #
Total comments: 12
Patch Set 3 : Addressed review comments #Patch Set 4 : Fixed test #
Total comments: 12
Patch Set 5 : Addressed review comments #
Messages
Total messages: 37 (17 generated)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== SSL Handler. BUG= ========== to ========== SSL Handler. BUG=602298 ==========
Description was changed from ========== SSL Handler. BUG=602298 ========== to ========== [ios] Implemented iOS analogue of SSLErrorHandler class. Currently on iOS SSL interstitials are presented in donwstream's Tab. This CL allows presenting them from WebClient::AllowCertificateError to be consistent with other platforms (where interstitials are presented from ContentBrowserClient::AllowCertificateError). Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than Tab code because the latter used to present Safe Browsing interstitials which can not be supported by WKWebView. BUG=602298 ==========
eugenebut@chromium.org changed reviewers: + estark@chromium.org
Hi Emily, I would like to move iOS interstitials presentation code from Tab (downstream code) to a separate class to be consistent with other platforms. Current implementation is here for reference: https://chromereviews.googleplex.com/456347013/ Please take a look. Thanks! https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/ios_chrome_t... File ios/chrome/ios_chrome_tests.gyp (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/ios_chrome_t... ios/chrome/ios_chrome_tests.gyp:90: '../../net/data/ssl/certificates/', Sylvain, do you know how to add this in gn build? I've tried this: "//net/data/ssl/certificates/ok_cert.pem", but it does not work (gn complains about ok_cert.pem duplication).
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/ios_chrome_t... File ios/chrome/ios_chrome_tests.gyp (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/ios_chrome_t... ios/chrome/ios_chrome_tests.gyp:90: '../../net/data/ssl/certificates/', On 2016/06/29 16:56:41, Eugene But wrote: > Sylvain, do you know how to add this in gn build? I've tried this: > "//net/data/ssl/certificates/ok_cert.pem", but it does not work (gn complains > about ok_cert.pem duplication). Any application that depends on //net:test_support_bundle_data (which is a dependency of //net:test_support IIRC) will get //net/data/ssl/certificates/ok_cert.pem packed in the application bundle as part of the build. The file will be accessible at net/data/ssl/certificates/ok_cert.pem into the application bundle (which is the correct location for such file according to the net team). So adding the dependency on //net:test_support should be enough to get this working with gn.
https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/ios_chrome_t... File ios/chrome/ios_chrome_tests.gyp (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/ios_chrome_t... ios/chrome/ios_chrome_tests.gyp:90: '../../net/data/ssl/certificates/', On 2016/06/29 17:10:03, sdefresne wrote: > On 2016/06/29 16:56:41, Eugene But wrote: > > Sylvain, do you know how to add this in gn build? I've tried this: > > "//net/data/ssl/certificates/ok_cert.pem", but it does not work (gn complains > > about ok_cert.pem duplication). > > Any application that depends on //net:test_support_bundle_data (which is a > dependency of //net:test_support IIRC) will get > //net/data/ssl/certificates/ok_cert.pem packed in the application bundle as part > of the build. The file will be accessible at > net/data/ssl/certificates/ok_cert.pem into the application bundle (which is the > correct location for such file according to the net team). > > So adding the dependency on //net:test_support should be enough to get this > working with gn. Thanks! It's already in gn bundle. I guess the answer is: no GN change is needed.
Hi Eugene, I'm not very familiar with ios but I did my best! https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.h (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.h:24: static void HandleSSLError(web::WebState* web_state, I'm not sure what the prevailing style is on ios, but in normal Chrome code I'd say it's a little weird to have a class that just has two static methods. Could you perhaps just make these standalone functions instead? https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.mm (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.mm:25: if (web_state->IsShowingWebInterstitial()) { Under what conditions would this happen? https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm:17: // Test cert filename. nit: I would just name these |kTestCertFileName| and |kTestHostName| and omit the comments. (Or even just leave the names as they are and you could still omit the comments IMO.) https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm:91: EXPECT_TRUE(proceed_callback_called); optional additional test: you could test that DontProceed() also calls the callback for overridable errors.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! PTAL https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.h (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.h:24: static void HandleSSLError(web::WebState* web_state, On 2016/06/29 23:00:15, estark wrote: > I'm not sure what the prevailing style is on ios, but in normal Chrome code I'd > say it's a little weird to have a class that just has two static methods. Could > you perhaps just make these standalone functions instead? No it's not prevailing style on iOS, and I do agree that a class with just static functions can just be replaced with namespaced static functions. My goal was to mirror chrome, which has SSLErrorHandler class with 4 static public functions (and I would argue that it could also be implemented as 4 namespaced functions). Currently IOSSSLErrorHandler does not have much meat in the implementation, but it will grow as we add more features (i.e. captive portal support). So unless you have strong opinion, I would leave this weird class as it is with a single static method. Please let me know if you disagree. https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.mm (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.mm:25: if (web_state->IsShowingWebInterstitial()) { On 2016/06/29 23:00:15, estark wrote: > Under what conditions would this happen? This is original code, which was written when we supported SafeBrowsing, so I guess it's for Safe Browsing. Replacing this with DCHECK would be more appropriate. https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm:17: // Test cert filename. On 2016/06/29 23:00:15, estark wrote: > nit: I would just name these |kTestCertFileName| and |kTestHostName| and omit > the comments. (Or even just leave the names as they are and you could still omit > the comments IMO.) Done. https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm:91: EXPECT_TRUE(proceed_callback_called); On 2016/06/29 23:00:15, estark wrote: > optional additional test: you could test that DontProceed() also calls the > callback for overridable errors. Done. Let me know if you think that I should also address code duplication. Personally I think it's ok to have them in tests for clarity.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.h (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.h:8: #include "base/callback.h" Can you instead include "base/callback_forward.h"? It should be enough for the header file (you'll need the include of "base/callback.h" in the implementation and the implementation of client code). https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.h:32: IOSSSLErrorHandler(); It looks like this class only has static method, so why not mark it as non constructible using DISALLOW_IMPLICIT_CONSTRUCTORS? Then you don't even have to provide implementation for the constructor in the implementation file. You may have to delete the destructor too: ~IOSSSLErrorHandler() = delete; https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.mm (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.mm:15: IOSSSLErrorHandler::~IOSSSLErrorHandler() {} Can you add new line between method definitions? https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.mm:17: /*static*/ "// static" is more common than "/* static */". https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm:29: // web::WebTestWithWebState overrides: nit: prefer to put code in constructor instead of overriding SetUp
https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/BUILD.gn File ios/chrome/BUILD.gn (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/BUILD.gn#new... ios/chrome/BUILD.gn:55: # TODO(crbug.com/624450): Add this test back once gn build contains all Should work once https://codereview.chromium.org/2119433002/ lands.
Description was changed from ========== [ios] Implemented iOS analogue of SSLErrorHandler class. Currently on iOS SSL interstitials are presented in donwstream's Tab. This CL allows presenting them from WebClient::AllowCertificateError to be consistent with other platforms (where interstitials are presented from ContentBrowserClient::AllowCertificateError). Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than Tab code because the latter used to present Safe Browsing interstitials which can not be supported by WKWebView. BUG=602298 ========== to ========== [ios] Implemented iOS analogue of SSLErrorHandler class. Currently on iOS SSL interstitials are presented in donwstream's Tab. This CL allows presenting them from WebClient::AllowCertificateError to be consistent with other platforms (where interstitials are presented from ContentBrowserClient::AllowCertificateError). Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than Tab code because the latter used to present Safe Browsing interstitials which can not be supported by WKWebView. BUG=602298 ==========
lgtm https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.h (right): https://codereview.chromium.org/2108783005/diff/20001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.h:24: static void HandleSSLError(web::WebState* web_state, On 2016/06/30 00:05:26, Eugene But wrote: > On 2016/06/29 23:00:15, estark wrote: > > I'm not sure what the prevailing style is on ios, but in normal Chrome code > I'd > > say it's a little weird to have a class that just has two static methods. > Could > > you perhaps just make these standalone functions instead? > No it's not prevailing style on iOS, and I do agree that a class with just > static functions can just be replaced with namespaced static functions. > My goal was to mirror chrome, which has SSLErrorHandler class with 4 static > public functions (and I would argue that it could also be implemented as 4 > namespaced functions). > > Currently IOSSSLErrorHandler does not have much meat in the implementation, but > it will grow as we add more features (i.e. captive portal support). So unless > you have strong opinion, I would leave this weird class as it is with a single > static method. Please let me know if you disagree. SSLErrorHandler has non-static protected/private methods though and is constructed from one of the static methods. But that seems fine to leave it as is if you think it'll grow.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/BUILD.gn File ios/chrome/BUILD.gn (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/BUILD.gn#new... ios/chrome/BUILD.gn:55: # TODO(crbug.com/624450): Add this test back once gn build contains all On 2016/06/30 12:01:17, sdefresne wrote: > Should work once https://codereview.chromium.org/2119433002/ lands. It does work now. Thank you for fixing this! https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.h (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.h:8: #include "base/callback.h" On 2016/06/30 11:58:16, sdefresne wrote: > Can you instead include "base/callback_forward.h"? It should be enough for the > header file (you'll need the include of "base/callback.h" in the implementation > and the implementation of client code). Done. https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.h:32: IOSSSLErrorHandler(); On 2016/06/30 11:58:16, sdefresne wrote: > It looks like this class only has static method, so why not mark it as non > constructible using DISALLOW_IMPLICIT_CONSTRUCTORS? Then you don't even have to > provide implementation for the constructor in the implementation file. You may > have to delete the destructor too: > > ~IOSSSLErrorHandler() = delete; Done. https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler.mm (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.mm:15: IOSSSLErrorHandler::~IOSSSLErrorHandler() {} On 2016/06/30 11:58:16, sdefresne wrote: > Can you add new line between method definitions? These were removed. https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler.mm:17: /*static*/ On 2016/06/30 11:58:16, sdefresne wrote: > "// static" is more common than "/* static */". Done. https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm (right): https://codereview.chromium.org/2108783005/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm:29: // web::WebTestWithWebState overrides: On 2016/06/30 11:58:16, sdefresne wrote: > nit: prefer to put code in constructor instead of overriding SetUp ASSERT_TRUE returns the value and can not be used inside the constructor. I do have a constructor for doing all set up work.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2108783005/#ps80001 (title: "Addressed review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [ios] Implemented iOS analogue of SSLErrorHandler class. Currently on iOS SSL interstitials are presented in donwstream's Tab. This CL allows presenting them from WebClient::AllowCertificateError to be consistent with other platforms (where interstitials are presented from ContentBrowserClient::AllowCertificateError). Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than Tab code because the latter used to present Safe Browsing interstitials which can not be supported by WKWebView. BUG=602298 ========== to ========== [ios] Implemented iOS analogue of SSLErrorHandler class. Currently on iOS SSL interstitials are presented in donwstream's Tab. This CL allows presenting them from WebClient::AllowCertificateError to be consistent with other platforms (where interstitials are presented from ContentBrowserClient::AllowCertificateError). Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than Tab code because the latter used to present Safe Browsing interstitials which can not be supported by WKWebView. BUG=602298 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [ios] Implemented iOS analogue of SSLErrorHandler class. Currently on iOS SSL interstitials are presented in donwstream's Tab. This CL allows presenting them from WebClient::AllowCertificateError to be consistent with other platforms (where interstitials are presented from ContentBrowserClient::AllowCertificateError). Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than Tab code because the latter used to present Safe Browsing interstitials which can not be supported by WKWebView. BUG=602298 ========== to ========== [ios] Implemented iOS analogue of SSLErrorHandler class. Currently on iOS SSL interstitials are presented in donwstream's Tab. This CL allows presenting them from WebClient::AllowCertificateError to be consistent with other platforms (where interstitials are presented from ContentBrowserClient::AllowCertificateError). Implementation of IOSSSLErrorHandler::HandleSSLError is even simpler than Tab code because the latter used to present Safe Browsing interstitials which can not be supported by WKWebView. BUG=602298 Committed: https://crrev.com/fbff955ad6b6f78b26582e896e5a79d0aaf4b2b8 Cr-Commit-Position: refs/heads/master@{#403288} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fbff955ad6b6f78b26582e896e5a79d0aaf4b2b8 Cr-Commit-Position: refs/heads/master@{#403288} |