Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Refactor login manager's crypto code

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Toolkit
Password Manager
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
The code login manger uses for doing encryption/decryption should ideally live in a separate component. This will eventually allow for greater flexibility, such as replacing it with alternative backends (eg, something based on the OS keyring/keychain) while still allowing encrypted signon data to live in signons.sqlite (and use storage-mozStorage.js).
(Assignee)

Comment 1

8 years ago
Created attachment 384187 [details] [diff] [review]
Patch v.1 (WIP)

First pass, mostly done.

This basically just moves _encrypt() / _decrypt() out of nsLoginManager.js and into crypto-SDR.js.

* Adds a new nsILoginManagerCrypto interface
* Cleans up error handling. I noticed all the userCanceled stuff wasn't really needed. We always just threw an exception when we saw that it was true, so now the exception is just thrown earlier.

Probably not worth changing storage-Legacy to use this.
Assignee: nobody → dolske
(Assignee)

Comment 2

8 years ago
Created attachment 398796 [details] [diff] [review]
Patch v.2
Attachment #384187 - Attachment is obsolete: true
Attachment #398796 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
Attachment #398796 - Flags: review?(gavin.sharp) → review?(mconnor)
(Assignee)

Comment 3

8 years ago
Created attachment 403411 [details] [diff] [review]
Patch v.3

Updated to tip.
Attachment #398796 - Attachment is obsolete: true
Attachment #403411 - Flags: review?(mconnor)
Attachment #398796 - Flags: review?(mconnor)
(Assignee)

Comment 4

8 years ago
Comment on attachment 403411 [details] [diff] [review]
Patch v.3

Let's run this by zpao too, and see if that will spur mconnor into a review. :)
Attachment #403411 - Flags: review?(paul)
Comment on attachment 403411 [details] [diff] [review]
Patch v.3

>+++ b/toolkit/components/passwordmgr/src/crypto-SDR.js
>+    __logService : null, // Console logging service, used for debugging.
>+    get _logService() {
>+        if (!this.__logService)
>+            this.__logService = Cc["@mozilla.org/consoleservice;1"].
>+                                getService(Ci.nsIConsoleService);
>+        return this.__logService;
>+    },
>+
>+    __decoderRing : null,  // nsSecretDecoderRing service
>+    get _decoderRing() {
>+        if (!this.__decoderRing)
>+            this.__decoderRing = Cc["@mozilla.org/security/sdr;1"].
>+                                 getService(Ci.nsISecretDecoderRing);
>+        return this.__decoderRing;
>+    },
>+
>+    __utfConverter : null, // UCS2 <--> UTF8 string conversion
>+    get _utfConverter() {
>+        if (!this.__utfConverter) {
>+            this.__utfConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>+                                  createInstance(Ci.nsIScriptableUnicodeConverter);
>+            this.__utfConverter.charset = "UTF-8";
>+        }
>+        return this.__utfConverter;
>+    },
>+
>+    _utfConverterReset : function() {
>+        this.__utfConverter = null;
>+    },
>+
>+    __observerService : null,
>+    get _observerService() {
>+        if (!this.__observerService)
>+            this.__observerService = Cc["@mozilla.org/observer-service;1"].
>+                                     getService(Ci.nsIObserverService);
>+        return this.__observerService;
>+    },

Do we want to start using XPCOMUtils.defineLazy(Service)Getter for these? It's not how the other password manager code is written (yet), but we might be good to start using it. Granted, it'll mean we have to do it in the function, not the prototype. Maybe that can be something we do all at once instead.

>+    _debug        : false, // mirrors signon.debug

Nit: No need for all those spaces

>+++ b/toolkit/components/passwordmgr/src/storage-mozStorage.js
>     _encryptLogin : function (login) {
>+        let encUsername, encPassword;
>+        encUsername = this._crypto.encrypt(login.username);
>+        encPassword = this._crypto.encrypt(login.password);

let encUsername = ...;
let encPassword = ...;

>@@ -1149,26 +1083,27 @@
>+                // user might have canceled master password entry, just ignore.

Nit: Start your sentence with a capital letter ;)

r=zpao with nits fixed & assuming all the tests pass. I'll leave the defineLazyGetter change up to you.
Attachment #403411 - Flags: review?(paul) → review+
(Assignee)

Updated

8 years ago
Attachment #403411 - Flags: review?(mconnor) → review?(vladimir)
Attachment #403411 - Flags: review?(vladimir) → review+
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)

> Do we want to start using XPCOMUtils.defineLazy(Service)Getter for these?

Eh, maybe, just to become consistent with other code. Spin off a bug if you'd like.

I fixed the other nits.
(Assignee)

Comment 7

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/ec5877cfb217
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Blocks: 534565
Blocks: 534589

Comment 8

8 years ago
just updated to latest nightly and hit this:

Failed to load XPCOM component: C:\Program Files\Mozilla\Mozilla Thunderbird\Beta\components\crypto-SDR.js
Error: Cc['@mozilla.org/login-manager/crypto/SDR;1'] is undefined
Source File: file:///C:/Program%20Files/Mozilla/Mozilla%20Thunderbird/Beta/components/storage-mozStorage.js
Line: 72
Fixed in http://hg.mozilla.org/comm-central/rev/0fa0825af0ac
You need to log in before you can comment on or make changes to this bug.