Closed
Bug 1363012
Opened 8 years ago
Closed 6 years ago
Implement browsingData.removePasswords WebExtension API method on android
Categories
(WebExtensions :: Android, enhancement, P5)
WebExtensions
Android
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: shatur, Assigned: shatur)
References
(Depends on 1 open bug)
Details
(Whiteboard: [triaged])
Attachments
(1 file)
This method clears saved passwords. The doc can be found at [1].
[1]. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removePasswords
Updated•8 years ago
|
Priority: -- → P5
Whiteboard: [triaged]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8879759 [details]
Bug 1363012 - Implement browsingData.removePasswords WebExtension API method on android.
https://reviewboard.mozilla.org/r/151114/#review157348
r-, because I think this should be done in a safer fashion.
::: mobile/android/components/extensions/ext-browsingData.js:14
(Diff revision 1)
> XPCOMUtils.defineLazyModuleGetter(this, "SharedPreferences",
> "resource://gre/modules/SharedPreferences.jsm");
>
> +const YIELD_PERIOD = 10;
> +
> +let clearPasswords = async function(options) {
Why are the implementions in your different PRs spread amongst this file and Sanitizer.jsm? That seems wrong to me. Is it possible to consolidate code that actually does the work in one location?
Here especially, since there's already "clear passwords" method in Sanitizer.jsm.
::: mobile/android/components/extensions/ext-browsingData.js:21
(Diff revision 1)
> + let yieldCounter = 0;
> +
> + if (options.since) {
> + // Iterate through the logins and delete any updated after our cutoff.
> + let logins = loginManager.getAllLogins();
> + for (let login of logins) {
We're backed by SQL, and so you could be doing this in a safer way, as a single transaction, especially considering the fact that the underlying API will also need to store deletions in a separate table.
Unfortunately, our current APIs don't let you perfrom this safely:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-mozStorage.js#283
While we want to move ownership of these databases to Android side, meaning that at some point this code will hopefully die, it shouldn't be too much effort to provide a transactional API for these compound operations in the meantime.
We have two storage implementations for login information, which share the same interface: storage-mozStorage (SQL-backed, used by Android), and storage-json (json-backed, used by desktop).
The two implementations have obviously different semantics and consistency guarantees. Change to nsILoginManager propsed in 1332611 will allow us to actually do this safely.
Attachment #8879759 -
Flags: review?(gkruglov) → review-
Comment 3•7 years ago
|
||
Comment on attachment 8879759 [details]
Bug 1363012 - Implement browsingData.removePasswords WebExtension API method on android.
Moving review to Shane because I have a lot on my plate right now - sorry for not doing this earlier.
Attachment #8879759 -
Flags: review?(mwein) → review?(mixedpuppy)
Updated•7 years ago
|
Attachment #8879759 -
Flags: review?(bob.silverberg)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8879759 [details]
Bug 1363012 - Implement browsingData.removePasswords WebExtension API method on android.
https://reviewboard.mozilla.org/r/151114/#review161296
Nice work, Tushar. I'm fine with the WebExtensions code, except for the issue about the implementation of `clearPasswords`.
::: mobile/android/components/extensions/ext-browsingData.js:14
(Diff revision 1)
> XPCOMUtils.defineLazyModuleGetter(this, "SharedPreferences",
> "resource://gre/modules/SharedPreferences.jsm");
>
> +const YIELD_PERIOD = 10;
> +
> +let clearPasswords = async function(options) {
I agree.
Attachment #8879759 -
Flags: review?(bob.silverberg) → review-
Updated•7 years ago
|
Attachment #8879759 -
Flags: review?(mixedpuppy)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 5•6 years ago
|
||
Hey Sebastian, apologies for the late ping. Could you advise next steps for this?
Flags: needinfo?(s.kaspari)
Updated•6 years ago
|
Mentor: s.kaspari
Flags: needinfo?(s.kaspari)
Comment 6•6 years ago
|
||
> Hey Sebastian, apologies for the late ping. Could you advise next steps for this?
Oh, good question.
I just un-assigned me as mentor. Back then I was mentoring a Google Summer of Code student who was working on adding WebExtension APIs to Firefox for Android.
I think there are no clear steps forward. Given that Fennec is in "maintenance mode" I'd recommend WONTFIX-ing this bug.
Comment 7•6 years ago
|
||
Woks for me! Thanks, Sebastian.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•