Open Bug 1171687 Opened 9 years ago Updated 2 years ago

test_logins_decrypt_failure.js fails on Android

Categories

(Toolkit :: Password Manager, defect, P5)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

People

(Reporter: MattN, Unassigned)

References

Details

(Whiteboard: [passwords:tech-debt])

Attachments

(3 files)

Test log
9.41 KB, text/plain
Details
Bug 1171687 - Part 1: Fix test_storage_mozStorage.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1171687 - Part 2: Fix running xpcshell tests that initialize NSS locally.
59 bytes, text/x-review-board-request
gbrown
: review+
glandium
: review+
Details
Attached file Test log — 
+++ This bug was initially created as a clone of Bug #1170772 +++

Bug 1170772 is enabling the xpcshell test directory on Android but test_storage_mozStorage.js and test_logins_decrypt_failure.js fail and need to be fixed before enabling.

(Quoting Geoff Brown [:gbrown] from bug 1170772 comment #3)
> test_storage_mozStorage.js and test_logins_decrypt_failure.js fail on
> Android:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d1affc45dad
> 
> http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/gbrown@mozilla.com-
> 4d1affc45dad/try-android-api-11/try_ubuntu64_vm_armv7_mobile_test-xpcshell-3-
> bm122-tests1-linux64-build99.txt.gz
Flags: qe-verify-
Flags: firefox-backlog+
Whiteboard: [passwords:tech-debt]
dkeeler: I started digging into these tickets, motivated by replacing password storage on Android.  (My thinking: we should make the existing tests work for the existing storage, and then make the existing tests work for the replacement storage.  Sadly, I didn't realize that the "storage" tests are really about upgrading and downgrading storage-specific SQLite DBs, so not germane to replacement storage.  Sigh.)  In any case, I think the try pushes will address test_storage_mozStorage.js, and I'll flag you for review when they're green.

However, I can't make progress on test_logins_decrypt_failure.js.  What is happening is that `token.reset()` followed by `token.changePassword("", "")` fails around http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/toolkit/components/passwordmgr/test/unit/test_logins_decrypt_failure.js#22.  I have tried `token.initPassword("")` as well.

There are similar tests that are disabled on Android due to something about smartcards which I don't understand; specifically http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/security/manager/ssl/tests/unit/test_pkcs11_token.js#80.

It's not clear to me that this flow that invokes `token.reset()` _should_ work on Android, and we definitely don't invoke it in Fennec's Master Password support.  Can you provide some light on this test, whether it should work on Android, and whether the usage of the internal token API is even correct?  Thanks!
Flags: needinfo?(dkeeler)
I have a green X8 job at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bed995b6c23c07254885ebb0a58c8f779f5a11f&selectedJob=108295051 with a log containing

[task 2017-06-19T19:18:34.368082Z] 19:18:34     INFO -  TEST-PASS | toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js | took 13364ms

and it looks like the Android x86 job is passing too.  \o/
As far as I can tell, the issue is that token.reset() deletes the table "nssPrivate" in key4.db: https://dxr.mozilla.org/mozilla-central/rev/95543bdc59bd038a3d5d084b85a4fec493c349ee/security/nss/lib/softoken/sdb.c#1625
This table is only ever created when the user cert/key DB is loaded, which for Firefox basically means NSS initialization: https://dxr.mozilla.org/mozilla-central/rev/95543bdc59bd038a3d5d084b85a4fec493c349ee/security/nss/lib/softoken/sdb.c#2068

I did figure out a way to get NSS to reload the cert/key DB module, and by changing the changePassword to initPassword, the test does pass (with a ton of the following:  0:06.92 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_FAILURE: Couldn't decrypt string" {file: "file:///home/keeler/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/components/crypto-SDR.js" line: 136}]" (which makes sense, because the backend can't decrypt the encrypted login entries).

However, this is a bit of a hack, and in any case we don't actually expose the ability to reset the key DB in this way (there *is* a resetpassword.xul, but you have to manually type it in to the location bar - no UI links to it). I'm tempted to say this test isn't relevant unless and until we want to actually expose this feature (and when we do, we'll have to actually make it work). Maybe for right now the best thing to do would be to create new, empty key DBs (one for android, one for the other platforms) and test that if you start xpcshell with them they can't decrypt the saved passwords - that would achieve most of what this test is aiming to test (without the portion that we don't support).
Flags: needinfo?(dkeeler)
Comment on attachment 8879325 [details]
Bug 1171687 - Part 1: Fix test_storage_mozStorage.

https://reviewboard.mozilla.org/r/150612/#review155368

(dkeeler from the commit message didn't match :keeler, not sure if you still want his review too)
Attachment #8879325 - Flags: review+
Comment on attachment 8879326 [details]
Bug 1171687 - Part 2: Fix running xpcshell tests that initialize NSS locally.

https://reviewboard.mozilla.org/r/150614/#review155544

You'll probably want some test harness peer to look at this for the implementation details side of things.
Attachment #8879326 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8879326 [details]
Bug 1171687 - Part 2: Fix running xpcshell tests that initialize NSS locally.

https://reviewboard.mozilla.org/r/150614/#review155666
Attachment #8879326 - Flags: review?(gbrown) → review+
Depends on: 1556117

I'm landing part 1 in bug 1556117 since I want the test coverage for bug 1148205.

Summary: test_storage_mozStorage.js and test_logins_decrypt_failure.js fail on Android: → test_logins_decrypt_failure.js fails on Android
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/807a86140aba
Fix test_storage_mozStorage. r=MattN

(In reply to Pulsebot from comment #11)

https://hg.mozilla.org/integration/mozilla-inbound/rev/807a86140aba
Fix test_storage_mozStorage. r=MattN

and I forgot to update the bug number after filing…

Keywords: leave-open
Keywords: leave-open
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: