Store PasswordExpiry and OAuthRefreshToken #1464
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add properties ICredential.PasswordExpiryUTC and ICredential.OAuthRefreshToken. These correspond to Git credential attributes password_expiry_utc and oauth_refresh_token, see https://git-scm.com/docs/git-credential#IOFMT. Previously these attributes were silently disarded. Plumb these properties from input to host provider to credential store to output. Credential store support for these attributes is optional, marked by new properties ICredentialStore.CanStorePasswordExpiryUTC and ICredentialStore.CanStoreOAuthRefreshToken. Implement support in CredentialCacheStore, SecretServiceCollection and WindowsCredentialManager. Add method IHostProvider.ValidateCredentialAsync. The default implementation simply checks expiry. Improve implementations of GenericHostProvider and GitLabHostProvider. Previously, GetCredentialAsync saved credentials as a side effect. This is no longer necessary. The workaround to store OAuth refresh tokens under a separate service is no longer necessary assuming CredentialStore.CanStoreOAuthRefreshToken. Querying GitLab to check token expiration is no longer necessary assuming CredentialStore.CanStorePasswordExpiryUTC.
hickford
force-pushed
the
expiry-and-oauth-refresh-token
branch
3 times, most recently
from
a2425b9 to
a21c066
Compare
hickford
changed the title
hickford
force-pushed
the
expiry-and-oauth-refresh-token
branch
from
a21c066 to
2a14b66
Compare
hickford
changed the title
|
Windows build ought to be fixed by #1418 |
hickford
force-pushed
the
expiry-and-oauth-refresh-token
branch
2 times, most recently
from
83c3f3a to
b59547b
Compare
ldennington
requested changes
Apr 15, 2024
hickford
force-pushed
the
expiry-and-oauth-refresh-token
branch
4 times, most recently
from
01fcd91 to
fdd7759
Compare
hickford
force-pushed
the
expiry-and-oauth-refresh-token
branch
from
fdd7759 to
60224e0
Compare
ldennington
approved these changes
Dec 14, 2024
PetSerAl
reviewed
Jan 1, 2025
Marshal.FreeHGlobal and Marshal.DestroyStructure
|
Hi, what is going on with this request? The last time I checked my GitHub
account was suspended.
…On Thu, Jan 2, 2025 at 4:43 PM M Hickford ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/shared/Core/HostProvider.cs
<#1464 (comment)>
:
> + Context.Trace.WriteLine("Existing credential satisfies validation.");
+ return credential;
+ }
+ else
+ {
+ Context.Trace.WriteLine("Existing credential fails validation.");
+ if (credential.OAuthRefreshToken != null)
+ {
+ Context.Trace.WriteLine("Found OAuth refresh token.");
+ input = new InputArguments(input, credential.OAuthRefreshToken);
+ }
+ Context.Trace.WriteLine("Erasing invalid credential...");
+ // Why necessary to erase? We can't be sure that storing a fresh
+ // credential will overwrite the invalid credential, particularly
+ // if the usernames differ.
+ Context.CredentialStore.Remove(service, credential);
Here the whole credential is removed from storage, including any OAuth
refresh token.
Typically GenerateCredentialAsync reauthenticates if OAuth refresh fails
for any reason.
Is improvement possible in the network failure case? I can't see how.
—
Reply to this email directly, view it on GitHub
<#1464 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BKZXMWTCXNISR23CCR3QSYD2IW6H5AVCNFSM6AAAAABQOMKEJGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRYGMYTONJYHE>
.
You are receiving this because you commented.Message ID:
***@***.***
com>
|
Limit iterations in GetCredentialAsync
PetSerAl
reviewed
Jan 3, 2025
There was a problem hiding this comment.
My opinion (feal free to disregard it): it is better not to remove refresh token unnecessary. I understand, that GCM can just reauthenticate, but that require extra, otherwise unnecessary, browser interaction from user, thus providing slightly worse user experience.
Anyways, I have no intentions to spend my time to pursue this. Error rate unlikely would be that hi to bother me that much.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add properties ICredential.PasswordExpiry and ICredential.OAuthRefreshToken. These correspond to Git credential attributes password_expiry_utc and oauth_refresh_token, see https://git-scm.com/docs/git-credential#IOFMT. Previously these attributes were silently disarded.
Plumb these properties from input to host provider to credential store to output.
Credential store support for these attributes is optional, marked by new properties CredentialStore.CanStorePasswordExpiry and ICredentialStore.CanStoreOAuthRefreshToken. Implement support in CredentialCacheStore, SecretServiceCollection and WindowsCredentialManager.
Add method IHostProvider.ValidateCredentialAsync. The default implementation simply checks expiry. Other implementations might query a server.
Improve implementations of GenericHostProvider and GitLabHostProvider. Previously, GetCredentialAsync saved credentials as a side effect. This is no longer necessary. The workaround to store OAuth refresh tokens under a separate service is no longer necessary assuming CredentialStore.CanStoreOAuthRefreshToken. Querying GitLab to check token expiration is no longer necessary assuming CredentialStore.CanStorePasswordExpiry.
Fixes #1463
Fixes #268