704682 - Expose password database as a content provider so Sync can update it

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Sync could be running when Firefox is not, since it will be a service. We should expose the Password DB as an SQLite content provider so Sync can update the DB as needed.

We need to make sure that the content provider is handing out access to the SQLite DB in the right Firefox profile, which for now is only the single default profile.

Comment 1

[cmtalbert]

(In reply to Mark Finkle (:mfinkle) from comment #0)

> We need to make sure that the content provider is handing out access to the
> SQLite DB in the right Firefox profile, which for now is only the single
> default profile.

Most of the test frameworks do not use the default profile.  They must create their own customized profiles and push them to the device and start fennec with the -profile flag.  I'm don't think that sync has any mochitests currently running on device, but if you write some to vet this change and you see orange, that's likely why unless you don't end up coding to this assumption.

Comment 2

[Richard Newman [:rnewman]]

mfinkle: later today I will take a look at the existing schema and suggest if any changes need to be made.

Sync on desktop currently tracks changes by watching observer notifications, which of course isn't the case in the native world: there's no tracking at all outside of the database.

Fingers crossed that passwords already tracks a guid and a lastModified time for each row…

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #2)
> mfinkle: later today I will take a look at the existing schema and suggest
> if any changes need to be made.
> 
> Sync on desktop currently tracks changes by watching observer notifications,
> which of course isn't the case in the native world: there's no tracking at
> all outside of the database.
> 
> Fingers crossed that passwords already tracks a guid and a lastModified time
> for each row…

If we use a ContentProvider (which I think we should), the API provides an easy way to track changes on the database (when db entries are added, deleted or updated).

Comment 4

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #3)

> If we use a ContentProvider (which I think we should), the API provides an
> easy way to track changes on the database (when db entries are added,
> deleted or updated).

lucasr: if you mean ContentObserver, I don't think it's suitable for our purposes. Unless we register an observer for every entry in the database (which I can't imagine is a good idea), we don't get told which items have changed, only that something has. The observer is expected to re-query, with the implicit assumption that the database contains everything they need to know.

I'm trying to avoid doing for our own password storage what I do for bookmarks and history: maintaining an entire parallel table and doing a diff to find out what changed locally since the last sync.

Am I missing something? If so, I'd dearly love to know!

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #4)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> 
> > If we use a ContentProvider (which I think we should), the API provides an
> > easy way to track changes on the database (when db entries are added,
> > deleted or updated).
> 
> lucasr: if you mean ContentObserver, I don't think it's suitable for our
> purposes. Unless we register an observer for every entry in the database
> (which I can't imagine is a good idea), we don't get told which items have
> changed, only that something has. The observer is expected to re-query, with
> the implicit assumption that the database contains everything they need to
> know.
> 
> I'm trying to avoid doing for our own password storage what I do for
> bookmarks and history: maintaining an entire parallel table and doing a diff
> to find out what changed locally since the last sync.
> 
> Am I missing something? If so, I'd dearly love to know!

You're right, the ContentObserver API is quite limited :-/ I guess you know about Android's SyncAdapter though? It looks like it's specifically meant to help syncing local and remote data. Maybe that's exactly what we need for Firefox Sync?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #5)

> You're right, the ContentObserver API is quite limited :-/ I guess you know
> about Android's SyncAdapter though? It looks like it's specifically meant to
> help syncing local and remote data. Maybe that's exactly what we need for
> Firefox Sync?

Yeah, we're implementing a SyncAdapter. That's really just an API for shuttling some stored state and an account through "you should sync now" calls, though; think of it as the active half of an Account. The adapter code itself is responsible for deciding what exactly syncing entails, which includes deciding which records are "dirty" and exchanging them with whatever upstream provider it uses.

I remain hopeful that there's some bit of the Android API that I don't know about, but really the whole thing seems like it's a little half-baked.

Comment 7

[Sriram Ramasubramanian [:sriram]]

I've been trying to get hold of ContentProviders. From what I see, Firefox will expose a ContentProvider. Sync will use a SyncAdapter and a ContentResolver (tied to this ContentProvider). The "dirty" records will be taken care by the Firefox (upon addition/deletion of records when user navigates a page). This needs to be queried by Sync using the same ContentProvider (with "dirty = 1" or "deleted = 1" and so on).

So, the Sync will:
1. Check for "dirty" records and update its values, from the server, based on dirty records.
2. Carrying out syncing more records from the server.
3. Carry out syncing "dirty" records with the server.

Firefox will:
1. Provide ContentProvider for normal addition/updation/deletion operation on the database.

I would have to try implementing a temporary sync service before I get more concrete on it.
I am going through: http://developer.android.com/resources/samples/SampleSyncAdapter/index.html 
This takes about dirty and deleted records: http://developer.android.com/resources/samples/SampleSyncAdapter/src/com/example/android/samplesync/platform/ContactManager.html

Comment 8

[Richard Newman [:rnewman]]

Summing up extensive and rambling IRC discussions:

* Password manager already has adequate date columns.
* Removed items imply deleted. Sync can figure this out (I think).
* We can't address master password yet: no way to prompt the user for their MP in a way that's secure to store for asynchronous syncing.
  * Can only sync when Fennec is running and has unlocked MP?
  * Refuse to sync passwords when MP is enabled?
    * Need API for asking whether MP is enabled for a given profile.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Basic skeleton

This is a WIP that has a basic skeleton for content provider. I am not sure how good it works. If things are working fine, I can polish and make things better based on what "methods" are needed.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

this allows other apps access to our passwords, right? If sync is being shipped in our apk, why do we need to provide a content provider?

Comment 11

[Richard Newman [:rnewman]]

(In reply to Brad Lassey [:blassey] from comment #10)
> this allows other apps access to our passwords, right?

It doesn't have to:

http://stackoverflow.com/questions/6120025/how-to-restrict-content-provider-data-across-applications

> If sync is being shipped in our apk, why do we need to provide a content provider?

For one thing, it's a neat decoupling technique. The most important part, though:

* We don't want to constrain Sync to only shipping inside a Fennec APK
* We definitely don't want a build constraint on Sync, which currently builds without hundreds of megs of m-c
* We can use content providers without offering universal access
* Using a content provider allows us to add additional client applications (e.g., "Fennec password fixer"!) in the future without bundling.

Comment 12

[Richard Newman [:rnewman]]

… oh, and providing a neat content provider interface allows some control over things like concurrent access. The alternative is presumably "have Sync open the .sqlite file", which is kinda awful.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

the alternative would be that sync could call though a shared API on GeckoAppShell. But if we can really control access to the content provider I have no real objection to using one.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Based on discussion in IRC, Matt Brubeck was telling me that we can have a "permission" set for Content Providers and only applications having that permission can access it. I looked into yet. And wanted to do it in round 2 (tonight). If we have that, then Sync can be installed separately and can have that "permission" to access Firefox's passswords.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 577422 [details] [diff] [review]
WIP: Basic skeleton

Looks fine so far. Lucas has plans in the Places provider to pass the "profile" name in as a Uri parameter. If no profile param is passed, we should fallback to "default"

Your code does the fallback already.

Later we can implement the "profile" as a Uri param. We might also want to merge the Password provider and the Places providers together. But that is later...

This approach looks OK as a start.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Checking Querying and Permissions

This WIP builds upon the older patch in using a SQLiteOpenHelper. This helper is working with the db easily. The db opened is inside the default profile for now. This can be changed based on the "URI" (once the actual URIs are finalized).

This uses LiveFolders to check if querying works. A LiveFolder can be added to the desktop (a separate application), to show the list of sites having saved passwords. This option will be removed when we push the final version to birch/m-c.

The permission strings are not localized yet. They are just there for testing purposes.

Things to do:
1. Creating db if one doesn't exist. -> better as a event to gecko.
2. Finalizing on the URIs (paths) to access this.
3. Finalizing on the Authorities to access this. (content://org.mozilla.gecko.providers/ or content://org.mozilla.fennec_xyz.providers)
4. List of extra helpers to make querying easier.
5. Experimenting with "signature" protectionLevel in permission.

Comment 17

[Ian Melven :imelven]

the protectionLevel 'signature' does look pretty interesting if we can sign Sync and Fennec with the same cert... 

if i understand correctly, without using protectionLevel = 'signature' any app that knows the right permission string can grant itself a custom permission required by another app.

i'm seeing conflicting info about whether these custom permissions are displayed to the user at install time, but it's only a pretty small mitigation if they are IMO.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 577743 [details] [diff] [review]
WIP: Checking Querying and Permissions

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

On the right track, but some code reuse and niggles to address. Syncing up with lucasr might be worthwhile!

::: mobile/android/base/AndroidManifest.xml.in
@@ +148,5 @@
>      </application>
> +
> +    <permission android:name="org.mozilla.gecko.PASSWORDS"
> +                android:label="Firefox passwords"
> +                android:protectionLevel="dangerous"/>

This should be "signature".

::: mobile/android/base/PasswordsProvider.java
@@ +102,5 @@
> +    @Override
> +    public boolean onCreate() {
> +        Log.i(LOGTAG, "Passwords OnCreate");
> +
> +        return true;

"You create a subclass implementing onCreate(SQLiteDatabase)... creating it if it does not".

Shouldn't this be calling CREATE TABLE somewhere? :)

You should also be creating indices as appropriate:

  static final String TABLE_PASSWORDS = "passwords";
  ...
  db.execSQL("CREATE INDEX passwords_guid_index ON " + TABLE_PASSWORDS + "(" + GeckoPassword.Password.GUID + ")");

@@ +105,5 @@
> +
> +        return true;
> +    }
> +
> +    private String getDatabasePath() {

Having a quick grep around the codebase, this looks like it's duplicated logic.

See base/db/BrowserProvider.java, getDatabasePath.

There should probably be some unified profile-aware database helper class here.

@@ +116,5 @@
> +        
> +        File profile = null;
> +        String[] files = home.list();
> +        for (String file : files) {
> +            if (file.endsWith(".default")) {

I'm guessing you should be using GeckoApp.mAppContext.getProfileDir(profile) here, and taking profile as an argument to getDatabasePath. (BrowserProvider's helper does this.)

@@ +162,5 @@
> +            Uri passwordUri = ContentUris.withAppendedId(GeckoPassword.CONTENT_URI, rowId);
> +            getContext().getContentResolver().notifyChange(uri, null);
> +            return passwordUri;
> +        }
> +

Don't you need to close the db handle in these methods? Either that, or memoize it. I've seen the runtime complain loudly in the log about db handles not being closed.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Initial code based on BrowserProvider

This WIP is based on Lucas's current code. I've basically copied the code to PasswordsProvider.java. Currently delete wouldn't work, but the rest should work. It would be great if this code can be tested, so that I can continue working on this.

In onCreate(), I am creating a table. Actually there are two tables (moz_logins and moz_disabledHosts) in the DB. I am not sure if I should be creating the other table too.

I am thinking of moving the code into BrowserProvider.java (with a new PASSWORDS_DATABASE_NAME and PASSWORDS_DATABASE_VERSION fields, that can switch between the two databases) -- since this reuses (or rather copies ;) ) a lot of functions written by Lucas. My initial attempt to factor out the code into a separate file failed. Things became too complex in passing parameters.
Instead, having all in BrowserProvider seems to be good to me.

I couldn't test this patch, except that Fennec didn't crash on load :).

Comment 20

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

The previous patch didn't expose the provider.
This has been rectified now.
When the Passwords related code moves back into BrowserProvider, the newly added Provider can be removed from the AndroidManifest file.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Adding deleting option

This patch adds the "deleted" column to the db. The implementation is same as Lucas's.

Comment 22

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Adding SyncColumns

This patch adds SyncColumns for generic query from Sync.

Comment 23

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP : SyncColumns with NULL checks

This patch adds null checks to selection and projection arguments.

Comment 24

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch makes sure the projection is proper (to be checked against when a query is passed).
The resulting columns are returned as needed by Sync -- using column aliasing.

Comment 25

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch tries to "alter table" directly.
If the table doesnt have the column, we are adding one.
If the table has one already, we would get an exception that we drop silently.

Comment 26

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Changes with respect to preprocessing in another bug

This patch preprocesses the PasswordsProvider file to reflect the changes in m-c.

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 582406 [details] [diff] [review]
WIP: Changes with respect to preprocessing in another bug

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

I've been running with this patch for a few days. Looks good to me, though I haven't given detailed review to PasswordsProvider.java.in (moved file?).

Clearing feedback flags from jvoll :)

Comment 28

[Richard Newman [:rnewman]]

Ping?

Comment 29

[Johnathan Nightingale [:johnath]]

So, Sriram is out for a while, this will need new ownership. Richard, can you help me understand what's next here?

Comment 30

[Wesley Johnston (:wesj)]

Since sriram's out and rnewman is here in sf for a bit, I'm grabbing this. Sounds like things are about ready to go?

I'll unbitrot and give it a run through before asking for review.

Comment 31

[Richard Newman [:rnewman]]

(In reply to Johnathan Nightingale [:johnath] from comment #29)
> So, Sriram is out for a while, this will need new ownership. Richard, can
> you help me understand what's next here?

Absolutely.

0. Clean up. Looks like wesj just grabbed it, so awesome.
1. Someone on the mobile team should review this code. If they're happy, I can give it a final pass, then we can…
2. Land it. (Signature-level permissions, as discussed in Comment 11.)
3. Fix Bug 711636, so we can actually get cleartext passwords into Sync's hands. This will need someone who knows how the PSM works.
4. Finish the relevant chunk of Sync on top (Marina's first task this month): Bug 709385.
5. Fix any bugs in this code that we find in doing so. I don't believe that Fennec has any automated Java testing, so the only automated exercise that this interface will get is when Sync's integration suite starts to cover it. We'll burn that bridge as we come to it, as Toby is fond of saying.

Comment 32

[Richard Newman [:rnewman]]

The obvious solution to Part 3 is for the content provider to take a ?cleartext=1 parameter. It can then transparently do encryption and decryption for writes and reads, without duplicating code in Sync, and also seamlessly handle the whole master password nonsense when we start to tackle that.

Comment 33

[Richard Newman [:rnewman]]

And, as you can see from the whiteboard: this needs sec review before it can face the world. imelven is already participating on this bug, and I am confident that there's nothing much to worry about, but they're the chaps who get to make that call!

CCing Erin so she knows that's on the horizon.

Comment 34

[Wesley Johnston (:wesj)]

Comment on attachment 582406 [details] [diff] [review]
WIP: Changes with respect to preprocessing in another bug

This applies fine (with a tweak in AndroidManifest.xml). I'm looking at the code and it doesn't look like we're actually storing all the columns from the database yet. Notably the encrypted password ones that bug 711636 will fix, but also things like usernameField and passwordField, the hostname, etc.

I'm not sure what the best plan is for those. Maybe sriram just assumed we'd have to send those to Gecko to be stored and so left them out here? If we just want to get a base piece in here so that sync can "work", we could check this in and do that work in separate bugs.

Comment 35

[Mark Finkle (:mfinkle) (use needinfo?)]

To be clear, we must use the exact same schema as Gecko uses for this database. In fact, Gecko needs to be able to read this database. All password management that happens in Gecko has to access the DB. Any non-standard columns or tables must not conflict with the core Gecko schema or prevent Gecko upgrades to the schema.

The password DB is not being handled like the history/bookmark DB's, where we are not letting Gecko internals access the DB directly. For passwords, Gecko has to be able to access the DB without knowing it was created by Java.

Comment 36

[Johnathan Nightingale [:johnath]]

[+dolske, because it says password manager on the label, and he knows things or, at least, he knew things once]

Comment 37

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #35)
> To be clear, we must use the exact same schema as Gecko uses for this
> database. In fact, Gecko needs to be able to read this database.

… and of course this has to be the same shape as desktop Firefox so that we can sync it correctly.

(I don't know if this sharing is actually implemented correctly, because I haven't read the rest of Fennec's source, but this patch certainly creates signons.sqlite in the profile directory. wesj will be familiar with how to make sure that everything gets created or opened in the right place!)


> The password DB is not being handled like the history/bookmark DB's, where
> we are not letting Gecko internals access the DB directly. For passwords,
> Gecko has to be able to access the DB without knowing it was created by Java.

Yup. The only trick will be making sure that the Gecko code is aware that the DB can be changed by external transactions, whether it's running or not, and locks/caches/etc. appropriately. That's a significant change in assumptions.

Comment 38

[Justin Dolske [:Dolske]]

Err, wow. The amount of essentially duplicate code here makes me really, really sad. :(

Why do we need this? Just so Sync can run when Firefox is not?

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Justin Dolske [:Dolske] from comment #38)
> Err, wow. The amount of essentially duplicate code here makes me really,
> really sad. :(
> 
> Why do we need this? Just so Sync can run when Firefox is not?

Exactly

Comment 40

[Richard Newman [:rnewman]]

(In reply to Justin Dolske [:Dolske] from comment #38)
> Err, wow. The amount of essentially duplicate code here makes me really,
> really sad. :(
> 
> Why do we need this? Just so Sync can run when Firefox is not?


There's a little more backstory, and more benefit to it than just that, but broadly speaking yes. Android Sync is a pile of Java code that Android pokes and prods. It doesn't get to run inside Gecko. 

All of this work is to make Fennec a bit more of a native Android citizen.

Comment 41

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 582406 [details] [diff] [review]
WIP: Changes with respect to preprocessing in another bug

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

I started to review, then realized that this isn't what we want at all. The passwords need to be stored in Gecko's store. Passwords are being handled very differently than bookmarks and history

::: mobile/android/base/AndroidManifest.xml.in
@@ +145,5 @@
>                    android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>
>  
> +        <provider android:name="@ANDROID_PACKAGE_NAME@.db.PasswordsProvider"
> +                  android:authorities="@ANDROID_PACKAGE_NAME@.db.passwords"
> +                  android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>

we should have signature protection on this

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +123,5 @@
> +        mSyncColumns.add(Passwords.DATE_CREATED);
> +        mSyncColumns.add(Passwords.DATE_MODIFIED);
> +    }
> +
> +    static final String qualifyColumn(String table, String column) {

move to a DB Utils class

@@ +127,5 @@
> +    static final String qualifyColumn(String table, String column) {
> +        return table + "." + column;
> +    }
> +
> +    public static String generateGuid() {

move to a DB Utils class

@@ +143,5 @@
> +    }
> +
> +    // This is available in Android >= 11. Implemented locally to be
> +    // compatible with older versions.
> +    public static String concatenateWhere(String a, String b) {

move to a DB Utils class

@@ +157,5 @@
> +    }
> +
> +    // This is available in Android >= 11. Implemented locally to be
> +    // compatible with older versions.
> +    public static String[] appendSelectionArgs(String[] originalValues, String[] newValues) {

move to a DB Utils class

Comment 42

[Wesley Johnston (:wesj)]

I think we're ok there because of:

Log.d(LOGTAG, "Getting database path for profile: " + profile);
File profileDir = GeckoApp.mAppContext.getProfileDir(profile);
...
String databasePath = new File(profileDir, DATABASE_NAME).getAbsolutePath();
Log.d(LOGTAG, "Successfully created database path for profile: " + databasePath);

We will have to update for the GeckoDirProvider change.

Comment 43

[Justin Dolske [:Dolske]]

I think we should meet to talk about this or something. There are a number of things I'm concerned about skimming though this:

* I'd like to better understand the design constraints of Sync on native mobile
* Lots of duplication with existing Toolkit code.
* No test coverage
* How are DB schema changes handled?
* What's up with this "IS_DELETED" column? Does this not actually delete removed records?

Comment 44

[Richard Newman [:rnewman]]

(In reply to Justin Dolske [:Dolske] from comment #43)
> I think we should meet to talk about this or something. There are a number
> of things I'm concerned about skimming though this:
> 
> * I'd like to better understand the design constraints of Sync on native
> mobile

The feature page (slightly stale, sorry) might address some of these.

https://wiki.mozilla.org/Services/AndroidSyncFP

In short: it's a chunk of pure Java that gets driven by Android, not by Fennec. It runs when Fennec doesn't, and it doesn't use any part of Gecko. Those constraints give rise to a lot of design decisions.

> * Lots of duplication with existing Toolkit code.

That's quite likely: Sync is an Android SyncAdapter, and it talks to Android ContentResolver/ContentProvider interfaces. Fennec library code has to provide these Java interfaces, and they should work without having to instantiate a Gecko runtime (if only because they can be called thousands of times without having guaranteed lifetimes, and that's a lot of overhead).

This kind of ContentProvider is supposed to be a slim layer of Java on top of a sqlite DB.

Code duplication is an inherent result of making Fennec more of an Android citizen (and thus exposing and using Javaish Android patterns like ContentProviders), rather than a pile of C++ and JavaScript pretending to be an Android citizen.

> * No test coverage

Sadly true for the rest of Fennec's Java bits, too :(

Android Sync's test suites will provide some coverage, at least.

> * How are DB schema changes handled?

As I recall, ContentProviders expose an onUpgrade method. The Android system infrastructure handles tracking of versions, calling this when necessary.

lucasr might have a better memory than me.

> * What's up with this "IS_DELETED" column? Does this not actually delete
> removed records?

Sync needs to know which records have been deleted since it last ran, so it can sync deletions up to your sync account.

In XUL Sync we can watch Places events. On Android we cannot: most likely Fennec and Sync won't be running at the same time, and regardless there's no easy channel for propagating those events. (It also imposes the work of tracking changes on Sync, which doesn't really have enough context to do so.)

Instead, we have Fennec flagging records as deleted. An "I am Sync!" parameter to the content provider dictates whether these records are visible to queries. They are lazily cleaned up after some time.

It's a little bit of a hack, but it's the most efficient way to do it.

Comment 45

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

need to schedule a review with secteam

Comment 46

[Wesley Johnston (:wesj)]

Attachment: Abandoned Patch

This is just some work I'm abandoning and want to save. Starts implementing nsILoginManagerStorage in a js->java wrapper. I think it builds but definitely shouldn't work.

Comment 47

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

:wesj so is this work being landed? If so a review is needed if not then we just need to make sure no one does land this before a review.

Comment 48

[Wesley Johnston (:wesj)]

:curtisk We're working on exposing Gecko's sql libraries to java and making the calls through that. I'm trying to write an interface so that most of the calls in this patch will work with minimal changes, but we're also planning to remove the IS_DELETED column added by this patch, and to fallback to starting Gecko any time we detect a schema_version change or when we need to create databases. We're also going to probably move the passwords crypto out of NSS and instead use something we can more easily access from Java.

Whew.... lots to get done here. All of which will need you guys to look at it. I'll make sure we've got security reviews before any of it lands.

Comment 49

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #48)
> ... but we're also planning to remove the IS_DELETED column added by this patch ...

s/remove/move, for clarity. The info still needs to be around somewhere, just likely not in this table.

Comment 50

[Wesley Johnston (:wesj)]

Attachment: WIP

This is nowhere near close to ready. Builds. Crashes when I try to step. Just saving my place. I started today hoping maybe I could just slip our database in in place of the android SQLiteDatabase component. That entails implementing... a lot. So I just trimmed that down to a few pieces I thought would be nice and started work implementing them, and then made some changes to the PasswordProvider to use it.

This applies on top of gcp's patch to expose libmozsqlite3.so (I have no idea what bug that is for).

Comment 51

[Wesley Johnston (:wesj)]

Attachment: WIP

Working well now. This expands SQLiteBridge.java to look more like a normal SQLiteDatabase in java.

Next up: testing results I get back from the Password provider.

Comment 52

[Ian Melven :imelven]

(In reply to Wesley Johnston (:wesj) from comment #48)
> :curtisk We're working on exposing Gecko's sql libraries to java and making
> the calls through that. I'm trying to write an interface so that most of the
> calls in this patch will work with minimal changes, but we're also planning
> to remove the IS_DELETED column added by this patch, and to fallback to
> starting Gecko any time we detect a schema_version change or when we need to
> create databases. We're also going to probably move the passwords crypto out
> of NSS and instead use something we can more easily access from Java.
> 
> Whew.... lots to get done here. All of which will need you guys to look at
> it. I'll make sure we've got security reviews before any of it lands.

i took a look through the previous patch before, see bug 710572 - i'll keep reviewing patches - this should also get some testing when it all lands - particularly to make sure only Fennec can access the passwords exposed as is intended.

Comment 53

[Wesley Johnston (:wesj)]

Attachment: patch

I think this is close, but want to spend a little more time tomorrow testing and cleaning things up as I find them.

Was hoping to get some feedback while I finished it up and tested.

Comment 54

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 589714 [details] [diff] [review]
patch

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

::: mobile/android/base/db/DBUtils.java
@@ +18,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Lucas Rocha <[e-mail]>

shouldn't this be you?

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +154,5 @@
> +        try {
> +            results = query(sb.toString(), selectionArgs);
> +            for (Object resultRow: results) {
> +                Object[] resultColumns = (Object[])resultRow;
> +                cursor.addRow(resultColumns);

would it make sense for query() to populate a passed in cursor rather than return an Object array?

::: mozglue/android/APKOpen.cpp
@@ +285,5 @@
>  SHELL_WRAPPER1(onChangeNetworkLinkStatus, jstring)
>  SHELL_WRAPPER1(reportJavaCrash, jstring)
>  SHELL_WRAPPER0(executeNextRunnable)
>  SHELL_WRAPPER1(cameraCallbackBridge, jbyteArray)
> +SHELL_WRAPPER3(notifyBatteryChange, jdouble, jboolean, jdouble)

??

Comment 55

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Tested this out quite a bit. Works well. Ready for a reviewing...

Comment 56

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Grr. qref hates me.

Comment 57

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 590931 [details] [diff] [review]
Patch v1

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

::: mobile/android/base/db/DBUtils.java
@@ +19,5 @@
> +    public static String generateGuid() {
> +        UUID uuid = UUID.randomUUID();
> +        return uuid.toString();
> +        //byte[] encodedBytes = Base64.encode(generateRandomBytes(9), Base64.URL_SAFE);
> +        //return new String(encodedBytes);

remove commented out code

@@ +22,5 @@
> +        //byte[] encodedBytes = Base64.encode(generateRandomBytes(9), Base64.URL_SAFE);
> +        //return new String(encodedBytes);
> +    }
> +
> +    private static byte[] generateRandomBytes(int length) {

this impl can be shared with sync

@@ +33,5 @@
> +    }
> +
> +    // This is available in Android >= 11. Implemented locally to be
> +    // compatible with older versions.
> +    public static String concatenateWhere(String a, String b) {

remove the concatenateWhere() in BrowserProvider.java http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#228

@@ +47,5 @@
> +    }
> +
> +    // This is available in Android >= 11. Implemented locally to be
> +    // compatible with older versions.
> +    public static String[] appendSelectionArgs(String[] originalValues, String[] newValues) {

remove the appendSelectionArgs() in BrowserProvider.java http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#242

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +276,5 @@
> +
> +                if (values.containsKey(CommonColumns._ID)) {
> +                    values.put(Passwords.ID, values.get(CommonColumns._ID).toString());
> +                    values.remove(CommonColumns._ID);
> +                }

please use a helper function for this

@@ +290,5 @@
> +                }
> +
> +                if (!values.containsKey(Passwords.HOSTNAME)) {
> +                    values.put(Passwords.HOSTNAME, "");
> +                }

and another helper for this. Also, no curly braces for a one line if statement

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

I haven't been following the MPLv2 news, are we supposed to be re-licensing files now?

@@ +131,5 @@
> +        try {
> +            for (Object resultRow: results) {
> +                Object[] resultColumns = (Object[])resultRow;
> +                if (resultColumns.length == mColumns.size())
> +                    cursor.addRow(resultColumns);

why not have query take a Cursor argument that it populates rather than passing the array across jni and then having to populate the Cursor as a second step?

It is fine to land with this, but please file a follow up unless there's a good reason not to do it.

Comment 58

[Richard Newman [:rnewman]]

Per IRC, please use the same GUID generation as the bookmarks provider, not UUID. That can be Sync's code if you would like to reuse that.

Comment 59

[Ian Melven :imelven]

I've been following this and have read the patch several times and have discussed some wrinkles with wesj and bsmith - see also bug 710572. Holding off on markign sec-review-complete until it lands to be sure what lands is what I reviewed, but this is good to go from my standpoint.

Comment 60

[Wesley Johnston (:wesj)]

I'll want to land this with the patch in bug 718817 (because this sorta relies on it). At that point we should still NOT be using this until I have bug 718760 wrapped up as well. But they should be fine sitting in the tree not bitrotting.

Comment 61

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 590931 [details] [diff] [review]
Patch v1

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

::: mozglue/android/SQLiteBridge.cpp
@@ +192,5 @@
> +      numPars = jenv->GetArrayLength(jParams);
> +      int sqlNumPars;
> +      sqlNumPars = f_sqlite3_bind_parameter_count(ppStmt);
> +      if (numPars != sqlNumPars) {
> +          LOG("Passed parameter count (%d) doesn't match SQL parameter count (%d)\n",

Stray LOG here. This won't catch the case now where the query has parameters and jParams is NULL, so maybe move the sqlNumPars check outside the first if.

@@ +198,5 @@
> +          asprintf(&errorMsg, "Passed parameter count (%d) doesn't match SQL parameter count (%d)\n",
> +              numPars, sqlNumPars);
> +          goto error_close;
> +      }
> +      // Bind parameters, if any

Stray whitespace.

@@ +243,5 @@
> +    // if the statement doesn't return any results, instead return the id and number of changed rows
> +    if (rc == SQLITE_DONE) {
> +        jclass integerClass = jenv->FindClass("java/lang/Integer");
> +        jmethodID intConstructor = jenv->GetMethodID(integerClass, "<init>", "(I)V");
> +        

Move this to JNI_Setup?

@@ +268,4 @@
>      // For each row, add an Object[] to the passed ArrayList,
>      // with that containing either String or ByteArray objects
>      // containing the columns
> +    while (rc != SQLITE_DONE) {

You'll conflict against m-c here, this was fixed "upstream" as well.

Comment 62

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch v2

Carrying forward blassey's review. 

I rebased this to m-c and incorporated my own review comments, minus the JNI_Setup thing, because wesj commented it failed when he tried.

Comment 63

[Wesley Johnston (:wesj)]

landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85346ae0ea99

NOTE, Sync should NOT use this in production until the crypto patches are done.

Comment 64

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85346ae0ea99

Comment 65

[Richard Newman [:rnewman]]

Good job, wesj and crew!

Will keep an eye out for everything else landing.