Reset corrupted credentials storage when applicable #2504
Conversation
|
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.
…eset creds and always display a warning message when reset takes place
marcosnav
force-pushed
the
mnv-creds-reset
branch
from
a6e3418 to
6f07f14
Compare
|
@sagerb @dotNomad PR ready for another look. Changed a bit the way of resetting credentials, now it is always controlled via an endpoint and the warning message is consistent so the user will always be informed when we clear up the credentials data.
The only way I've found is updating it with bad data directly on keychain. I tried replicating it using only the Publisher but couldn't find a scenario that saves data in a bad state, so my assumption is that only an external influence on the system outside Publisher leads to a bad state. |
|
Thanks for jumping on this, Marcos! I know that I'm coming to this late, but would there be any way for us to skip over invalid credentials when we parse these? We might be stuck here since we us a single file and we wouldn't be able to write back to that file a new, correct credential. But in an ideal world we wouldn't be required to wipe anything, just let the user know that there are no (valid) credentials, they should make a new one to continue. That would also make this more robust to other unknown failures. But it's possible the architecture we have with a single credential file and writing to that file prevents us from doing that. A slightly separate issue (though it would be side-stepped if we could do the above) is that it's not super user-friendly to delete a file from the filesystem without any sort of confirmation or backup of what was deleted. I know that this circumstance should be pretty rare (basically only people at Posit who used earlier versions of the publisher), but it's not something we should do generally. We should either add a confirmation that tells someone we are about to delete that file and that if there's anything important in it they should check it, or we should create a backup version of it before we delete it (and we should tell folks we did that). |
internal/credentials/file.go
Outdated
| @@ -207,6 +207,12 @@ 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") | |||
There was a problem hiding this comment.
We will have a different behavior here anyway (see my comment on the PR), so this suggestion is a little moot, but I'm going to make it anyway since precisions and conciseness are skills and seeing edits is one way to learn that
| c.log.Warn("Corrupted credentials data found. A reset was applied to stored data to be able to proceed.", "credentials_service", "file") | |
| c.log.Warn("Corrupted credentials data found. The stored data was reset.", "credentials_service", "file") |
You are right that the work required to skip invalid credentials would require non-trivial effort. Since there are many possibilities for a file to not comply with TOML or with the expected schema.
Yes, I was thinking that re-creating credentials in Publisher is an easy thing, however Connect does not display values for already created API Keys, so having to do that again is not great. I like the idea of backing up the file, it is totally possible to |
Sounds good. We'll want to craft the message to be clear + actionable (ideally also lead someone to that file easily), but that should help folks not get stuck.
Yeah, totally. In many cases creating a new API key is NBD, but if for whatever reason (the person doesn't remember how, they were given this key by someone else, they don't remember the URL(s) of the connect server(s) they had setup, etc.) it would be particularly painful. |
|
Updated PR, now backing up the previous credentials file and displaying to the user the backup file path. The message Additional log with file backup path |
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 displayed to let the user know that very likely it'll be necessary to recreate the credentials.
Warning message screenshot
Output logged warning, notice the
credentials_servicelog field to track the strategy being cleared upFixes #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 loading 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:The previous credentials file should be backed up and can be found at
.credentials-file-{DATE}Checklist