|
|
Chromium Code Reviews
DescriptionCaptive portal certificate list should be checked when name mismatch is the only error
When there are errors in addition to a name-mismatch error, the captive portal
certificate list feature should not display a captive portal error. This was supposed
to be the case with the current implementation but it's buggy. The current check is
done by checking cert_error in SSLErrorHandler which is not sufficient, because a
cert_error of CERT_ERROR_COMMON_NAME_INVALID doesn't necessarily mean that it's the
only error, it means that net::MapCertStatusToNetError determined
CERT_ERROR_COMMON_NAME_INVALID to be the most important error.
There are also existing test cases that were trying to test this behavior, but they
were using authority-invalid errors to do so. This CL adds an extra test to check
for name-mismatch and weak key errors.
BUG=691163
Review-Url: https://codereview.chromium.org/2690333006
Cr-Commit-Position: refs/heads/master@{#454177}
Committed: https://chromium.googlesource.com/chromium/src/+/4f8acbd89df62b20df694810ec309c1bf1e0d6c9
Patch Set 1 #
Total comments: 7
Patch Set 2 : estark comments #
Total comments: 10
Patch Set 3 : estark comments and rebase #
Total comments: 6
Patch Set 4 : estark comments #Patch Set 5 : Fix build #Patch Set 6 : Fix Android tests #
Messages
Total messages: 30 (13 generated)
Description was changed from ========== Captive portal certificate list should be checked when name mismatch is the only error BUG=691163 ========== to ========== Captive portal certificate list should be checked when name mismatch is the only error When there are errors in addition to a name-mismatch error, the captive portal certificate list feature should not display a captive portal error. This was supposed to be the case with the current implementation but it's buggy. The current check is done by checking cert_error in SSLErrorHandler which is not sufficient, because a cert_error of CERT_ERROR_COMMON_NAME_INVALID doesn't necessarily mean that it's the only error, it means that net::MapCertStatusToNetError determined CERT_ERROR_COMMON_NAME_INVALID to be the most important error. There are also existing test cases that tried test this behavior, but they were using authority-invalid errors to do so. This CL adds an extra test to check for name-mismatch and weak key errors. BUG=691163 ==========
meacer@chromium.org changed reviewers: + estark@chromium.org
estark: PTAL? Thanks.
https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:4303: net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, nit: it might be good to have a sanity check that MapCertStatusToNetError() returns COMMON_NAME_INVALID. In other words make sure that SSLErrorHandler is looking at all the errors, not just the most serious one. https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:251: EXPECT_EQ(expected_cert_error, error_handler()->cert_error_for_testing()); I feel meh about having this cert_error_for_testing() getter. Is it just to sanity-check that the SSLErrorHandler was recreated with the appropriate error? If so, I don't think it's worth exposing the implementation detail just for a sanity-check. (especially because you can effectively do the same sanity-check by just checking that MapCertStatusToNetError is what you expect) https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:842: ResetErrorHandler(net::CERT_STATUS_COMMON_NAME_INVALID | same nit about sanity-checking to make sure the code isn't only looking at the most serious error
https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:4303: net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, On 2017/02/15 00:31:41, estark wrote: > nit: it might be good to have a sanity check that MapCertStatusToNetError() > returns COMMON_NAME_INVALID. Hmm, I thought about checking for name mismatch error through SSLErrorHandler::cert_error but there is no way to access the SSLErrorHandler instance. Just to understand your suggestion, are you saying we should check MapCertStatusToNetError output here? > In other words make sure that SSLErrorHandler is > looking at all the errors, not just the most serious one. I'm not sure I understand this part. How would MapCertStatusToNetError returning COMMON_NAME_INVALID ensure that?
Description was changed from ========== Captive portal certificate list should be checked when name mismatch is the only error When there are errors in addition to a name-mismatch error, the captive portal certificate list feature should not display a captive portal error. This was supposed to be the case with the current implementation but it's buggy. The current check is done by checking cert_error in SSLErrorHandler which is not sufficient, because a cert_error of CERT_ERROR_COMMON_NAME_INVALID doesn't necessarily mean that it's the only error, it means that net::MapCertStatusToNetError determined CERT_ERROR_COMMON_NAME_INVALID to be the most important error. There are also existing test cases that tried test this behavior, but they were using authority-invalid errors to do so. This CL adds an extra test to check for name-mismatch and weak key errors. BUG=691163 ========== to ========== Captive portal certificate list should be checked when name mismatch is the only error When there are errors in addition to a name-mismatch error, the captive portal certificate list feature should not display a captive portal error. This was supposed to be the case with the current implementation but it's buggy. The current check is done by checking cert_error in SSLErrorHandler which is not sufficient, because a cert_error of CERT_ERROR_COMMON_NAME_INVALID doesn't necessarily mean that it's the only error, it means that net::MapCertStatusToNetError determined CERT_ERROR_COMMON_NAME_INVALID to be the most important error. There are also existing test cases that were trying to test this behavior, but they were using authority-invalid errors to do so. This CL adds an extra test to check for name-mismatch and weak key errors. BUG=691163 ==========
On 2017/02/15 00:37:29, Mustafa Emre Acer wrote: > https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... > chrome/browser/ssl/ssl_browser_tests.cc:4303: > net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, > On 2017/02/15 00:31:41, estark wrote: > > nit: it might be good to have a sanity check that MapCertStatusToNetError() > > returns COMMON_NAME_INVALID. > > Hmm, I thought about checking for name mismatch error through > SSLErrorHandler::cert_error but there is no way to access the SSLErrorHandler > instance. > > Just to understand your suggestion, are you saying we should check > MapCertStatusToNetError output here? > > > In other words make sure that SSLErrorHandler is > > looking at all the errors, not just the most serious one. > > I'm not sure I understand this part. How would MapCertStatusToNetError returning > COMMON_NAME_INVALID ensure that? Hrm, here's what I was thinking. I thought the point of this test was to check that SSLErrorHandler doesn't just look at the most serious error and show the captive portal interstitial if it is COMMON_NAME_INVALID -- but instead that if there are less serious errors, it sees those and doesn't show the captive portal interstitial. So in pseudocode, I was thinking you might want to do something like this: CertStatus error = COMMON_NAME_INVALID | WEAK_SIGNATURE; // Sanity check that, if SSLErrorHandler only looked at the most serious error, it would see COMMON_NAME_INVALID. ASSERT_EQ(COMMON_NAME_INVALID, MapCertStatusToNetError(error)); SSLErrorHandler::HandleError(error); // Even though the most serious error is COMMON_NAME_INVALID, SSLErrorHandler should see that there are less serious errors and not show the captive portal interstitial. EXPECT_EQ(SSLBlockingPage, web_contents->GetInterstitialType()); Does that make sense? (I might be misunderstanding what you're trying to test.)
On 2017/02/16 07:01:33, estark (slow thru Feb 22) wrote: > On 2017/02/15 00:37:29, Mustafa Emre Acer wrote: > > > https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... > > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > > > https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... > > chrome/browser/ssl/ssl_browser_tests.cc:4303: > > net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, > > On 2017/02/15 00:31:41, estark wrote: > > > nit: it might be good to have a sanity check that MapCertStatusToNetError() > > > returns COMMON_NAME_INVALID. > > > > Hmm, I thought about checking for name mismatch error through > > SSLErrorHandler::cert_error but there is no way to access the SSLErrorHandler > > instance. > > > > Just to understand your suggestion, are you saying we should check > > MapCertStatusToNetError output here? > > > > > In other words make sure that SSLErrorHandler is > > > looking at all the errors, not just the most serious one. > > > > I'm not sure I understand this part. How would MapCertStatusToNetError > returning > > COMMON_NAME_INVALID ensure that? > > Hrm, here's what I was thinking. I thought the point of this test was to check > that SSLErrorHandler doesn't just look at the most serious error and show the > captive portal interstitial if it is COMMON_NAME_INVALID -- but instead that if > there are less serious errors, it sees those and doesn't show the captive portal > interstitial. So in pseudocode, I was thinking you might want to do something > like this: > > CertStatus error = COMMON_NAME_INVALID | WEAK_SIGNATURE; > // Sanity check that, if SSLErrorHandler only looked at the most serious error, > it would see COMMON_NAME_INVALID. > ASSERT_EQ(COMMON_NAME_INVALID, MapCertStatusToNetError(error)); > SSLErrorHandler::HandleError(error); > // Even though the most serious error is COMMON_NAME_INVALID, SSLErrorHandler > should see that there are less serious errors and not show the captive portal > interstitial. > EXPECT_EQ(SSLBlockingPage, web_contents->GetInterstitialType()); > > Does that make sense? (I might be misunderstanding what you're trying to test.) No, we are on the same page. I was a bit confused by your wording about SSLErrorHandler looking at all errors. I thought you meant actually looping through errors or something like that, but it seems you meant looking at the most serious error. The |cert_error_for_testing| was intended to do that, but I removed it and am now doing the check before setting the cert status. PTAL?
(and the comments) https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:4303: net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, Done. https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:251: EXPECT_EQ(expected_cert_error, error_handler()->cert_error_for_testing()); On 2017/02/15 00:31:41, estark (slow thru Feb 22) wrote: > I feel meh about having this cert_error_for_testing() getter. Is it just to > sanity-check that the SSLErrorHandler was recreated with the appropriate error? Correct. > If so, I don't think it's worth exposing the implementation detail just for a > sanity-check. (especially because you can effectively do the same sanity-check > by just checking that MapCertStatusToNetError is what you expect) I was trying to avoid doing that explicit check, in case SSLErrorHandler does some transformation to cert status other than MapCertStatusToNetError which we would miss. But okay, let's be consistent with the browser tests and do the same here. Note that I'm only doing this for name mismatch + weak signature case since the other two (name mismatch only or authority invalid only) are pretty straightforward (i.e. adding EXPECT_EQ(CERT_ERR_COMMON_NAME_INVALID, MapCertStatusToNetError(CERT_STATUS_COMMON_NAME_INVALID)) seems redundant) https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:842: ResetErrorHandler(net::CERT_STATUS_COMMON_NAME_INVALID | On 2017/02/15 00:31:41, estark (slow thru Feb 22) wrote: > same nit about sanity-checking to make sure the code isn't only looking at the > most serious error Done.
lgtm, thanks for removing the test getter! https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:4304: EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, nit: maybe explain what you're doing here: "Sanity check that COMMON_NAME_INVALID is seen as the net error, since the test is designed to verify that SSLErrorHandler notices other errors in the CertStatus even when COMMON_NAME_INVALID is the net error." another nit: I think technically sanity checks like this are supposed to be ASSERT_EQ instead of EXPECT (?) https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:559: // name-mismatch. nit: since we keep forgetting, it would probably be helpful to elaborate here on why this is the right thing to do. Something along the lines of: "If there are multiple errors, it indicates that the captive portal landing page itself will have SSL errors, and so it's not a very helpful place to direct the user to go." (is that right? I can't remember) https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:197: class SSLErrorHandlerNameMismatchTest : public ChromeRenderViewHostTestHarness { It's a little weird that you use this fixture to test errors that aren't name mismatch errors. Maybe rename to SSLErrorHandlerCertStatusTest or something more general? https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:819: TEST_F(SSLErrorHandlerNameMismatchTest, (see above, it's confusing that this is named NameMismatchTest but tests an error other than name mismatch) https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:844: EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, ditto nits about explaining what this is checking and ASSERT_EQ
https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:4304: EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, On 2017/02/25 01:33:17, estark wrote: > nit: maybe explain what you're doing here: "Sanity check that > COMMON_NAME_INVALID is seen as the net error, since the test is designed to > verify that SSLErrorHandler notices other errors in the CertStatus even when > COMMON_NAME_INVALID is the net error." > > another nit: I think technically sanity checks like this are supposed to be > ASSERT_EQ instead of EXPECT (?) Done. https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:559: // name-mismatch. On 2017/02/25 01:33:18, estark wrote: > nit: since we keep forgetting, it would probably be helpful to elaborate here on > why this is the right thing to do. Something along the lines of: "If there are > multiple errors, it indicates that the captive portal landing page itself will > have SSL errors, and so it's not a very helpful place to direct the user to go." > (is that right? I can't remember) I think that's right, because I can't find any other justification for this. Added the note. I also wrote a doc to enumerate various scenarios: https://docs.google.com/document/d/128GcLCNHyQLtv-kYOk1-6nwU_78iAhQpQWyPkN5b6... https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:197: class SSLErrorHandlerNameMismatchTest : public ChromeRenderViewHostTestHarness { On 2017/02/25 01:33:18, estark wrote: > It's a little weird that you use this fixture to test errors that aren't name > mismatch errors. Maybe rename to SSLErrorHandlerCertStatusTest or something more > general? Done. Splitted captive portal tests into a separate class. https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:819: TEST_F(SSLErrorHandlerNameMismatchTest, On 2017/02/25 01:33:18, estark wrote: > (see above, it's confusing that this is named NameMismatchTest but tests an > error other than name mismatch) Done. https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:844: EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, On 2017/02/25 01:33:18, estark wrote: > ditto nits about explaining what this is checking and ASSERT_EQ Done.
estark: I splitted some tests so you might want to take another quick look.
lgtm https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:4364: // Sanity check that COMMON_NAME_INVALID is seen as the net error, since the optional nit: I'd maybe put this comment the line below, right above the ASSERT_EQ https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:890: // Sanity check that AUTHORITY_INVALID is seen as the net error. same optional nit https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:906: // Sanity check that COMMON_NAME_INVALID is seen as the net error, since the ditto
Thanks! https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:4364: // Sanity check that COMMON_NAME_INVALID is seen as the net error, since the On 2017/02/28 21:12:06, estark wrote: > optional nit: I'd maybe put this comment the line below, right above the > ASSERT_EQ Done. https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:890: // Sanity check that AUTHORITY_INVALID is seen as the net error. On 2017/02/28 21:12:06, estark wrote: > same optional nit Done. https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:906: // Sanity check that COMMON_NAME_INVALID is seen as the net error, since the On 2017/02/28 21:12:06, estark wrote: > ditto Done.
The CQ bit was checked by meacer@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/2690333006/#ps60001 (title: "estark comments")
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
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by meacer@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/2690333006/#ps80001 (title: "Fix build")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by meacer@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/2690333006/#ps100001 (title: "Fix Android tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488421711302350,
"parent_rev": "df2469d2aacf348710be6f823bb4228696edef38", "commit_rev":
"4f8acbd89df62b20df694810ec309c1bf1e0d6c9"}
Message was sent while issue was closed.
Description was changed from ========== Captive portal certificate list should be checked when name mismatch is the only error When there are errors in addition to a name-mismatch error, the captive portal certificate list feature should not display a captive portal error. This was supposed to be the case with the current implementation but it's buggy. The current check is done by checking cert_error in SSLErrorHandler which is not sufficient, because a cert_error of CERT_ERROR_COMMON_NAME_INVALID doesn't necessarily mean that it's the only error, it means that net::MapCertStatusToNetError determined CERT_ERROR_COMMON_NAME_INVALID to be the most important error. There are also existing test cases that were trying to test this behavior, but they were using authority-invalid errors to do so. This CL adds an extra test to check for name-mismatch and weak key errors. BUG=691163 ========== to ========== Captive portal certificate list should be checked when name mismatch is the only error When there are errors in addition to a name-mismatch error, the captive portal certificate list feature should not display a captive portal error. This was supposed to be the case with the current implementation but it's buggy. The current check is done by checking cert_error in SSLErrorHandler which is not sufficient, because a cert_error of CERT_ERROR_COMMON_NAME_INVALID doesn't necessarily mean that it's the only error, it means that net::MapCertStatusToNetError determined CERT_ERROR_COMMON_NAME_INVALID to be the most important error. There are also existing test cases that were trying to test this behavior, but they were using authority-invalid errors to do so. This CL adds an extra test to check for name-mismatch and weak key errors. BUG=691163 Review-Url: https://codereview.chromium.org/2690333006 Cr-Commit-Position: refs/heads/master@{#454177} Committed: https://chromium.googlesource.com/chromium/src/+/4f8acbd89df62b20df694810ec30... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4f8acbd89df62b20df694810ec30... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
