Closed Bug 1559458 Opened 5 years ago Closed 5 years ago

Provide a recovery solution for AVG users who had `logins.json` access from Firefox prevented

Categories

(Toolkit :: Password Manager, task, P1)

All
Windows
task

Tracking

()

RESOLVED FIXED

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

User Story

Affected users:
* First release Firefox version with new signing certificate was 67.0.2
* Windows only
* AVG users at the time 67.0.2 was first started (may have uninstalled AVG in the meantime)
* Should have a `logins.json.corrupt*` files modified (which timestamp actually changes in this case) since 67.0.2 release (with some buffer for clock skew)

Recovery:
* Close Login Manager access to logins.json
* Move any existing `logins.json` aside. If that fails then maybe the user doesn't have the AVG fix yet and it's preventing us from accessing it?
* Let the user choose the file to restore if they have more than one otherwise just give a confirmation dialog.
* Restart the browser to re-initialize the password manager
* Translations: https://pontoon.mozilla.org/projects/restore-logins-extension/

Attachments

(2 files, 10 obsolete files)

In case we need it I will starting writing code that could be run on user's machines to recover their logins from the logins.json.corrupt* files caused by AVG interfering with Firefox's access to logins.json.

User Story: (updated)
User Story: (updated)
User Story: (updated)
User Story: (updated)
Depends on: 1559503
Attached patch WIP 1 for feedback (obsolete) — Splinter Review

Work in progress for initial feedback

Attachment #9072306 - Flags: feedback?(kmaglione+bmo)
User Story: (updated)
Comment on attachment 9072306 [details] [diff] [review]
WIP 1 for feedback

Review of attachment 9072306 [details] [diff] [review]:
-----------------------------------------------------------------

::: experiments/skeleton/api.js
@@ +2,5 @@
> +/* global ExtensionAPI */
> +ChromeUtils.defineModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +ChromeUtils.defineModuleGetter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
> +
> +ChromeUtils.import("resource://gre/modules/Console.jsm");

This is a no-op. Please remove.

@@ +7,5 @@
> +const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +
> +const env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +const ADDON_ID = "hotfix-avg-logins-bug-1559458@mozilla.com";

There's no reason to hardcode this. You can just get `extension.id` from your API instance.

