-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store PasswordExpiry and OAuthRefreshToken #1464
base: main
Are you sure you want to change the base?
Store PasswordExpiry and OAuthRefreshToken #1464
Conversation
a2425b9
to
a21c066
Compare
a21c066
to
2a14b66
Compare
Windows build ought to be fixed by #1418 |
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.
83c3f3a
to
b59547b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Most of my comments are small things - the only major issue that I'd need to see updated before approving is not having expiry implemented for Windows Credential Manager.
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here 😞.
} | ||
|
||
/// <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for changing this from class
to record
?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! Lovely cleanup.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious again on the change to record
.
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