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

Option to disable keychain credentials #2579

Open
wants to merge 3 commits into
base: sagerb-remove-unused-command-code
Choose a base branch
from

Conversation

sagerb
Copy link
Collaborator

@sagerb sagerb commented Jan 31, 2025

Intent

This PR introduces a configuration option to disable the use of the KeyRing for credential storage and instead use the file system. This is extremely useful for development as well as a good escape hatch for customers who may not have a working KeyRing environment (or preference there-towards).

Resolves #2519

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Approach

I first added a new configuration option for the publisher extension, which is enabled by default:
2025-01-31 at 11 23 AM

I then added a corresponding command line option for the agent. This was a more straightforward change than passing the option for every credential-related call. The impact of this decision is that the user must restart the extension after changing the configuration setting. Since this won't be very common, I believe this was a reasonable optimization to keep the complexity low.

$(just executable-path) ui -h
Usage: publisher ui [<path>] [flags]

Arguments:
  [<path>]    Sets the current working directory for the agent.

Flags:
  -h, --help                  Show context-sensitive help.
  -v, --verbose               Enable verbose logging. Use -vv or --verbose=2 for debug logging.

      --listen=HOST[:PORT]    Network address to listen on.
      --use-keychain          Use Keychain services to store/manage credentials.

After those two changes were in place, I added a helper function within the extension to access the setting, then updated the service spawning code to reflect the current configuration value. The agent then stores that value and each time the agent's credential code initializes, it either bypasses or attempts to use the KeyChain dependency.

The agent's debug launch profile has been updated to support launching the agent with the command line set. I set the initial value to false as I found myself wanting to debug using the file-based credentials more often than the KeyChain alternative. If this is not desired by the developer, they can easily change it within their own config.

User Impact

New feature, but default maintains the behavior they had previously.

Automated Tests

Unit tests added to validate the desired behavior.

Directions for Reviewers

Launching a debug version of the extension from your VSCode instance for the extension, will work for verification. The configuration value is respected and maintained after being set, when you then reload the window with Developer: Reload Window. I'd suggest verification of the functionality by:

  1. Confirm the configuration setting for the extension is set to enabled (checked). If have to change it, then reload the VSCode window
  2. Activate the publisher extension and view the credentials. You should be able to confirm that your existing credentials is listed.
  3. Change the configuration setting to disabled and reload the window.
  4. Activate the publisher extension and view the credentials. You should either have none or whatever you had stored within the ~.connect-credentials file.
  5. Add a new credential and make sure it shows up in your credential list
  6. Reload the window again and make sure the credentials have remained.
  7. Change the configuration setting back to enabled and reload the window
  8. You should be able to validate that the original KeyChain based credentials are now shown.

Checklist

@sagerb sagerb self-assigned this Jan 31, 2025
Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

It looks like this was branched off of the CLI removal. Can we change the base, or rebase this to isolate just the keychain credential changes? That will probably get CI passing again.

@sagerb sagerb changed the base branch from main to sagerb-remove-unused-command-code January 31, 2025 22:37
@sagerb
Copy link
Collaborator Author

sagerb commented Jan 31, 2025

It looks like this was branched off of the CLI removal. Can we change the base, or rebase this to isolate just the keychain credential changes? That will probably get CI passing again.

Oh yes, sorry about that. I've rebased the diff and updated from that updated branch as well. Thanks!

@sagerb sagerb requested a review from dotNomad January 31, 2025 22:38
Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

This is very clean. I just had a suggestion to trim up the description of the setting, and make it more similar to other VS Code setting descriptions.

@sagerb sagerb added the blocked Blocked on something label Feb 13, 2025
@sagerb
Copy link
Collaborator Author

sagerb commented Feb 13, 2025

To get CI to pass, we have to merge #2584 first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked on something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add configuration option to prioritize use of credential file over keyring
2 participants