Closed
Bug 1429148
Opened 6 years ago
Closed 6 years ago
Password policies: lock "remember passwords" and don't allow a master password to be set
Categories
(Firefox :: Enterprise Policies, enhancement, P1)
Firefox
Enterprise Policies
Tracking
()
VERIFIED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | verified |
People
(Reporter: Felipe, Assigned: Felipe)
References
Details
Attachments
(3 files)
Two different password-related policies requested for the MVP: - Don't remember passwords (signon.rememberSignons) - Don't allow a master password to be set* *CCK2 only hides the dialog to set a master password from the Preferences dialog. This might be enough to do too on our MVP. Fully disabling the master password functionality seems more work.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•6 years ago
|
||
For the first policy, originally I was gonna do as most other policies to be only about disabling it. However, there was a request in the enterprise mailing list about also enforcing this to remain true (because users were inadvertently changing this setting), and since this is a trivial change in implementation, I thought it would be nice to support it.
Summary: Password policies: don't remember passwords and don't allow a master password → Password policies: lock "remember passwords" and don't allow a master password to be set
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8951121 [details] Bug 1429148 - Policy: Enforce choice for the Remember Passwords setting. https://reviewboard.mozilla.org/r/220374/#review226618 Thanks
Attachment #8951121 -
Flags: review?(MattN+bmo) → review+
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8951122 [details] Bug 1429148 - Add nsIPK11Token.isInternalKeyToken. https://reviewboard.mozilla.org/r/220376/#review226626 r=me on the stuff outside /security/manager/ ::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_masterpassword.js:6 (Diff revision 1) > +add_task(async function test_policy_remember_passwords() { > + await setupPolicyEngineWithJson({ > + "policies": { > + "DisableMasterPassword": true > + } > + }); > + > + await BrowserTestUtils.withNewTab("about:preferences#privacy", async browser => { > + // eslint-disable-next-line mozilla/no-cpows-in-tests > + ok(browser.contentDocument.getElementById("useMasterPassword").disabled, "Master Password checkbox was disabled"); Ideally you would test what happens if a MP was already set (if the behaviour differs). ::: browser/components/preferences/in-content/privacy.js:1138 (Diff revision 1) > checkbox.checked = !noMP; > + checkbox.disabled = !Services.policies.isAllowed("masterPassword"); > }, Should it only be disabled if `noMP` is true so that a user who already has a master password can still remove it? ::: security/manager/pki/resources/content/device_manager.js:167 (Diff revision 1) > } > } > } > + > + if (!Services.policies.isAllowed("masterPassword") && > + selected_token.tokenName == getNSSString("InternalToken")) { I think it's preferred to compare the token to `tokendb.getInternalKeyToken()` and I'm not technically a peer of security/ so you should probably check with keeler. I've never read this code before.
Attachment #8951122 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #5) > ::: browser/components/preferences/in-content/privacy.js:1138 > (Diff revision 1) > > checkbox.checked = !noMP; > > + checkbox.disabled = !Services.policies.isAllowed("masterPassword"); > > }, > > Should it only be disabled if `noMP` is true so that a user who already has > a master password can still remove it? Good point. > > ::: security/manager/pki/resources/content/device_manager.js:167 > (Diff revision 1) > > } > > } > > } > > + > > + if (!Services.policies.isAllowed("masterPassword") && > > + selected_token.tokenName == getNSSString("InternalToken")) { > > I think it's preferred to compare the token to > `tokendb.getInternalKeyToken()` and I'm not technically a peer of security/ > so you should probably check with keeler. I've never read this code before. I tried this but unfortunately the comparison doesn't work. I don't know why though
Assignee | ||
Updated•6 years ago
|
Attachment #8951122 -
Flags: review?(dkeeler)
Assignee | ||
Comment 7•6 years ago
|
||
Keeler, can you take a look at the device_manager.js part of the patch. This is a policy for sysadmins to block users from setting a master password.
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8951122 [details] Bug 1429148 - Add nsIPK11Token.isInternalKeyToken. https://reviewboard.mozilla.org/r/220376/#review226650 It would be nice to have some sort of test for the device manager (similar to the related tests in security/manager/ssl/tests/mochitest/browser). Also, what happens if the user has already set a password? It seems this would make it impossible to un-set it? (Maybe that's the intended behavior, and in any case it won't be a problem if the user's profile has this policy from the beginning.) It looks like we'll need some way to check if a token is the internal token, so r- for now. ::: security/manager/pki/resources/content/device_manager.js:167 (Diff revision 1) > } > } > } > + > + if (!Services.policies.isAllowed("masterPassword") && > + selected_token.tokenName == getNSSString("InternalToken")) { Unfortunately I don't think a simple equality check will work (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd have to do is call `PK11_IsInternalKeySlot(mSlot.get())`) ::: security/manager/pki/resources/content/device_manager.js:168 (Diff revision 1) > } > } > + > + if (!Services.policies.isAllowed("masterPassword") && > + selected_token.tokenName == getNSSString("InternalToken")) { > + pw_toggle = true; I think this needs to be `"true"`, actually, given the rest of this code...
Attachment #8951122 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo) from comment #8) > It would be nice to have some sort of test for the device manager (similar > to the related tests in security/manager/ssl/tests/mochitest/browser). Also, Ok, I'll find a way to test the device_manager part too. Shouldn't be too hard. > what happens if the user has already set a password? It seems this would > make it impossible to un-set it? (Maybe that's the intended behavior, and in > any case it won't be a problem if the user's profile has this policy from > the beginning.) Matt also mentioned this in comment 5, so I'll leave the checkbox in about:preferences enabled if a master password already exists, to allow it to be unset. Perhaps I'll also leave this button in the device_manager enabled if selected_token.hasPassword Also, I'll change the policy name from "DisableMasterPassword" to "CreateMasterPassword" (which will expect a `false` parameter), to make it more explicit that this will only protect before a master password is set in a profile. > > ::: security/manager/pki/resources/content/device_manager.js:167 > (Diff revision 1) > > } > > } > > } > > + > > + if (!Services.policies.isAllowed("masterPassword") && > > + selected_token.tokenName == getNSSString("InternalToken")) { > > Unfortunately I don't think a simple equality check will work > (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we > should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd > have to do is call `PK11_IsInternalKeySlot(mSlot.get())`) Yeah, I can do it. However, I should note that when the user clicks in the "Change Password" button, the token is already passed by name (see https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/security/manager/pki/resources/content/device_manager.js#357), see this would offer no extra benefit AFAICT. Let me know if I should make that change anyway or keep the string comparison. > > ::: security/manager/pki/resources/content/device_manager.js:168 > (Diff revision 1) > > } > > } > > + > > + if (!Services.policies.isAllowed("masterPassword") && > > + selected_token.tokenName == getNSSString("InternalToken")) { > > + pw_toggle = true; > > I think this needs to be `"true"`, actually, given the rest of this code... Good catch. it still works with boolean true, but better to follow the rest of the code.
Assignee | ||
Comment 10•6 years ago
|
||
Needinfo'ing for the question below: (In reply to :Felipe Gomes (needinfo me!) from comment #9) > (In reply to David Keeler [:keeler] (use needinfo) from comment #8) > > Unfortunately I don't think a simple equality check will work > > (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we > > should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd > > have to do is call `PK11_IsInternalKeySlot(mSlot.get())`) > > Yeah, I can do it. However, I should note that when the user clicks in the > "Change Password" button, the token is already passed by name (see > https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/security/manager/pki/resources/content/device_manager.js#357 ), > so this would offer no extra benefit AFAICT. > > Let me know if I should still add an isInternalKeyToken function, or keep the string > comparison.
Flags: needinfo?(dkeeler)
Sorry, missed this. (In reply to :Felipe Gomes (needinfo me!) from comment #10) > Needinfo'ing for the question below: > > (In reply to :Felipe Gomes (needinfo me!) from comment #9) > > (In reply to David Keeler [:keeler] (use needinfo) from comment #8) > > > Unfortunately I don't think a simple equality check will work > > > (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we > > > should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd > > > have to do is call `PK11_IsInternalKeySlot(mSlot.get())`) > > > > Yeah, I can do it. However, I should note that when the user clicks in the > > "Change Password" button, the token is already passed by name (see > > https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/security/manager/pki/resources/content/device_manager.js#357 ), > > so this would offer no extra benefit AFAICT. Honestly, this isn't ideal either, but since it already exists and since it's essentially going token -> get name -> get token by that same name, I think it's a little less likely to go wrong. > > > > Let me know if I should still add an isInternalKeyToken function, or keep the string > > comparison. I still think it would be useful.
Flags: needinfo?(dkeeler)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8951122 [details] Bug 1429148 - Add nsIPK11Token.isInternalKeyToken. https://reviewboard.mozilla.org/r/220376/#review227894 Perfect - thanks.
Attachment #8951122 -
Flags: review?(dkeeler) → review+
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8952567 [details] Bug 1429148 - Policy: Don't let a Master Password to be set. https://reviewboard.mozilla.org/r/221818/#review227906 PSM parts look good to me. Thanks for adding the device manager test. I had a look at the PSM-specific parts of that, and they seem correct (although I'm not an expert on browser-chrome tests).
Attachment #8952567 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 16•6 years ago
|
||
Comment on attachment 8952567 [details] Bug 1429148 - Policy: Don't let a Master Password to be set. r=MattN. Matt had already given r+ with nits for this patch, but since I added a new patch in the queue, ReviewBoard got confused
Attachment #8952567 -
Flags: review?(MattN+bmo) → review+
Comment 17•6 years ago
|
||
Pushed by felipc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0c3184f918f0 Policy: Enforce choice for the Remember Passwords setting. r=MattN https://hg.mozilla.org/integration/autoland/rev/ffe15fb2541c Add nsIPK11Token.isInternalKeyToken. r=keeler,MattN https://hg.mozilla.org/integration/autoland/rev/c9dc10752a47 Policy: Don't let a Master Password to be set. r=keeler
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0c3184f918f0 https://hg.mozilla.org/mozilla-central/rev/ffe15fb2541c https://hg.mozilla.org/mozilla-central/rev/c9dc10752a47
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee | ||
Comment 19•6 years ago
|
||
Note for testing: Two policies landed here: "RememberPasswords" - this one can accept both *true* and *false* values, and it should enforce that value to the pref "signon.rememberSignons", and the corresponding checkbox in about:preferences ("Remember logins and passwords for websites") "CreateMasterPassword" - this one can only accept *false* as a value, and if false, it should not allow a master password to be created. If a Master Password already exists prior to the policy being used, it *won't* block the master password checkbox, as a way to possibly allowing it to be unset
Comment 20•6 years ago
|
||
Thanks Felipe for your notes in comment 19. We tested both policies using JSON file. We verified them manually as fixed. "RememberPasswords" policy is tested with values "true" and "false" and "CreateMasterPassword" with "false". Detailed information about test steps and runs is available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734 The bug will also be retested with ADMX files when it is ready for testing.
Updated•6 years ago
|
status-firefox59:
affected → ---
Comment 21•6 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•