fix: #1802 confirm cached credential before rejecting #1803
Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
I left a few nits based on the repo style guide, but they aren't critical.
Also, I merged your change with mine in #1804 to make sure that everything built, passed unit tests, and ran against a full production repo just fine. The merge is in derrickstolee:oath-change-2024.
I'm currently running the default functional test suite as an extra precaution, but I don't anticipate that this change will have any effect because the test suite operates against a public repo and thus does not test the auth stack in any way.
You may also want to cherry-pick abcb73f since that will set the default mode to oauth, giving us faster adoption of the mode in VFS for Git customers.
| @@ -91,9 +91,27 @@ public void RejectCredentials(ITracer tracer, string credentialString) | |||
| { | |||
| lock (this.gitAuthLock) | |||
| { | |||
| var cachedCredentialAtStartOfReject = this.cachedCredentialString; | |||
There was a problem hiding this comment.
The norm in this repo is to not use var unless the right-hand-side is a constructor explicitly mentioning the type that the variable will become.
| var cachedCredentialAtStartOfReject = this.cachedCredentialString; | |
| string cachedCredentialAtStartOfReject = this.cachedCredentialString; | |
| { | ||
| // We can't assume that the credential store's cached credential is the same as the one we have. | ||
| // Reload the credential from the store to ensure we're rejecting the correct one. | ||
| var attemptsBeforeCheckingExistingCredential = this.numberOfAttempts; |
There was a problem hiding this comment.
The norm in this repo is to not use var unless the right-hand-side is a constructor explicitly mentioning the type that the variable will become.
| var attemptsBeforeCheckingExistingCredential = this.numberOfAttempts; | |
| int attemptsBeforeCheckingExistingCredential = this.numberOfAttempts; | |
To more rapidly adopt OAuth tokens, set that as the default for GVFS repos. This will update on mount, so all users will update to this mode when they upgrade to a version including this commit. This may cause some initial frustration as the first time I ran a fetch in OAuth mode my local clone had a failure with GCM and defaulted to username/password checks over command line. The second fetch worked just fine, though.
|
@derrickstolee Feedback taken |
Issue
When GCM is in OAuth mode, rejecting the credential is equivalent to a sign-out request, which means the next attempt to get a credential will always cause a pop-up. See #1802
** Changes **
Before rejecting a credential try getting the credential from GCM again.
If GCM still returns the same credential, then tell GCM to reject it.
If GCM returns a different credential, don't attempt to reject the previous one - just return and let the caller try the new credential instead.