Page MenuHomePhabricator

Bug 1693878 - Add websites-with-shared-credential-backends dump to tree. r=leplatrem
ClosedPublic

Authored by tgiles on Jan 26 2021, 10:00 PM.

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tgiles created this revision.
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Jan 26 2021, 10:00 PM
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
tgiles updated this revision to Diff 391167.

Fix failing tests due to new autocomplete behavior and missing await keyword on mocked remote settings DB

tgiles requested review of this revision.Feb 5 2021, 9:30 PM
tgiles added a reviewer: leplatrem.

You should publish this data on the server first and use curl to save it in the tree, in order to keep timestamps and all.

Alternatively, you can add an empty dump {data:[]} and let the scheduled job to update the file in-tree automatically when you will have added records on the server.

This revision now requires changes to proceed.Feb 8 2021, 1:39 PM
tgiles updated this revision to Diff 397601.

Update websites-with-shared-credential-backends dump via curled records on prod server

tgiles updated this revision to Diff 399503.

Add tests for related realms, remove debug code

This revision is now accepted and ready to land.Feb 17 2021, 3:56 PM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other.

tgiles updated this revision to Diff 401714.

Fix duplicated ebay.com login case where the same ebay login would appear twice in the autocomplete dropdown. Add related realms checking to isOriginMatching in LoginHelper. Extended searchAndDedupeLogins to use related realms in LoginManagerParent. Added logic to _searchLogins in storage-json to utilize related realms. Remove searchRelatedLogins. Change find related realms comparison logic to prevent 'evil-ebay.com' matching 'ebay.com'. Add note aboute the nested array structure of the related realms collection. Add unit test for findRelatedRealms. Need to add logic so that the URLs from the related realms collection are appended with the formOrigin scheme only when the ID of the collection changes or when the formOrigin itself changes.

tgiles retitled this revision from Bug 1120684 - Add websites-with-shared-credential-backends dump to tree. r=leplatrem to Bug 1687996 - Add websites-with-shared-credential-backends dump to tree. r=leplatrem.
tgiles changed the Bugzilla Bug ID from 1120684 to 1687996.

Separate adding data to remote settings into separate bug so that it can land without the downstream changes affecting the patch

This revision is now accepted and ready to land.Feb 19 2021, 7:20 PM

Sorry for the duplicate request, I'm still figure out how to handle updating one patch in a stack without affecting all the patches in said stack. I went ahead and created a different bug to associate this work with so I can land it and not worry about affecting the status of this patch anymore

This revision is now accepted and ready to land.Feb 19 2021, 7:23 PM
tgiles updated this revision to Diff 402166.
tgiles retitled this revision from Bug 1687996 - Add websites-with-shared-credential-backends dump to tree. r=leplatrem to Bug 1693878 - Add websites-with-shared-credential-backends dump to tree. r=leplatrem.
tgiles changed the Bugzilla Bug ID from 1687996 to 1693878.

Ignore this one as well, put in the wrong bug to associate the patch with 🙃

This revision is now accepted and ready to land.Feb 19 2021, 7:26 PM
tjr added inline comments.
services/settings/dumps/main/websites-with-shared-credential-backends.json
1

Do you think we could have this file pretty-printed in-tree to allow easier analysis?

services/settings/dumps/main/websites-with-shared-credential-backends.json
1

I'm not sure what we gain from pretty-printing in tree, since any updates from Remote Settings will override the pretty print by default (AFAIK). All the analysis of this data should occur during the review request at the Remote Settings level.

In an ideal world, we could easily be able to split this data into separate records so that we get the incremental sync for free from Remote Settings...which would also greatly simplify the review process. However we made the decision to ship the data like this for ease of implementation. Maybe this warrants an investigation to see if we can transform the data structure when we add the data to Remote Settings.