Reset corrupted credentials storage when applicable #2504
Conversation
| ); | ||
| export const errCredentialsResetMessage = () => { | ||
| return "Stored credentials for Posit Publisher found in an unrecognizable state. A reset was required in order to proceed."; | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Could you expand a little bit on your statement:
Isn't the reason why we won't show a warning is because the storage would have already been reset? |
There was a problem hiding this comment.
Here is an example of malformed data that can be used to update the keychain record. This when keyring is supported, Desktop version of Positron or VSCode.
Is there a good way for me to reproduce malformed data in my keychain record?
For testing workbench, update the
.connect-credentialsfile with an unrecognizable schema
Testing on Workbench this works, but I am not getting the notification of the reset when reloading the page / accessing the session for the first time. If I set .connect-credentials to something invalid and refresh the home view I see the notification, but we will need to look at this flow to ensure that notification is shown.
That may be what you were talking about here:
A warning will be shown if the IDE is already open, if a new Positron or VSCode session is open, we won't show a warning, since the early stages of initialization of the extension do not communicate status with the extension.
But like @sagerb I think I need some more details on this, and what we could possibly do to get around it.
|
|
||
| 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.
Should we have two separate logs so we know which credential data we are resetting between the keyring and the file?
| 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) |
There was a problem hiding this comment.
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/resetendpoint that we can call when we get certain errors - only call the enforcement when the APIs are actually called
Intent
This PR introduces a new reset step for credentials storage, both keyring and file based, when the data gets into a bad or unrecognizable state.
A warning will be shown if the IDE is already open, if a new Positron or VSCode session is open, we won't show a warning, since the early stages of initialization of the extension do not communicate status with the extension.
Warning message screenshot
Output logged warning
Fixes #2491 #2492
Type of Change
Approach
If we detect a malformed state of the credentials data, we proceed to reset it.
User Impact
There were a couple reported cases internally in which the Publisher extension wouldn't finish loadind and didn't provide much details other than some hints in the output logs.
This due to a corrupted or old and unsupported credentials data.
With this fix, the unusable credentials data will be reset and the user will be able to continue using Publisher.
Automated Tests
Updated and added tests accordingly.
Directions for Reviewers
Here is an example of malformed data that can be used to update the keychain record. This when keyring is supported, Desktop version of Positron or VSCode.
For testing workbench, update the
.connect-credentialsfile with an unrecognizable schema, e.g:Checklist