@@ +12,5 @@
> +
> +function fix(desiredFile) {
> +  let loginsJSONPath = OS.Path.join(OS.Constants.Path.profileDir, "logins.json");
> +  let recoveryBackupPath = OS.Path.join(OS.Constants.Path.profileDir, "logins.json.before-recovery");
> +  if (OS.File.exists(loginsJSONPath)) {

This needs an `await`. `OS.File.exists` returns a promise, which will always evaluate to true.

@@ +24,5 @@
> +async function getCorruptFiles() {
> +  let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
> +  let files = [];
> +  try {
> +    await iterator.forEach(async function onEntry(entry) {

You probably want `for await (let entry of iterator) ...`

@@ +33,5 @@
> +      let file = {
> +        entry,
> +        stats,
> +      };
> +      if ("winLastWriteDate" in entry) {

Should this not be "winCreationDate"?

@@ +49,5 @@
> +    });
> +  } finally {
> +    iterator.close();
> +  }
> +  console.log("e", files);

This looks wrong.

@@ +56,5 @@
> +
> +function stageFix(path) {
> +  console.log("stage fix ", path);
> +  // TODO: test directory names with spaces and non-ascii
> +  env.set("MOZ_RESTORE_LOGINS", path);

I'm not entirely clear on the purpose of requiring a restart for this...

@@ +62,5 @@
> +}
> +
> +function uninstall() {
> +  console.log("uninstalling");
> +  AddonManager.getAddonByID(ADDON_ID, function(addon) {

These APIs don't accept callbacks anymore. They return promises.
Attached patch v.1 Extension (obsolete) — Splinter Review

Address feedback

Attachment #9072306 - Attachment is obsolete: true
Attachment #9072306 - Flags: feedback?(kmaglione+bmo)
Attachment #9072315 - Flags: feedback?(kmaglione+bmo)
Attached patch v1.1 Extension (obsolete) — Splinter Review
Attachment #9072315 - Attachment is obsolete: true
Attachment #9072315 - Flags: feedback?(kmaglione+bmo)
Attachment #9072316 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 9072315 [details] [diff] [review]
v.1 Extension

Review of attachment 9072315 [details] [diff] [review]:
-----------------------------------------------------------------

::: api.js
@@ +7,5 @@
> +
> +async function fix(desiredFile) {
> +  Services.logins.wrappedJSObject._storage.wrappedJSObject.terminate();
> +  try {
> +    await Services.logins.wrappedJSObject._storage.wrappedJSObject._store.finalize();

Maybe just store the value of `Services.logins.wrappedJSObject._storage.wrappedJSObject` and reuse it?

@@ +21,5 @@
> +  }
> +
> +  console.log("restoring", desiredFile);
> +  await OS.File.copy(desiredFile, loginsJSONPath);
> +  await uninstall();

This needs to pass the extension ID...

@@ +31,5 @@
> +  let files = [];
> +  try {
> +    for await (let entry of iterator) {
> +      if (!entry.name.startsWith("logins.json") || !entry.name.includes(".corrupt")) {
> +        return null;

`continue`

@@ +49,5 @@
> +  } finally {
> +    iterator.close();
> +  }
> +
> +  return files;

It seems strange that we sort the files if there's no exception but return them unsorted if their is an exception...

@@ +92,5 @@
> +    let labels = corrupt.map(file => {
> +      console.log(file.creationDate);
> +      let msDiff = now - file.creationDate;
> +      let days = Math.floor(msDiff / MS_IN_DAY);
> +      console.log(days);

These two console logs seem unnecessary. At the very least they need some context...
Attachment #9072315 - Attachment is obsolete: false
Comment on attachment 9072316 [details] [diff] [review]
v1.1 Extension

Review of attachment 9072316 [details] [diff] [review]:
-----------------------------------------------------------------

::: api.js
@@ +38,5 @@
> +      let file = {
> +        entry,
> +        stats,
> +      };
> +      file.creationDate = entry.winCreationDate || 0; // TODO

This should probably just be `entry.winCreationDate || entry.lastModificationDate`

@@ +42,5 @@
> +      file.creationDate = entry.winCreationDate || 0; // TODO
> +      files.push(file);
> +    }
> +
> +    return files.sort(function compare(a, b) {

I think just move this to after the finally clause.
Attached patch v1.2 Extension (obsolete) — Splinter Review
Attachment #9072315 - Attachment is obsolete: true
Attachment #9072316 - Attachment is obsolete: true
Attachment #9072316 - Flags: feedback?(kmaglione+bmo)
Attachment #9072320 - Flags: feedback?(kmaglione+bmo)
Attachment #9072320 - Flags: feedback?(kmaglione+bmo) → review+

Can you sign this so QA can start testing?

Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)

QA's testing on Windows 10 was good so I sent an email to AMO admins to have the signed en-US attachment 9072334 [details] uploaded there. Localization has started at https://pontoon.mozilla.org/projects/restore-logins-extension/ and we will upload a new version around Tuesday with localizations for top locales if everything goes well.

AMO rejects strict_min_versions which aren't on the list at https://addons.mozilla.org/en-US/firefox/pages/appversions/ so unless TheOne is able to override that for us we will have to loosen it. I don't think it's a big deal since it means we will also help users who downgraded Firefox but still don't know how to fix their logins.json in their profile folder.

Changes since the last signed version (for en-US): https://github.com/mozilla/restore-logins-extension/compare/v1.0.0-beta.1...v1.2.x This also includes some minor copy updates from PMM.

Apologies for flagging so many people… I'm not sure who is checking bugmail this weekend. Could someone please sign this with the Mozilla signing cert.?

Thank you very much

Attachment #9072353 - Flags: feedback?(nhnt11)
Attachment #9072353 - Flags: feedback?(mcooper)
Attachment #9072353 - Flags: feedback?(markh)
Attachment #9072353 - Flags: feedback?(kmaglione+bmo)

The add-on code looks ok from an AMO perspective, just needs a signature now.

Wei or Adrian, could you please sign the file in comment 12 with the internal certificate?

Flags: needinfo?(wezhou)
Flags: needinfo?(autrilla)
See Also: → 1558765
Group: mozilla-employee-confidential
Flags: needinfo?(wezhou)

SIgned file attached. Please test it.

Thanks.

Flags: needinfo?(autrilla)

Since v1.2.0 would need QA again (due to changed minVersion), we built another version that includes localization while we are working on getting QA folks

tl;dr: This unsigned version (1.3) introduces localization.

Attachment #9072320 - Attachment is obsolete: true
Attachment #9072330 - Attachment is obsolete: true
Attachment #9072334 - Attachment is obsolete: true
Attachment #9072353 - Attachment is obsolete: true
Attachment #9072363 - Attachment is obsolete: true
Attachment #9072353 - Flags: feedback?(nhnt11)
Attachment #9072353 - Flags: feedback?(mcooper)
Attachment #9072353 - Flags: feedback?(markh)
Attachment #9072353 - Flags: feedback?(kmaglione+bmo)

This is the signed version of v1.3, ready for QA.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Group: mozilla-employee-confidential
User Story: (updated)

Added pt-BR, fr, & es-ES translations. I believe we aren't planning on adding more languages after this.

Please sign this extension and wait for approval in the Slack channel before publishing on AMO.

Anyone else CC'd who can sign, feel free to do so.

Attachment #9072372 - Attachment is obsolete: true
Flags: needinfo?(awagner)
See Also: → 1559459

Unfortunately, I don't have the key to sign.

Maybe Wei or Adrian are around?

Flags: needinfo?(wezhou)
Flags: needinfo?(awagner)
Flags: needinfo?(autrilla)
Flags: needinfo?(wezhou)
Flags: needinfo?(autrilla)

There were no objections on Slack so please make 1.3.1 the latest version on AMO.

Thank you

Flags: needinfo?(awagner)
Attachment #9072373 - Attachment is obsolete: true

Version 1.3.1 is posted and approved. There were some slight issues during upload, let me know if this caused any trouble.

Flags: needinfo?(awagner)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: