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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"request": "launch",
"mode": "auto",
"program": "${workspaceFolder}/cmd/publisher/main.go",
"args": ["ui", "--listen=localhost:9001", "-vv"],
"args": ["ui", "--listen=localhost:9001", "-vv", "--use-keychain=false"],
"cwd": "${workspaceFolder}/test/sample-content"
}
]
Expand Down
8 changes: 6 additions & 2 deletions cmd/publisher/commands/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@ package commands

import (
"github.com/posit-dev/publisher/internal/cli_types"
"github.com/posit-dev/publisher/internal/credentials"
"github.com/posit-dev/publisher/internal/events"
"github.com/posit-dev/publisher/internal/services/api"
"github.com/posit-dev/publisher/internal/util"
"github.com/r3labs/sse/v2"
)

type UICmd struct {
Path util.Path `help:"Sets the current working directory for the agent." arg:"" default:"."`
Listen string `help:"Network address to listen on." placeholder:"HOST[:PORT]" default:"localhost:0"`
Path util.Path `help:"Sets the current working directory for the agent." arg:"" default:"."`
Listen string `help:"Network address to listen on." placeholder:"HOST[:PORT]" default:"localhost:0"`
UseKeychain bool `help:"Use Keychain services to store/manage credentials." default:"true"`
}

func (cmd *UICmd) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContext) error {
Expand All @@ -31,6 +33,8 @@ func (cmd *UICmd) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContext) err
return err
}

credentials.UseKeychain = cmd.UseKeychain

// Auto-initialize if needed. This will be replaced by an API call from the UI
// for better error handling and startup performance.
svc := api.NewService(
Expand Down
5 changes: 5 additions & 0 deletions extensions/vscode/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@
"markdownDescription": "Verify TLS certificates for connections. Only **disable** this setting if you experience certificate verification errors and the Posit Connect server is using a self-signed, expired, or otherwise misconfigured certificate.",
"type": "boolean",
"default": true
},
"positPublisher.useKeyChainCredentialStorage": {
"markdownDescription": "Controls whether Posit Publisher will use the system keychain to store credentials. When disabled, or on a system without a keychain, `~/.connect-credentials` is used. After changing this setting, you must restart VS Code for it to go into effect.",
"type": "boolean",
"default": true
}
}
}
Expand Down
12 changes: 11 additions & 1 deletion extensions/vscode/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,20 @@ export const create = async (
context: ExtensionContext,
path: string,
port: number,
useKeyChain: boolean,
subcommand: string = "ui",
): Promise<[string, string[]]> => {
const executable = await getExecutableBinary(context);
return [executable, [subcommand, "-vv", `--listen=${HOST}:${port}`, path]];
return [
executable,
[
subcommand,
"-vv",
`--listen=${HOST}:${port}`,
`--use-keychain=${useKeyChain}`,
path,
],
];
};

