Inserting passwords should check if they're deleted first

RESOLVED FIXED in Firefox 13

Status

()

defect
P2
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 +)

Details

(Whiteboard: [sync])

Attachments

(2 attachments, 1 obsolete attachment)

Apparently sync can get into a condition where:

1.) User deletes password 1 from device 1
2.) User modifies password 1 on device 2
3.) Sync modifies (re-adding in the process) password 1 on device 1

The password is now in both the passwords and the deleted_passwords table at the same time. I'm not aware of any problems that should cause with our current code, but its an odd state, and its prone to causing problems later.

When new records are added to passwords, we should remove them from deleted passwords if they exist (ideally in the same transaction, but we don't have transaction support with GeckoSqlite right now).
Blocks: 704682
wes, help prioritize this.  is this required to ship beta?  Ship Release?
Sounds like a data issue we should fix for beta
Assignee: nobody → wjohnston
Priority: -- → P2
Edge case, certainly, but best to nip data consistency issues in the bud. Should be a two-line fix (with forty lines of tests!).
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
Posted patch Patch (obsolete) — Splinter Review
This will try and delete from the deleted items table before inserting for both passwords and form history.
Attachment #601835 - Flags: review?(rnewman)
Posted patch TestSplinter Review
Test for passwords. Probably should write a similar one for Form history, but I need to leave.
Attachment #601836 - Flags: review?(rnewman)
I should note this builds on top of the test patches in bug 725881 and the ones in bug 718760 (test patches are bitrotted in the bugs by changes that have happened to sqlitebridge, hence aren't landed quite yet)
Comment on attachment 601835 [details] [diff] [review]
Patch

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

General approach looks good.

::: mobile/android/base/db/FormHistoryProvider.java.in
@@ +144,5 @@
>      public void initGecko() {
>          GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormHistory:Init", null));
>      }
>  
> +    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db) {

All of these methods should have @Override annotations.

@@ +148,5 @@
> +    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db) {
> +        try {
> +            // if this entry is in the deleted table, remove it
> +            String[] guid = {values.getAsString("guid")};
> +            db.delete(TABLE_DELETED_FORM_HISTORY, BrowserContract.DeletedFormHistory.GUID + " = ?", guid);

I would rephrase this for safety and clarity.

    if (!values.containsKey(FormHistory.GUID)) {
      // Caller didn't specify a GUID, so no work to do.
      return;
    }
    String guid = values.getAsString(FormHistory.GUID);
    if (guid == null) {
      // Caller explicitly inserting with GUID = null.
      // I would check to see if this is allowed, but FormHistoryProvider.java.in
      // doesn't include the schema...
      db.delete(TABLE_DELETED_FORM_HISTORY, 
                BrowserContract.DeletedFormHistory.GUID + " IS NULL",
                null);
      return;
    }
    db.delete(TABLE_DELETED_FORM_HISTORY, 
              BrowserContract.DeletedFormHistory.GUID + " = ?",
              new String[] { guid });
  }

@@ +150,5 @@
> +            // if this entry is in the deleted table, remove it
> +            String[] guid = {values.getAsString("guid")};
> +            db.delete(TABLE_DELETED_FORM_HISTORY, BrowserContract.DeletedFormHistory.GUID + " = ?", guid);
> +        } catch (SQLiteBridgeException ex) {
> +            Log.i(getLogTag(), "Error removing entry");

Log.w(getLogTag(), "Error removing entry with GUID " + guid, ex);

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +211,5 @@
>      }
>  
> +    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db) {
> +        try {
> +            // if this entry is in the deleted table, remove it

Capitalization and punctuation.

@@ +213,5 @@
> +    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db) {
> +        try {
> +            // if this entry is in the deleted table, remove it
> +            String[] guid = {values.getAsString("guid")};
> +            db.delete(TABLE_DELETED_PASSWORDS, BrowserContract.DeletedPasswords.GUID + " = ?", guid);

Same pattern as in FormHistoryProvider.java.in.

@@ +215,5 @@
> +            // if this entry is in the deleted table, remove it
> +            String[] guid = {values.getAsString("guid")};
> +            db.delete(TABLE_DELETED_PASSWORDS, BrowserContract.DeletedPasswords.GUID + " = ?", guid);
> +        } catch (SQLiteBridgeException ex) {
> +            Log.i(getLogTag(), "Error removing entry");

Log.w(getLogTag(), "Error removing entry with GUID " + guid, ex);

@@ +229,5 @@
>              values.put(Passwords.ENCRYPTED_USERNAME, res);
>          }
>      }
>   
> +    public void onPreUpdate(ContentValues values, Uri uri, SQLiteBridge db) {

Again, @Overrides.
Attachment #601835 - Flags: review?(rnewman) → feedback+
Comment on attachment 601836 [details] [diff] [review]
Test

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

r+ with nits.

::: mobile/android/base/tests/BaseTest.java.in
@@ +159,5 @@
>          File profile = new File(mProfile);
>          String dbPath = new File(profile, dbName).getPath();
>  
>          Cursor c = mActions.querySql(dbPath, sqlCommand);
> +        SqliteCompare(c, cvs);

I trust you on this :)

::: mobile/android/base/tests/testBug725052.java.in
@@ +1,1 @@
> +#filter substitution

License.

@@ +12,5 @@
> +import android.util.Log;
> +import java.util.ArrayList;
> +import java.lang.reflect.Method;
> +
> +public class testBug725052 extends BaseTest {

I thought as a company we had elected to use useful names for tests, rather than just bug numbers?

Looking at the names in robocop.ini, I'd opt for testPasswordUndeletion.

@@ +15,5 @@
> +
> +public class testBug725052 extends BaseTest {
> +    public void testBug725052() {
> +        setTestType("mochitest");
> +        Context context = (Context)getActivity();

Nit: space between cast and value.

@@ +43,5 @@
> +            Method encrypt = nss.getMethod("encrypt", contextClass, stringClass, stringClass);
> +    
> +            cvs[0].put("guid", "MtaVURdK-eRl");
> +  
> +            // Attempt to insert into the db

Punctuation and capitalization.

@@ +44,5 @@
> +    
> +            cvs[0].put("guid", "MtaVURdK-eRl");
> +  
> +            // Attempt to insert into the db
> +            deletedPasswordUri = (Uri)deletedpwds.getField("CONTENT_URI").get(null);

Nit: space between cast and value. (And so throughout.)

@@ +53,5 @@
> +            Uri uri = cr.insert(deletedPasswordUri, cvs[0]);
> +            mAsserter.is(uri, null, "Insert returned null correctly");
> +    
> +            // This should fail the first time round because there is no pw database
> +            // Wait for gecko to reply and then we'll try again

Punctuation etc.

@@ +59,5 @@
> +            contentEventExpecter.blockForEvent();
> +            cr.insert(deletedPasswordUri, cvs[0]);
> +  
> +            // attempting to insert another item with the same guid should
> +            // remove the item from deletedPasswords database

Capitalization etc.

@@ +85,5 @@
> +
> +    public void tearDown() throws Exception {
> +        super.tearDown();
> +  
> +        // remove the entire signons.sqlite file

Capitalization and punctuation.
Attachment #601836 - Flags: review?(rnewman) → review+
> All of these methods should have @Override annotations.

Just to be clear, these methods are abstract in GeckoProvider. From what I know, @Override provides us with anything extra beyond different compile errors.

Looking back, I think you're right in bug 731491 though. These particular methods shouldn't be abstract. We can just stub them out in GeckoProvider and override them here. I'll do that in 731491.
(In reply to Wesley Johnston (:wesj) from comment #9)
> > All of these methods should have @Override annotations.
> 
> Just to be clear, these methods are abstract in GeckoProvider. From what I
> know, @Override provides us with anything extra beyond different compile
> errors.

@Override means that if the superclass methods change (e.g., adding an argument), your compile will fail rather than silently creating a non-overriding method. That's less important for abstract methods (in that you'd still get a compilation failure), but not if you end up with any kind of hierarchy…

I don't really see much point in applying careful thought: "oh, I don't need this annotation, because…". Just add @Override to every method that overrides a superclass's method. Every single one. If nothing else, it's inline documentation as you scan the source file.
Posted patch Patch v2Splinter Review
Nits addressed.
Attachment #601835 - Attachment is obsolete: true
Attachment #602983 - Flags: review?(rnewman)
Comment on attachment 602983 [details] [diff] [review]
Patch v2

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

r=me with remaining nits addressed. If you're done for the night, let me know and I'll fix and land.

::: mobile/android/base/db/FormHistoryProvider.java.in
@@ +149,5 @@
> +        if (!values.containsKey(FormHistory.GUID)) {
> +            return;
> +        }
> +        String guid = values.getAsString(FormHistory.GUID);
> +        String where = BrowserContract.DeletedFormHistory.GUID + " = ?";

Lift this WHERE value out and make it private static final. No point computing it each time. (If you're lucky, the compiler will do this for you, but I don't like to count on luck!)

@@ +153,5 @@
> +        String where = BrowserContract.DeletedFormHistory.GUID + " = ?";
> +        String[] args = new String[] { guid };
> +        if (guid == null) {
> +            where = BrowserContract.DeletedFormHistory.GUID + " IS NULL";
> +            args = null;

… then no point allocating a new array above, mm?

@@ +156,5 @@
> +            where = BrowserContract.DeletedFormHistory.GUID + " IS NULL";
> +            args = null;
> +        }
> +        try {
> +            db.delete(TABLE_DELETED_FORM_HISTORY, where, args);

In fact, you might as well say:

  private static final String WHERE_GUID_IS_NULL = BrowserContract.DeletedFormHistory.GUID + " IS NULL";
  private static final String WHERE_GUID_IS_VALUE = BrowserContract.DeletedFormHistory.GUID + " = ?";
  ...

  try {
    String guid = values.getAsString(FormHistory.GUID);
    if (guid == null) {
      db.delete(TABLE_DELETED_FORM_HISTORY, WHERE_GUID_IS_NULL, null);
      return;
    }
    String[] args = new String[] { guid };
    db.delete(TABLE_DELETED_FORM_HISTORY, WHERE_GUID_IS_VALUE, args);
  } catch(SQLiteBridgeException ex) {
    Log.w(getLogTag(), "Error removing entry with GUID " + guid, ex);
  }

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +214,5 @@
> +    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db) {
> +        if (values.containsKey(Passwords.GUID)) {
> +            String guid = values.getAsString(Passwords.GUID);
> +            String where = BrowserContract.DeletedPasswords.GUID + " = ?";
> +            String[] args = new String[] { guid };

Same here.
Attachment #602983 - Flags: review?(rnewman) → review+
Backed out on inbound to investigate XUL Fennec crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5940fe8d1af9
Backed out temporarily because of broken Android builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a536d22f312c
and one of the reflected methods i need has had its name changed (by me!) making this test fail. backed just the test out for now.
Whiteboard: [sync]
https://hg.mozilla.org/mozilla-central/rev/11dea4451258
the test has been backed out, as said in comment 18, leaving open for it.
The test fix should be easy, but I don't want to block beta on it.
blocking-fennec1.0: beta+ → ?
feature blocks, the test doesn't. Open another bug for the test
Status: ASSIGNED → RESOLVED
blocking-fennec1.0: ? → +
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.