From af224e5a024c49d67767b8ef57bc465199f5113a Mon Sep 17 00:00:00 2001 From: Marcos Navarro Date: Sat, 21 Dec 2024 01:46:50 +0900 Subject: [PATCH 1/6] Reset credentials strategy for corrupted data, new error to bubble up to warn the user --- internal/credentials/credentials.go | 44 ++++++++++++- internal/credentials/credentials_test.go | 65 +++++++++++++++++++ internal/credentials/errors.go | 10 +++ internal/credentials/file.go | 7 +- internal/credentials/file_test.go | 28 ++++++++ internal/credentials/keyring.go | 9 ++- internal/credentials/keyring_test.go | 28 ++++++++ .../credentials/testdata/toml/to-reset.toml | 1 + internal/types/error.go | 1 + 9 files changed, 188 insertions(+), 5 deletions(-) create mode 100644 internal/credentials/testdata/toml/to-reset.toml diff --git a/internal/credentials/credentials.go b/internal/credentials/credentials.go index 8dd9ddada..615e933d3 100644 --- a/internal/credentials/credentials.go +++ b/internal/credentials/credentials.go @@ -31,6 +31,7 @@ package credentials import ( "encoding/json" + "errors" "github.com/posit-dev/publisher/internal/logging" "github.com/posit-dev/publisher/internal/types" @@ -86,21 +87,58 @@ type CredentialsService interface { Get(guid string) (*Credential, error) List() ([]Credential, error) Set(name string, url string, ak string) (*Credential, error) + Reset() error } // 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. +// Additionally, tries to load the current credentials to handle resetting in case of data corruption. func NewCredentialsService(log logging.Logger) (CredentialsService, error) { krService := NewKeyringCredentialsService(log) if krService.IsSupported() { - return krService, nil + err := enforceKeyringIntegrity(krService, log) + return krService, err } log.Debug("Fallback to file managed credentials service due to unavailable system keyring") fcService, err := NewFileCredentialsService(log) if err != nil { - return nil, types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) + return fcService, enforceFilecredsIntegrity(fcService, err, log) } - return fcService, nil } + +func enforceKeyringIntegrity(ks *keyringCredentialsService, log logging.Logger) error { + // Catch any error that could require attention early + _, err := ks.List() + if err == nil { + return nil + } + + // If error from trying to fetch credentials list is not well known + // credentials handling is unavailable + if !errors.Is(err, &CorruptedError{}) && !errors.Is(err, &LoadError{}) { + return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) + } + + log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed") + err = ks.Reset() + if err != nil { + return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) + } + + return types.NewAgentError(types.ErrorCredentialCorruptedReset, err, nil) +} + +func enforceFilecredsIntegrity(fc *fileCredentialsService, err error, log logging.Logger) error { + if errors.Is(err, &LoadError{}) { + log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed") + err = fc.Reset() + if err != nil { + return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) + } + return types.NewAgentError(types.ErrorCredentialCorruptedReset, err, nil) + } + + return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) +} diff --git a/internal/credentials/credentials_test.go b/internal/credentials/credentials_test.go index 6e35e68f9..6710089e5 100644 --- a/internal/credentials/credentials_test.go +++ b/internal/credentials/credentials_test.go @@ -7,6 +7,8 @@ import ( "testing" "github.com/posit-dev/publisher/internal/logging/loggingtest" + "github.com/posit-dev/publisher/internal/types" + "github.com/posit-dev/publisher/internal/util" "github.com/posit-dev/publisher/internal/util/utiltest" "github.com/spf13/afero" "github.com/stretchr/testify/suite" @@ -124,3 +126,66 @@ func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringErrFallba s.NoError(err) s.Implements((*CredentialsService)(nil), credservice) } + +func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringResetsOnCorruptedCred() { + keyring.MockInit() + + corruptedData := `{"":{"guid":"18cd5640-bee5-4b2a-992a-a2725ab6103d","version":0,"data":"unrecognizbledata"}}` + keyring.Set(ServiceName, "credentials", corruptedData) + + keyringCorruptedSave, err := keyring.Get(ServiceName, "credentials") + s.NoError(err) + s.Equal(keyringCorruptedSave, corruptedData) + + s.log.On("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed").Return() + + credservice, err := NewCredentialsService(s.log) + s.NotNil(err) + s.Implements((*CredentialsService)(nil), credservice) + + // Confirm agent error of corrupted credential bubbles up to warn the user + _, isCorruptedErr := types.IsAgentErrorOf(err, types.ErrorCredentialCorruptedReset) + s.True(isCorruptedErr) + + keyringUpdatedData, err := keyring.Get(ServiceName, "credentials") + s.NoError(err) + s.Equal(keyringUpdatedData, "{}") +} + +func (s *CredentialsServiceTestSuite) TestNewCredentialsService_FallbackFileResetsOnCorruptedCred() { + // Use an in memory filesystem for this test + // avoiding to manipulate users ~/.connect-credentials + fsys = afero.NewMemMapFs() + defer func() { fsys = afero.NewOsFs() }() + dirPath, err := util.UserHomeDir(fsys) + s.NoError(err) + + dirPath = dirPath.Join(".connect-credentials") + corruptedContents := []byte("unrecognizable_key = this is bad") + err = afero.WriteFile(fsys, dirPath.String(), corruptedContents, 0644) + s.NoError(err) + + keyringErr := errors.New("this is a teapot, unsupported system") + keyring.MockInitWithError(keyringErr) + + // Verify the corrupted contents are in + credsData, err := afero.ReadFile(fsys, dirPath.String()) + s.NoError(err) + s.Equal(credsData, corruptedContents) + + 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("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed").Return() + + credservice, err := NewCredentialsService(s.log) + s.NotNil(err) + s.Implements((*CredentialsService)(nil), credservice) + + // Confirm agent error of corrupted credential bubbles up to warn the user + _, isCorruptedErr := types.IsAgentErrorOf(err, types.ErrorCredentialCorruptedReset) + s.True(isCorruptedErr) + + credsData, err = afero.ReadFile(fsys, dirPath.String()) + s.NoError(err) + s.Equal(credsData, []byte("[credentials]\n")) +} diff --git a/internal/credentials/errors.go b/internal/credentials/errors.go index bd967b8a1..8e0536923 100644 --- a/internal/credentials/errors.go +++ b/internal/credentials/errors.go @@ -16,6 +16,11 @@ func (e *CorruptedError) Error() string { return fmt.Sprintf("credential '%s' is corrupted", e.GUID) } +func (e *CorruptedError) Is(target error) bool { + _, isCorrupterErr := target.(*CorruptedError) + return isCorrupterErr +} + type LoadError struct { Err error } @@ -28,6 +33,11 @@ func (e *LoadError) Error() string { return fmt.Sprintf("failed to load credentials: %v", e.Err) } +func (e *LoadError) Is(target error) bool { + _, isLoadErr := target.(*LoadError) + return isLoadErr +} + type NotFoundError struct { GUID string } diff --git a/internal/credentials/file.go b/internal/credentials/file.go index d4f2f9b88..fb483e8df 100644 --- a/internal/credentials/file.go +++ b/internal/credentials/file.go @@ -93,7 +93,7 @@ func NewFileCredentialsService(log logging.Logger) (*fileCredentialsService, err // Verify file can be modified, will create if not exists err = fservice.setup() if err != nil { - return nil, err + return fservice, err } return fservice, nil @@ -207,6 +207,11 @@ func (c *fileCredentialsService) Delete(guid string) error { return nil } +func (c *fileCredentialsService) Reset() error { + newData := newFileCredentials() + return c.saveFile(newData) +} + func (c *fileCredentialsService) setup() error { _, err := c.credsFilepath.Stat() if os.IsNotExist(err) { diff --git a/internal/credentials/file_test.go b/internal/credentials/file_test.go index 78435482f..bc1bbc777 100644 --- a/internal/credentials/file_test.go +++ b/internal/credentials/file_test.go @@ -523,3 +523,31 @@ func (s *FileCredentialsServiceSuite) TestSet_ConflictErr() { }) } } + +func (s *FileCredentialsServiceSuite) TestReset() { + cs := &fileCredentialsService{ + log: s.loggerMock, + credsFilepath: s.testdata.Join("to-reset.toml"), + } + + _, err := cs.load() + s.NoError(err) + + _, err = cs.Set("newcred", "https://b2.connect-server:3939/connect", "abcdeC2aqbh7dg8TO43XPu7r56YDh002") + s.NoError(err) + + _, err = cs.Set("newcredtwo", "https://b5.connect-server:3939/connect", "abcdeC2aqbh7dg8TO43XPu7r56YDh007") + s.NoError(err) + + list, err := cs.List() + s.NoError(err) + s.Len(list, 2) + + err = cs.Reset() + s.NoError(err) + + // Creds wiped out + list, err = cs.List() + s.NoError(err) + s.Len(list, 0) +} diff --git a/internal/credentials/keyring.go b/internal/credentials/keyring.go index bcced33cc..2210c0703 100644 --- a/internal/credentials/keyring.go +++ b/internal/credentials/keyring.go @@ -127,6 +127,13 @@ func (ks *keyringCredentialsService) Set(name string, url string, ak string) (*C return &cred, nil } +// Resets the CredentialTable from keyring +// it is a last resort in case the keyring data turns out to be irrecognizable +func (ks *keyringCredentialsService) Reset() error { + newTable := make(map[string]CredentialRecord) + return ks.save(newTable) +} + func (ks *keyringCredentialsService) checkForConflicts( table *map[string]CredentialRecord, c *Credential) error { @@ -159,7 +166,7 @@ func (ks *keyringCredentialsService) save(table CredentialTable) error { return nil } -// Loads the CredentialTable from keyRing +// Loads the CredentialTable from keyring func (ks *keyringCredentialsService) load() (CredentialTable, error) { data, err := keyring.Get(ServiceName, "credentials") if err != nil { diff --git a/internal/credentials/keyring_test.go b/internal/credentials/keyring_test.go index 15a2e7564..013b1b3a7 100644 --- a/internal/credentials/keyring_test.go +++ b/internal/credentials/keyring_test.go @@ -162,3 +162,31 @@ func (s *KeyringCredentialsTestSuite) TestDelete() { s.Error(err) s.log.AssertExpectations(s.T()) } + +func (s *KeyringCredentialsTestSuite) TestReset() { + cs := keyringCredentialsService{ + log: s.log, + } + + creds, err := cs.List() + s.NoError(err) + s.Equal(creds, []Credential{}) + + // Add a couple creds to be assert on the list again + _, err = cs.Set("example", "https://a.example.com", "12345") + s.NoError(err) + _, err = cs.Set("example2", "https://b.example.com", "12345") + s.NoError(err) + + creds, err = cs.List() + s.NoError(err) + s.Len(creds, 2) + + err = cs.Reset() + s.NoError(err) + + // Creds wiped out + creds, err = cs.List() + s.NoError(err) + s.Len(creds, 0) +} diff --git a/internal/credentials/testdata/toml/to-reset.toml b/internal/credentials/testdata/toml/to-reset.toml new file mode 100644 index 000000000..5bbe47685 --- /dev/null +++ b/internal/credentials/testdata/toml/to-reset.toml @@ -0,0 +1 @@ +[credentials] diff --git a/internal/types/error.go b/internal/types/error.go index 0af9943e4..705f9d240 100644 --- a/internal/types/error.go +++ b/internal/types/error.go @@ -19,6 +19,7 @@ const ( ErrorUnknownTOMLKey ErrorCode = "unknownTOMLKey" ErrorInvalidConfigFiles ErrorCode = "invalidConfigFiles" ErrorCredentialServiceUnavailable ErrorCode = "credentialsServiceUnavailable" + ErrorCredentialCorruptedReset ErrorCode = "credentialCorruptedReset" ErrorCertificateVerification ErrorCode = "errorCertificateVerification" ErrorRenvPackageVersionMismatch ErrorCode = "renvPackageVersionMismatch" ErrorRenvPackageSourceMissing ErrorCode = "renvPackageSourceMissing" From 349c41f977fab8ceb939481f2f19bb6088765ceb Mon Sep 17 00:00:00 2001 From: Marcos Navarro Date: Sat, 21 Dec 2024 03:15:26 +0900 Subject: [PATCH 2/6] Handle reset credentials in UI --- extensions/vscode/src/state.ts | 9 +++++++++ extensions/vscode/src/utils/errorTypes.ts | 18 +++++++++++++++++- internal/accounts/provider_credentials.go | 7 ++++++- internal/services/api/get_credentials.go | 5 +++++ internal/types/api_errors.go | 14 ++++++++++++++ internal/types/api_errors_test.go | 20 ++++++++++++++++++++ 6 files changed, 71 insertions(+), 2 deletions(-) diff --git a/extensions/vscode/src/state.ts b/extensions/vscode/src/state.ts index b5e5aa357..e9d788b04 100644 --- a/extensions/vscode/src/state.ts +++ b/extensions/vscode/src/state.ts @@ -19,6 +19,10 @@ import { getStatusFromError, getSummaryStringFromError, } from "src/utils/errors"; +import { + isErrCredentialsReset, + errCredentialsResetMessage, +} from "src/utils/errorTypes"; import { DeploymentSelector, SelectionState } from "src/types/shared"; import { LocalState, Views } from "./constants"; @@ -283,6 +287,11 @@ export class PublisherState implements Disposable { this.credentials = response.data; }); } catch (error: unknown) { + if (isErrCredentialsReset(error)) { + const warnMsg = errCredentialsResetMessage(); + window.showWarningMessage(warnMsg); + return; + } const summary = getSummaryStringFromError("refreshCredentials", error); window.showErrorMessage(summary); } diff --git a/extensions/vscode/src/utils/errorTypes.ts b/extensions/vscode/src/utils/errorTypes.ts index 5902eef56..e447e7063 100644 --- a/extensions/vscode/src/utils/errorTypes.ts +++ b/extensions/vscode/src/utils/errorTypes.ts @@ -17,7 +17,8 @@ export type ErrorCode = | "deployedContentNotRunning" | "tomlValidationError" | "tomlUnknownError" - | "pythonExecNotFound"; + | "pythonExecNotFound" + | "credentialCorruptedReset"; export type axiosErrorWithJson = AxiosError & { @@ -162,6 +163,15 @@ export type ErrInvalidConfigFiles = MkErrorDataType< export const isErrInvalidConfigFile = mkErrorTypeGuard("invalidConfigFile"); +// Invalid configuration file(s) +export type ErrCredentialsReset = MkErrorDataType<"credentialCorruptedReset">; +export const isErrCredentialsReset = mkErrorTypeGuard( + "credentialCorruptedReset", +); +export const errCredentialsResetMessage = () => { + return "Stored credentials for Posit Publisher found in an unrecognizable state. A reset was required in order to proceed."; +}; + // Tries to match an Axios error that comes with an identifiable Json structured data // defaulting to be ErrUnknown message when export function resolveAgentJsonErrorMsg(err: axiosErrorWithJson) { @@ -185,5 +195,11 @@ export function resolveAgentJsonErrorMsg(err: axiosErrorWithJson) { return errPythonExecNotFoundErrorMessage(err); } + // Ignore errors coming from credentials being reset, + // a warning is shown when PublisherState is updated. + if (isErrPythonExecNotFoundError(err)) { + return errPythonExecNotFoundErrorMessage(err); + } + return errUnknownMessage(err as axiosErrorWithJson); } diff --git a/internal/accounts/provider_credentials.go b/internal/accounts/provider_credentials.go index 276307906..1c16d0fb1 100644 --- a/internal/accounts/provider_credentials.go +++ b/internal/accounts/provider_credentials.go @@ -5,6 +5,7 @@ package accounts import ( "github.com/posit-dev/publisher/internal/credentials" "github.com/posit-dev/publisher/internal/logging" + "github.com/posit-dev/publisher/internal/types" ) type CredentialsProvider struct { @@ -14,7 +15,11 @@ type CredentialsProvider struct { func NewCredentialsProvider(log logging.Logger) (*CredentialsProvider, error) { cs, err := credentials.NewCredentialsService(log) if err != nil { - return nil, err + // Ignore errors from credentials reset at this point + _, isCredsReset := types.IsAgentErrorOf(err, types.ErrorCredentialCorruptedReset) + if !isCredsReset { + return nil, err + } } return &CredentialsProvider{cs}, nil diff --git a/internal/services/api/get_credentials.go b/internal/services/api/get_credentials.go index 24280cf1c..633204d71 100644 --- a/internal/services/api/get_credentials.go +++ b/internal/services/api/get_credentials.go @@ -21,6 +21,11 @@ func GetCredentialsHandlerFunc(log logging.Logger) http.HandlerFunc { apiErr.JSONResponse(w) return } + if aerr.Code == types.ErrorCredentialCorruptedReset { + apiErr := types.APIErrorCredentialCorruptedResetFromAgentError(*aerr) + apiErr.JSONResponse(w) + return + } } InternalError(w, req, log, err) return diff --git a/internal/types/api_errors.go b/internal/types/api_errors.go index 03a2435ea..aa1cc814d 100644 --- a/internal/types/api_errors.go +++ b/internal/types/api_errors.go @@ -162,6 +162,20 @@ func APIErrorCredentialsUnavailableFromAgentError(aerr AgentError) APIErrorCrede } } +type APIErrorCredentialCorruptedReset struct { + Code ErrorCode `json:"code"` +} + +func (apierr *APIErrorCredentialCorruptedReset) JSONResponse(w http.ResponseWriter) { + jsonResult(w, http.StatusConflict, apierr) +} + +func APIErrorCredentialCorruptedResetFromAgentError(aerr AgentError) APIErrorCredentialCorruptedReset { + return APIErrorCredentialCorruptedReset{ + Code: ErrorCredentialCorruptedReset, + } +} + type APIErrorPythonExecNotFound struct { Code ErrorCode `json:"code"` } diff --git a/internal/types/api_errors_test.go b/internal/types/api_errors_test.go index e59488887..1d0c3ac80 100644 --- a/internal/types/api_errors_test.go +++ b/internal/types/api_errors_test.go @@ -88,3 +88,23 @@ func (s *ApiErrorsSuite) TestAPIErrorPythonExecNotFoundFromAgentError() { s.Equal(http.StatusUnprocessableEntity, rec.Result().StatusCode) s.Contains(bodyRes, `{"code":"pythonExecNotFound"}`) } + +func (s *ApiErrorsSuite) TestAPIErrorCredentialCorruptedResetFromAgentError() { + agentErr := AgentError{ + Message: "", + Code: ErrorCredentialCorruptedReset, + Err: errors.New("unknown field error"), + Data: ErrorData{}, + } + + rec := httptest.NewRecorder() + + apiError := APIErrorCredentialCorruptedResetFromAgentError(agentErr) + s.Equal(apiError.Code, ErrorCredentialCorruptedReset) + + apiError.JSONResponse(rec) + + bodyRes := rec.Body.String() + s.Equal(http.StatusConflict, rec.Result().StatusCode) + s.Contains(bodyRes, `{"code":"credentialCorruptedReset"}`) +} From 6f07f1481c9ffcb00c9a5cc33b79acf6f1190cae Mon Sep 17 00:00:00 2001 From: Marcos Navarro Date: Thu, 26 Dec 2024 23:49:20 +0900 Subject: [PATCH 3/6] Handle credentials reset with more consistency, new API endpoint to reset creds and always display a warning message when reset takes place --- .../vscode/src/api/resources/Credentials.ts | 7 + extensions/vscode/src/state.test.ts | 131 ++++++++++++++++- extensions/vscode/src/state.ts | 24 ++- .../src/test/unit-test-utils/factories.ts | 8 + extensions/vscode/src/utils/errorTypes.ts | 13 +- internal/accounts/provider_credentials.go | 17 ++- internal/credentials/credentials.go | 46 +----- internal/credentials/credentials_test.go | 65 --------- .../credentialstest/credentialstest.go | 42 ++++++ internal/credentials/file.go | 1 + internal/credentials/file_test.go | 3 + internal/credentials/keyring.go | 1 + internal/credentials/keyring_test.go | 3 + internal/services/api/api_service.go | 13 +- internal/services/api/get_credentials.go | 32 ++-- internal/services/api/get_credentials_test.go | 138 ++++++++++++++++++ internal/services/api/reset_credentials.go | 41 ++++++ .../services/api/reset_credentials_test.go | 126 ++++++++++++++++ internal/types/api_errors.go | 10 +- internal/types/api_errors_test.go | 8 +- internal/types/error.go | 2 +- 21 files changed, 575 insertions(+), 156 deletions(-) create mode 100644 internal/credentials/credentialstest/credentialstest.go create mode 100644 internal/services/api/get_credentials_test.go create mode 100644 internal/services/api/reset_credentials.go create mode 100644 internal/services/api/reset_credentials_test.go diff --git a/extensions/vscode/src/api/resources/Credentials.ts b/extensions/vscode/src/api/resources/Credentials.ts index 3e6e36c75..440d5fef5 100644 --- a/extensions/vscode/src/api/resources/Credentials.ts +++ b/extensions/vscode/src/api/resources/Credentials.ts @@ -46,6 +46,13 @@ export class Credentials { return this.client.delete(`credentials/${guid}`); } + // Returns: + // 204 - success (no response) + // 503 - credentials service unavailable + reset() { + return this.client.delete(`credentials`); + } + // Returns: // 200 - with possible results in TestResult object // for URL only: no user or error in TestResult diff --git a/extensions/vscode/src/state.test.ts b/extensions/vscode/src/state.test.ts index 3d38f0d87..a5ce34977 100644 --- a/extensions/vscode/src/state.test.ts +++ b/extensions/vscode/src/state.test.ts @@ -8,6 +8,7 @@ import { selectionStateFactory, preContentRecordFactory, configurationFactory, + credentialFactory, } from "src/test/unit-test-utils/factories"; import { mkExtensionContextStateMock } from "src/test/unit-test-utils/vscode-mocks"; import { LocalState } from "./constants"; @@ -27,6 +28,7 @@ class mockApiClient { readonly credentials = { list: vi.fn(), + reset: vi.fn(), }; } @@ -39,6 +41,14 @@ vi.mock("src/api", async (importOriginal) => { }; }); +vi.mock("src/utils/progress", () => { + return { + showProgress: vi.fn((_title, _view: string, until: () => Promise) => { + return until(); + }), + }; +}); + vi.mock("vscode", () => { // mock Disposable const disposableMock = vi.fn(); @@ -47,6 +57,7 @@ vi.mock("vscode", () => { // mock window const windowMock = { showErrorMessage: vi.fn(), + showWarningMessage: vi.fn(), showInformationMessage: vi.fn(), }; @@ -385,6 +396,124 @@ describe("PublisherState", () => { }); }); + describe("refreshCredentials", () => { + test("Calls to fetch credentials and stores to state", async () => { + const fakeCredsFetch = credentialFactory.buildList(3); + mockClient.credentials.list.mockResolvedValue({ + data: fakeCredsFetch, + }); + + const { mockContext } = mkExtensionContextStateMock({}); + const publisherState = new PublisherState(mockContext); + + // Creds are empty initially + expect(publisherState.credentials).toEqual([]); + + // Populated with API results + await publisherState.refreshCredentials(); + expect(publisherState.credentials).toEqual(fakeCredsFetch); + }); + + describe("errors", () => { + test("api error - not corrupted data", async () => { + const axiosErr = new AxiosError(); + axiosErr.response = { + data: "this is terrible", + status: 500, + statusText: "500", + headers: {}, + config: { headers: new AxiosHeaders() }, + }; + mockClient.credentials.list.mockRejectedValueOnce(axiosErr); + + const { mockContext } = mkExtensionContextStateMock({}); + const publisherState = new PublisherState(mockContext); + + // Creds are empty initially + expect(publisherState.credentials).toEqual([]); + + // Creds are still empty + await publisherState.refreshCredentials(); + expect(publisherState.credentials).toEqual([]); + + // Error mssage is processed + expect(window.showErrorMessage).toHaveBeenCalledWith( + "this is terrible", + ); + }); + + test("api error - corrupted data - defers to reset creds", async () => { + const axiosErr = new AxiosError(); + axiosErr.response = { + data: { + code: "credentialsCorrupted", + }, + status: 409, + statusText: "409", + headers: {}, + config: { headers: new AxiosHeaders() }, + }; + mockClient.credentials.list.mockRejectedValue(axiosErr); + mockClient.credentials.reset.mockResolvedValue({}); + + const { mockContext } = mkExtensionContextStateMock({}); + const publisherState = new PublisherState(mockContext); + + // Creds are empty initially + expect(publisherState.credentials).toEqual([]); + + // Creds are still empty + await publisherState.refreshCredentials(); + expect(publisherState.credentials).toEqual([]); + + // Error message is not called + expect(window.showErrorMessage).not.toHaveBeenCalled(); + + // Calls to reset + expect(mockClient.credentials.reset).toHaveBeenCalled(); + }); + }); + }); + + describe("resetCredentials", () => { + test("calls to reset credentials and shows a warning", async () => { + mockClient.credentials.reset.mockResolvedValue({}); + + const { mockContext } = mkExtensionContextStateMock({}); + const publisherState = new PublisherState(mockContext); + + publisherState.credentials = credentialFactory.buildList(3); + await publisherState.resetCredentials(); + + expect(publisherState.credentials).toEqual([]); + + // Warning message is called + expect(window.showWarningMessage).toHaveBeenCalledWith( + "Unrecognizable credentials for Posit Publisher were found and removed. Credentials may need to be recreated.", + ); + }); + + test("on api error - shows message", async () => { + const axiosErr = new AxiosError(); + axiosErr.response = { + data: "terrible, could not reset", + status: 500, + statusText: "500", + headers: {}, + config: { headers: new AxiosHeaders() }, + }; + mockClient.credentials.reset.mockRejectedValueOnce(axiosErr); + + const { mockContext } = mkExtensionContextStateMock({}); + const publisherState = new PublisherState(mockContext); + + await publisherState.resetCredentials(); + expect(window.showErrorMessage).toHaveBeenCalledWith( + "terrible, could not reset", + ); + }); + }); + test.todo("getSelectedConfiguration", () => {}); test.todo("refreshContentRecords", () => {}); @@ -394,6 +523,4 @@ describe("PublisherState", () => { test.todo("validConfigs", () => {}); test.todo("configsInError", () => {}); - - test.todo("refreshCredentials", () => {}); }); diff --git a/extensions/vscode/src/state.ts b/extensions/vscode/src/state.ts index e9d788b04..285f2d7af 100644 --- a/extensions/vscode/src/state.ts +++ b/extensions/vscode/src/state.ts @@ -20,8 +20,8 @@ import { getSummaryStringFromError, } from "src/utils/errors"; import { - isErrCredentialsReset, - errCredentialsResetMessage, + isErrCredentialsCorrupted, + errCredentialsCorruptedMessage, } from "src/utils/errorTypes"; import { DeploymentSelector, SelectionState } from "src/types/shared"; import { LocalState, Views } from "./constants"; @@ -287,9 +287,8 @@ export class PublisherState implements Disposable { this.credentials = response.data; }); } catch (error: unknown) { - if (isErrCredentialsReset(error)) { - const warnMsg = errCredentialsResetMessage(); - window.showWarningMessage(warnMsg); + if (isErrCredentialsCorrupted(error)) { + this.resetCredentials(); return; } const summary = getSummaryStringFromError("refreshCredentials", error); @@ -297,6 +296,21 @@ export class PublisherState implements Disposable { } } + // Calls to reset all credentials data stored. + // Meant to be a last resort when we get loading data or corrupted data errors. + async resetCredentials() { + const warnMsg = errCredentialsCorruptedMessage(); + try { + const api = await useApi(); + await api.credentials.reset(); + this.credentials = []; + window.showWarningMessage(warnMsg); + } catch (err: unknown) { + const summary = getSummaryStringFromError("resetCredentials", err); + window.showErrorMessage(summary); + } + } + findCredential(name: string) { return findCredential(name, this.credentials); } diff --git a/extensions/vscode/src/test/unit-test-utils/factories.ts b/extensions/vscode/src/test/unit-test-utils/factories.ts index 5afcc04f4..bd15e3354 100644 --- a/extensions/vscode/src/test/unit-test-utils/factories.ts +++ b/extensions/vscode/src/test/unit-test-utils/factories.ts @@ -8,6 +8,7 @@ import { ContentRecordState, } from "src/api/types/contentRecords"; import { ContentType, Configuration } from "src/api/types/configurations"; +import { Credential } from "src/api/types/credentials"; import { DeploymentSelectorState } from "src/types/shared"; export const selectionStateFactory = Factory.define( @@ -85,3 +86,10 @@ export const contentRecordFactory = Factory.define( configurationRelPath: `report/path/configuration-${sequence}`, }), ); + +export const credentialFactory = Factory.define(({ sequence }) => ({ + guid: `44a468b8-09c7-4c6d-a7a3-8cf164ddbaf${sequence}`, + name: `Credential ${sequence}`, + url: `https://connect.${sequence}.site.com/connect`, + apiKey: `qwerty-${sequence}`, +})); diff --git a/extensions/vscode/src/utils/errorTypes.ts b/extensions/vscode/src/utils/errorTypes.ts index e447e7063..3a84c8b13 100644 --- a/extensions/vscode/src/utils/errorTypes.ts +++ b/extensions/vscode/src/utils/errorTypes.ts @@ -18,7 +18,7 @@ export type ErrorCode = | "tomlValidationError" | "tomlUnknownError" | "pythonExecNotFound" - | "credentialCorruptedReset"; + | "credentialsCorrupted"; export type axiosErrorWithJson = AxiosError & { @@ -164,12 +164,11 @@ export const isErrInvalidConfigFile = mkErrorTypeGuard("invalidConfigFile"); // Invalid configuration file(s) -export type ErrCredentialsReset = MkErrorDataType<"credentialCorruptedReset">; -export const isErrCredentialsReset = mkErrorTypeGuard( - "credentialCorruptedReset", -); -export const errCredentialsResetMessage = () => { - return "Stored credentials for Posit Publisher found in an unrecognizable state. A reset was required in order to proceed."; +export type ErrCredentialsCorrupted = MkErrorDataType<"credentialsCorrupted">; +export const isErrCredentialsCorrupted = + mkErrorTypeGuard("credentialsCorrupted"); +export const errCredentialsCorruptedMessage = () => { + return "Unrecognizable credentials for Posit Publisher were found and removed. Credentials may need to be recreated."; }; // Tries to match an Axios error that comes with an identifiable Json structured data diff --git a/internal/accounts/provider_credentials.go b/internal/accounts/provider_credentials.go index 1c16d0fb1..7f576a3ff 100644 --- a/internal/accounts/provider_credentials.go +++ b/internal/accounts/provider_credentials.go @@ -3,23 +3,26 @@ package accounts import ( + "errors" + "github.com/posit-dev/publisher/internal/credentials" "github.com/posit-dev/publisher/internal/logging" - "github.com/posit-dev/publisher/internal/types" ) type CredentialsProvider struct { cs credentials.CredentialsService } +// We can ignore errors related to malformed data on the initial loader +// API consumers handle resetting malformed data when needed. +func errIsNotLoadError(err error) bool { + return err != nil && !errors.Is(err, &credentials.LoadError{}) && !errors.Is(err, &credentials.CorruptedError{}) +} + func NewCredentialsProvider(log logging.Logger) (*CredentialsProvider, error) { cs, err := credentials.NewCredentialsService(log) - if err != nil { - // Ignore errors from credentials reset at this point - _, isCredsReset := types.IsAgentErrorOf(err, types.ErrorCredentialCorruptedReset) - if !isCredsReset { - return nil, err - } + if errIsNotLoadError(err) { + return nil, err } return &CredentialsProvider{cs}, nil diff --git a/internal/credentials/credentials.go b/internal/credentials/credentials.go index 615e933d3..db5b9fccd 100644 --- a/internal/credentials/credentials.go +++ b/internal/credentials/credentials.go @@ -31,10 +31,8 @@ package credentials import ( "encoding/json" - "errors" "github.com/posit-dev/publisher/internal/logging" - "github.com/posit-dev/publisher/internal/types" ) const ServiceName = "Posit Publisher Safe Storage" @@ -82,6 +80,8 @@ func (cr *CredentialRecord) ToCredential() (*Credential, error) { } } +type CredServiceFactory = func(log logging.Logger) (CredentialsService, error) + type CredentialsService interface { Delete(guid string) error Get(guid string) (*Credential, error) @@ -92,53 +92,17 @@ 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. -// Additionally, tries to load the current credentials to handle resetting in case of data corruption. func NewCredentialsService(log logging.Logger) (CredentialsService, error) { krService := NewKeyringCredentialsService(log) if krService.IsSupported() { - err := enforceKeyringIntegrity(krService, log) - return krService, err + return krService, nil } log.Debug("Fallback to file managed credentials service due to unavailable system keyring") fcService, err := NewFileCredentialsService(log) if err != nil { - return fcService, enforceFilecredsIntegrity(fcService, err, log) - } - return fcService, nil -} - -func enforceKeyringIntegrity(ks *keyringCredentialsService, log logging.Logger) error { - // Catch any error that could require attention early - _, err := ks.List() - if err == nil { - return nil - } - - // If error from trying to fetch credentials list is not well known - // credentials handling is unavailable - if !errors.Is(err, &CorruptedError{}) && !errors.Is(err, &LoadError{}) { - return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) - } - - log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed") - err = ks.Reset() - if err != nil { - return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) - } - - return types.NewAgentError(types.ErrorCredentialCorruptedReset, err, nil) -} - -func enforceFilecredsIntegrity(fc *fileCredentialsService, err error, log logging.Logger) error { - if errors.Is(err, &LoadError{}) { - log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed") - err = fc.Reset() - if err != nil { - return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) - } - return types.NewAgentError(types.ErrorCredentialCorruptedReset, err, nil) + return fcService, err } - return types.NewAgentError(types.ErrorCredentialServiceUnavailable, err, nil) + return fcService, nil } diff --git a/internal/credentials/credentials_test.go b/internal/credentials/credentials_test.go index 6710089e5..6e35e68f9 100644 --- a/internal/credentials/credentials_test.go +++ b/internal/credentials/credentials_test.go @@ -7,8 +7,6 @@ import ( "testing" "github.com/posit-dev/publisher/internal/logging/loggingtest" - "github.com/posit-dev/publisher/internal/types" - "github.com/posit-dev/publisher/internal/util" "github.com/posit-dev/publisher/internal/util/utiltest" "github.com/spf13/afero" "github.com/stretchr/testify/suite" @@ -126,66 +124,3 @@ func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringErrFallba s.NoError(err) s.Implements((*CredentialsService)(nil), credservice) } - -func (s *CredentialsServiceTestSuite) TestNewCredentialsService_KeyringResetsOnCorruptedCred() { - keyring.MockInit() - - corruptedData := `{"":{"guid":"18cd5640-bee5-4b2a-992a-a2725ab6103d","version":0,"data":"unrecognizbledata"}}` - keyring.Set(ServiceName, "credentials", corruptedData) - - keyringCorruptedSave, err := keyring.Get(ServiceName, "credentials") - s.NoError(err) - s.Equal(keyringCorruptedSave, corruptedData) - - s.log.On("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed").Return() - - credservice, err := NewCredentialsService(s.log) - s.NotNil(err) - s.Implements((*CredentialsService)(nil), credservice) - - // Confirm agent error of corrupted credential bubbles up to warn the user - _, isCorruptedErr := types.IsAgentErrorOf(err, types.ErrorCredentialCorruptedReset) - s.True(isCorruptedErr) - - keyringUpdatedData, err := keyring.Get(ServiceName, "credentials") - s.NoError(err) - s.Equal(keyringUpdatedData, "{}") -} - -func (s *CredentialsServiceTestSuite) TestNewCredentialsService_FallbackFileResetsOnCorruptedCred() { - // Use an in memory filesystem for this test - // avoiding to manipulate users ~/.connect-credentials - fsys = afero.NewMemMapFs() - defer func() { fsys = afero.NewOsFs() }() - dirPath, err := util.UserHomeDir(fsys) - s.NoError(err) - - dirPath = dirPath.Join(".connect-credentials") - corruptedContents := []byte("unrecognizable_key = this is bad") - err = afero.WriteFile(fsys, dirPath.String(), corruptedContents, 0644) - s.NoError(err) - - keyringErr := errors.New("this is a teapot, unsupported system") - keyring.MockInitWithError(keyringErr) - - // Verify the corrupted contents are in - credsData, err := afero.ReadFile(fsys, dirPath.String()) - s.NoError(err) - s.Equal(credsData, corruptedContents) - - 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("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed").Return() - - credservice, err := NewCredentialsService(s.log) - s.NotNil(err) - s.Implements((*CredentialsService)(nil), credservice) - - // Confirm agent error of corrupted credential bubbles up to warn the user - _, isCorruptedErr := types.IsAgentErrorOf(err, types.ErrorCredentialCorruptedReset) - s.True(isCorruptedErr) - - credsData, err = afero.ReadFile(fsys, dirPath.String()) - s.NoError(err) - s.Equal(credsData, []byte("[credentials]\n")) -} diff --git a/internal/credentials/credentialstest/credentialstest.go b/internal/credentials/credentialstest/credentialstest.go new file mode 100644 index 000000000..8614a9dca --- /dev/null +++ b/internal/credentials/credentialstest/credentialstest.go @@ -0,0 +1,42 @@ +package credentialstest + +// Copyright (C) 2024 by Posit Software, PBC. + +import ( + "github.com/stretchr/testify/mock" + + "github.com/posit-dev/publisher/internal/credentials" +) + +type CredentialsServiceMock struct { + mock.Mock +} + +func NewCredentialsServiceMock() *CredentialsServiceMock { + return &CredentialsServiceMock{} +} + +func (m *CredentialsServiceMock) Delete(guid string) error { + args := m.Called(guid) + return args.Error(0) +} + +func (m *CredentialsServiceMock) Get(guid string) (*credentials.Credential, error) { + args := m.Called(guid) + return args.Get(0).(*credentials.Credential), args.Error(1) +} + +func (m *CredentialsServiceMock) List() ([]credentials.Credential, error) { + args := m.Called() + return args.Get(0).([]credentials.Credential), args.Error(1) +} + +func (m *CredentialsServiceMock) Set(name string, url string, ak string) (*credentials.Credential, error) { + args := m.Called(name, url, ak) + return args.Get(0).(*credentials.Credential), args.Error(1) +} + +func (m *CredentialsServiceMock) Reset() error { + args := m.Called() + return args.Error(0) +} diff --git a/internal/credentials/file.go b/internal/credentials/file.go index fb483e8df..c85a5a68a 100644 --- a/internal/credentials/file.go +++ b/internal/credentials/file.go @@ -208,6 +208,7 @@ func (c *fileCredentialsService) Delete(guid string) error { } func (c *fileCredentialsService) Reset() error { + c.log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "file") newData := newFileCredentials() return c.saveFile(newData) } diff --git a/internal/credentials/file_test.go b/internal/credentials/file_test.go index bc1bbc777..3553f8a18 100644 --- a/internal/credentials/file_test.go +++ b/internal/credentials/file_test.go @@ -543,6 +543,9 @@ func (s *FileCredentialsServiceSuite) TestReset() { s.NoError(err) s.Len(list, 2) + // Expected Log Warn + s.loggerMock.On("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "file").Return() + err = cs.Reset() s.NoError(err) diff --git a/internal/credentials/keyring.go b/internal/credentials/keyring.go index 2210c0703..0cb586ab3 100644 --- a/internal/credentials/keyring.go +++ b/internal/credentials/keyring.go @@ -130,6 +130,7 @@ func (ks *keyringCredentialsService) Set(name string, url string, ak string) (*C // Resets the CredentialTable from keyring // it is a last resort in case the keyring data turns out to be irrecognizable func (ks *keyringCredentialsService) Reset() error { + ks.log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "keyring") newTable := make(map[string]CredentialRecord) return ks.save(newTable) } diff --git a/internal/credentials/keyring_test.go b/internal/credentials/keyring_test.go index 013b1b3a7..7176c3aeb 100644 --- a/internal/credentials/keyring_test.go +++ b/internal/credentials/keyring_test.go @@ -182,6 +182,9 @@ func (s *KeyringCredentialsTestSuite) TestReset() { s.NoError(err) s.Len(creds, 2) + // Expected Log Warn + s.log.On("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "keyring").Return() + err = cs.Reset() s.NoError(err) diff --git a/internal/services/api/api_service.go b/internal/services/api/api_service.go index 752dd658a..467153b36 100644 --- a/internal/services/api/api_service.go +++ b/internal/services/api/api_service.go @@ -7,6 +7,7 @@ import ( "path" "github.com/posit-dev/publisher/internal/accounts" + "github.com/posit-dev/publisher/internal/credentials" "github.com/posit-dev/publisher/internal/events" "github.com/posit-dev/publisher/internal/logging" "github.com/posit-dev/publisher/internal/services/api/files" @@ -83,8 +84,9 @@ func RouterHandlerFunc(base util.AbsolutePath, lister accounts.AccountList, log Methods(http.MethodPost) // GET /api/credentials - r.Handle(ToPath("credentials"), GetCredentialsHandlerFunc(log)). - Methods(http.MethodGet) + r.Handle(ToPath("credentials"), GetCredentialsHandlerFunc(log, func(log logging.Logger) (credentials.CredentialsService, error) { + return credentials.NewCredentialsService(log) + })).Methods(http.MethodGet) // GET /api/credentials/$GUID r.Handle(ToPath("credentials", "{guid}"), GetCredentialHandlerFunc(log)). @@ -94,10 +96,15 @@ func RouterHandlerFunc(base util.AbsolutePath, lister accounts.AccountList, log r.Handle(ToPath("credentials"), PostCredentialFuncHandler(log)). Methods(http.MethodPost) - // DELETE /api/credentials + // DELETE /api/credentials/$GUID r.Handle(ToPath("credentials", "{guid}"), DeleteCredentialHandlerFunc(log)). Methods(http.MethodDelete) + // DELETE /api/credentials + r.Handle(ToPath("credentials"), ResetCredentialsHandlerFunc(log, func(log logging.Logger) (credentials.CredentialsService, error) { + return credentials.NewCredentialsService(log) + })).Methods(http.MethodDelete) + // POST /api/test-credentials r.Handle(ToPath("test-credentials"), PostTestCredentialsHandlerFunc(log)). Methods(http.MethodPost) diff --git a/internal/services/api/get_credentials.go b/internal/services/api/get_credentials.go index 633204d71..3e1b580c3 100644 --- a/internal/services/api/get_credentials.go +++ b/internal/services/api/get_credentials.go @@ -4,6 +4,7 @@ package api import ( "encoding/json" + "errors" "net/http" "github.com/posit-dev/publisher/internal/credentials" @@ -11,29 +12,28 @@ import ( "github.com/posit-dev/publisher/internal/types" ) -func GetCredentialsHandlerFunc(log logging.Logger) http.HandlerFunc { +func handleCredServiceError(w http.ResponseWriter, err error) { + agentErr := types.AsAgentError(err) + if errors.Is(err, &credentials.LoadError{}) || errors.Is(err, &credentials.CorruptedError{}) { + apiErr := types.APIErrorCredentialsCorruptedFromAgentError(*agentErr) + apiErr.JSONResponse(w) + return + } + apiErr := types.APIErrorCredentialsUnavailableFromAgentError(*agentErr) + apiErr.JSONResponse(w) +} + +func GetCredentialsHandlerFunc(log logging.Logger, credserviceFactory credentials.CredServiceFactory) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { - cs, err := credentials.NewCredentialsService(log) + cs, err := credserviceFactory(log) if err != nil { - if aerr, ok := err.(*types.AgentError); ok { - if aerr.Code == types.ErrorCredentialServiceUnavailable { - apiErr := types.APIErrorCredentialsUnavailableFromAgentError(*aerr) - apiErr.JSONResponse(w) - return - } - if aerr.Code == types.ErrorCredentialCorruptedReset { - apiErr := types.APIErrorCredentialCorruptedResetFromAgentError(*aerr) - apiErr.JSONResponse(w) - return - } - } - InternalError(w, req, log, err) + handleCredServiceError(w, err) return } creds, err := cs.List() if err != nil { - InternalError(w, req, log, err) + handleCredServiceError(w, err) return } w.Header().Set("content-type", "application/json") diff --git a/internal/services/api/get_credentials_test.go b/internal/services/api/get_credentials_test.go new file mode 100644 index 000000000..98ec0e324 --- /dev/null +++ b/internal/services/api/get_credentials_test.go @@ -0,0 +1,138 @@ +// Copyright (C) 2024 by Posit Software, PBC. + +package api + +import ( + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/posit-dev/publisher/internal/credentials" + "github.com/posit-dev/publisher/internal/credentials/credentialstest" + "github.com/posit-dev/publisher/internal/logging" + "github.com/posit-dev/publisher/internal/util/utiltest" + "github.com/stretchr/testify/suite" +) + +type GetCredentialsSuite struct { + utiltest.Suite + log logging.Logger + credservice *credentialstest.CredentialsServiceMock + credsFactory credentials.CredServiceFactory +} + +func TestGetCredentialsSuite(t *testing.T) { + suite.Run(t, new(GetCredentialsSuite)) +} + +func (s *GetCredentialsSuite) SetupTest() { + s.log = logging.New() + s.credservice = credentialstest.NewCredentialsServiceMock() + s.credsFactory = func(log logging.Logger) (credentials.CredentialsService, error) { + return s.credservice, nil + } +} + +func (s *GetCredentialsSuite) TestGetCredsList_Empty() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("GET", path, nil) + s.NoError(err) + + s.credservice.On("List").Return([]credentials.Credential{}, nil) + + rec := httptest.NewRecorder() + h := GetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + var res []credentials.Credential + dec := json.NewDecoder(rec.Body) + s.NoError(dec.Decode(&res)) + s.Len(res, 0) + s.Equal(http.StatusOK, rec.Result().StatusCode) +} + +func (s *GetCredentialsSuite) TestGetCredsListOk() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("GET", path, nil) + s.NoError(err) + + fakeCredsList := []credentials.Credential{ + { + GUID: "44a468b8-09c7-4c6d-a7a3-8cf164ddbaf8", + Name: "one", + URL: "https://one.com/connect", + ApiKey: "123456", + }, + { + GUID: "1b48b862-ce66-484c-9f03-a39870a0cfb5", + Name: "two", + URL: "https://two.com/connect", + ApiKey: "123456", + }, + } + s.credservice.On("List").Return(fakeCredsList, nil) + + rec := httptest.NewRecorder() + h := GetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + var res []credentials.Credential + dec := json.NewDecoder(rec.Body) + s.NoError(dec.Decode(&res)) + s.Len(res, 2) + s.Equal(res, fakeCredsList) + s.Equal(http.StatusOK, rec.Result().StatusCode) +} + +func (s *GetCredentialsSuite) TestGetCredsList_LoadError() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("GET", path, nil) + s.NoError(err) + + credsError := credentials.NewLoadError(errors.New("could not load!")) + s.credservice.On("List").Return([]credentials.Credential{}, credsError) + + rec := httptest.NewRecorder() + h := GetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + bodyRes := rec.Body.String() + s.Equal(http.StatusConflict, rec.Result().StatusCode) + s.Contains(bodyRes, `{"code":"credentialsCorrupted"}`) +} + +func (s *GetCredentialsSuite) TestGetCredsList_CorruptedError() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("GET", path, nil) + s.NoError(err) + + credsError := credentials.NewCorruptedError("qwerty") + s.credservice.On("List").Return([]credentials.Credential{}, credsError) + + rec := httptest.NewRecorder() + h := GetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + bodyRes := rec.Body.String() + s.Equal(http.StatusConflict, rec.Result().StatusCode) + s.Contains(bodyRes, `{"code":"credentialsCorrupted"}`) +} + +func (s *GetCredentialsSuite) TestGetCredsList_UnknownServiceErr() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("GET", path, nil) + s.NoError(err) + + credsError := errors.New("something errrddd") + s.credservice.On("List").Return([]credentials.Credential{}, credsError) + + rec := httptest.NewRecorder() + h := GetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + bodyRes := rec.Body.String() + s.Equal(http.StatusServiceUnavailable, rec.Result().StatusCode) + s.Contains(bodyRes, `{"code":"credentialsServiceUnavailable"}`) +} diff --git a/internal/services/api/reset_credentials.go b/internal/services/api/reset_credentials.go new file mode 100644 index 000000000..dccfddeca --- /dev/null +++ b/internal/services/api/reset_credentials.go @@ -0,0 +1,41 @@ +package api + +// Copyright (C) 2023 by Posit Software, PBC. + +import ( + "errors" + "net/http" + + "github.com/posit-dev/publisher/internal/credentials" + "github.com/posit-dev/publisher/internal/logging" + "github.com/posit-dev/publisher/internal/types" +) + +// We can ignore errors related to malformed data, we are resetting it ain't we? +func errIsNotLoadError(err error) bool { + return err != nil && !errors.Is(err, &credentials.LoadError{}) && !errors.Is(err, &credentials.CorruptedError{}) +} + +func unavailableCredsRes(w http.ResponseWriter, err error) { + agentErr := types.AsAgentError(err) + apiErr := types.APIErrorCredentialsUnavailableFromAgentError(*agentErr) + apiErr.JSONResponse(w) +} + +func ResetCredentialsHandlerFunc(log logging.Logger, credserviceFactory credentials.CredServiceFactory) http.HandlerFunc { + return func(w http.ResponseWriter, req *http.Request) { + cs, err := credserviceFactory(log) + if errIsNotLoadError(err) { + unavailableCredsRes(w, err) + return + } + + err = cs.Reset() + if err != nil { + unavailableCredsRes(w, err) + return + } + + w.WriteHeader(http.StatusNoContent) + } +} diff --git a/internal/services/api/reset_credentials_test.go b/internal/services/api/reset_credentials_test.go new file mode 100644 index 000000000..63754bee4 --- /dev/null +++ b/internal/services/api/reset_credentials_test.go @@ -0,0 +1,126 @@ +// Copyright (C) 2024 by Posit Software, PBC. + +package api + +import ( + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/posit-dev/publisher/internal/credentials" + "github.com/posit-dev/publisher/internal/credentials/credentialstest" + "github.com/posit-dev/publisher/internal/logging" + "github.com/posit-dev/publisher/internal/util/utiltest" + "github.com/stretchr/testify/suite" +) + +type ResetCredsSuite struct { + utiltest.Suite + log logging.Logger + credservice *credentialstest.CredentialsServiceMock + credsFactory credentials.CredServiceFactory +} + +func TestResetCredsSuite(t *testing.T) { + suite.Run(t, new(ResetCredsSuite)) +} + +func (s *ResetCredsSuite) SetupTest() { + s.log = logging.New() + s.credservice = credentialstest.NewCredentialsServiceMock() + s.credsFactory = func(log logging.Logger) (credentials.CredentialsService, error) { + return s.credservice, nil + } +} + +func (s *ResetCredsSuite) TestResetOk() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("DELETE", path, nil) + s.NoError(err) + + s.credservice.On("Reset").Return(nil) + + rec := httptest.NewRecorder() + h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + s.Equal(http.StatusNoContent, rec.Result().StatusCode) +} + +func (s *ResetCredsSuite) TestReset_EvenWithLoadError() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("DELETE", path, nil) + s.NoError(err) + + // Even if factory returns a LoadError, + // reset proceeds, that is the reason we are resetting data. + s.credsFactory = func(log logging.Logger) (credentials.CredentialsService, error) { + return s.credservice, credentials.NewLoadError(errors.New("load errsss")) + } + + s.credservice.On("Reset").Return(nil) + + rec := httptest.NewRecorder() + h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + s.Equal(http.StatusNoContent, rec.Result().StatusCode) +} + +func (s *ResetCredsSuite) TestReset_EvenWithCorruptedError() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("DELETE", path, nil) + s.NoError(err) + + // Even if factory returns a CorruptedError, + // reset proceeds, that is the reason we are resetting data. + s.credsFactory = func(log logging.Logger) (credentials.CredentialsService, error) { + return s.credservice, credentials.NewCorruptedError("qwerty") + } + + s.credservice.On("Reset").Return(nil) + + rec := httptest.NewRecorder() + h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + s.Equal(http.StatusNoContent, rec.Result().StatusCode) +} + +func (s *ResetCredsSuite) TestReset_UnknownError() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("DELETE", path, nil) + s.NoError(err) + + // Unknown errors reach the surface, + // reset proceeds, that is the reason we are resetting data. + s.credsFactory = func(log logging.Logger) (credentials.CredentialsService, error) { + return s.credservice, errors.New("this is terrible") + } + + rec := httptest.NewRecorder() + h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + bodyRes := rec.Body.String() + s.Equal(http.StatusServiceUnavailable, rec.Result().StatusCode) + s.Contains(bodyRes, `{"code":"credentialsServiceUnavailable"}`) +} + +func (s *ResetCredsSuite) TestReset_ErrorWhileResetting() { + path := "http://example.com/api/credentials" + req, err := http.NewRequest("DELETE", path, nil) + s.NoError(err) + + // Errors derived from resetting the data reach the surface. + s.credservice.On("Reset").Return(errors.New("this is terrible")) + + rec := httptest.NewRecorder() + h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) + h(rec, req) + + bodyRes := rec.Body.String() + s.Equal(http.StatusServiceUnavailable, rec.Result().StatusCode) + s.Contains(bodyRes, `{"code":"credentialsServiceUnavailable"}`) +} diff --git a/internal/types/api_errors.go b/internal/types/api_errors.go index aa1cc814d..57c40f1e3 100644 --- a/internal/types/api_errors.go +++ b/internal/types/api_errors.go @@ -162,17 +162,17 @@ func APIErrorCredentialsUnavailableFromAgentError(aerr AgentError) APIErrorCrede } } -type APIErrorCredentialCorruptedReset struct { +type APIErrorCredentialsCorrupted struct { Code ErrorCode `json:"code"` } -func (apierr *APIErrorCredentialCorruptedReset) JSONResponse(w http.ResponseWriter) { +func (apierr *APIErrorCredentialsCorrupted) JSONResponse(w http.ResponseWriter) { jsonResult(w, http.StatusConflict, apierr) } -func APIErrorCredentialCorruptedResetFromAgentError(aerr AgentError) APIErrorCredentialCorruptedReset { - return APIErrorCredentialCorruptedReset{ - Code: ErrorCredentialCorruptedReset, +func APIErrorCredentialsCorruptedFromAgentError(aerr AgentError) APIErrorCredentialsCorrupted { + return APIErrorCredentialsCorrupted{ + Code: ErrorCredentialsCorrupted, } } diff --git a/internal/types/api_errors_test.go b/internal/types/api_errors_test.go index 1d0c3ac80..4f6b8940b 100644 --- a/internal/types/api_errors_test.go +++ b/internal/types/api_errors_test.go @@ -92,19 +92,19 @@ func (s *ApiErrorsSuite) TestAPIErrorPythonExecNotFoundFromAgentError() { func (s *ApiErrorsSuite) TestAPIErrorCredentialCorruptedResetFromAgentError() { agentErr := AgentError{ Message: "", - Code: ErrorCredentialCorruptedReset, + Code: ErrorCredentialsCorrupted, Err: errors.New("unknown field error"), Data: ErrorData{}, } rec := httptest.NewRecorder() - apiError := APIErrorCredentialCorruptedResetFromAgentError(agentErr) - s.Equal(apiError.Code, ErrorCredentialCorruptedReset) + apiError := APIErrorCredentialsCorruptedFromAgentError(agentErr) + s.Equal(apiError.Code, ErrorCredentialsCorrupted) apiError.JSONResponse(rec) bodyRes := rec.Body.String() s.Equal(http.StatusConflict, rec.Result().StatusCode) - s.Contains(bodyRes, `{"code":"credentialCorruptedReset"}`) + s.Contains(bodyRes, `{"code":"credentialsCorrupted"}`) } diff --git a/internal/types/error.go b/internal/types/error.go index 705f9d240..736e35bfa 100644 --- a/internal/types/error.go +++ b/internal/types/error.go @@ -19,7 +19,7 @@ const ( ErrorUnknownTOMLKey ErrorCode = "unknownTOMLKey" ErrorInvalidConfigFiles ErrorCode = "invalidConfigFiles" ErrorCredentialServiceUnavailable ErrorCode = "credentialsServiceUnavailable" - ErrorCredentialCorruptedReset ErrorCode = "credentialCorruptedReset" + ErrorCredentialsCorrupted ErrorCode = "credentialsCorrupted" ErrorCertificateVerification ErrorCode = "errorCertificateVerification" ErrorRenvPackageVersionMismatch ErrorCode = "renvPackageVersionMismatch" ErrorRenvPackageSourceMissing ErrorCode = "renvPackageSourceMissing" From e6a2d1e91fd17d759377c1cb58aca56c5155682b Mon Sep 17 00:00:00 2001 From: Marcos Navarro Date: Tue, 31 Dec 2024 00:42:35 +0900 Subject: [PATCH 4/6] Backup credentials file on reset --- internal/credentials/credentials.go | 2 +- .../credentialstest/credentialstest.go | 4 +- internal/credentials/file.go | 46 +++++++++++++++++-- internal/credentials/file_test.go | 42 +++++++++++++++-- internal/credentials/keyring.go | 7 +-- internal/credentials/keyring_test.go | 2 +- internal/services/api/reset_credentials.go | 15 +++++- .../services/api/reset_credentials_test.go | 15 +++--- 8 files changed, 110 insertions(+), 23 deletions(-) diff --git a/internal/credentials/credentials.go b/internal/credentials/credentials.go index db5b9fccd..164f1c690 100644 --- a/internal/credentials/credentials.go +++ b/internal/credentials/credentials.go @@ -87,7 +87,7 @@ type CredentialsService interface { Get(guid string) (*Credential, error) List() ([]Credential, error) Set(name string, url string, ak string) (*Credential, error) - Reset() error + Reset() (string, error) } // The main credentials service constructor that determines if the system's keyring is available to be used, diff --git a/internal/credentials/credentialstest/credentialstest.go b/internal/credentials/credentialstest/credentialstest.go index 8614a9dca..84652c155 100644 --- a/internal/credentials/credentialstest/credentialstest.go +++ b/internal/credentials/credentialstest/credentialstest.go @@ -36,7 +36,7 @@ func (m *CredentialsServiceMock) Set(name string, url string, ak string) (*crede return args.Get(0).(*credentials.Credential), args.Error(1) } -func (m *CredentialsServiceMock) Reset() error { +func (m *CredentialsServiceMock) Reset() (string, error) { args := m.Called() - return args.Error(0) + return args.Get(0).(string), args.Error(1) } diff --git a/internal/credentials/file.go b/internal/credentials/file.go index c85a5a68a..253548e27 100644 --- a/internal/credentials/file.go +++ b/internal/credentials/file.go @@ -4,8 +4,10 @@ package credentials import ( "fmt" + "io" "os" "sync" + "time" "github.com/google/uuid" "github.com/pelletier/go-toml/v2" @@ -207,10 +209,48 @@ func (c *fileCredentialsService) Delete(guid string) error { return nil } -func (c *fileCredentialsService) Reset() error { - c.log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "file") +// Resets the credentials file +// it is a last resort in case the data turns out to be irrecognizable +// Returns the backup path of the original credentials file +func (c *fileCredentialsService) Reset() (string, error) { + copyFilename, err := c.backupFile() + if err != nil { + return "", err + } + c.log.Warn("Corrupted credentials data found. The stored data was reset.", "credentials_service", "file") + c.log.Warn("Previous credentials file backed up.", "credentials_backup", copyFilename) newData := newFileCredentials() - return c.saveFile(newData) + return copyFilename, c.saveFile(newData) +} + +func (c *fileCredentialsService) backupFile() (string, error) { + backupTimestamp := time.Now().Format(time.DateOnly) + credsCopyPath := c.credsFilepath.Dir().Join(fmt.Sprintf("%s-%s", ondiskFilename, backupTimestamp)) + + _, err := credsCopyPath.Stat() + if os.IsNotExist(err) { + file, err := credsCopyPath.Create() + if err != nil { + return "", err + } + file.Close() + } + + credsCopyFile, err := credsCopyPath.OpenFile(os.O_TRUNC|os.O_RDWR, 0644) + if err != nil { + return "", err + } + defer credsCopyFile.Close() + + credsFile, err := c.credsFilepath.Open() + if err != nil { + return "", err + } + defer credsFile.Close() + + _, err = io.Copy(credsCopyFile, credsFile) + + return credsCopyPath.String(), err } func (c *fileCredentialsService) setup() error { diff --git a/internal/credentials/file_test.go b/internal/credentials/file_test.go index 3553f8a18..02cd20e86 100644 --- a/internal/credentials/file_test.go +++ b/internal/credentials/file_test.go @@ -3,9 +3,11 @@ package credentials import ( + "fmt" "os" "runtime" "testing" + "time" "github.com/posit-dev/publisher/internal/logging/loggingtest" "github.com/posit-dev/publisher/internal/util" @@ -530,13 +532,19 @@ func (s *FileCredentialsServiceSuite) TestReset() { credsFilepath: s.testdata.Join("to-reset.toml"), } - _, err := cs.load() + expectedCredsBackupPath := s.testdata.Join(fmt.Sprintf(".connect-credentials-%s", time.Now().Format(time.DateOnly))) + + // Creds backup shouldn't exist and should be cleared after each test + _, err := expectedCredsBackupPath.Stat() + s.ErrorIs(err, os.ErrNotExist) + + _, err = cs.load() s.NoError(err) - _, err = cs.Set("newcred", "https://b2.connect-server:3939/connect", "abcdeC2aqbh7dg8TO43XPu7r56YDh002") + credOne, err := cs.Set("newcred", "https://b2.connect-server:3939/connect", "abcdeC2aqbh7dg8TO43XPu7r56YDh002") s.NoError(err) - _, err = cs.Set("newcredtwo", "https://b5.connect-server:3939/connect", "abcdeC2aqbh7dg8TO43XPu7r56YDh007") + credTwo, err := cs.Set("newcredtwo", "https://b5.connect-server:3939/connect", "abcdeC2aqbh7dg8TO43XPu7r56YDh007") s.NoError(err) list, err := cs.List() @@ -544,7 +552,8 @@ func (s *FileCredentialsServiceSuite) TestReset() { s.Len(list, 2) // Expected Log Warn - s.loggerMock.On("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "file").Return() + s.loggerMock.On("Warn", "Corrupted credentials data found. The stored data was reset.", "credentials_service", "file").Return() + s.loggerMock.On("Warn", "Previous credentials file backed up.", "credentials_backup", expectedCredsBackupPath.String()).Return() err = cs.Reset() s.NoError(err) @@ -553,4 +562,29 @@ func (s *FileCredentialsServiceSuite) TestReset() { list, err = cs.List() s.NoError(err) s.Len(list, 0) + + // Creds backup exists + _, err = expectedCredsBackupPath.Stat() + s.NoError(err) + + // Backup content is properly written + backupContents, err := expectedCredsBackupPath.ReadFile() + s.NoError(err) + + s.Equal(fmt.Sprintf(`[credentials] +[credentials.newcred] +guid = '%s' +version = 0 +url = 'https://b2.connect-server:3939/connect' +api_key = 'abcdeC2aqbh7dg8TO43XPu7r56YDh002' + +[credentials.newcredtwo] +guid = '%s' +version = 0 +url = 'https://b5.connect-server:3939/connect' +api_key = 'abcdeC2aqbh7dg8TO43XPu7r56YDh007' +`, credOne.GUID, credTwo.GUID), string(backupContents)) + + err = os.Remove(expectedCredsBackupPath.String()) + s.NoError(err) } diff --git a/internal/credentials/keyring.go b/internal/credentials/keyring.go index 0cb586ab3..2f5d0738f 100644 --- a/internal/credentials/keyring.go +++ b/internal/credentials/keyring.go @@ -129,10 +129,11 @@ func (ks *keyringCredentialsService) Set(name string, url string, ak string) (*C // Resets the CredentialTable from keyring // it is a last resort in case the keyring data turns out to be irrecognizable -func (ks *keyringCredentialsService) Reset() error { - ks.log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "keyring") +// There is no backup for keyring data due to encryption, always returns empty string +func (ks *keyringCredentialsService) Reset() (string, error) { + ks.log.Warn("Corrupted credentials data found. The stored data was reset.", "credentials_service", "keyring") newTable := make(map[string]CredentialRecord) - return ks.save(newTable) + return "", ks.save(newTable) } func (ks *keyringCredentialsService) checkForConflicts( diff --git a/internal/credentials/keyring_test.go b/internal/credentials/keyring_test.go index 7176c3aeb..58fb2c55b 100644 --- a/internal/credentials/keyring_test.go +++ b/internal/credentials/keyring_test.go @@ -183,7 +183,7 @@ func (s *KeyringCredentialsTestSuite) TestReset() { s.Len(creds, 2) // Expected Log Warn - s.log.On("Warn", "Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "keyring").Return() + s.log.On("Warn", "Corrupted credentials data found. The stored data was reset.", "credentials_service", "keyring").Return() err = cs.Reset() s.NoError(err) diff --git a/internal/services/api/reset_credentials.go b/internal/services/api/reset_credentials.go index dccfddeca..bd46ebb71 100644 --- a/internal/services/api/reset_credentials.go +++ b/internal/services/api/reset_credentials.go @@ -3,6 +3,7 @@ package api // Copyright (C) 2023 by Posit Software, PBC. import ( + "encoding/json" "errors" "net/http" @@ -24,18 +25,28 @@ func unavailableCredsRes(w http.ResponseWriter, err error) { func ResetCredentialsHandlerFunc(log logging.Logger, credserviceFactory credentials.CredServiceFactory) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { + result := struct { + BackupFile string `json:"backupFile"` + }{} + cs, err := credserviceFactory(log) if errIsNotLoadError(err) { unavailableCredsRes(w, err) return } - err = cs.Reset() + backupFile, err := cs.Reset() if err != nil { unavailableCredsRes(w, err) return } - w.WriteHeader(http.StatusNoContent) + if backupFile != "" { + result.BackupFile = backupFile + } + + w.Header().Set("content-type", "application/json") + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(result) } } diff --git a/internal/services/api/reset_credentials_test.go b/internal/services/api/reset_credentials_test.go index 63754bee4..a031c3b0d 100644 --- a/internal/services/api/reset_credentials_test.go +++ b/internal/services/api/reset_credentials_test.go @@ -39,13 +39,14 @@ func (s *ResetCredsSuite) TestResetOk() { req, err := http.NewRequest("DELETE", path, nil) s.NoError(err) - s.credservice.On("Reset").Return(nil) + s.credservice.On("Reset").Return(".backup-file", nil) rec := httptest.NewRecorder() h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) h(rec, req) - s.Equal(http.StatusNoContent, rec.Result().StatusCode) + s.Equal(http.StatusOK, rec.Result().StatusCode) + s.Contains(rec.Body.String(), `{"backup_file":".backup-file"}`) } func (s *ResetCredsSuite) TestReset_EvenWithLoadError() { @@ -59,13 +60,13 @@ func (s *ResetCredsSuite) TestReset_EvenWithLoadError() { return s.credservice, credentials.NewLoadError(errors.New("load errsss")) } - s.credservice.On("Reset").Return(nil) + s.credservice.On("Reset").Return("", nil) rec := httptest.NewRecorder() h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) h(rec, req) - s.Equal(http.StatusNoContent, rec.Result().StatusCode) + s.Equal(http.StatusOK, rec.Result().StatusCode) } func (s *ResetCredsSuite) TestReset_EvenWithCorruptedError() { @@ -79,13 +80,13 @@ func (s *ResetCredsSuite) TestReset_EvenWithCorruptedError() { return s.credservice, credentials.NewCorruptedError("qwerty") } - s.credservice.On("Reset").Return(nil) + s.credservice.On("Reset").Return("", nil) rec := httptest.NewRecorder() h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) h(rec, req) - s.Equal(http.StatusNoContent, rec.Result().StatusCode) + s.Equal(http.StatusOK, rec.Result().StatusCode) } func (s *ResetCredsSuite) TestReset_UnknownError() { @@ -114,7 +115,7 @@ func (s *ResetCredsSuite) TestReset_ErrorWhileResetting() { s.NoError(err) // Errors derived from resetting the data reach the surface. - s.credservice.On("Reset").Return(errors.New("this is terrible")) + s.credservice.On("Reset").Return("", errors.New("this is terrible")) rec := httptest.NewRecorder() h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) From 4bd1fdc7fd127be1d98c535d2b0858bf42731ce4 Mon Sep 17 00:00:00 2001 From: Marcos Navarro Date: Tue, 31 Dec 2024 01:33:41 +0900 Subject: [PATCH 5/6] Display backup file path to the user --- extensions/vscode/src/state.test.ts | 6 ++++-- extensions/vscode/src/state.ts | 4 ++-- extensions/vscode/src/utils/errorTypes.ts | 9 +++++++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/extensions/vscode/src/state.test.ts b/extensions/vscode/src/state.test.ts index a5ce34977..d31d4472b 100644 --- a/extensions/vscode/src/state.test.ts +++ b/extensions/vscode/src/state.test.ts @@ -477,7 +477,9 @@ describe("PublisherState", () => { describe("resetCredentials", () => { test("calls to reset credentials and shows a warning", async () => { - mockClient.credentials.reset.mockResolvedValue({}); + mockClient.credentials.reset.mockResolvedValue({ + data: { backupFile: "backup-file" }, + }); const { mockContext } = mkExtensionContextStateMock({}); const publisherState = new PublisherState(mockContext); @@ -489,7 +491,7 @@ describe("PublisherState", () => { // Warning message is called expect(window.showWarningMessage).toHaveBeenCalledWith( - "Unrecognizable credentials for Posit Publisher were found and removed. Credentials may need to be recreated.", + "Unrecognizable credentials for Posit Publisher were found and removed. Credentials may need to be recreated. Previous credentials data backed up at backup-file", ); }); diff --git a/extensions/vscode/src/state.ts b/extensions/vscode/src/state.ts index 285f2d7af..0c03b2f44 100644 --- a/extensions/vscode/src/state.ts +++ b/extensions/vscode/src/state.ts @@ -299,11 +299,11 @@ export class PublisherState implements Disposable { // Calls to reset all credentials data stored. // Meant to be a last resort when we get loading data or corrupted data errors. async resetCredentials() { - const warnMsg = errCredentialsCorruptedMessage(); try { const api = await useApi(); - await api.credentials.reset(); + const response = await api.credentials.reset(); this.credentials = []; + const warnMsg = errCredentialsCorruptedMessage(response.data.backupFile); window.showWarningMessage(warnMsg); } catch (err: unknown) { const summary = getSummaryStringFromError("resetCredentials", err); diff --git a/extensions/vscode/src/utils/errorTypes.ts b/extensions/vscode/src/utils/errorTypes.ts index 3a84c8b13..eb71ffefe 100644 --- a/extensions/vscode/src/utils/errorTypes.ts +++ b/extensions/vscode/src/utils/errorTypes.ts @@ -167,8 +167,13 @@ export const isErrInvalidConfigFile = export type ErrCredentialsCorrupted = MkErrorDataType<"credentialsCorrupted">; export const isErrCredentialsCorrupted = mkErrorTypeGuard("credentialsCorrupted"); -export const errCredentialsCorruptedMessage = () => { - return "Unrecognizable credentials for Posit Publisher were found and removed. Credentials may need to be recreated."; +export const errCredentialsCorruptedMessage = (backupFile: string) => { + let msg = + "Unrecognizable credentials for Posit Publisher were found and removed. Credentials may need to be recreated."; + if (backupFile) { + msg += ` Previous credentials data backed up at ${backupFile}`; + } + return msg; }; // Tries to match an Axios error that comes with an identifiable Json structured data From f3f69ab041d8069fd7a8661dc7ddb341d937363d Mon Sep 17 00:00:00 2001 From: Marcos Navarro Date: Tue, 31 Dec 2024 01:39:50 +0900 Subject: [PATCH 6/6] Fix a couple tests --- internal/credentials/file_test.go | 3 ++- internal/credentials/keyring_test.go | 2 +- internal/services/api/reset_credentials_test.go | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/credentials/file_test.go b/internal/credentials/file_test.go index 02cd20e86..66189ea1f 100644 --- a/internal/credentials/file_test.go +++ b/internal/credentials/file_test.go @@ -555,8 +555,9 @@ func (s *FileCredentialsServiceSuite) TestReset() { s.loggerMock.On("Warn", "Corrupted credentials data found. The stored data was reset.", "credentials_service", "file").Return() s.loggerMock.On("Warn", "Previous credentials file backed up.", "credentials_backup", expectedCredsBackupPath.String()).Return() - err = cs.Reset() + backupPath, err := cs.Reset() s.NoError(err) + s.Equal(backupPath, expectedCredsBackupPath.String()) // Creds wiped out list, err = cs.List() diff --git a/internal/credentials/keyring_test.go b/internal/credentials/keyring_test.go index 58fb2c55b..fe16bbabf 100644 --- a/internal/credentials/keyring_test.go +++ b/internal/credentials/keyring_test.go @@ -185,7 +185,7 @@ func (s *KeyringCredentialsTestSuite) TestReset() { // Expected Log Warn s.log.On("Warn", "Corrupted credentials data found. The stored data was reset.", "credentials_service", "keyring").Return() - err = cs.Reset() + _, err = cs.Reset() s.NoError(err) // Creds wiped out diff --git a/internal/services/api/reset_credentials_test.go b/internal/services/api/reset_credentials_test.go index a031c3b0d..71ad2098e 100644 --- a/internal/services/api/reset_credentials_test.go +++ b/internal/services/api/reset_credentials_test.go @@ -39,14 +39,14 @@ func (s *ResetCredsSuite) TestResetOk() { req, err := http.NewRequest("DELETE", path, nil) s.NoError(err) - s.credservice.On("Reset").Return(".backup-file", nil) + s.credservice.On("Reset").Return("backup-file", nil) rec := httptest.NewRecorder() h := ResetCredentialsHandlerFunc(s.log, s.credsFactory) h(rec, req) s.Equal(http.StatusOK, rec.Result().StatusCode) - s.Contains(rec.Body.String(), `{"backup_file":".backup-file"}`) + s.Contains(rec.Body.String(), `{"backupFile":"backup-file"}`) } func (s *ResetCredsSuite) TestReset_EvenWithLoadError() {