const getExecutableBinary = (context: ExtensionContext): string => {
Expand Down
14 changes: 12 additions & 2 deletions extensions/vscode/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ export async function activate(context: ExtensionContext) {
const stream = new EventStream(port);
context.subscriptions.push(stream);

service = new Service(context, port, useExternalAgent);
const useKeyChain = extensionSettings.useKeyChainCredentialStorage();

service = new Service(context, port, useExternalAgent, useKeyChain);
service.start();

const watchers = new WatcherManager();
Expand Down Expand Up @@ -119,10 +121,18 @@ export async function deactivate() {

export const extensionSettings = {
verifyCertificates(): boolean {
// set value from extension configuration - defaults to true
// get value from extension configuration - defaults to true
const configuration = workspace.getConfiguration("positPublisher");
const value: boolean | undefined =
configuration.get<boolean>("verifyCertificates");
return value !== undefined ? value : true;
},
useKeyChainCredentialStorage(): boolean {
// get value from extension configuration - defaults to true
const configuration = workspace.getConfiguration("positPublisher");
const value: boolean | undefined = configuration.get<boolean>(
"useKeyChainCredentialStorage",
);
return value !== undefined ? value : true;
},
};
11 changes: 9 additions & 2 deletions extensions/vscode/src/servers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ import * as workspaces from "src/workspaces";
export class Server implements Disposable {
readonly port: number;
readonly outputChannel: OutputChannel;
readonly useKeyChain: boolean;

process: ChildProcessWithoutNullStreams | undefined = undefined;

constructor(port: number) {
constructor(port: number, useKeyChain: boolean) {
this.port = port;
this.outputChannel = window.createOutputChannel(`Posit Publisher`);
this.useKeyChain = useKeyChain;
}

/**
Expand All @@ -38,7 +40,12 @@ export class Server implements Disposable {
// todo - make this configurable
const path = workspaces.path();
// Create command to send to terminal stdin
const [command, args] = await commands.create(context, path!, this.port);
const [command, args] = await commands.create(
context,
path!,
this.port,
this.useKeyChain,
);
// Spawn child process
this.process = spawn(command, args);
// Handle error output
Expand Down
5 changes: 4 additions & 1 deletion extensions/vscode/src/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,20 @@ export class Service implements Disposable {
private server: Server;
private agentURL: string;
private useExternalAgent: boolean;
private useKeyChain: boolean;

constructor(
context: ExtensionContext,
port: number,
useExternalAgent: boolean,
useKeyChain: boolean,
) {
this.context = context;
this.useExternalAgent = useExternalAgent;
this.useKeyChain = useKeyChain;

this.agentURL = `http://${HOST}:${port}/api`;
this.server = new Server(port);
this.server = new Server(port, this.useKeyChain);
initApi(this.isUp(), this.agentURL);
}

Expand Down
17 changes: 13 additions & 4 deletions internal/credentials/credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ type CredentialRecord struct {

type CredentialTable = map[string]CredentialRecord

var UseKeychain bool = true

// ToCredential converts a CredentialRecord to a Credential based on its version.
func (cr *CredentialRecord) ToCredential() (*Credential, error) {
switch cr.Version {
Expand Down Expand Up @@ -93,12 +95,19 @@ type CredentialsService interface {
// The main credentials service constructor that determines if the system's keyring is available to be used,
// if not, returns a file based credentials service.
func NewCredentialsService(log logging.Logger) (CredentialsService, error) {
krService := NewKeyringCredentialsService(log)
if krService.IsSupported() {
return krService, nil
var fcService *fileCredentialsService = nil

if UseKeychain {
krService := NewKeyringCredentialsService(log)
if krService.IsSupported() {
log.Debug("Using keychain for credential storage.")
return krService, nil
}
log.Debug("Fallback to file managed credentials service due to unavailable system keyring.")
} else {
log.Debug("Configuration has disabled keychain credentials. Using file managed credentials instead.")
}

log.Debug("Fallback to file managed credentials service due to unavailable system keyring")
fcService, err := NewFileCredentialsService(log)
if err != nil {
return fcService, err
Expand Down
38 changes: 37 additions & 1 deletion internal/credentials/credentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ func (s *CredentialsServiceTestSuite) TestCredentialRecord_VersionErr() {

func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringOK() {
keyring.MockInit()

// Don't set credentials.UseKeychain to any value, let it default

s.log.On("Debug", "Using keychain for credential storage.").Return()

credservice, err := NewCredentialsService(s.log)
s.NoError(err)
s.Implements((*CredentialsService)(nil), credservice)
Expand All @@ -118,7 +123,38 @@ func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringErrFallba
keyring.MockInitWithError(keyringErr)

s.log.On("Debug", "System keyring service is not available", "error", "failed to load credentials: this is a teapot, unsupported system").Return()
s.log.On("Debug", "Fallback to file managed credentials service due to unavailable system keyring").Return()
s.log.On("Debug", "Fallback to file managed credentials service due to unavailable system keyring.").Return()

credservice, err := NewCredentialsService(s.log)
s.NoError(err)
s.Implements((*CredentialsService)(nil), credservice)
}

func (s *CredentialsServiceTestSuite) TestNewCredentialsService_WithUseKeyringTrue() {
keyring.MockInit()

// set credentials.UseKeychain to true
UseKeychain = true

s.log.On("Debug", "Using keychain for credential storage.").Return()

credservice, err := NewCredentialsService(s.log)
s.NoError(err)
s.Implements((*CredentialsService)(nil), credservice)
}

func (s *CredentialsServiceTestSuite) TestNewCredentialsService_WithUseKeyringFalse() {
// Use an in memory filesystem for this test
// avoiding to manipulate users ~/.connect-credentials
fsys = afero.NewMemMapFs()
defer func() { fsys = afero.NewOsFs() }()

keyring.MockInit()

s.log.On("Debug", "Configuration has disabled keychain credentials. Using file managed credentials instead.").Return()

// set credentials.UseKeychain to true
UseKeychain = false

credservice, err := NewCredentialsService(s.log)
s.NoError(err)
Expand Down
5 changes: 5 additions & 0 deletions internal/logging/loggingtest/mock_logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,8 @@ func (m *MockLogger) WithArgs(args ...any) logging.Logger {
mockArgs := m.Called(args...)
return mockArgs.Get(0).(logging.Logger)
}

func (m *MockLogger) Debug(msg string, args ...any) {
mockArgs := append([]any{msg}, args...)
m.Called(mockArgs...)
}
Loading