1825749 - Only run IdentityCredentialStorageCleaner if FedCM is enabled

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect, P3)

Description

[Paul Zühlcke [:pbz]]

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

Comment 1

[Hannah Peuckmann]

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).

Comment 2

[Noah Osuolale]

I am looking into this bug

Comment 3

[Hannah Peuckmann]

@Noah Osuolale, thank you for looking into this! I will assign you to the bug, please needinfo me if you have any questions.

Comment 4

[Noah Osuolale]

Attachment: code snippet

``

Comment 5

[Noah Osuolale]

Attachment: code snippet

``

Comment 6

[Noah Osuolale]

Attachment: code snippet

``

Comment 7

[Noah Osuolale]

Attachment: code snippet

Comment 8

[Noah Osuolale]

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

Comment 9

[Hannah Peuckmann]

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

Comment 10

[Noah Osuolale]

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

[Hannah Peuckmann]

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.

Comment 12

[Noah Osuolale]

Attachment: Bug 1825749 - Only run IdentityCredentialStorageCleaner if FedCM is enabled. r?hpeuckmann

Comment 13

[Noah Osuolale]

Hi hpeuckmann,
The above patch is open for review. Thanks

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3522f568e1a5
Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann

Comment 15

[Cristina Horotan [:chorotan]]

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 push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=3522f568e1a50281479313fc1dbc13c28524c581&selectedTaskRun=PVbciZ-5SUqnyvKh7--bgg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=412271654&repo=autoland&lineNumber=2933

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e35b8568fd3f
Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/e35b8568fd3f