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
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
from
2a14b66
to
83c3f3a
Compare
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
from
83c3f3a
to
b59547b
Compare
ldennington
requested changes
Apr 15, 2024
| /// The expiry date of the password. This is Git's password_expiry_utc | ||
| /// attribute. https://git-scm.com/docs/git-credential#Documentation/git-credential.txt-codepasswordexpiryutccode | ||
| /// </summary> | ||
| DateTimeOffset? PasswordExpiry { get => null; } |
There was a problem hiding this comment.
This may need to be updated to avoid default interface methods given the current situation with .NET 8 on Windows.
| /// An OAuth refresh token. This is Git's oauth_refresh_token | ||
| /// attribute. https://git-scm.com/docs/git-credential#Documentation/git-credential.txt-codeoauthrefreshtokencode | ||
| /// </summary> | ||
| string OAuthRefreshToken { get => null; } |
| } | ||
|
|
||
| /// <summary> | ||
| /// Represents a credential (username/password pair) that Git can use to authenticate to a remote repository. | ||
| /// </summary> | ||
| public class GitCredential : ICredential | ||
| public record GitCredential : ICredential |
There was a problem hiding this comment.
What was the reason for changing this from class to record?
Comment on lines
+66
to
+72
| public string Account { get; init; } | ||
|
|
||
| public string Password { get; init; } | ||
|
|
||
| public string Password { get; } | ||
| public DateTimeOffset? PasswordExpiry { get; init; } | ||
|
|
||
| public string OAuthRefreshToken { get; init; } |
There was a problem hiding this comment.
The inits may need to be removed given the current situation with .NET 8 on Windows.
| public ICredential Get(string service, string account) | ||
| { | ||
| var input = MakeGitCredentialsEntry(service, account); | ||
| var input = MakeGitCredentialsEntry(service, new GitCredential(account, null)); |
There was a problem hiding this comment.
Can/should we add a GitCredential constructor that just takes an account?
| // { | ||
| // IntPtr attrPtr = Marshal.ReadIntPtr(credential.Attributes, i * ptrSize); | ||
| // Win32CredentialAttribute attr = Marshal.PtrToStructure<Win32Credential>(attrPtr); | ||
| // } |
There was a problem hiding this comment.
Will want to add this attribute before merging.
| @@ -175,50 +175,14 @@ internal AuthenticationModes GetSupportedAuthenticationModes(Uri targetUri) | |||
| return modes; | |||
| } | |||
|
|
|||
| // <remarks>Stores OAuth tokens as a side effect</remarks> | |||
| public override async Task<ICredential> GetCredentialAsync(InputArguments input) | |||
| public override async Task<bool> ValidateCredentialAsync(Uri remoteUri, ICredential credential) | |||
| @@ -276,18 +225,28 @@ protected override void ReleaseManagedResources() | |||
| base.ReleaseManagedResources(); | |||
| } | |||
|
|
|||
| private string GetRefreshTokenServiceName(InputArguments input) | |||
| private string GetRefreshTokenServiceName(Uri remoteUri) | |||
There was a problem hiding this comment.
Is the remoteUri parameter being used here?
| @@ -93,7 +94,7 @@ private IEnumerable<TestCredential> Query(string service, string account) | |||
| } | |||
| } | |||
|
|
|||
| public class TestCredential : ICredential | |||
| public record TestCredential : ICredential | |||
There was a problem hiding this comment.
Curious again on the change to record.
| @@ -1,7 +1,7 @@ | |||
|
|
|||
| namespace GitCredentialManager.Interop.Windows | |||
| { | |||
| public class WindowsCredential : ICredential | |||
| public record WindowsCredential : ICredential | |||
There was a problem hiding this comment.
Curious again on the change to record.
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