Closed Bug 1729640 Opened 3 months ago Closed 1 month ago

Isolate sites that users are logged in or have passwords saved for using Android Fission

Categories

(Toolkit :: Password Manager, enhancement, P2)

Unspecified
Android
enhancement

Tracking

()

RESOLVED FIXED
96 Branch
Fission Milestone Future
Tracking Status
firefox-esr91 --- disabled
firefox94 --- disabled
firefox95 --- disabled
firefox96 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [fission:android:m3])

Attachments

(8 files)

In Bug 1723797, we added a pref to only isolate High-Value sites in the mobile platform.
This bug is used to track how we can identify High-Value sites with password-related heuristics.

Right now, the plan is identifying a High-Value site when the site meets any of the following conditions:

  1. The site has a saved login in the password manager
  2. The user has logged in to the site

For condition (2), it will be done by detecting when users filled-in their username/password in a site and then submit the login form.

Additional notes:

  • Login via OAuth is not covered in this bug.
Fission Milestone: --- → Future
OS: Unspecified → Android
Whiteboard: [fission:android:m3]
Depends on: 1723797
No longer depends on: 1723801

One issue we have to solve in the bug is that geckoview only supports fetching logins async, which means we can’t check whether a site has a saved login synchronously.

As discussed offline with Andrea, if synchronously checking whether a site has a saved login is not possible, one possible approach is checking saved logins BEFORE starting the load and then writing the permission to the permission manager when the result is returned. So when the load happens (ShouldIsolate func in Bug 1723797), the permission might be there. But I guess that also means we can’t guarantee if the result can come back in time.

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

  1. Support getAllLoginsAsync
  2. Be notified when a login is added or removed from geckoview.

Hi Agi, I would like to know your opinion on this and if you have any suggestions, really appreciated the help!

Flags: needinfo?(agi)

In general I would much rather keep checking logins asynchronously, most of GeckoView expects that things are done asynchronously and this would be a big departure from that.

(In reply to Dimi Lee [:dimi] from comment #1)

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

  1. Support getAllLoginsAsync

Maybe I'm missing something but it looks like we do support getAllLoginsAsync?

  1. Be notified when a login is added or removed from geckoview.

I would argue that maybe we don't need this? when new logins are added to GeckoView that would normally mean that a user logged in to a website and decided to save the credentials, AFAIK the only other way to add a login is to do it manually, but I would think it's OK if we don't catch manually-added logins for this? They will be caught in the next restart anyway (which happens very frequently on Android).

So my suggestion would be to fetch logins around startup to compile the list of "high value" websites and then ignore the ones added manually until the next session/restart.

Flags: needinfo?(agi)

(In reply to [PTO until 22nd] Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #2)

In general I would much rather keep checking logins asynchronously, most of GeckoView expects that things are done asynchronously and this would be a big departure from that.

(In reply to Dimi Lee [:dimi] from comment #1)

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

  1. Support getAllLoginsAsync

Maybe I'm missing something but it looks like we do support getAllLoginsAsync?

I guess not? It looks like the function either returns a empty list or throws Cr.NS_ERROR_NOT_IMPLEMENTED.

  1. Be notified when a login is added or removed from geckoview.

I would argue that maybe we don't need this? when new logins are added to GeckoView that would normally mean that a user logged in to a website and decided to save the credentials, AFAIK the only other way to add a login is to do it manually, but I would think it's OK if we don't catch manually-added logins for this? They will be caught in the next restart anyway (which happens very frequently on Android).

That is a good point! I agree that manually adding/removing logins is an edge case that we can ignore. Manually adding/removing logins rarely happens and Restart happens very frequently on Android. As for the case when a user logged in to a website (no matter they choose to save logins or not), theoretically, it will be caught by the form submission heuristic.

So my suggestion would be to fetch logins around startup to compile the list of "high value" websites and then ignore the ones added manually until the next session/restart.

Yes, I agree.

Flags: needinfo?(agi)

(In reply to Dimi Lee [:dimi] from comment #3)

(In reply to Dimi Lee [:dimi] from comment #1)

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

  1. Support getAllLoginsAsync

Maybe I'm missing something but it looks like we do support getAllLoginsAsync?

I guess not? It looks like the function either returns a empty list or throws Cr.NS_ERROR_NOT_IMPLEMENTED.

You're correct, I misread the code earlier. So yeah we can implement that as part of this bug I guess (leaving the ni open in case I missed something, I'll come back to it after my PTO).

Let me know if you need any more information from me :)

Flags: needinfo?(agi)

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #5)

Let me know if you need any more information from me :)

Hi Agi,
Yes, Since the plan is getting all logins while initialization, we'll need geckoview add an API to support fetching all logins.
Could someone in your team work on that?

Flags: needinfo?(agi)

Redirecting to etoop.

Flags: needinfo?(agi) → needinfo?(etoop)

This patch:

  1. adds two 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission
  2. moves 'AddHighValuePermission' from HttpBaseChannel to ProcessIsolation
    to support more high-value permission type.

The new 'searchAllLoginsAsync' API works the same as the getAllLoginsAsync API.
The purpose of adding the API is to make C++ caller easier to use.

Depends on D127101

Sites that match the following heuristic are considered high-value:

  1. Sites that have a password stored in the password manager
  2. Sites that we detect that users submit a form with a password

This patch adds LoginDetectionService to detect login attempts.
When LoginDetectionService finds a site that matches any of the above
heuristics, it adds the corresponding high-value permission to the
permission manager.

Depends on D127104

Right now, all high-value permissions have the same expiration time, but
we might want to set difference expiration time for different
high-value permission type in the future.

Depends on D127105

The preference should be enabled before rolling out "fission for authenticated sites"
to makes sure we have the permissions there when the feature is on.

Depends on D127106

Attachment #9243696 - Attachment description: Bug 1729640 - P3. Notify form submission event → Bug 1729640 - P3. Notify event when a form with password is submitted
Attachment #9243697 - Attachment description: Bug 1729640 - P4. Setting high-value permission for sites that are considered is logged in. → WIP: Bug 1729640 - P4. Setting high-value permission for sites that are considered is logged in.
Depends on: 1733423
Attachment #9243697 - Attachment description: WIP: Bug 1729640 - P4. Setting high-value permission for sites that are considered is logged in. → Bug 1729640 - P4. Setting high-value permission for sites that are considered is logged in.
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae9c5de3902a
P1. Extend AddHighValuePermission to support 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission. r=necko-reviewers,farre,kershaw
https://hg.mozilla.org/integration/autoland/rev/c7ffe85380d7
P2. Add a nsILoginManager API to fetch all saved logins asynchronously. r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/66376bee8cef
P3. Notify event when a form with password is submitted r=sfoster,tgiles,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/f44a8f37d84c
P4. Setting high-value permission for sites that are considered is logged in. r=farre,sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/43dd05680186
P5. Add a preference to control the expiration time of login related high-value permission. r=farre
https://hg.mozilla.org/integration/autoland/rev/9ec5346cf9d9
P6. Add a preference to enable setting `HighValueIsLoggedIn` permission when fission is off. r=farre
https://hg.mozilla.org/integration/autoland/rev/4d3532da5652
P7. Add test cases for isolating sites by login related heuristics r=farre
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7c50c85f911
P1. Extend AddHighValuePermission to support 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission. r=necko-reviewers,farre,kershaw
https://hg.mozilla.org/integration/autoland/rev/4ce2eebab0c4
P2. Add a nsILoginManager API to fetch all saved logins asynchronously. r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/fbd6928e34ef
P3. Notify event when a form with password is submitted r=sfoster,tgiles,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/b8fd0343ca42
P4. Setting high-value permission for sites that are considered is logged in. r=farre,sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/5ff8d3adc6eb
P5. Add a preference to control the expiration time of login related high-value permission. r=farre
https://hg.mozilla.org/integration/autoland/rev/fcd4b833ad73
P6. Add a preference to enable setting `HighValueIsLoggedIn` permission when fission is off. r=farre
https://hg.mozilla.org/integration/autoland/rev/5b2a1799ce4e
P7. Add test cases for isolating sites by login related heuristics r=farre
Flags: needinfo?(etoop)
Flags: needinfo?(dlee)
Regressions: 1739431

Backed out for causing permafailures on browser_web_process_isolation.js

[task 2021-11-04T20:29:58.922Z] 20:29:58     INFO - TEST-PASS | dom/ipc/tests/browser_web_process_isolation.js | Frame org_after_isLoggedIn[0][0] has the expected number of children - 
[task 2021-11-04T20:29:58.922Z] 20:29:58     INFO - Leaving test bound test_isolate_high_value
[task 2021-11-04T20:29:58.924Z] 20:29:58     INFO - Buffered messages finished
[task 2021-11-04T20:29:58.925Z] 20:29:58     INFO - TEST-UNEXPECTED-FAIL | dom/ipc/tests/browser_web_process_isolation.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - 
[task 2021-11-04T20:29:58.926Z] 20:29:58     INFO - GECKO(8951) | MEMORY STAT | vsize 3088MB | residentFast 445MB | heapAllocated 192MB
[task 2021-11-04T20:29:58.927Z] 20:29:58     INFO - TEST-OK | dom/ipc/tests/browser_web_process_isolation.js | took 113992ms
Status: RESOLVED → REOPENED
Flags: needinfo?(dlee)
Resolution: FIXED → ---
Target Milestone: 96 Branch → ---

Open the test tab takes some time in this testcase
(https://searchfox.org/mozilla-central/rev/d2f8488b6a704443a5c5bfc6d2878171b5f0d393/dom/ipc/tests/browser_web_process_isolation.js#31-43)
, and we open the tab multiple times in this testcase.

Since we add 2 more test scenarios for the testcase, this testcase need more time to finish.

Flags: needinfo?(dlee)
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8ab662198b7
P1. Extend AddHighValuePermission to support 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission. r=necko-reviewers,farre,kershaw
https://hg.mozilla.org/integration/autoland/rev/c1a757f547b9
P2. Add a nsILoginManager API to fetch all saved logins asynchronously. r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/a765b54eec8c
P3. Notify event when a form with password is submitted r=sfoster,tgiles,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/11e52a262096
P4. Setting high-value permission for sites that are considered is logged in. r=farre,sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/ea5fc06ac247
P5. Add a preference to control the expiration time of login related high-value permission. r=farre
https://hg.mozilla.org/integration/autoland/rev/c05cc9249ca5
P6. Add a preference to enable setting `HighValueIsLoggedIn` permission when fission is off. r=farre
https://hg.mozilla.org/integration/autoland/rev/767daf581e65
P7. Add test cases for isolating sites by login related heuristics r=farre
https://hg.mozilla.org/integration/autoland/rev/8a0a41762955
P8. Request longer timeout for browser_web_process_isolation test. r=farre
See Also: → 1737669

Setting status-firefox95=disabled. We don't need to uplift this fix to Beta 95 because Android Fission is not enabled yet.

Blocks: 1740226
Blocks: 1741610
Blocks: 1741895
No longer blocks: 1738752
Depends on: 1738752
You need to log in before you can comment on or make changes to this bug.