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

Reset corrupted credentials storage when applicable #2504

Open
wants to merge 2 commits into
base: main
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
9 changes: 9 additions & 0 deletions extensions/vscode/src/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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);
}
Expand Down
18 changes: 17 additions & 1 deletion extensions/vscode/src/utils/errorTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ export type ErrorCode =
| "deployedContentNotRunning"
| "tomlValidationError"
| "tomlUnknownError"
| "pythonExecNotFound";
| "pythonExecNotFound"
| "credentialCorruptedReset";

export type axiosErrorWithJson<T = { code: ErrorCode; details: unknown }> =
AxiosError & {
Expand Down Expand Up @@ -162,6 +163,15 @@ export type ErrInvalidConfigFiles = MkErrorDataType<
export const isErrInvalidConfigFile =
mkErrorTypeGuard<ErrInvalidConfigFiles>("invalidConfigFile");

// Invalid configuration file(s)
export type ErrCredentialsReset = MkErrorDataType<"credentialCorruptedReset">;
export const isErrCredentialsReset = mkErrorTypeGuard<ErrInvalidConfigFiles>(
"credentialCorruptedReset",
);
export const errCredentialsResetMessage = () => {
return "Stored credentials for Posit Publisher found in an unrecognizable state. A reset was required in order to proceed.";
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

The current wording might leave the user trying to figure out how to reset the credentials even though we have already done it for them. It's long and could use some further refinement, but how about something like this?

Unable to use existing stored credentials for Posit Publisher (reason: unrecognizable state). The existing credentials have been deleted. Recreate them using the Credentials section or the Command Pallet's "Posit Publisher: New Credential" command.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it should be very clear that something has already been done. Another suggestion for the re-word:

Unrecognizable credentials for Posit Publisher were found and removed. Credentials may need to be recreated.

I went with "removed" instead of "deleted" to be a bit softer but still clear that they are no longer on the system. I left the recreation message at the end since it will be collapsed most of the time, and since they have already made credentials (we know this because they needed a reset) did not include instructions.


// 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) {
Expand All @@ -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<ErrUnknown>);
}
7 changes: 6 additions & 1 deletion internal/accounts/provider_credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down
44 changes: 41 additions & 3 deletions internal/credentials/credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ package credentials

import (
"encoding/json"
"errors"

"github.com/posit-dev/publisher/internal/logging"
"github.com/posit-dev/publisher/internal/types"
Expand Down Expand Up @@ -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)
Comment on lines +99 to +106
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was a bit confused how the credentials were being reset right when the extension activated, but if I'm understanding correctly the enforcement happens before the APIs are even called since it occurs during the setup of the Credentials Service.

Perhaps we can do something with a different flow to ensure that notifications are always present?

Some potential ideas (these are really quick so take with a large grain of salt):

  • have a POST /api/credentials/reset endpoint that we can call when we get certain errors
  • only call the enforcement when the APIs are actually called

}

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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have two separate logs so we know which credential data we are resetting between the keyring and the file?

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)
}
65 changes: 65 additions & 0 deletions internal/credentials/credentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"))
}
10 changes: 10 additions & 0 deletions internal/credentials/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
7 changes: 6 additions & 1 deletion internal/credentials/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
28 changes: 28 additions & 0 deletions internal/credentials/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
9 changes: 8 additions & 1 deletion internal/credentials/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
28 changes: 28 additions & 0 deletions internal/credentials/keyring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
1 change: 1 addition & 0 deletions internal/credentials/testdata/toml/to-reset.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[credentials]
5 changes: 5 additions & 0 deletions internal/services/api/get_credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions internal/types/api_errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand Down
Loading
Loading