1238103 - Basic Auth credentials are queried and saved without protocol decoration

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Stephan Leroux [:sleroux]]

STR:

1. Navigate to Login List
2. Tap Edit
3. Tap 'Select All' button

Expected:

Button title should update to 'Deselect All'

Actual:

Button title remains the same

-----

I wasn't able to reproduce this but :tecgirl was and after some debugging it looks like the condition here [1] doesn't pass. The data source is reporting a higher number of items than that are visible.

[1] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Login%20Management/LoginListViewController.swift#L146

Comment 1

[Stephan Leroux [:sleroux]]

I've updated my test branch with some extensive logging to help pin down the issue. I've scoped the logging for logins only to display their hostnames and exclude anything else.

:tecgirl when you get a chance could you try running this patch and sending over the logs?

Branch: https://github.com/mozilla/firefox-ios/tree/sleroux/Bug1238103-MissingLoginsBug
New commit: https://github.com/mozilla/firefox-ios/commit/c1113ebb8bb1d9dd131399c0c39c4a33bc01f63a

Comment 2

[Robin Andersen [:tecgirl]]

Attachment: output logs from 6s sim

Comment 3

[Stephan Leroux [:sleroux]]

Looks like the logic for grabbing the baseDomain from the hostname URL is failing because 2 of the logins don't include the scheme:

phonebook.mozilla.org
pto.mozilla.org

:rnewman, is this normal for Logins to have their hostnames not include a scheme? These logins were pulled down from the server without modification.

Comment 4

[Richard Newman [:rnewman]]

I think this is how we save Basic Auth logins on iOS, actually! I'm looking on my own desktop profile, and I have two entries for pto.mo -- one saved from desktop last year, and one saved from iOS this year.

I wondered why we weren't autofilling those.

Comment 5

[Richard Newman [:rnewman]]

For Basic Auth, challenge.protectionSpace.host is a bare hostname, no protocol.

When we come to save the login, login.hostname is bare.

So this is definitely on us!

Comment 6

[Richard Newman [:rnewman]]

Proposed solution: we should be saving and querying the protocol (which is the field `protocol` on an NSURLProtectionSpace) as part of the URL for Basic Auth challenges, rather than just using the challenge host directly.

If we want to be super thorough, we can migrate existing records, but we'd have to be careful to avoid collisions with existing records.

Comment 7

[Richard Newman [:rnewman]]

Proposing tracking because (a) we won't match synced basic auth logins from desktop, (b) we're polluting our logins DB, (c) it b0rks this feature.

Comment 8

[Stephan Leroux [:sleroux]]

I've been looking into this more closer and found that there is a a nomenclature problem between what we consider is a `hostname` and what NSURLProtectionSpace considers a 'host'. Theres a few things happening:

1. When encountering a regular login (non-Basic Auth), we create a credential using the url directly. For example, if I log into news.ycombinator.com, we invoke createWithHostname [1] and pass https://news.ycombinator.com as the 'hostname'. Technically its not actually the host but the host + protocol.

2. When encountering a basic auth login, we create a credential using only the bare 'host' where 'host' is the proper definition of a URL host [2]. In the case of https://pto.mozilla.org, the protocol is thrown away and we only store the host part.

