Closed Bug 1450761 Opened 6 years ago Closed 6 years ago

Policy: Hide "Add Exception" on certificate error

Categories

(Firefox :: Enterprise Policies, enhancement)

Product:
Firefox
Component:
Enterprise Policies
Version:
60 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox60 + verified
firefox61 --- verified

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(3 files)

Bug 1450761 - Add pref to disable the Add Exception button on certificate error pages.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1450761 - Policy to disable the Add Exception button on certificate error pages.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Follow-up
1.32 KB, patch
Details | Diff | Splinter Review 
Hide the Add Exception button
Depends on: 1442719
Comment on attachment 8964393 [details]
Bug 1450761 - Add pref to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233116/#review238572


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/about/browser_aboutCertError.js:348
(Diff revision 1)
> +
> +  for (let useFrame of [false, true]) {
> +    let tab = await openErrorPage(BAD_CERT, useFrame);
> +    let browser = tab.linkedBrowser;
> +
> +    let message = await ContentTask.spawn(browser, {frame: useFrame}, async function({frame}) {

Error: 'message' is assigned a value but never used. [eslint: no-unused-vars]
Comment on attachment 8964393 [details]
Bug 1450761 - Add pref to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233116/#review238884

::: browser/base/content/aboutNetError.xhtml:373
(Diff revision 2)
>  
>              // set the checkbox
>              checkbox.checked = !!options.automatic;
>            }
> +          if (options && options.disableAddExceptionButton) {
> +            document.getElementById("exceptionDialogButton").disabled = true;

I don't see what good disabling the button but keeping it visible does. There's nothing that the user can do to enable the button so it will just be a tease.
Attachment #8964393 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8964393 [details]
> Bug 1450761 - Add pref to disable the Add Exception button on certificate
> error pages.
> 
> https://reviewboard.mozilla.org/r/233116/#review238884
> 
> ::: browser/base/content/aboutNetError.xhtml:373
> (Diff revision 2)
> >  
> >              // set the checkbox
> >              checkbox.checked = !!options.automatic;
> >            }
> > +          if (options && options.disableAddExceptionButton) {
> > +            document.getElementById("exceptionDialogButton").disabled = true;
> 
> I don't see what good disabling the button but keeping it visible does.
> There's nothing that the user can do to enable the button so it will just be
> a tease.

This has been the UX direction that we're following for Policies. Every policy is disabling items instead of hiding them, so this is just to keep consistent with the other policies.
Comment on attachment 8964394 [details]
Bug 1450761 - Policy to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233118/#review238888
Attachment #8964394 - Flags: review?(jaws) → review+
(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> > Comment on attachment 8964393 [details]
> > Bug 1450761 - Add pref to disable the Add Exception button on certificate
> > error pages.
> > 
> > https://reviewboard.mozilla.org/r/233116/#review238884
> > 
> > ::: browser/base/content/aboutNetError.xhtml:373
> > (Diff revision 2)
> > >  
> > >              // set the checkbox
> > >              checkbox.checked = !!options.automatic;
> > >            }
> > > +          if (options && options.disableAddExceptionButton) {
> > > +            document.getElementById("exceptionDialogButton").disabled = true;
> > 
> > I don't see what good disabling the button but keeping it visible does.
> > There's nothing that the user can do to enable the button so it will just be
> > a tease.
> 
> This has been the UX direction that we're following for Policies. Every
> policy is disabling items instead of hiding them, so this is just to keep
> consistent with the other policies.

In the prefs we include a string explaining why some items are disabled but we don't do that on this screen. We should bring this up with UX.
:amin is out on PTO until tomorrow and I understand that this has a high priority to land soon so this can be uplifted to Beta.

I talked to :shorlander and he agreed with hiding the button in this case:
"[U]nless we are going to add some way to explain why it is disabled, I agree that a it should be hidden and not disabled ("Greyed out"). Although that does make the "Advanced --> Show Info that does nothing" flow kind of strange. Anyway, future improvements aside, I agree we shouldn't show a disabled button that has no context around why it is disabled."
Comment on attachment 8964393 [details]
Bug 1450761 - Add pref to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233116/#review239310

r=me if you switch to hiding instead of disabling.
Attachment #8964393 - Flags: review- → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca550bc9020b
Add pref to disable the Add Exception button on certificate error pages. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a473ea3d1bf
Policy to disable the Add Exception button on certificate error pages. r=jaws
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62839ba7b41c
Follow-up, remove check that was added in the previous version of the patch, but is not needed anymore. r=fgomes
[Tracking Requested - why for this release]:
tracking-firefox60: --- → ?
Summary: Policy: Disable "Add Exception" on certificate error → Policy: Hide "Add Exception" on certificate error
https://hg.mozilla.org/mozilla-central/rev/ca550bc9020b
https://hg.mozilla.org/mozilla-central/rev/4a473ea3d1bf
https://hg.mozilla.org/mozilla-central/rev/62839ba7b41c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
We tested this on the latest nightly with JSON policy format and it is verified as fixed.
With this policy, "Add Exception" option on certificate error pages can be disabled (hidden).

We will retest this with adm policy format when ready.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: