Improve the SSL error display when the clock is wrong.

Improve the SSL error display when the clock is wrong.

Show the clock error page when the clock is too far in the future as well as
too far in the past.

Do not show text suggesting the user can fix it by reloading later; the
clock is definitely wrong and reloading won't help.

BUG=349653
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291507

[palmer, 2014-07-26 00:15:51]

This code is complicated and needs a refactor that defends against the
combinatorial explosion in error message strings, but for now this helps in a
few areas.

[felt, 2014-07-26 01:12:36]

https://codereview.chromium.org/416403006/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/416403006/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:596:
l10n_util::GetStringUTF16(IDS_SSL_NONOVERRIDABLE_RELOAD_BUTTON));
should we use the IDS_SSL_OVERRIDABLE_SAFETY_BUTTON in both cases since, as you
suggest, reloading doesn't help?

https://codereview.chromium.org/416403006/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:607: return
webui::GetI18nTemplateHtml(html, &load_time_data);
this appears to return without setting a number of parts of the warning,
including the primary paragraph, the open button text, the close button text,
and likely others that I can't spot

[felt, 2014-07-26 01:13:06]

https://codereview.chromium.org/416403006/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/416403006/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:607: return
webui::GetI18nTemplateHtml(html, &load_time_data);
On 2014/07/26 01:12:36, felt wrote:
> this appears to return without setting a number of parts of the warning,
> including the primary paragraph, the open button text, the close button text,
> and likely others that I can't spot

oh wait, the primary paragraph is set... but it looks like some others aren't?

[palmer, 2014-07-28 20:57:17]

> oh wait, the primary paragraph is set... but it looks like some others aren't?

That's on purpose — what more can be said, when the clock is definitely
wrong/the user's Chrome is pre-historic?

[felt, 2014-07-28 23:41:06]

On 2014/07/28 20:57:17, Chromium Palmer wrote:
> > oh wait, the primary paragraph is set... but it looks like some others
aren't?
> 
> That's on purpose — what more can be said, when the clock is definitely
> wrong/the user's Chrome is pre-historic?

I don't know how to easily test this to check, but I would expect this current
CL to throw a number of errors when you run it.

<a href="#" id="details-button" class="small-link"
i18n-content="openDetails"></a>

If openDetails is not set (which it isn't), that should throw an error.

The errorCode also looks like it isn't being set, so I would expect this to
throw an error:

$('error-code').textContent = loadTimeData.getString('errorCode');

Etc.

[palmer, 2014-08-07 01:26:15]

https://codereview.chromium.org/416403006/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/416403006/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:607: return
webui::GetI18nTemplateHtml(html, &load_time_data);
> oh wait, the primary paragraph is set... but it looks like some others aren't?

I changed it; see what you think now.

[felt, 2014-08-07 18:09:09]

This seems to have removed some changes I liked, like don't show the reload
button if reloading isn't going to help.

Also I thought you were going to do something with the details paragraph if it's
a bad clock error?

https://codereview.chromium.org/416403006/diff/1/chrome/browser/ssl/ssl_block...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/416403006/diff/1/chrome/browser/ssl/ssl_block...
chrome/browser/ssl/ssl_blocking_page.cc:678: // TODO(palmer): Is this line dead
code?
No. For some bizarre reason webui depends on this. It is ugly but what seems to
be done at the moment. /

[palmer, 2014-08-12 01:13:30]

> This seems to have removed some changes I liked, like don't show the reload
> button if reloading isn't going to help.

I think it's best to leave it, so people can click the obvious Reload button
after updating their clocks.

> Also I thought you were going to do something with the details paragraph if
it's
> a bad clock error?

Added.

[felt, 2014-08-12 19:35:09]

https://codereview.chromium.org/416403006/diff/100001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/416403006/diff/100001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:452: if (!bad_clock) {
The "final paragraph" is the sentence with a link to proceed through the error.
Removing the final paragraph has the effect of making it impossible to proceed
through the error.

Is that intentional? If so, perhaps you should set overridable_ to false and
follow that logic path. Otherwise, simply hiding the final paragraph will cause
the warning page to explode since it expects it to be there for an overridable
SSL error.

interstitial_v2.html assumes that finalParagraph is always set, and there will
be an error if it isn't:
<p i18n-values=".innerHTML:finalParagraph" id="final-paragraph"></p>

Similarly, interstitial_v2.js assumes that there is a proceed link in the
finalParagraph if the page is an overridable SSL warning:

  if (overridable) {
    $('proceed-link').addEventListener('click', function(event) {
      sendCommand(ssl ? CMD_PROCEED : SB_CMD_PROCEED);
    });
  } else if (!ssl) {
    $('final-paragraph').classList.add('hidden');
  }

[palmer, 2014-08-12 22:45:34]

https://codereview.chromium.org/416403006/diff/100001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/416403006/diff/100001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:452: if (!bad_clock) {
> The "final paragraph" is the sentence with a link to proceed through the
error.
> Removing the final paragraph has the effect of making it impossible to proceed
> through the error.
> 
> Is that intentional?

No, it was an artefact of previous hacking. Fixed now.

[felt, 2014-08-13 00:37:39]

lgtm

[palmer, 2014-08-19 20:55:16]

agl, can I get your OWNERS review? Thanks!

[Ryan Sleevi, 2014-08-20 05:03:07]

LGTM with changes, with felt@ having the wording final say.

https://codereview.chromium.org/416403006/diff/120001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/416403006/diff/120001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1252: To establish a secure network connection,
your device needs a correct clock. This is because the authentication
certificates that web sites use are issued for particular time periods. Since
your device's clock is wrong, Chromium cannot properly check certificates.
Wording suggestion, felt to override:

This is because the certificates websites use to identify themselves when
establishing a secure connection are only valid for a specific time period.


Justification:
1) Ideally, we'd conceptually match it to things like drivers license or IDs,
but that's too hard a concept to translate. But hopefully this language gets us
closer
2) I want to avoid "complex" terms like "authentication certificates" or
"issuance", and instead try to focus on concepts that may be more relatable
(conceptually), and in line with our other messages (which are about "identity"
of the site you're talking to)

https://codereview.chromium.org/416403006/diff/120001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1260: To establish a secure network connection,
your computer needs a correct clock. This is because the authentication
certificates that web sites use are issued for particular time periods. Since
your computer's clock is wrong, Chromium cannot properly check certificates.
Ditto

[palmer, 2014-08-22 18:32:50]

https://codereview.chromium.org/416403006/diff/120001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/416403006/diff/120001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1252: To establish a secure network connection,
your device needs a correct clock. This is because the authentication
certificates that web sites use are issued for particular time periods. Since
your device's clock is wrong, Chromium cannot properly check certificates.
On 2014/08/20 05:03:06, Ryan Sleevi wrote:
> Wording suggestion, felt to override:
> 
> This is because the certificates websites use to identify themselves when
> establishing a secure connection are only valid for a specific time period.
> 
> 
> Justification:
> 1) Ideally, we'd conceptually match it to things like drivers license or IDs,
> but that's too hard a concept to translate. But hopefully this language gets
us
> closer
> 2) I want to avoid "complex" terms like "authentication certificates" or
> "issuance", and instead try to focus on concepts that may be more relatable
> (conceptually), and in line with our other messages (which are about
"identity"
> of the site you're talking to)

Done.

https://codereview.chromium.org/416403006/diff/120001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1260: To establish a secure network connection,
your computer needs a correct clock. This is because the authentication
certificates that web sites use are issued for particular time periods. Since
your computer's clock is wrong, Chromium cannot properly check certificates.
On 2014/08/20 05:03:07, Ryan Sleevi wrote:
> Ditto

Done.

[palmer, 2014-08-22 18:32:55]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-08-22 18:36:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/palmer@chromium.org/416403006/140001

[commit-bot: I haz the power, 2014-08-22 19:41:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-22 21:32:18]

Committed patchset #8 (140001) as 291507