The changes made in bug 401575 allow users to add security exceptions directly from the error page, and currently the exception URL is auto-populated and the cert is pre-fetched. There is significant concern that this makes things too easy for users but aside from that, there is currently no way for, e.g., corporate deployments, to change this behaviour. There are two behaviours to manipulate here - pre-populating the string and pre-fetching the cert, but since only 3 of the 4 states make sense (can't pre-fetch without pre-populating) I suggest a three-state pref like: browser.ssl_override_behaviour with: 0: no pre-populate, no pre-fetch 1: pre-populate, no pre-fetch 2: pre-populate, pre-fetch

This seems like the right approach. Good work!

(In reply to comment #0) > 0: no pre-populate, no pre-fetch > 1: pre-populate, no pre-fetch > 2: pre-populate, pre-fetch I agree this seems like the right way to do it; the default would be 0 presumably? > browser.ssl_override_behaviour Shouldn't it be in American English? ;)

Bug 399696 will add support for temporary exceptions, and probably a checkbox to the add-exception dialog (permanent or not). Where should the default state for that checkbox be discussed? In this bug or in 399696? Should the checkbox state on dialog open vary based on the context the user came from? One could imagine: - if user comes from error page, default to temporary - if user comes from cert manager, default to permanent On the other hand, maybe it's confusing if the checkbox has a different default state based on the context. I think I'm slightly in favor to always default to temporary.

Should the name of the pref indicate it's for browsers only? (mail uses SSL, too, but doesn't have a direct link to the dialog) Maybe the pref should even be application specific? There are embedding apps that do not use the code in mozilla/security/manager/pki.

(In reply to comment #2) > (In reply to comment #0) > > 0: no pre-populate, no pre-fetch > > 1: pre-populate, no pre-fetch > > 2: pre-populate, pre-fetch > > I agree this seems like the right way to do it; the default would be 0 > presumably? The behaviour in 401575 implictly makes the default 2, but it could be changed here, yes. I'm not sure which default I'd argue for once power users have the ability to change it. > > browser.ssl_override_behaviour > > Shouldn't it be in American English? ;) Urr. Probably. :) (In reply to comment #3) > Bug 399696 will add support for temporary exceptions, and probably a checkbox > to the add-exception dialog (permanent or not). > > Where should the default state for that checkbox be discussed? > In this bug or in 399696? I think it should be discussed there, because that dialog lives in PSM, and its behaviour (and preferences) are a PSM-level issue. This bug tracks the behaviour that Firefox exhibits with the custom button it adds to the error page. The ssl_override_behavior pref discussed here could be added somewhere generic, but would be meaningless to user agents that don't have the custom code to implement this button. > Should the checkbox state on dialog open vary based on the context the user > came from? > ... > I think I'm slightly in favor to always default to temporary. As I say, I think the discussion should happen in the other bug, but I am inclined to agree that that is a sensible default for all cases in PSM. Application implementers could choose a different default if they had special circumstances for that interaction. (In reply to comment #4) > Should the name of the pref indicate it's for browsers only? > > (mail uses SSL, too, but doesn't have a direct link to the dialog) > > Maybe the pref should even be application specific? There are embedding apps > that do not use the code in mozilla/security/manager/pki. As I say, I think this bug is a Firefox RFE since there isn't a generic UI to control here, only a firefox-specific one. As a result, I think the pref should be contributed by browser/ code, and not show up in applications where it wouldn't make sense, or would have no effect. Maybe I'm wrong here?

If the default is 2, users will NEVER change it to anything else. No point in having a pref that users will never change.

(In reply to comment #5) > The behaviour in 401575 implictly makes the default 2, but it could be changed > here, yes. I'm not sure which default I'd argue for once power users have the > ability to change it. IMHO, what Kai said in bug 401575 comment 47 seems to me a good argument for setting it to either 0 or 1, because it requires the user to stop and read (and hopefully think) about what they are doing instead of being able to click straight through (even with pre-filling, they would still have to wait while the certificate is fetched): "... It also means, at this state the dialog contains much less information. The user's attention (hopefully) might be drawn to the bold warning further. The user can not yet click-through, because the confirm-button is not yet enabled (we enable it once the user clicks get-certificate.) It also means, the dialog feels like we guide the user, and the user actually notices the two steps going on in the dialog: - general warning first (banks would not ask you to do this) - click the button - get the status - require to confirm what I see I think with that change, we still make it easily discoverable to override, but require a bit more thinking." And power users who don't like this behavior can just change the pref to 2.

a pref that corporations can choose to lock to another value is still moderately valuable.

This patch adds the preference, wires it up in browser.js, and adds/fixes the argument handling code in exceptionDialog.js as well. This patch sets a default of 1, meaning that the cert dialog will be pre-populated with the site's URL, but as Kai mentions in bug 401575, by not pre-fetching it, we reduce the amount of text the user sees, and remove the ability to immediately click the override button.

Comment on attachment 287442 [details] [diff] [review] Add pref - default=1 (prepopulate, don't pre-fetch) Right now the dialog comes up with the location pre-filled but the "get cert" button disabled. I think you want to enable button when you set the location value. gDialog.getButton("extra2").disabled = false;

Whoops - thanks kai - I attached a stale patch. I getElementById from the document instead of getting a button from the dialog - I don't think the difference matters here since the same element is returned?

Comment on attachment 287542 [details] [diff] [review] Enable button on location pre-populate >- var params = { location : content.location.href, >- exceptionAdded : false }; >+ var params = {}; Fine with me if it's really not necessary to declare them. >+ var args = window.arguments; >+ if (args && args[0]) { >+ if (args[0].location) { >+ ... >+ if (args[0].prefetchCert) >+ ... >+ } >+ >+ // Set out parameter to false by default >+ args.exceptionAdded = false; I wonder if you you want args[0].exceptionAdded, because that's what you use below: >+ if(args && args[0]) >+ args[0].exceptionAdded = true;

This is wanted, not blocking, but it's also already done, so I'm going to at least put it into review queues. Beltzner - the only UI change here is in default behaviour. This patch defaults the pref to "1" - meaning that the exception dialog is pre-populated with the URL, but the certificate is not pre-fetched. Kai's argument is that this also means the user is looking at less text when the exception dialog first shows up, so they might actually notice the text that does exist. Power users can change this pref to "2" to restore pre-populate+pre-fetch, and corporate deployments or safety-conscious types can set it down to 0 to open the dialog unpopulated. gavin/kai - tagged both for review of your respective blocks (though I think Kai has already given his side a pass)

Comment on attachment 289372 [details] [diff] [review] Kai's nits, and fix a broken line in browser.js looks good to me, r=kengert

Comment on attachment 289372 [details] [diff] [review] Kai's nits, and fix a broken line in browser.js >Index: browser/app/profile/firefox.js It'd be good to let the SeaMonkey folk know about this pref (file a bug, CC neil and/or kairo), they might want to do something other than the default. >Index: browser/base/content/browser.js >- var params = { location : content.location.href, >- exceptionAdded : false }; >+ var params = {}; nit: I'd just leave exceptionAdded in the initializer and get rid of the "default" case. >Index: security/manager/pki/resources/content/exceptionDialog.js >+ if(args && args[0]) nit: space after "if" :)

Comment on attachment 289553 [details] [diff] [review] Gavin's nits addressed Slick. Well done. I think Kai's right, though we might want to revisit that dialog design a little to make the taskflow more evident.

Comment on attachment 289553 [details] [diff] [review] Gavin's nits addressed Requesting approval now that all the reviews are in. This is wanted+, but generally agreed to be a pretty good fix. It changes the default bheaviour for adding an SSL exception to pre-populate the URL, but not pre-fetch the certificate. This makes the dialog text stand out more on initial presentation (since there's less of it) and makes the act a more deliberate one, since users have to fetch the cert, and hence see the warnings, before being able to proceed. It allows the current "pre-populate and pre-fetch" behaviour to be restored through about:config though, and also allows deployers to turn off all pre-populating if they still think it makes the process too mindless.

Comment on attachment 289553 [details] [diff] [review] Gavin's nits addressed a=beltzner for drivers

Checking in browser/app/profile/firefox.js; /cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js new revision: 1.231; previous revision: 1.230 done Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.899; previous revision: 1.898 done Checking in security/manager/pki/resources/content/exceptionDialog.js; /cvsroot/mozilla/security/manager/pki/resources/content/exceptionDialog.js,v <-- exceptionDialog.js new revision: 1.7; previous revision: 1.6 done