NOTE: Both these cases generate hostnames that are now synced with hostnames being valid URLS (https://new.ycombinator.com) and invalid, host-only strings (pto.mozilla.org).

3. A Login struct represents its hostname data from an instance of NSURLProtectionSpace [3]. When we construct logins from the DB using the factory in SQLiteLogins [4], we're assigning the full hostname stored in the DB as the host inside NSURLProtectionSpace. This logically doesn't make sense considering that a NSURLProtectionSpace's host property is supposed to only store the host and not the scheme/protocol + host.

To reconcile the differences between host/hostnames, I propose:

1. Modify the factory to properly set the host and protocol params of NSURLProtectionSpace by converting the hostname into a URL and extracting the scheme/host from it.

2. Update the hostname getter [3] to not just return the protectionSpace.host but prefix it with protectionSpace.protocol + ://

3. Update the hostname constructor [1] to convert the hostname into a URL and assign the scheme/host to the NSURLProtectionSpace's protocol/host properties.

This works for newly created logins but I'm not sure of a good plan yet to migrate or handle already created logins with bare hostnames.

As a side note, this same discrepancy is the cause of basic auth prompts not being auto-filled. The NSURLProtectionSpace we provide the getLoginsForProtectionSpace method contains a proper host and protocol but the arguments we pass to the DB only consider the host [5].

[1] https://github.com/mozilla/firefox-ios/blob/master/Storage/Logins.swift#L181
[2] https://github.com/mozilla/firefox-ios/blob/master/Storage/Logins.swift#L192
[3] https://github.com/mozilla/firefox-ios/blob/master/Storage/Logins.swift#L126
[4] https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/SQLiteLogins.swift#L136
[5] https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/SQLiteLogins.swift#L215

Comment 9

[Stephan Leroux [:sleroux]]

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

I've attached a WIP patch implementing the proposed solution w/o consideration for migration.

Comment 10

[Richard Newman [:rnewman]]

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

Comments left on the PR. Note in particular that we also should include non-standard ports in the URL.

Re migration: we have a couple of options.

We can "soft-migrate":

* Don't sync rows that don't start with a protocol.
* When we try to match one during an auth challenge, match against the DB without the protocol. 
  * Example: we're on https://example.org:5050. Match "example.org" as well as "https://example.org:5050" (and perhaps "https://example.org").
* If the challenge succeeds, replace the hostname in the DB with the correct expanded hostname, and mark the record as needing to be synced. Tada! We migrated one row.
* Take extreme care: if the expanded URL exists in the DB already, simply delete this row, optionally updating the other row's password to match the one that worked.

We can "hard-migrate":

* Guess the protocol (https?), perhaps by checking history.
* Assume no custom port.
* Rewrite every row directly, marking them all as needing to be synced.

Comment 11

[Stephan Leroux [:sleroux]]

I've updated the PR to include the review nits and also started down the soft-migrate path.

Couple of things:

1. Looks like whenever we build up the login using the factory, if no scheme is provided when building a NSURLProtectionSpace then it defaults to http. This actually works nicely because it forces Login.hostname to be a valid URL all the time. This resolves the bug where not all logins were displaying the LoginListViewController.

2. The soft-migrate works pretty well for a single entry that doesn't match. I've updated the query to search for scheme://host and just host [1]. When one entry is returned, this is great because we can just update the hostname and use that credentials. Ez-Pz. The complex part is when we find duplicate logins. You mentioned that we should simply delete the partial hostname entry if a full entry is available but how do we know that the password associated with that one is the 'correct' one? There is a chance that the locally generated one is newer/correct than the full url one, no?

[1] https://github.com/mozilla/firefox-ios/commit/71ca814ae61fe46350f846fadeec9c4a6e56a46e#diff-58cadf5237e7721e748de06632b7ebceR43

Comment 12

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #11)
> You mentioned that we should simply delete the partial hostname
> entry if a full entry is available but how do we know that the password
> associated with that one is the 'correct' one? There is a chance that the
> locally generated one is newer/correct than the full url one, no?

Yes, exactly. I wrote:

--
Take extreme care: if the expanded URL exists in the DB already, simply delete this row, optionally updating the other row's password to match the one that worked.
--

The end result should be:

* The desktop row is preserved. It probably has the oldest GUID, and it's safest to keep the row that other clients are actively using.
* The one that used to have the malformed hostname is marked as deleted for Sync.
* The last password that successfully logged in is kept.

The usual update login call path will probably take care of the last… and in the soft migration case, it's only use of a login that will cause migration, so you're at the coal face at the right time.

There's a pretty good chance that both rows will have the same password, so in that case you can just drop the malformed one altogether.

Comment 13

[Stephan Leroux [:sleroux]]

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

I've gone ahead and finished the soft-migration path by:

1. Bubbling up information allow me to know which logins contain malformed URLS by adding the hasMalformedHostname flag. Unfortunately since malformed hostnames look default to http://, there is no way of telling the difference between a valid http:// scheme and a malformed one.

2. For duplicate entries, find the first entry that matches the protectionSpace and is not malformed. For malformed entries, trim them from the DB.

Comment 14

[Richard Newman [:rnewman]]

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

Comments left. A few tweaks and at least one bug. Needs a basic test!

Comment 15

[Stephan Leroux [:sleroux]]

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

Addressed PR comments, fixed the bug, and added unit tests for the various scenarios.

Comment 16

[Richard Newman [:rnewman]]

One main concern — out-of-range.

Comment 17

[Stephan Leroux [:sleroux]]

master 4e77934bf9081dbcc8675880b35f3eb3f23c2040