author | Jared Wein <jwein@mozilla.com> |
Thu, 16 Apr 2020 21:23:58 +0000 | |
changeset 524510 | c539a1124b485abc9a9cdf4b2603baf51ecac337 |
parent 524509 | 1104082fbe0e5f67eaa161d56fc9e9a87e415a97 |
child 524511 | 39ee5303a7e89a03482a9a1cb13a39f3760fff3c |
push id | 113227 |
push user | jwein@mozilla.com |
push date | Thu, 16 Apr 2020 21:35:07 +0000 |
treeherder | autoland@9ff15f0fcf97 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | MattN |
bugs | 1628029 |
milestone | 77.0a1 |
first release with | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
last release without | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
--- a/browser/components/aboutlogins/AboutLoginsChild.jsm +++ b/browser/components/aboutlogins/AboutLoginsChild.jsm @@ -132,17 +132,17 @@ class AboutLoginsChild extends JSWindowA }); break; } case "AboutLoginsOpenPreferences": { this.sendAsyncMessage("AboutLogins:OpenPreferences"); break; } case "AboutLoginsRecordTelemetryEvent": { - let { method, object, extra = {} } = event.detail; + let { method, object, extra = {}, value = null } = event.detail; if (method == "open_management") { let { docShell } = this.browsingContext; // Compare to the last time open_management was recorded for the same // outerWindowID to not double-count them due to a redirect to remove // the entryPoint query param (since replaceState isn't allowed for // about:). Don't use performance.now for the tab since you can't // compare that number between different tabs and this JSM is shared. @@ -158,17 +158,17 @@ class AboutLoginsChild extends JSWindowA lastOpenManagementOuterWindowID = docShell.outerWindowID; } try { Services.telemetry.recordEvent( TELEMETRY_EVENT_CATEGORY, method, object, - null, + value, extra ); } catch (ex) { Cu.reportError( "AboutLoginsChild: error recording telemetry event: " + ex.message ); } break; @@ -193,17 +193,36 @@ class AboutLoginsChild extends JSWindowA } } } receiveMessage(message) { switch (message.name) { case "AboutLogins:MasterPasswordResponse": if (masterPasswordPromise) { - masterPasswordPromise.resolve(message.data); + masterPasswordPromise.resolve(message.data.result); + try { + let { + method, + object, + extra = {}, + value = null, + } = message.data.telemetryEvent; + Services.telemetry.recordEvent( + TELEMETRY_EVENT_CATEGORY, + method, + object, + value, + extra + ); + } catch (ex) { + Cu.reportError( + "AboutLoginsChild: error recording telemetry event: " + ex.message + ); + } } break; case "AboutLogins:Setup": let waivedContent = Cu.waiveXrays(this.browsingContext.window); waivedContent.AboutLoginsUtils.masterPasswordEnabled = message.data.masterPasswordEnabled; waivedContent.AboutLoginsUtils.passwordRevealVisible = message.data.passwordRevealVisible;
--- a/browser/components/aboutlogins/AboutLoginsParent.jsm +++ b/browser/components/aboutlogins/AboutLoginsParent.jsm @@ -318,81 +318,112 @@ class AboutLoginsParent extends JSWindow case "AboutLogins:MasterPasswordRequest": { let messageId = message.data; if (!messageId) { throw new Error( "AboutLogins:MasterPasswordRequest: Message ID required for MasterPasswordRequest." ); } - if (Date.now() < AboutLogins._authExpirationTime) { - this.sendAsyncMessage("AboutLogins:MasterPasswordResponse", true); - return; - } + let loggedIn = false; + let telemetryEvent; + + try { + // This does no harm if master password isn't set. + let tokendb = Cc[ + "@mozilla.org/security/pk11tokendb;1" + ].createInstance(Ci.nsIPK11TokenDB); + let token = tokendb.getInternalKeyToken(); - // This does no harm if master password isn't set. - let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"].createInstance( - Ci.nsIPK11TokenDB - ); - let token = tokendb.getInternalKeyToken(); + if (Date.now() < AboutLogins._authExpirationTime) { + loggedIn = true; + telemetryEvent = { + object: token.hasPassword ? "master_password" : "os_auth", + method: "reauthenticate", + value: "success_no_prompt", + }; + return; + } - let loggedIn = false; - // Use the OS auth dialog if there is no master password - if (!token.hasPassword && !OS_AUTH_ENABLED) { - loggedIn = true; - } else if (!token.hasPassword && OS_AUTH_ENABLED) { - if (AppConstants.platform == "macosx") { - // OS Auth dialogs on macOS must only provide the "reason" that the prompt - // is being displayed. - messageId += "-macosx"; + // Use the OS auth dialog if there is no master password + if (!token.hasPassword && !OS_AUTH_ENABLED) { + loggedIn = true; + telemetryEvent = { + object: "os_auth", + method: "reauthenticate", + value: "success_disabled", + }; + return; } - let [messageText, captionText] = await AboutLoginsL10n.formatMessages( - [ + if (!token.hasPassword && OS_AUTH_ENABLED) { + if (AppConstants.platform == "macosx") { + // OS Auth dialogs on macOS must only provide the "reason" that the prompt + // is being displayed. + messageId += "-macosx"; + } + let [ + messageText, + captionText, + ] = await AboutLoginsL10n.formatMessages([ { id: messageId, }, { id: "about-logins-os-auth-dialog-caption", }, - ] - ); - loggedIn = await OSKeyStore.ensureLoggedIn( - messageText.value, - captionText.value, - ownerGlobal, - false - ); - } else { + ]); + let result = await OSKeyStore.ensureLoggedIn( + messageText.value, + captionText.value, + ownerGlobal, + false + ); + loggedIn = result.authenticated; + telemetryEvent = { + object: "os_auth", + method: "reauthenticate", + value: result.auth_details, + }; + return; + } // Force a log-out of the Master Password. token.checkPassword(""); // If a master password prompt is already open, just exit early and return false. // The user can re-trigger it after responding to the already open dialog. if (Services.logins.uiBusy) { - this.sendAsyncMessage("AboutLogins:MasterPasswordResponse", false); + loggedIn = false; return; } // So there's a master password. But since checkPassword didn't succeed, we're logged out (per nsIPK11Token.idl). try { // Relogin and ask for the master password. token.login(true); // 'true' means always prompt for token password. User will be prompted until // clicking 'Cancel' or entering the correct password. } catch (e) { // An exception will be thrown if the user cancels the login prompt dialog. // User is also logged out of Software Security Device. } loggedIn = token.isLoggedIn(); + telemetryEvent = { + object: "master_password", + method: "reauthenticate", + value: loggedIn ? "success" : "fail", + }; + } finally { + if (loggedIn) { + const AUTH_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes + AboutLogins._authExpirationTime = Date.now() + AUTH_TIMEOUT_MS; + } + this.sendAsyncMessage("AboutLogins:MasterPasswordResponse", { + result: loggedIn, + telemetryEvent, + }); } - - if (loggedIn) { - const AUTH_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes - AboutLogins._authExpirationTime = Date.now() + AUTH_TIMEOUT_MS; - } - this.sendAsyncMessage("AboutLogins:MasterPasswordResponse", loggedIn); break; } case "AboutLogins:Subscribe": { AboutLogins._authExpirationTime = Number.NEGATIVE_INFINITY; if (!AboutLogins._observersAdded) { Services.obs.addObserver(AboutLogins, "passwordmgr-crypto-login"); Services.obs.addObserver( AboutLogins,
--- a/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js +++ b/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js @@ -77,20 +77,23 @@ add_task(async function test_telemetry_e await SpecialPowers.spawn(gBrowser.selectedBrowser, [], async function() { let loginItem = content.document.querySelector("login-item"); let copyButton = loginItem.shadowRoot.querySelector( ".copy-password-button" ); copyButton.click(); }); await reauthObserved; - await LoginTestUtils.telemetry.waitForEventCount(4); + // When reauth is observed an extra telemetry event will be recorded + // for the reauth, hence the event count increasing by 2 here, and later + // in the test as well. + await LoginTestUtils.telemetry.waitForEventCount(5); } let nextTelemetryEventCount = OSKeyStoreTestUtils.canTestOSKeyStoreLogin() - ? 5 + ? 6 : 4; let promiseNewTab = BrowserTestUtils.waitForNewTab( gBrowser, TEST_LOGIN3.origin + "/" ); await SpecialPowers.spawn(gBrowser.selectedBrowser, [], async function() { let loginItem = content.document.querySelector("login-item"); @@ -102,16 +105,17 @@ add_task(async function test_telemetry_e BrowserTestUtils.removeTab(newTab); await LoginTestUtils.telemetry.waitForEventCount(nextTelemetryEventCount++); // Show the password if (OSKeyStoreTestUtils.canTestOSKeyStoreLogin()) { let reauthObserved = forceAuthTimeoutAndWaitForOSKeyStoreLogin({ loginResult: true, }); + nextTelemetryEventCount++; // An extra event is observed for the reauth event. await SpecialPowers.spawn(gBrowser.selectedBrowser, [], async function() { let loginItem = content.document.querySelector("login-item"); let revealCheckbox = loginItem.shadowRoot.querySelector( ".reveal-password-checkbox" ); revealCheckbox.click(); }); await reauthObserved; @@ -122,25 +126,24 @@ add_task(async function test_telemetry_e let loginItem = content.document.querySelector("login-item"); let revealCheckbox = loginItem.shadowRoot.querySelector( ".reveal-password-checkbox" ); revealCheckbox.click(); }); await LoginTestUtils.telemetry.waitForEventCount(nextTelemetryEventCount++); - reauthObserved = forceAuthTimeoutAndWaitForOSKeyStoreLogin({ - loginResult: true, - }); + // Don't force the auth timeout here to check that `auth_skipped: true` is set as + // in `extra`. + nextTelemetryEventCount++; // An extra event is observed for the reauth event. await SpecialPowers.spawn(gBrowser.selectedBrowser, [], async function() { let loginItem = content.document.querySelector("login-item"); let editButton = loginItem.shadowRoot.querySelector(".edit-button"); editButton.click(); }); - await reauthObserved; await LoginTestUtils.telemetry.waitForEventCount(nextTelemetryEventCount++); await SpecialPowers.spawn(gBrowser.selectedBrowser, [], async function() { let loginItem = content.document.querySelector("login-item"); let usernameField = loginItem.shadowRoot.querySelector( 'input[name="username"]' ); usernameField.value = "user1-modified"; @@ -233,20 +236,23 @@ add_task(async function test_telemetry_e }); await LoginTestUtils.telemetry.waitForEventCount(nextTelemetryEventCount++); const testOSAuth = OSKeyStoreTestUtils.canTestOSKeyStoreLogin(); let expectedEvents = [ [true, "pwmgr", "open_management", "direct"], [true, "pwmgr", "select", "existing_login", null, { breached: "true" }], [true, "pwmgr", "copy", "username", null, { breached: "true" }], + [testOSAuth, "pwmgr", "reauthenticate", "os_auth", "success"], [testOSAuth, "pwmgr", "copy", "password", null, { breached: "true" }], [true, "pwmgr", "open_site", "existing_login", null, { breached: "true" }], + [testOSAuth, "pwmgr", "reauthenticate", "os_auth", "success"], [testOSAuth, "pwmgr", "show", "password", null, { breached: "true" }], [testOSAuth, "pwmgr", "hide", "password", null, { breached: "true" }], + [testOSAuth, "pwmgr", "reauthenticate", "os_auth", "success_no_prompt"], [testOSAuth, "pwmgr", "edit", "existing_login", null, { breached: "true" }], [testOSAuth, "pwmgr", "save", "existing_login", null, { breached: "true" }], [true, "pwmgr", "delete", "existing_login", null, { breached: "true" }], [true, "pwmgr", "new", "new_login"], [true, "pwmgr", "cancel", "new_login"], [true, "pwmgr", "select", "existing_login", null, { vulnerable: "true" }], [true, "pwmgr", "copy", "username", null, { vulnerable: "true" }], [true, "pwmgr", "delete", "existing_login", null, { vulnerable: "true" }],
--- a/browser/components/preferences/in-content/privacy.js +++ b/browser/components/preferences/in-content/privacy.js @@ -1947,17 +1947,17 @@ var gPrivacyPane = { }, ]); let loggedIn = await OSKeyStore.ensureLoggedIn( messageText.value, captionText.value, window, false ); - if (!loggedIn) { + if (!loggedIn.authenticated) { return; } } gSubDialog.open( "chrome://mozapps/content/preferences/changemp.xhtml", "resizable=no", null,
--- a/browser/extensions/formautofill/FormAutofillParent.jsm +++ b/browser/extensions/formautofill/FormAutofillParent.jsm @@ -336,17 +336,17 @@ class FormAutofillParent extends JSWindo if (data.guid) { await gFormAutofillStorage.addresses.update(data.guid, data.address); } else { await gFormAutofillStorage.addresses.add(data.address); } break; } case "FormAutofill:SaveCreditCard": { - if (!(await FormAutofillUtils.ensureLoggedIn())) { + if (!(await FormAutofillUtils.ensureLoggedIn()).authenticated) { log.warn("User canceled encryption login"); return undefined; } await gFormAutofillStorage.creditCards.add(data.creditcard); break; } case "FormAutofill:RemoveAddresses": { data.guids.forEach(guid => gFormAutofillStorage.addresses.remove(guid)); @@ -685,17 +685,17 @@ class FormAutofillParent extends JSWindo if (state == "disable") { Services.prefs.setBoolPref( "extensions.formautofill.creditCards.enabled", false ); return; } - if (!(await FormAutofillUtils.ensureLoggedIn())) { + if (!(await FormAutofillUtils.ensureLoggedIn()).authenticated) { log.warn("User canceled encryption login"); return; } let changedGUIDs = []; if (creditCard.guid) { if (state == "update") { await gFormAutofillStorage.creditCards.update(
--- a/browser/extensions/formautofill/content/manageDialog.js +++ b/browser/extensions/formautofill/content/manageDialog.js @@ -367,16 +367,17 @@ class ManageCreditCards extends ManageRe * * @param {object} creditCard [optional] */ async openEditDialog(creditCard) { // Ask for reauth if user is trying to edit an existing credit card. if ( !creditCard || (await FormAutofillUtils.ensureLoggedIn(reauthPasswordPromptMessage)) + .authenticated ) { let decryptedCCNumObj = {}; if (creditCard && creditCard["cc-number-encrypted"]) { try { decryptedCCNumObj["cc-number"] = await OSKeyStore.decrypt( creditCard["cc-number-encrypted"] ); } catch (ex) {
--- a/toolkit/components/telemetry/Events.yaml +++ b/toolkit/components/telemetry/Events.yaml @@ -602,26 +602,45 @@ pwmgr: Sent when opening the password management UI. bug_numbers: [1543499, 1454733, 1545172, 1550631, 1622971] notification_emails: ["loines@mozilla.com", "passwords-dev@mozilla.org", "sfoster@mozilla.com"] products: - "firefox" record_in_processes: [main, content] release_channel_collection: opt-out expiry_version: never + reauthenticate: + description: > + Measure how often users are asked to authenticate with their Operating System or Master Password to gain access to stored passwords. + Possible values are as follows, + "success_no_prompt" should be used when the feature is enabled but no prompt is given to the user because they have recently authenticated. + "success_disabled" is used when the feature is disabled. + "success_unsupported_platform" should be set when the user attempts to authenticate on an unsupported platform. + objects: [ + "master_password", + "os_auth", + ] + methods: ["reauthenticate"] + bug_numbers: + - 1628029 + expiry_version: never + notification_emails: ["loines@mozilla.com", "passwords-dev@mozilla.org", "jaws@mozilla.com"] + release_channel_collection: opt-out + products: + - "firefox" + record_in_processes: [content] mgmt_interaction: description: > - These events record interactions on the about:logins page. Sort methods have an accompanying - value that specifies what order the list of logins is sorted with: {last_changed, last_used, title}. + These events record interactions on the about:logins page. extra_keys: breached: > Whether the login is marked as breached or not. If a login is both breached and vulnerable, it will only be reported as breached. vulnerable: > Whether the login is marked as vulnerable or not. If a login is both breached and vulnerable, it will only be reported as breached. - sort_key: The key that is used for sorting the login-list + sort_key: The key that is used for sorting the login-list. Should only be set with the "sort" method. objects: [ "existing_login", "list", "new_login", "password", "username", ] methods: [ @@ -638,23 +657,22 @@ pwmgr: "save", "select", "show", "sort", ] bug_numbers: - 1548463 - 1600958 + - 1549115 expiry_version: never notification_emails: ["loines@mozilla.com", "passwords-dev@mozilla.org", "jaws@mozilla.com"] release_channel_collection: opt-out products: - "firefox" - - "fennec" - - "geckoview" record_in_processes: [content] autocomplete_field: objects: ["generatedpassword"] methods: ["autocomplete_field", "autocomplete_shown"] description: > "autocomplete_field": The first time each unique generated password is used to fill a login field - i.e. the user selects it from from the autocomplete dropdown on a password input "autocomplete_shown": The first time the password generation option is shown in the autocomplete dropdown on a password input for a site per session bug_numbers: [1548878, 1616356]
--- a/toolkit/modules/OSKeyStore.jsm +++ b/toolkit/modules/OSKeyStore.jsm @@ -88,17 +88,17 @@ var OSKeyStore = { log.debug("_ensureReauth: _testReauth: ", this._testReauth); switch (this._testReauth) { case "pass": Services.obs.notifyObservers( null, "oskeystore-testonly-reauth", "pass" ); - break; + return { authenticated: true, auth_details: "success" }; case "cancel": Services.obs.notifyObservers( null, "oskeystore-testonly-reauth", "cancel" ); throw new Components.Exception( "Simulating user cancelling login dialog", @@ -140,19 +140,19 @@ var OSKeyStore = { * re-auth the user for viewing passwords you may also get a * KeyChain prompt to allow the app to access the stored key even * though that's not at all relevant for the re-auth. We skip the * code here so that we can postpone deciding on how we want to * handle this problem (multiple channels) until we actually use * the key storage. If we start creating keys on macOS by running * this code we'll potentially have to do extra work to cleanup * the mess later. - * @returns {Promise<boolean>} True if it's logged in or no password is set - * and false if it's still not logged in (prompt - * canceled or other error). + * @returns {Promise<Object>} Object with the following properties: + * authenticated: {boolean} Set to true if the user successfully authenticated. + * auth_details: {String?} Details of the authentication result. */ async ensureLoggedIn( reauth = false, dialogCaption = "", parentWindow = null, generateKeyIfNotAvailable = true ) { if ( @@ -196,59 +196,64 @@ var OSKeyStore = { .asyncReauthenticateUser(reauth, dialogCaption, parentWindow) .then(reauthResult => { if (typeof reauthResult == "boolean" && !reauthResult) { throw new Components.Exception( "User canceled OS reauth entry", Cr.NS_ERROR_FAILURE ); } + return { authenticated: true, auth_details: "success" }; }); } else { log.debug("ensureLoggedIn: Skipping reauth on unsupported platforms"); - unlockPromise = Promise.resolve(); + unlockPromise = Promise.resolve({ + authenticated: true, + auth_details: "success_unsupported_platform", + }); } } else { - unlockPromise = Promise.resolve(); + unlockPromise = Promise.resolve({ authenticated: true }); } if (generateKeyIfNotAvailable) { - unlockPromise = unlockPromise.then(async () => { + unlockPromise = unlockPromise.then(async reauthResult => { if (!(await nativeOSKeyStore.asyncSecretAvailable(this.STORE_LABEL))) { log.debug( "ensureLoggedIn: Secret unavailable, attempt to generate new secret." ); let recoveryPhrase = await nativeOSKeyStore.asyncGenerateSecret( this.STORE_LABEL ); // TODO We should somehow have a dialog to ask the user to write this down, // and another dialog somewhere for the user to restore the secret with it. // (Intentionally not printing it out in the console) log.debug( "ensureLoggedIn: Secret generated. Recovery phrase length: " + recoveryPhrase.length ); } + return reauthResult; }); } unlockPromise = unlockPromise.then( - () => { + reauthResult => { log.debug("ensureLoggedIn: Logged in"); this._pendingUnlockPromise = null; this._isLocked = false; - return true; + return reauthResult; }, err => { log.debug("ensureLoggedIn: Not logged in", err); this._pendingUnlockPromise = null; this._isLocked = true; - return false; + return { authenticated: false, auth_details: "fail" }; } ); this._pendingUnlockPromise = unlockPromise; return this._pendingUnlockPromise; }, @@ -262,17 +267,17 @@ var OSKeyStore = { * * @param {string} cipherText Encrypted string including the algorithm details. * @param {boolean|string} reauth If set to a string, prompt the reauth login dialog. * The string may be shown on the native OS * login dialog. Empty strings and `true` are disallowed. * @returns {Promise<string>} resolves to the decrypted string, or rejects otherwise. */ async decrypt(cipherText, reauth = false) { - if (!(await this.ensureLoggedIn(reauth))) { + if (!(await this.ensureLoggedIn(reauth)).authenticated) { throw Components.Exception( "User canceled OS unlock entry", Cr.NS_ERROR_ABORT ); } let bytes = await nativeOSKeyStore.asyncDecryptBytes( this.STORE_LABEL, cipherText @@ -282,17 +287,17 @@ var OSKeyStore = { /** * Encrypts a string and returns cipher text containing algorithm information used for decryption. * * @param {string} plainText Original string without encryption. * @returns {Promise<string>} resolves to the encrypted string (with algorithm), otherwise rejects. */ async encrypt(plainText) { - if (!(await this.ensureLoggedIn())) { + if (!(await this.ensureLoggedIn()).authenticated) { throw Components.Exception( "User canceled OS unlock entry", Cr.NS_ERROR_ABORT ); } // Convert plain text into a UTF-8 binary string plainText = unescape(encodeURIComponent(plainText));
--- a/toolkit/modules/tests/xpcshell/test_osKeyStore.js +++ b/toolkit/modules/tests/xpcshell/test_osKeyStore.js @@ -29,17 +29,21 @@ add_task(async function setup() { // Ensure that the appropriate initialization has happened. do_get_profile(); const testText = "test string"; let cipherText; add_task(async function test_encrypt_decrypt() { - Assert.equal(await OSKeyStore.ensureLoggedIn(), true, "Started logged in."); + Assert.equal( + (await OSKeyStore.ensureLoggedIn()).authenticated, + true, + "Started logged in." + ); cipherText = await OSKeyStore.encrypt(testText); Assert.notEqual(testText, cipherText); let plainText = await OSKeyStore.decrypt(cipherText); Assert.equal(testText, plainText); }); @@ -62,32 +66,32 @@ add_task(async function test_reauth() { Assert.equal(ex.message, "User canceled OS unlock entry"); Assert.equal(ex.result, Cr.NS_ERROR_ABORT); } await reauthObserved; reauthObserved = OSKeyStoreTestUtils.waitForOSKeyStoreLogin(false); await new Promise(resolve => TestUtils.executeSoon(resolve)); Assert.equal( - await OSKeyStore.ensureLoggedIn("test message"), + (await OSKeyStore.ensureLoggedIn("test message")).authenticated, false, "Reauth cancelled." ); await reauthObserved; reauthObserved = OSKeyStoreTestUtils.waitForOSKeyStoreLogin(true); await new Promise(resolve => TestUtils.executeSoon(resolve)); let plainText2 = await OSKeyStore.decrypt(cipherText, "prompt message text"); await reauthObserved; Assert.equal(testText, plainText2); reauthObserved = OSKeyStoreTestUtils.waitForOSKeyStoreLogin(true); await new Promise(resolve => TestUtils.executeSoon(resolve)); Assert.equal( - await OSKeyStore.ensureLoggedIn("test message"), + (await OSKeyStore.ensureLoggedIn("test message")).authenticated, true, "Reauth logged in." ); await reauthObserved; }); add_task(async function test_decryption_failure() { try {