-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
Comment on lines
+99
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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):
|
||
} | ||
|
||
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
[credentials] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.