From f2914a8658aa0d0e881aa7aaa5336dae17ef6944 Mon Sep 17 00:00:00 2001 From: Justin Dolske Date: Sun, 12 Oct 2008 20:05:11 -0700 Subject: [PATCH] Bug 394611 - Always prompt the user before changing a stored password. r=gavin (relanding with workaround to avoid leak) --- .../components/passwordmgr/src/nsLoginManager.js | 23 +--- .../passwordmgr/src/nsLoginManagerPrompter.js | 21 ++-- toolkit/components/passwordmgr/test/Makefile.in | 4 + .../passwordmgr/test/notification_common.js | 54 +++++++++ .../passwordmgr/test/subtst_notifications_10.html | 25 ++++ .../passwordmgr/test/subtst_notifications_8.html | 27 +++++ .../passwordmgr/test/subtst_notifications_9.html | 27 +++++ .../passwordmgr/test/test_notifications.html | 130 ++++++++++----------- .../components/passwordmgr/test/test_prompt.html | 127 +++++++++++++++++++- 9 files changed, 336 insertions(+), 102 deletions(-) create mode 100644 toolkit/components/passwordmgr/test/notification_common.js create mode 100644 toolkit/components/passwordmgr/test/subtst_notifications_10.html create mode 100644 toolkit/components/passwordmgr/test/subtst_notifications_8.html create mode 100644 toolkit/components/passwordmgr/test/subtst_notifications_9.html diff --git a/toolkit/components/passwordmgr/src/nsLoginManager.js b/toolkit/components/passwordmgr/src/nsLoginManager.js index ee6259a82..be19a2299 100644 --- a/toolkit/components/passwordmgr/src/nsLoginManager.js +++ b/toolkit/components/passwordmgr/src/nsLoginManager.js @@ -871,26 +871,11 @@ LoginManager.prototype = { if (existingLogin) { this.log("Found an existing login matching this form submission"); - /* - * Change password if needed. - * - * If the login has a username, change the password w/o prompting - * (because we can be fairly sure there's only one password - * associated with the username). But for logins without a - * username, ask the user... Some sites use a password-only "login" - * in different contexts (enter your PIN, answer a security - * question, etc), and without a username we can't be sure if - * modifying an existing login is the right thing to do. - */ + // Change password if needed. if (existingLogin.password != formLogin.password) { - if (formLogin.username) { - this.log("...Updating password for existing login."); - this.modifyLogin(existingLogin, formLogin); - } else { - this.log("...passwords differ, prompting to change."); - prompter = getPrompter(win); - prompter.promptToChangePassword(existingLogin, formLogin); - } + this.log("...passwords differ, prompting to change."); + prompter = getPrompter(win); + prompter.promptToChangePassword(existingLogin, formLogin); } return; diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js index bf151103b..24791c066 100644 --- a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js +++ b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js @@ -429,7 +429,7 @@ LoginManagerPrompter.prototype = { // be prompted for authentication again, which brings us here. var notifyBox = this._getNotifyBox(); if (notifyBox) - this._removeSaveLoginNotification(notifyBox); + this._removeLoginNotifications(notifyBox); var [hostname, httpRealm] = this._getAuthTarget(aChannel, aAuthInfo); @@ -503,8 +503,11 @@ LoginManagerPrompter.prototype = { this.log("Updating password for " + username + " @ " + hostname + " (" + httpRealm + ")"); - // update password - this._pwmgr.modifyLogin(selectedLogin, newLogin); + if (notifyBox) + this._showChangeLoginNotification(notifyBox, + selectedLogin, newLogin); + else + this._pwmgr.modifyLogin(selectedLogin, newLogin); } else { this.log("Login unchanged, no further action needed."); @@ -664,17 +667,21 @@ LoginManagerPrompter.prototype = { /* - * _removeSaveLoginNotification + * _removeLoginNotifications * */ - _removeSaveLoginNotification : function (aNotifyBox) { - + _removeLoginNotifications : function (aNotifyBox) { var oldBar = aNotifyBox.getNotificationWithValue("password-save"); - if (oldBar) { this.log("Removing save-password notification bar."); aNotifyBox.removeNotification(oldBar); } + + oldBar = aNotifyBox.getNotificationWithValue("password-change"); + if (oldBar) { + this.log("Removing change-password notification bar."); + aNotifyBox.removeNotification(oldBar); + } }, diff --git a/toolkit/components/passwordmgr/test/Makefile.in b/toolkit/components/passwordmgr/test/Makefile.in index 36f572032..b6db1c122 100644 --- a/toolkit/components/passwordmgr/test/Makefile.in +++ b/toolkit/components/passwordmgr/test/Makefile.in @@ -77,6 +77,7 @@ MOCHI_TESTS = \ MOCHI_CONTENT = \ pwmgr_common.js \ prompt_common.js \ + notification_common.js \ authenticate.sjs \ formsubmit.sjs \ subtst_notifications_1.html \ @@ -85,6 +86,9 @@ MOCHI_CONTENT = \ subtst_notifications_4.html \ subtst_notifications_5.html \ subtst_notifications_6.html \ + subtst_notifications_8.html \ + subtst_notifications_9.html \ + subtst_notifications_10.html \ $(NULL) XPCSHELL_TESTS = unit diff --git a/toolkit/components/passwordmgr/test/notification_common.js b/toolkit/components/passwordmgr/test/notification_common.js new file mode 100644 index 000000000..0349ea59f --- /dev/null +++ b/toolkit/components/passwordmgr/test/notification_common.js @@ -0,0 +1,54 @@ +/* + * getNotificationBox + * + * Fetches the notification box for the specified window. + */ +function getNotificationBox(aWindow) { + var chromeWin = aWindow + .QueryInterface(Ci.nsIInterfaceRequestor) + .getInterface(Ci.nsIWebNavigation) + .QueryInterface(Ci.nsIDocShellTreeItem) + .rootTreeItem + .QueryInterface(Ci.nsIInterfaceRequestor) + .getInterface(Ci.nsIDOMWindow) + .QueryInterface(Ci.nsIDOMChromeWindow); + + // Don't need .wrappedJSObject here, unlike when chrome does this. + var notifyBox = chromeWin.getNotificationBox(aWindow); + return notifyBox; +} + + +/* + * getNotificationBar + * + */ +function getNotificationBar(aBox, aKind) { + ok(true, "Looking for " + aKind + " notification bar"); + // Sometimes callers wants a bar, sometimes not. Allow 0 or 1, but not 2+. + ok(aBox.allNotifications.length <= 1, "Checking for multiple notifications"); + return aBox.getNotificationWithValue(aKind); +} + + +/* + * clickNotificationButton + * + * Clicks the specified notification button. + */ +function clickNotificationButton(aBar, aButtonName) { + // This is a bit of a hack. The notification doesn't have an API to + // trigger buttons, so we dive down into the implementation and twiddle + // the buttons directly. + var buttons = aBar.getElementsByTagName("button"); + var clicked = false; + for (var i = 0; i < buttons.length; i++) { + if (buttons[i].label == aButtonName) { + buttons[i].click(); + clicked = true; + break; + } + } + + ok(clicked, "Clicked \"" + aButtonName + "\" button"); +} diff --git a/toolkit/components/passwordmgr/test/subtst_notifications_10.html b/toolkit/components/passwordmgr/test/subtst_notifications_10.html new file mode 100644 index 000000000..5fafc9849 --- /dev/null +++ b/toolkit/components/passwordmgr/test/subtst_notifications_10.html @@ -0,0 +1,25 @@ + + + Subtest for Login Manager notifications + + +

Subtest 10

+
+ + +
+ + + + diff --git a/toolkit/components/passwordmgr/test/subtst_notifications_8.html b/toolkit/components/passwordmgr/test/subtst_notifications_8.html new file mode 100644 index 000000000..9d1766e0d --- /dev/null +++ b/toolkit/components/passwordmgr/test/subtst_notifications_8.html @@ -0,0 +1,27 @@ + + + Subtest for Login Manager notifications + + +

Subtest 8

+
+ + + +
+ + + + diff --git a/toolkit/components/passwordmgr/test/subtst_notifications_9.html b/toolkit/components/passwordmgr/test/subtst_notifications_9.html new file mode 100644 index 000000000..d3cbbfa73 --- /dev/null +++ b/toolkit/components/passwordmgr/test/subtst_notifications_9.html @@ -0,0 +1,27 @@ + + + Subtest for Login Manager notifications + + +

Subtest 9

+
+ + + +
+ + + + diff --git a/toolkit/components/passwordmgr/test/test_notifications.html b/toolkit/components/passwordmgr/test/test_notifications.html index 2a62b3248..73edc600e 100644 --- a/toolkit/components/passwordmgr/test/test_notifications.html +++ b/toolkit/components/passwordmgr/test/test_notifications.html @@ -5,6 +5,7 @@ + @@ -36,79 +37,13 @@ var subtests = [ "subtst_notifications_1.html", // 13 "subtst_notifications_6.html", // 14 "subtst_notifications_1.html", // 15 - "subtst_notifications_6.html" + "subtst_notifications_6.html", // 16 + "subtst_notifications_8.html", // 17 + "subtst_notifications_8.html", // 18 + "subtst_notifications_9.html", // 19 + "subtst_notifications_10.html" // 20 ]; -/* - * getNotificationBox - * - * Fetches the notification box for the specified window. - */ -function getNotificationBox(aWindow) { -/* - var chromeWin = aWindow - .QueryInterface(Ci.nsIInterfaceRequestor) - .getInterface(Ci.nsIWebNavigation) - .QueryInterface(Ci.nsIDocShellTreeItem) - .rootTreeItem - .QueryInterface(Ci.nsIInterfaceRequestor) - .getInterface(Ci.nsIDOMWindow); - // Don't need .wrappedJSObject here, unlike when chrome does this. - var browserWin = chromeWin.browserDOMWindow; - var notifyBox = browserWin.getNotificationBox(aWindow); - return notifyBox; -*/ - // Find the which contains aWindow, by looking - // through all the open windows and all the in each. - var wm = Cc["@mozilla.org/appshell/window-mediator;1"]. - getService(Ci.nsIWindowMediator); - var enumerator = wm.getEnumerator("navigator:browser"); - var tabbrowser = null; - var foundBrowser = null; - - while (!foundBrowser && enumerator.hasMoreElements()) { - var win = enumerator.getNext(); - tabbrowser = win.getBrowser(); - foundBrowser = tabbrowser.getBrowserForDocument(aWindow.document); - } - - // Return the notificationBox associated with the browser. - return tabbrowser.getNotificationBox(foundBrowser); -} - - -/* - * getNotificationBar - * - */ -function getNotificationBar(aBox, aKind) { - ok(true, "Looking for " + aKind + " notification bar"); - return aBox.getNotificationWithValue(aKind); -} - - -/* - * clickNotificationButton - * - * Clicks the specified notification button. - */ -function clickNotificationButton(aBar, aButtonName) { - // This is a bit of a hack. The notification doesn't have an API to - // trigger buttons, so we dive down into the implementation and twiddle - // the buttons directly. - var buttons = aBar.getElementsByTagName("button"); - var clicked = false; - for (var i = 0; i < buttons.length; i++) { - if (buttons[i].label == aButtonName) { - buttons[i].click(); - clicked = true; - break; - } - } - - ok(clicked, "Clicked \"" + aButtonName + "\" button"); -} - var ignoreLoad = false; function handleLoad(aEvent) { @@ -322,6 +257,59 @@ function checkTest() { ok(bar, "got notification bar"); clickNotificationButton(bar, "Not Now"); pwmgr.removeLogin(login1B); + + // Add login for the next tests + pwmgr.addLogin(login1); + break; + + case 17: + // Check for change-password bar, u+p login on u+p form. (not changed) + is(gotUser, "notifyu1", "Checking submitted username"); + is(gotPass, "pass2", "Checking submitted password"); + bar = getNotificationBar(notifyBox, "password-change"); + ok(bar, "got notification bar"); + clickNotificationButton(bar, "Don't Change"); + break; + + case 18: + // Check for change-password bar, u+p login on u+p form. + is(gotUser, "notifyu1", "Checking submitted username"); + is(gotPass, "pass2", "Checking submitted password"); + bar = getNotificationBar(notifyBox, "password-change"); + ok(bar, "got notification bar"); + clickNotificationButton(bar, "Change"); + + // cleanup + login1.password = "pass2"; + pwmgr.removeLogin(login1); + login1.password = "notifyp1"; + + // Add login for the next test + pwmgr.addLogin(login2); + break; + + // ...can't change a u+p login on a p-only form... + + case 19: + // Check for change-password bar, p-only login on u+p form. + // (needed a different subtest for this because the login created in + // test_0init was interfering) + is(gotUser, "", "Checking submitted username"); + is(gotPass, "pass2", "Checking submitted password"); + bar = getNotificationBar(notifyBox, "password-change"); + ok(bar, "got notification bar"); + clickNotificationButton(bar, "Change"); + break; + + case 20: + // Check for change-password bar, p-only login on p-only form. + is(gotUser, "null", "Checking submitted username"); + is(gotPass, "notifyp1", "Checking submitted password"); + bar = getNotificationBar(notifyBox, "password-change"); + ok(bar, "got notification bar"); + clickNotificationButton(bar, "Change"); + + pwmgr.removeLogin(login2); break; default: diff --git a/toolkit/components/passwordmgr/test/test_prompt.html b/toolkit/components/passwordmgr/test/test_prompt.html index 41b3cfdf5..9d304c1f2 100644 --- a/toolkit/components/passwordmgr/test/test_prompt.html +++ b/toolkit/components/passwordmgr/test/test_prompt.html @@ -6,6 +6,7 @@ + @@ -22,12 +23,15 @@ Login Manager test: username/password prompts /** Test for Login Manager: username / password prompts. **/ netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); -var pwmgr, login1, login2A, login2B, login3A, login3B; +var pwmgr, tmplogin, login1, login2A, login2B, login3A, login3B, login4; function initLogins() { pwmgr = Cc["@mozilla.org/login-manager;1"]. getService(Ci.nsILoginManager); + tmpLogin = Cc["@mozilla.org/login-manager/loginInfo;1"]. + createInstance(Ci.nsILoginInfo); + login1 = Cc["@mozilla.org/login-manager/loginInfo;1"]. createInstance(Ci.nsILoginInfo); login2A = Cc["@mozilla.org/login-manager/loginInfo;1"]. @@ -38,6 +42,8 @@ function initLogins() { createInstance(Ci.nsILoginInfo); login3B = Cc["@mozilla.org/login-manager/loginInfo;1"]. createInstance(Ci.nsILoginInfo); + login4 = Cc["@mozilla.org/login-manager/loginInfo;1"]. + createInstance(Ci.nsILoginInfo); login1.init("http://example.com", null, "http://example.com", "", "examplepass", "", ""); @@ -45,17 +51,19 @@ function initLogins() { "user1name", "user1pass", "", ""); login2B.init("http://example2.com", null, "http://example2.com", "user2name", "user2pass", "", ""); - login3A.init("http://localhost:8888", null, "mochitest", "mochiuser1", "mochipass1", "", ""); login3B.init("http://localhost:8888", null, "mochitest2", "mochiuser2", "mochipass2", "", ""); + login4.init("http://localhost:8888", null, "mochitest3", + "mochiuser3", "mochipass3-old", "", ""); pwmgr.addLogin(login1); pwmgr.addLogin(login2A); pwmgr.addLogin(login2B); pwmgr.addLogin(login3A); pwmgr.addLogin(login3B); + pwmgr.addLogin(login4); } function finishTest() { @@ -65,10 +73,19 @@ function finishTest() { pwmgr.removeLogin(login2B); pwmgr.removeLogin(login3A); pwmgr.removeLogin(login3B); + pwmgr.removeLogin(login4); SimpleTest.finish(); } +/* + * handleDialog + * + * Invoked a short period of time after calling startCallbackTimer(), and + * allows testing the actual auth dialog while it's being displayed. Tests + * should call startCallbackTimer() each time the auth dialog is expected (the + * timer is a one-shot). + */ function handleDialog(doc, testNum) { netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); ok(true, "handleDialog running for test " + testNum); @@ -235,6 +252,27 @@ function handleDialog(doc, testNum) { is(password, "mochipass2", "Checking filled password"); break; + // (1002 doesn't trigger a dialog) + + case 1003: + is(username, "mochiuser1", "Checking filled username"); + is(password, "mochipass1", "Checking filled password"); + passfield.setAttribute("value", "mochipass1-new"); + break; + + case 1004: + is(username, "mochiuser3", "Checking filled username"); + is(password, "mochipass3-old", "Checking filled password"); + passfield.setAttribute("value", "mochipass3-new"); + break; + + case 1005: + is(username, "", "Checking filled username"); + is(password, "", "Checking filled password"); + userfield.setAttribute("value", "mochiuser3"); + passfield.setAttribute("value", "mochipass3-old"); + break; + default: ok(false, "Uhh, unhandled switch for testNum #" + testNum); break; @@ -250,6 +288,11 @@ function handleDialog(doc, testNum) { } +/* + * handleLoad + * + * Called when a load event is fired at the subtest's iframe. + */ function handleLoad() { netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); ok(true, "handleLoad running for test " + testNum); @@ -295,6 +338,73 @@ function handleLoad() { is(username, "mochiuser1", "Checking for echoed username"); is(password, "mochipass1", "Checking for echoed password"); + // Same realm we've already authenticated to, but with a different + // expected password (to trigger an auth prompt, and change-password + // notification bar). + startCallbackTimer(); + iframe.src = "authenticate.sjs?user=mochiuser1&pass=mochipass1-new"; + break; + + case 1003: + testNum++; + is(authok, "PASS", "Checking for successful authentication"); + is(username, "mochiuser1", "Checking for echoed username"); + is(password, "mochipass1-new", "Checking for echoed password"); + + // Check for the notification bar, and change the password. + bar = getNotificationBar(notifyBox, "password-change"); + ok(bar, "got notification bar"); + clickNotificationButton(bar, "Change"); + + // Housekeeping: change it back + tmpLogin.init("http://localhost:8888", null, "mochitest", + "mochiuser1", "mochipass1-new", "", ""); + pwmgr.modifyLogin(tmpLogin, login3A); + + // Same as last test, but for a realm we haven't already authenticated + // to (but have an existing saved login for, so that we'll trigger + // a change-password notification bar. + startCallbackTimer(); + iframe.src = "authenticate.sjs?user=mochiuser3&pass=mochipass3-new&realm=mochitest3"; + break; + + case 1004: + testNum++; + is(authok, "PASS", "Checking for successful authentication"); + is(username, "mochiuser3", "Checking for echoed username"); + is(password, "mochipass3-new", "Checking for echoed password"); + + // Check for the notification bar, and change the password. + bar = getNotificationBar(notifyBox, "password-change"); + ok(bar, "got notification bar"); + clickNotificationButton(bar, "Change"); + + // Housekeeping: change it back to the original login4. Actually, + // just delete it and we'll re-add it as the next test. + tmpLogin.init("http://localhost:8888", null, "mochitest3", + "mochiuser3", "mochipass3-new", "", ""); + pwmgr.removeLogin(tmpLogin); + // Clear cached auth from this subtest, and avoid leaking due to bug 459620. + var authMgr = Cc['@mozilla.org/network/http-auth-manager;1']. + getService(Ci.nsIHttpAuthManager); + authMgr.clearAll(); + + // Trigger a new prompt, so we can test adding a new login. + startCallbackTimer(); + iframe.src = "authenticate.sjs?user=mochiuser3&pass=mochipass3-old&realm=mochitest3"; + break; + + case 1005: + testNum++; + is(authok, "PASS", "Checking for successful authentication"); + is(username, "mochiuser3", "Checking for echoed username"); + is(password, "mochipass3-old", "Checking for echoed password"); + + // Check for the notification bar, and change the password. + bar = getNotificationBar(notifyBox, "password-save"); + ok(bar, "got notification bar"); + clickNotificationButton(bar, "Remember"); + finishTest(); break; @@ -337,7 +447,14 @@ var pword = { value : null }; var result = { value : null }; var isOk; -// XXX Add test for host that doesn't yet exist to test login-saving logic +// The notification box (not *bar*) is a constant, per-tab container. So, we +// only need to fetch it once. +var notifyBox = getNotificationBox(window.top); +ok(notifyBox, "Got notification box"); + +// Remove any notification bars that might be left over from other tests. +notifyBox.removeAllNotifications(true); + // ===== test 1 ===== var testNum = 1; @@ -689,8 +806,8 @@ iframe.onload = handleLoad; // clear plain HTTP auth sessions before the test, to allow // running them more than once. -var authMgr = Components.classes['@mozilla.org/network/http-auth-manager;1'] - .getService(Components.interfaces.nsIHttpAuthManager); +var authMgr = Cc['@mozilla.org/network/http-auth-manager;1']. + getService(Ci.nsIHttpAuthManager); authMgr.clearAll(); // ===== test 1000 ===== -- 2.11.4.GIT