1221588 - Add allLogins call to BrowserLogins and implementation to SQLiteLogins to retrieve all of the user’s logins optionally filtered by username

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Stephan Leroux [:sleroux]]

For the login management UI, we want to display all of the available logins for the user. To do this, we'll need to add a method to the BrowserLogins protocol and implementation to SQLiteLogins to retreive all of the user's logins. Additionally, this method show take in a filter option so we are able to filter by username.

Comment 1

[Richard Newman [:rnewman]]

This is something like

    public func getAllLogins() -> Deferred<Maybe<Cursor<LoginData>>> {
        let projection = SQLiteLogins.MainWithLastUsedColumns

        let sql =
        "SELECT \(projection) FROM " +
        "\(TableLoginsLocal) WHERE is_deleted = 0 " +
        "UNION ALL " +
        "SELECT \(projection) FROM " +
        "\(TableLoginsMirror) WHERE is_overridden = 0 " +
        "ORDER BY \(orderBy)"

        return db.runQuery(sql, args: args, factory: SQLiteLogins.LoginDataFactory)
    }

This can obviously be simplified to project and populate only the columns we need -- no point returning a LoginData when we just want username + host.

Comment 2

[Stephan Leroux [:sleroux]]

Might make sense to retrieve the full data object since we'll probably be passing it through to the detail controller in memory and make more detailed changes there. That way we can just send back the memory object back to the DB for updates.

Comment 3

[Richard Newman [:rnewman]]

Ultimately this probably comes down to perf -- if it's not too expensive to snarf the entire database into memory, then querying for everything up-front would certainly make things simpler.

Note that it's possible for a sync to happen between preparing the list and generating the detail view. Fetching just the displayed item would reduce the race window, but of course not eliminate it.

Comment 4

[Stephan Leroux [:sleroux]]

Ideally the list view would be notified of when changes are made to the logins table so we can invalidate. I know I've mentioned it before but does it make sense to have some observer here to tell us when the data in that table changes so we can invalidate? I'm not sure what the best way of doing that would be from sqlite though. I guess we would have to latch on to the insert/update transactions we make and notify using the changeset?

Comment 5

[Richard Newman [:rnewman]]

There are, what, five or six different Cocoa ways of doing this? :D

Yeah, we can observe if we want to. I suggest filing a follow-up.

Comment 6

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1269

Comment 7

[Richard Newman [:rnewman]]

N.B., the bug description mentions filtering, but the code doesn't.

FWIW, on desktop we filter on any string -- username, hostname, and password.

Comment 8

[Stephan Leroux [:sleroux]]

My bad - yes, this patch should include filtering. You mention on desktop on any string - do you just mean by username, hostname, and password?

Comment 9

[Stephan Leroux [:sleroux]]

So I'm thinking of changing the API to be the following:

public func getAllLoginsByField(field: SearchableLoginField, alphabetically: Bool, searchTerm: String?)

To allow us to search by Hostname, Username, and Passwords. Do you think it makes sense to use something like FTS for this?

Comment 10

[Stephan Leroux [:sleroux]]

Nevermind - I'm going to start with basic LIKE query, write some perf unit test for it and see how that goes for now.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #8)
> My bad - yes, this patch should include filtering. You mention on desktop on
> any string - do you just mean by username, hostname, and password?

Yup. The key is that you can search for a password (e.g., to change dupes after a leak) or a username (e.g., to find sites you might need to change your work email to personal), not just host.

I think we should search all fields at the same time, like desktop:

  hostname LIKE '...' OR username LIKE '..." OR password LIKE '...'

Comment 12

[Stephan Leroux [:sleroux]]

Comment on attachment 8688021 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1269

I've replaced the commit in the PR to a new one with the new search API and address the nits you've mentioned:

https://github.com/mozilla/firefox-ios/commit/847044f501c9dd595385bd74e37ec2e08b3aa23a

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8688021 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1269

I think we only need one input string (only one text box…). Also `contains` and SQL escaping.

Comment 14

[Stephan Leroux [:sleroux]]

I've updated the PR with:

* Better search API that takes a single term
* No more SQL injection - uses the args parameter to properly bind the query arguments
* Fixed tests to reflect new search API
* Added perf tests for single/all search queries

Note: On my machine, looks like the search query to find all matching results for 1000 entries is ~200ms. Might be worth optimizing later with FTS.

Comment 15

[Stephan Leroux [:sleroux]]

Landed

a3ca2369537d0ad8e2babc40e6042f91897c38e6