Closed Bug 1825749 Opened 1 year ago Closed 1 year ago

Only run IdentityCredentialStorageCleaner if FedCM is enabled

Categories

(Toolkit :: Data Sanitization, defect, P3)

defect

Tracking

()

RESOLVED FIXED
114 Branch
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

Priority: -- → P3

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

Mentor: hpeuckmann
Keywords: good-first-bug

I am looking into this bug

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

Assignee: nobody → osuolale49
Attached file code snippet
``
Attached file code snippet
``
Attached file code snippet
``
Attached file code snippet

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

Flags: needinfo?(hpeuckmann)

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

Flags: needinfo?(hpeuckmann)

Hi hpeuckmann,
After going through your latest comment, I came up with this, https://paste.mozilla.org/q9Jw1F23. Am I on good track?

Flags: needinfo?(hpeuckmann)

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.

Flags: needinfo?(hpeuckmann)

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

Attachment #9327121 - Attachment description: WIP: Bug 1825749 - Only run IdentityCredentialStorageCleaner if FedCM is enabled. r?hpeuckmann → Bug 1825749 - Only run IdentityCredentialStorageCleaner if FedCM is enabled. r?hpeuckmann
Pushed by hpeuckmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3522f568e1a5
Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann
Pushed by hpeuckmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e35b8568fd3f
Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: