-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Web browser and PAT support for GitLab #591
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
Conversation
8db9d63
to
b64f860
Compare
a70bff5
to
a8bb5e3
Compare
7354d28
to
4076146
Compare
NB. The OAuth apps on https://gitlab.com and https://gitlab.freedesktop.org are owned my account on each system. Ideally they'd be owned by a common Git Credential Manager group, but I don't think cross-instance groups are possible, and creating independent groups on each system seems more than trouble than its worth. |
dc84d33
to
e51a07d
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.
First let me say I am excited to see such a contribution from the community! 🎉
This is the first new host provider we've had in a long time (since Bitbucket)
and we're always happy to see such engagement in GCM.
I have left a few comments about style, typos, etc, but overall it looks good.
There are however two main changes we'd need to see before we could take this:
1. Remove hardcoded public instances other than gitlab.com
As I mentioned in another review comment, we should not hardcode public
instances outside of the host-maintained offering (gitlab.com, github.com, etc).
Doing so opens the project and maintainers up to a lot of risk and burden.
Instances come and go over time. Some may construe support of some instances as
being an endorsement of that entity. The potentially unbounded number of
requests to add a new instance is not scalable. It's not fair to decide to take
some instances over others.
You have already added support to add custom instances by config. I would
suggest we try and leverage that for all cases. Setting the client ID/secret and
redirect URI scoped to a hostname is possible. e.g.,:
[credential "gitlab.example.com"]
provider = gitlab
gitLabDevClientId = 123456
gitLabDevClientSecret = abcdef
gitLabDevRedirectUri = http://127.0.0.1
[credential "another-example.com/gitlab"]
provider = gitlab
gitLabDevClientId = 890123
gitLabDevClientSecret = xyzwsfs
gitLabDevRedirectUri = http://127.0.0.1:56800/callback
We can also leverage the ICommandProvider
interface to provide a custom
command for GitLab, to manage this config in a easier way. For example imagine
the following command-line interface:
git credential-manager-core gitlab list
git credential-manager-core gitlab add <url> <id> <secret> <redirect>
git credential-manager-core gitlab remove <url>
add
would set the appropriate config as above, making it easier for users to
add their preferred instances.
Ideally engage GitLab to have a GCM OAuth app deployed as a default app in new
instances, and/or have some public metadata discovery for such IDs if not the
same across instances. GitHub Enterprise Server and GitHub AE all have a GCM
OAuth app deployed with well-known settings.
2. Implement support for OAuth refresh tokens
This is an important feature to support if we're going to support OAuth with
GitLab (given that expiring tokens exist).
The general idea here is to check for a RT if no AT is found (or is not valid), and try and use
that first before prompting for the whole interactive auth dance again.
This is the same as the Bitbucket host provider, which I outline here:
public async Task<ICredential> GetCredentialAsync(InputArguments input)
{
Uri remoteUri = input.GetRemoteUri();
string service = GetServiceName(remoteUri);
string account = input.UserName;
// Check for an existing user/pass, PAT, or OAuth token
var credential = Context.CredentialStore.Get(service, account);
// Validate the token/credential is valid
bool valid = await ValidateCredentialAsync(remoteUri, credential);
// Do we have a valid token to return?
if (valid)
{
return credential;
}
// Something like: https://gitlab.com/refresh_token
string refreshService = GetRefreshServiceName(remoteUri);
// Check for an OAuth refresh token
var refreshCredential = Context.CredentialStore.Get(refreshService, account);
// Try and use the RT to mint a new AT
if (refreshCredential != null)
{
try
{
var newCred = await RefreshOAuthTokenAsync(remoteUri, refreshCredential.Password);
if (newCred != null)
{
return newCred;
}
}
catch (Exception ex)
{
Context.Trace.WriteLine("Failed to use refresh token");
Context.Trace.WriteException(ex);
}
}
// Fall through to interactive auth
AuthenticationModes modes = GetSupportedAuthenticationModes(remoteUri);
AuthenticationPromptResult result = await _auth.GetAuthenticationAsync(targetUri, account, modes);
switch (result.AuthenticationMode)
{
case AuthenticationModes.Browser:
return await CreateOAuthTokenAsync(remoteUri, account);
...
}
}
private async Task<bool> ValidateCredentialAsync(Uri remoteUri, ICredential credential)
{
if (credential is null) return false;
// TODO: Make a HTTP request using the credential to validate the token
// is still valid.
// If we fail to validate, we should assume it is valid.
return true;
}
private async Task<ICredential> CreateOAuthTokenAsync(Uri remoteUri, string userName)
{
OAuth2TokenResult result = await _auth.GetOAuthTokenViaBrowserAsync(targetUri);
// Resolve the username for this new access token
GitLabUserInfo userInfo = await _api.GetUserInfoAsync(targetUri, result.AccessToken);
// Store the refresh token if we have been given one
if (!string.IsNullOrWhiteSpace(result.RefreshToken))
{
string refreshServiceName = GetRefreshServiceName(remoteUri);
_context.CredentialStore.AddOrUpdate(refreshServiceName, userInfo.UserName, result.RefreshToken);
}
return new GitCredential("oauth2", result.AccessToken);
}
private async Task<ICredential> RefreshOAuthTokenAsync(Uri remoteUri, string refreshToken, string refreshService)
{
// Use refresh token to mint a new access token
try
{
OAuth2TokenResult refreshResult = await _auth.GetOAuthTokenViaRefreshTokenAsync(targetUri, refreshToken);
// Resolve the username for this refresh token
GitLabUserInfo newUserInfo = await _api.GetUserInfoAsync(targetUri, refreshResult.AccessToken);
// Store the refresh token if we have been given a new one
if (!string.IsNullOrWhiteSpace(refreshResult.RefreshToken))
{
_context.CredentialStore.AddOrUpdate(refreshService, newUserInfo.UserName, refreshResult.RefreshToken);
}
return new GitCredential("oauth2", refreshResult.AccessToken);
}
catch (OAuth2Exception)
{
// Failed to use refresh token
return null;
}
}
I realise this a big wall of text and comments on your work.
Let me say again that we do appreciate your efforts here to bring GitLab to GCM,
and comments here are to ensure that support for a new host provider covers all
the cases in a way that's sustainable for the project.
New host providers contributions are held to a high standard, and it's great
stuff to see that you've clearly put in a lot of high quality work so far!
Thank you ❤️
// username oauth2 https://gitlab.com/gitlab-org/gitlab/-/issues/349461 | ||
return new GitCredential("oauth2", result.AccessToken); |
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 is a shame 😢 .. but something we may just have to do until (if?) they fix this.
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.
So this could cause a problem if you push to two repos on the same host with different users? Could you give an example?
I should also add that we've taken the liberty of creating a
Feel free to update your PR to use these values for the gitlab.com instance. This should work, but please let us know if there are problems. |
7e79dec
to
13370ca
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.
Hey @hickford, thanks for the updates to the PR! Overall this is looking much better.
There are two bugs (one crashing, one behavioural) however that'll need fixing before it can be merged. I've called them out in the review comments. Once these are fixed this PR can be merged 😃
3bcbaa4
to
a9b3a9a
Compare
Co-authored-by: Matthew John Cheetham <[email protected]>
Co-authored-by: Matthew John Cheetham <[email protected]>
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.
Thanks @hickford for the quick turn around there. This looks good to merge right now. Thanks again for the contribution! 🎉
Many thanks! Is there already a plan, when this extension is released to "GCM for Windows"? |
Hi @muttebe - There are no plans to port this to Git Credenital Manager for Windows, as I believe that project has been deprecated in favor of GCM. |
Hi @ldennington! Thank you for your fast reply! I fully agree with you! Actually i am intrested in how often the GCM is released and escpecially when it is released next time? |
Hi! |
Apologies for the delay here @LeonardSchmitt67. We don't have a set cadence for GCM releases. However, since we ship with Git for Windows, we keep that release cadence in mind and try to get new bits out ~1 week ahead of that. The next release cycle for Git for Windows begins on April 4th, so we're targeting sometime around March 28th for our next release. |
Thanks a lot! @ldennington |
GitLab support similar to the current GitHub support:
Fixes #589 and https://gitlab.com/gitlab-org/gitlab/-/issues/225215
Progress so far:
gitlab.
and links to documentationCaveats: