chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 #552

himanshusinghs · 2023-07-04T11:40:48Z

Description

In this PR, we added a migration step to migrate secrets one by one, earlier stored using keytar to VsCode's SecretStorage. The pseudo-code followed here is the following:

## Migration process
- Extension activates
- Initiate a call to fetch saved connections
  - Get all the connections from our connection storage
  - For each connection
    - perform the old connection modal to new connection info migration
    - return connection info merged with secrets
      - Check if we have `secretStorageLocation` key set in root of connection info object
      - Yes? (We attempted a migration already for this connection)
        - Is `secretStorageLocation` === `vscode.Keytar`? (We tried migrating but failed, keytar was unavailable)
          - Return the connection info without merging the secrets
        - Is `secretStorageLocation` === `vscode.SecretStorage`?
          - Fetch the secret from `vscode.SecretStorage`
          - Merge it with connection info and return the merged connection info
      - No? (The connection is yet to be migrated)
        - Check if keytar is available
        - No?
          - Set `secretStorageLocation` key to `vscode.Keytar`
          - Log as migration error that keytar is unavailable
          - Return the connection info as it is
        - Yes?
          - Check if credentials are in keytar
          - Yes?
            - Copy the credentials over to `vscode.SecretStorage`
            - Set `secretStorageLocation` key on connection info to `vscode.SecretStorage`
            - Return connection info merged with retrieved secrets
          - No?
            - Set `secretStorageLocation` key to `vscode.SecretStorageLocation`
            - Save '{}' to vscode.SecretStorageLocation for this connection
            - Return connection info merged with empty object for secret
  - Check if there were any connections that we could not migrate and notify the user of them and also track the telemetry event

If we fail to migrate for any connection, we notify the user using vscode's information message API. An example here:

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Dependents

Caused by: #546
Resolves: #476

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

…t storage

src/connectionController.ts

src/test/suite/connectionController.test.ts

src/storage/storageController.ts

src/connectionController.ts

addaleax · 2023-07-04T12:37:41Z

src/connectionController.ts

+          connectionsWithSecretsInKeytar
+            .map(({ connectionName }) => connectionName)
+            .join(', '),
+        ].join('\n')


Do we have any intuition about what the cause of this error would typically be in practice? What do we expect the user to do about this? Is there some kind of error message that we could bubble up to the user or a log that we could point them to?

This one in particular will happen when the keytar shim will no longer be available for us to use(in the next public release of vscode). This is not actually an error but an error like condition which we plan to notify the users about using the vscode.showInformationMessage. Unfortunately the only action users can take about it is to remove the connection from the explorer view and add it again manually

We are not explicitly stating the same in the error message because I was not sure about it but I do think that it might be ok to call that out specifically in the message that we show. Iff we decide to show the message to remove the connection, then we might even just provide users a small button to purge connections that we could not migrate (because of keytar not available anymore).

The message here could be more descriptive saying that keytar is being removed from vscode and we tried to migrate connections, but some failed. @GaurabAryal, maybe the product team could help with better phrasing. I think this is important to explain what happened and the alternatives users have. I am not sure if listing connections by names will work well if users have many saved connections. Some of those can have duplicate default names like localhost. This is why my suggestion was to show a broken leaf icon for such connections to easier identify those for users and let them take corresponding action. For this PR the improved message would be enough, but maybe it would be also nice to have s follow-up PR with an improved UX.

Yea @GaurabAryal is in touch with designers to have the broken leaf prepared but until then maybe I will sit with him for a better copy.

PR updated with the approved copy - here

src/connectionController.ts

addaleax · 2023-07-04T12:43:13Z

src/connectionController.ts


+      // We tried migrating this connection earlier but failed because Keytar was not
+      // available. So we return simply the connection without secrets.


Could this happen when the user clicks away a request to unlock their OS keyring because they are repetitive/annoying if they have multiple connections stored? In that case it would be reasonable to assume that a subsequent attempt to get credentials would succeed, right?

Yea, for such connections it would be safe to assume that they can be re-tried for migration.

There is one thing not so much clear to me - how / when would a user get a prompt to unlock the keyring? Asking this because it did not happen for me? Could be because I have been using the extension for sometime and might have allowed it at some point or could it be because keytar module does not prompt?

could it be because keytar module does not prompt?

keytar doesn’t do the prompting as far as I know, it integrates with the OS keychains instead, so when exactly the prompt occurs will depend on the OS and its configuration.

(I think we get prompts on macOS when upgrading Electron in Compass, for example. I’m not sure if something similar happens with VSCode at some point.)

It happened before for vscode as well but does not happen anymore for a year or so. Not sure if we did something or vscode handles it differently. @Anemy do you have any context about it?

In that case - I would assume that the keytar module will still be available (unless it was removed) but the getPassword call should fail which should throw an error. For this reason we will never mark such connection as migrated and thus they should be good for re-trying. But I should to test that.

There's only a prompt for the first time the credential store is to be used for an app (only for some os's that are more stringent with permissions). If something with how the app is built (maybe bundle name? don't remember off the top of my head) changes then this also happens. When we update the electron version in compass-dev we see the prompts as the identifier changes then. As Alena mentioned it happened once around two years ago for VSCode which caused users to see this prompt again. I don't think we need to worry much about that, I don't see it as a bad thing to account for though.
As Anna said, the prompt is from the OS's credential store. Maybe the user could have configured it in a way where they get a prompt each time it's requested.

https://github.com/mongodb-js/compass/blob/main/packages/hadron-build/lib/target.js

Understood and thank you for the explanation here @Anemy
In that case, we can still safely assume that keytar will be available to our extension, it is just that the calls to get secret would fail (if the user did not unlock or took too long to unlock), right?
If so then I would particularly account for that failure (originating from keytar.getPassword).

As of now, these errors would just bubble up and eventually get caught in _getConnectionInfoWithSecret and the connection will not be marked as migrated (ready to be re-attempted for migration).

I just addressed this in my commit here. The connections which will fail migration for anything other than keytar not available are not marked as migrated and hence can be re-attempted again in the next load.

There is a kinda bad side effect of this which is - such connections (for which our _migrateConnectionWithKeytarSecrets rejected) will not appear in our Connection tree when the extension loads. I was also able to confirm from the following code that this is an existing behaviour.

I have created this ticket to define and agree upon a behaviour of "loading of the connections" when we are not able to fetch the secrets (for whichever reason) for some of the connections. Please let me know if this is something we should absolutely fix within this PR.

src/connectionController.ts

…bove the keytar secret removal

…criptive

…nge event

… only for failed migrations in the current run

src/test/suite/connectionController.test.ts

…that no notification / tracking was sent if the failed migrations are not from current load of extension

alenakhineika · 2023-07-05T12:32:03Z

src/storage/storageController.ts

+export type SecretStorageLocationType =
+  | typeof SecretStorageLocation.Keytar
+  | typeof SecretStorageLocation.SecretStorage;
+


Could it be just export type SecretStorageLocationType = 'vscode.Keytar' | 'vscode.SecretStorage';? Do we need at all SecretStorageLocation? cc @addaleax

Yea I think it could be. I probably did it this way just so I also have a string const to use in places where the types are not available from the context (for example tests and a couple other places in the code as well where we declare a connection info like object with secret storage).

Anemy

Looking good, left a question on keytar handling. Also +1 on updating the text on the info message we show for those cases where things can't be loaded.

src/connectionController.ts

alenakhineika · 2023-07-05T12:49:01Z

I feel like the PR is good to be merged when the text message we show to users in case of migration failure is updated 👍 great job! 👏

…er connections

chore: added migration step to migrate keytar secrets to vscode secre…

5f85bb7

…t storage

himanshusinghs requested a review from alenakhineika July 4, 2023 11:44

prettier reformatting

7e0e4c7

alenakhineika reviewed Jul 4, 2023

View reviewed changes

src/connectionController.ts Outdated Show resolved Hide resolved

addaleax reviewed Jul 4, 2023

View reviewed changes

alenakhineika reviewed Jul 4, 2023

View reviewed changes

src/connectionController.ts Outdated Show resolved Hide resolved

alenakhineika reviewed Jul 4, 2023

View reviewed changes

src/connectionController.ts Outdated Show resolved Hide resolved

himanshusinghs added 10 commits July 4, 2023 15:42

PR feedback - better variable

f624554

PR feedback - replaces usage of assert with assert.strictEqual

dcd2a11

PR feedback - reposition removing secrets from vscode.SecretStorage a…

6c67419

…bove the keytar secret removal

PR feedback - replace {} with Object.create(null)

eea1fcc

PR feedback - renamed multiple migration method to make them more des…

2970158

…criptive

PR feedback - remove unnecessary type aliasing

b83d44b

PR feedback - better error description:

0742e4b

PR feedback - remove unnecessary variable to check for connection cha…

cdd9018

…nge event

PR feedback - use type unions instead of enums

e6ea5bf

PR feedback - parallelise connection secrets fetch and track / notify…

1249543

… only for failed migrations in the current run

alenakhineika reviewed Jul 5, 2023

View reviewed changes

src/test/suite/connectionController.test.ts Outdated Show resolved Hide resolved

alenakhineika reviewed Jul 5, 2023

View reviewed changes

src/test/suite/connectionController.test.ts Show resolved Hide resolved

PR feedback - changed misleading comment, added a test for verifying …

36e17c0

…that no notification / tracking was sent if the failed migrations are not from current load of extension

himanshusinghs force-pushed the VSCODE-435-keytar-secrets-migration branch from 24bfe9e to 36e17c0 Compare July 5, 2023 12:21

alenakhineika reviewed Jul 5, 2023

View reviewed changes

Anemy reviewed Jul 5, 2023

View reviewed changes

src/connectionController.ts Outdated Show resolved Hide resolved

src/connectionController.ts Show resolved Hide resolved

alenakhineika reviewed Jul 5, 2023

View reviewed changes

src/connectionController.ts Outdated Show resolved Hide resolved

PR feedback - catch possible errors from keytar.deletePassword

808e60e

himanshusinghs force-pushed the VSCODE-435-keytar-secrets-migration branch from e65ab1b to 4388609 Compare July 5, 2023 17:15

ensures that failure of keytar migration do not affect loading of oth…

4250dd6

…er connections

himanshusinghs force-pushed the VSCODE-435-keytar-secrets-migration branch from 4388609 to 4250dd6 Compare July 5, 2023 17:31

lint fixes

ee19bb1

PR feedback - updated the copy of the message for failed migration

8cca2be

alenakhineika approved these changes Jul 6, 2023

View reviewed changes

himanshusinghs merged commit c2b23a1 into main Jul 6, 2023

himanshusinghs deleted the VSCODE-435-keytar-secrets-migration branch July 6, 2023 11:40

himanshusinghs linked an issue Jul 14, 2023 that may be closed by this pull request

Keytar Migration Plan #546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 #552

chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 #552

himanshusinghs commented Jul 4, 2023 •

edited by alenakhineika

Loading

addaleax Jul 4, 2023

himanshusinghs Jul 4, 2023

alenakhineika Jul 4, 2023

himanshusinghs Jul 5, 2023

himanshusinghs Jul 6, 2023 •

edited

Loading

addaleax Jul 4, 2023

himanshusinghs Jul 4, 2023

addaleax Jul 4, 2023

alenakhineika Jul 4, 2023

himanshusinghs Jul 5, 2023

Anemy Jul 5, 2023

himanshusinghs Jul 5, 2023

alenakhineika Jul 5, 2023

himanshusinghs Jul 5, 2023

alenakhineika Jul 5, 2023

himanshusinghs Jul 5, 2023

Anemy left a comment

alenakhineika commented Jul 5, 2023


		// We tried migrating this connection earlier but failed because Keytar was not
		// available. So we return simply the connection without secrets.

chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 #552

chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 #552

Conversation

himanshusinghs commented Jul 4, 2023 • edited by alenakhineika Loading

Description

Checklist

Motivation and Context

Dependents

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshusinghs Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anemy left a comment

Choose a reason for hiding this comment

alenakhineika commented Jul 5, 2023

himanshusinghs commented Jul 4, 2023 •

edited by alenakhineika

Loading

himanshusinghs Jul 6, 2023 •

edited

Loading