Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to support importing passwords from Google as CSV #4601

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

CDRussell
Copy link
Member

@CDRussell CDRussell commented May 30, 2024

Task/Issue URL: https://app.asana.com/0/608920331025315/1207415218447811/f

Description

Adds ability to import passwords exported as a CSV from Google Password Manager (GPM). This functionality is used by the higher up branches in the stack to provide a guided way to import from GPM, but can be tested directly from autofill dev settings and choosing to import a CSV file.

Test CSV files are available in Example CSV files for testing

Steps to test this PR

Happy path: all unique

  • Fresh install internal build type, and launch autofill dev settings
  • Choose Import CSV
  • Choose CSV file containing only unique entries and verify all are imported (e.g., choose example_csv_20.csv and make sure 20 are imported)

Happy path: importing with some duplicates in the list

  • Delete all saved credentials
  • Choose Import CSV again, and this time choose file containing duplicates
  • verify correct amount imported (e.g., choose dupes_20_total_17_unique.csv and make sure 17 are imported)

Happy path: importing with a duplicate already saved in the DB

  • Add a credential that matches what is in the CSV you will export
  • Import the CSV
  • Verify the correct amount imported and the already-saved one wasn't duplicated

Non-CSV files

  • Choose Import CSV
  • Pick a file that isn't a password CSV export
  • Make sure it handles this gracefully

Copy link
Member Author

CDRussell commented May 30, 2024

@malmstein
Copy link
Contributor

The Purge!

@malmstein malmstein closed this Oct 11, 2024
@CDRussell CDRussell reopened this Oct 11, 2024
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from 845c86d to 893a6a5 Compare October 14, 2024 13:44
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 14994b6 to e2de18a Compare October 14, 2024 13:44
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from 893a6a5 to 312493c Compare October 21, 2024 11:34
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch 2 times, most recently from 32750c3 to 7513429 Compare October 21, 2024 12:57
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from 312493c to 81b43d5 Compare October 21, 2024 16:48
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch 3 times, most recently from cac73ac to bb779c2 Compare October 22, 2024 11:30
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from 81b43d5 to b8de40d Compare October 22, 2024 11:33
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch 2 times, most recently from f891e83 to 65bfb74 Compare October 22, 2024 12:21
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from b8de40d to 2fd61e5 Compare October 22, 2024 12:23
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 65bfb74 to 7844c97 Compare October 22, 2024 12:23
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from 2fd61e5 to 96b165b Compare October 22, 2024 12:29
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 7844c97 to 27edd79 Compare October 22, 2024 12:29
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from 96b165b to b1d1ab9 Compare October 22, 2024 12:31
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 27edd79 to f7b551c Compare October 22, 2024 12:32
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch 2 times, most recently from f66114d to 556da80 Compare November 6, 2024 18:13
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from b52488e to 8c7ae28 Compare November 8, 2024 10:52
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch 4 times, most recently from 06d2c9a to 079b1fa Compare November 11, 2024 10:41
@CDRussell CDRussell force-pushed the feature/craig/autofill_import_via_gcm_feature_flag branch from 8c7ae28 to 5256082 Compare November 11, 2024 11:18
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 079b1fa to 2d3d4f5 Compare November 11, 2024 11:19
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch 3 times, most recently from c404a89 to 136d9b2 Compare November 11, 2024 12:08
@CDRussell CDRussell marked this pull request as ready for review November 11, 2024 12:12
@CDRussell CDRussell changed the base branch from feature/craig/autofill_import_via_gcm_feature_flag to graphite-base/4601 November 11, 2024 12:28
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 136d9b2 to c3bdb8c Compare November 11, 2024 12:29
@CDRussell CDRussell changed the base branch from graphite-base/4601 to develop November 11, 2024 12:29
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch 2 times, most recently from 43060e6 to 1f65e50 Compare November 11, 2024 13:19
}

@Test
fun whenDomainCannotBeNormalizedThenIsIncludedUnmodified() = runTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this one 😅. If we don't know how to normalize the input, we just return the input. But then we store it as domain attribute...but that can be anything. 🤔

You have more context than me on Autofill, but what are the side effects of not having a domain in the domain attribute? I assume we are not validating the user manually enters something valid either...so at this point, I wonder why do we even normalize, or why do we even filter the "android://..." objects...

Short term, just to consider if this is the expected behavior, and then we can chat about the class fields without blocking the changes here as it seems a more broad discussion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior mirrors what we do when saving credentials. If we can, we normalize the URLs when we encounter them, or when the user enters them manually (a product decision from way back). If the URL is unrecongizable as a URL and therefore we can't normalize it (when saving from autofill or from a user entering manually), we save it unaltered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are the side effects of not having a domain in the domain attribute

if it's unrecognizable, we won't be able to match on it and it won't be offered as an option in the autofill dialogs but will still be stored in the full credential list

Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. Let's discuss on zoom the models being used here, so I can have more clarity and decide next steps.

Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as I don't feel this is a blocker, but let's revisit together.

@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 1f65e50 to 8ddadf3 Compare November 12, 2024 16:51
@CDRussell CDRussell force-pushed the feature/craig/autofill_csv_import branch from 8ddadf3 to d2141a9 Compare November 13, 2024 14:26
@CDRussell CDRussell merged commit c5a59ad into develop Nov 13, 2024
6 checks passed
@CDRussell CDRussell deleted the feature/craig/autofill_csv_import branch November 13, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants