Only run IdentityCredentialStorageCleaner if FedCM is enabled
Categories
(Toolkit :: Data Sanitization, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox114 | --- | fixed |
People
(Reporter: pbz, Assigned: osuolale49, Mentored, NeedInfo)
References
Details
(Keywords: good-first-bug)
Attachments
(5 files)
dom.security.credentialmanagement.identity.enabled
is currently disabled by default. We should not run the IdentityCredentialStorageCleaner
if the feature is disabled. Currently we run it every time we perform a clean operation and it instantiates the IdentityCredentialStorageService
even if the feature is disabled.
The cleaner is implemented here: https://searchfox.org/mozilla-central/rev/e941c8a6d49b4f62f361446de7a80214fb899186/toolkit/components/cleardata/ClearDataService.sys.mjs#1421
Updated•1 year ago
|
Comment 1•1 year ago
|
||
Marking this as a good first bug!
What needs to be done is to check the dom.security.credentialmanagement.identity.enabled
pref before running the IdentityCredentialStorageCleaner
. This could ether be done by not matching the cleaner if the pref is not enabled so the cleaner is not executed (here) or simply checking for the pref in each method of the cleaner (here).
Updated•1 year ago
|
Assignee | ||
Comment 2•1 year ago
|
||
I am looking into this bug
Comment 3•1 year ago
|
||
@Noah Osuolale, thank you for looking into this! I will assign you to the bug, please needinfo me if you have any questions.
Assignee | ||
Comment 4•1 year ago
|
||
``
Assignee | ||
Comment 5•1 year ago
|
||
``
Assignee | ||
Comment 6•1 year ago
|
||
``
Assignee | ||
Comment 7•1 year ago
|
||
Assignee | ||
Comment 8•1 year ago
|
||
Please kindly check this implementation, in the toolkit/components/cleardata/ClearDataService.sys.mjs file to be sure if I am on good track.
https://paste.mozilla.org/TLMp1OwR
Reporter | ||
Updated•1 year ago
|
Comment 9•1 year ago
|
||
Hey Noah! Sorry for the delay. Thank you for your effort so far :)
Your code snipped is a good start, to be able to read the pref you need to ask our pref service to get it for you, you can see an example here. Basically the first argument that you pass in is the pref you want to read an the second argument is the default value that should be returned in case something goes wrong, see the signature of getBoolPref
here
Assignee | ||
Comment 10•1 year ago
|
||
Hi hpeuckmann,
After going through your latest comment, I came up with this, https://paste.mozilla.org/q9Jw1F23. Am I on good track?
Comment 11•1 year ago
|
||
Thanks for the update!
I would rather have the default value to be false
here. We should really only run the cleaner if the pref is true
.
If something goes wrong during our call to getBoolPref
we would execute the cleaner although the pref might be set to false. I'd rather under clear than over clear here.
Besides that your patch looks good to me. I'd suggest you go ahead and submit it for review, you can also submit your patch as a WIP patch first and request review later. When you request review, please put me in as a blocking reviewer.
Assignee | ||
Comment 12•1 year ago
|
||
Assignee | ||
Comment 13•1 year ago
|
||
Hi hpeuckmann,
The above patch is open for review. Thanks
Updated•1 year ago
|
Comment 14•1 year ago
|
||
Pushed by hpeuckmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3522f568e1a5 Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann
Comment 15•1 year ago
|
||
Backed out changeset 3522f568e1a5 (Bug 1825749) for causing xpcshell failures at est_identity_credential_storage.js
Backout: https://hg.mozilla.org/integration/autoland/rev/49224cf22355ff1b62edae340db171aaa82ec01b
Failure log: https://treeherder.mozilla.org/logviewer?job_id=412271654&repo=autoland&lineNumber=2933
Comment 16•1 year ago
|
||
Pushed by hpeuckmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e35b8568fd3f Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann
Comment 17•11 months ago
|
||
bugherder |
Description
•