Apply suggestions from code review

Co-authored-by: Matthew John Cheetham <[email protected]>

Author: hickford

docs/environment.md

@@ -169,6 +169,7 @@ ID|Provider
 `auto` _(default)_|_\[automatic\]_ ([learn more](autodetect.md))
 `azure-repos`|Azure Repos
 `github`|GitHub
+`gitlab`|GitLab<br/>_(supports OAuth in browser, personal access token and Basic Authentication)_
 `generic`|Generic (any other provider not listed above)
 
 Automatic provider selection is based on the remote URL.
@@ -206,6 +207,7 @@ Authority|Provider(s)
 `auto` _(default)_|_\[automatic\]_
 `msa`, `microsoft`, `microsoftaccount`,<br/>`aad`, `azure`, `azuredirectory`,</br>`live`, `liveconnect`, `liveid`|Azure Repos<br/>_(supports Microsoft Authentication)_
 `github`|GitHub<br/>_(supports GitHub Authentication)_
+`gitlab`|GitLab<br/>_(supports OAuth in browser, personal access token and Basic Authentication)_
 `basic`, `integrated`, `windows`, `kerberos`, `ntlm`,<br/>`tfs`, `sso`|Generic<br/>_(supports Basic and Windows Integrated Authentication)_
 
 #### Example

docs/gitlab.md

@@ -1,21 +1,17 @@
 # GitLab support
 
-git-credential-manager supports the following public GitLab instances out the box:
+Git Credential Manager supports [gitlab.com](https://gitlab.com) out the box.
 
-* https://gitlab.com
-* https://gitlab.freedesktop.org
-
-If you'd like support for another puublic instance, please [open an issue](https://github.com/GitCredentialManager/git-credential-manager/issues).
-
-## Using on a custom instance
+## Using on a another instance
 
 To use on another instance, eg. `https://gitlab.example.com` requires setup and configuration:
 
-1. [Create an OAuth application](https://docs.gitlab.com/ee/integration/oauth_provider.html). This can be at the user, group or instance level. Specify name `git-credential-manager` and redirect URI `http://127.0.0.1/`. Select options 'Confidential', 'Expire access tokens' and scope 'write_repository'.
+1. [Create an OAuth application](https://docs.gitlab.com/ee/integration/oauth_provider.html). This can be at the user, group or instance level. Specify a name and use a redirect URI of `http://127.0.0.1/`. _Unselect_ the 'Confidential' option, and ensure the 'Expire access tokens' option is selected. Set the scope to 'write_repository'.
 2. Copy the application ID and configure `git config --global credential.https://gitlab.example.com.GitLabDevClientId <APPLICATION_ID>`
 3. Copy the application secret and configure `git config --global credential.https://gitlab.example.com.GitLabDevClientSecret <APPLICATION_SECRET>`
-4. For good measure, configure `git config --global credential.https://gitlab.example.com.provider gitlab`
-5. Verify the config is as expected `git config --global --get-urlmatch credential https://gitlab.example.com`
+4. Configure authentication modes to include 'browser' `git config --global credential.https://gitlab.example.com.gitLabAuthModes browser`
+5. For good measure, configure `git config --global credential.https://gitlab.example.com.provider gitlab`
+6. Verify the config is as expected `git config --global --get-urlmatch credential https://gitlab.example.com`
 
 ### Clearing config
 
@@ -35,7 +31,6 @@ Select an authentication method for 'https://gitlab.com/':
 option (enter for default): 
 ```
 
-
 If you have a preferred authentication mode, you can specify [credential.gitLabAuthModes](configuration.md#credential.gitLabAuthModes):
 
     `git config --global credential.gitlabauthmodes browser`

src/shared/GitLab.Tests/GitLabHostProviderTests.cs

@@ -14,9 +14,9 @@ public class GitLabHostProviderTests
         [Theory]
         [InlineData("https", "gitlab.com", true)]
         [InlineData("http", "gitlab.com", true)]
-        [InlineData("https", "gitlab.freedesktop.org", true)]
-        [InlineData("https", "gitlab.gnome.org", true)]
+        [InlineData("https", "gitlab.example.com", true)]
         [InlineData("https", "github.com", false)]
+        [InlineData("https", "github.example.com", false)]
         public void GitLabHostProvider_IsSupported(string protocol, string host, bool expected)
         {
             var input = new InputArguments(new Dictionary<string, string>

src/shared/GitLab/GitLabAuthentication.cs

@@ -69,6 +69,7 @@ public AuthenticationPromptResult GetAuthentication(Uri targetUri, string userNa
             switch (modes)
             {
                 case AuthenticationModes.Basic:
+                case AuthenticationModes.Pat:
                     ThrowIfUserInteractionDisabled();
                     ThrowIfTerminalPromptsDisabled();
                     Context.Terminal.WriteLine("Enter GitLab credentials for '{0}'...", targetUri);
@@ -82,22 +83,12 @@ public AuthenticationPromptResult GetAuthentication(Uri targetUri, string userNa
                         Context.Terminal.WriteLine("Username: {0}", userName);
                     }
 
-                    string password = Context.Terminal.PromptSecret("Password");
-
-                    return new AuthenticationPromptResult(
-                        AuthenticationModes.Basic, new GitCredential(userName, password));
+                    string password_or_token = Context.Terminal.PromptSecret(modes == AuthenticationModes.Basic ? "Password" : "Personal access token");
+                    return new AuthenticationPromptResult(modes, new GitCredential(userName, password_or_token));
 
                 case AuthenticationModes.Browser:
                     return new AuthenticationPromptResult(AuthenticationModes.Browser);
 
-                case AuthenticationModes.Pat:
-                    ThrowIfUserInteractionDisabled();
-                    ThrowIfTerminalPromptsDisabled();
-                    Context.Terminal.WriteLine("Enter GitLab personal access token for '{0}'...", targetUri);
-                    string pat = Context.Terminal.PromptSecret("Token");
-                    return new AuthenticationPromptResult(
-                        AuthenticationModes.Pat, new GitCredential(userName, pat));
-
                 case AuthenticationModes.None:
                     throw new ArgumentOutOfRangeException(nameof(modes), @$"At least one {nameof(AuthenticationModes)} must be supplied");
 

src/shared/GitLab/GitLabConstants.cs

@@ -4,38 +4,11 @@
 
 namespace GitLab
 {
-    public record GitLabApplication
-    {
-        public GitLabApplication(string host, string oAuthClientId, string oAuthClientSecret)
-        {
-            Host = host;
-            OAuthClientId = oAuthClientId;
-            OAuthClientSecret = oAuthClientSecret;
-        }
-
-        public string Host { get; }
-        public string OAuthClientId { get; }
-        public string OAuthClientSecret { get; }
-    }
-
-
     public static class GitLabConstants
     {
-        // To add an instance, follow https://docs.gitlab.com/ee/integration/oauth_provider.html
-        // specify callback/redirect URI http://127.0.0.1/ and check 'confidential', 'expire access tokens', 'write_repository'
-        private static readonly GitLabApplication[] GitLabApplications =
-        {
-            new GitLabApplication(host: "gitlab.com",
-                // https://gitlab.com/oauth/applications/207177/edit owned by hickford
-                oAuthClientId: "d8a14250a4d1beacaad67dd6fabaab1e0408b581ca73ae4a76cc7170d3f8afd1",
-                oAuthClientSecret : "58b5f5e0c99a5be9ac13f4ba15992cc72c5594386e82aecac94da964147d3151"
-            ),
-            new GitLabApplication(host: "gitlab.freedesktop.org",
-                // https://gitlab.freedesktop.org/oauth/applications/52 owned by hickford
-                oAuthClientId: "6503d8c5a27187628440d44e0352833a2b49bce540c546c22a3378c8f5b74d45",
-                oAuthClientSecret: "2ae9343a034ff1baadaef1e7ce3197776b00746a02ddf0323bb34aca8bff6dc1")
-        };
-        public static readonly IDictionary<string, GitLabApplication> GitLabApplicationsByHost = GitLabApplications.ToDictionary(x => x.Host, StringComparer.InvariantCultureIgnoreCase);
+        // owned by https://gitlab.com/gitcredentialmanager
+        public const string OAuthClientId = "172b9f227872b5dde33f4d9b1db06a6a5515ae79508e7a00c973c85ce490671e";
+        public const string OAuthClientSecret = "7da92770d1447508601e4ba026bc5eb655c8268e818cd609889cc9bae2023f39";
 
         public static readonly Uri OAuthRedirectUri = new Uri("http://127.0.0.1/");
         // https://docs.gitlab.com/ee/api/oauth2.html#authorization-code-flow
@@ -57,11 +30,13 @@ public static class GitConfiguration
         {
             public static class Credential
             {
-                public const string AuthenticationModes = "GitLabAuthModes";
-                public const string DevOAuthClientId = "GitLabDevClientId";
-                public const string DevOAuthClientSecret = "GitLabDevClientSecret";
-                public const string DevOAuthRedirectUri = "GitLabDevRedirectUri";
+                public const string AuthenticationModes = "gitLabAuthModes";
+                public const string DevOAuthClientId = "gitLabDevClientId";
+                public const string DevOAuthClientSecret = "gitLabDevClientSecret";
+                public const string DevOAuthRedirectUri = "gitLabDevRedirectUri";
             }
         }
+
+        public static bool IsGitLabDotCom(Uri uri) => StringComparer.OrdinalIgnoreCase.Equals(uri.Host, "gitlab.com");
     }
 }

src/shared/GitLab/GitLabHostProvider.cs

@@ -15,20 +15,20 @@ public class GitLabHostProvider : HostProvider
             "write_repository",
         };
 
-        private readonly IGitLabAuthentication _GitLabAuth;
+        private readonly IGitLabAuthentication _gitLabAuth;
 
         public GitLabHostProvider(ICommandContext context)
             : this(context, new GitLabAuthentication(context)) { }
 
-        public GitLabHostProvider(ICommandContext context, IGitLabAuthentication GitLabAuth)
+        public GitLabHostProvider(ICommandContext context, IGitLabAuthentication gitLabAuth)
             : base(context)
         {
-            EnsureArgument.NotNull(GitLabAuth, nameof(GitLabAuth));
+            EnsureArgument.NotNull(gitLabAuth, nameof(gitLabAuth));
 
-            _GitLabAuth = GitLabAuth;
+            _gitLabAuth = gitLabAuth;
         }
 
-        public override string Id => "GitLab";
+        public override string Id => "gitlab";
 
         public override string Name => "GitLab";
 
@@ -48,15 +48,15 @@ public override bool IsSupported(InputArguments input)
                 return false;
             }
 
-            // Split port number and hostname from host input argument
-            if (!input.TryGetHostAndPort(out string hostName, out _))
+            if (GitLabConstants.IsGitLabDotCom(input.GetRemoteUri()))
             {
-                return false;
+                return true;
             }
 
-            if (GitLabConstants.GitLabApplicationsByHost.ContainsKey(hostName))
+            // Split port number and hostname from host input argument
+            if (!input.TryGetHostAndPort(out string hostName, out _))
             {
-                return true;
+                return false;
             }
 
             string[] domains = hostName.Split(new char[] { '.' });
@@ -78,6 +78,7 @@ public override bool IsSupported(HttpResponseMessage response)
                 return false;
             }
 
+            // as seen at eg. https://salsa.debian.org/apt-team/apt.git
             // not always present https://gitlab.com/gitlab-org/gitlab/-/issues/349464
             return response.Headers.Contains("X-Gitlab-Feature-Category");
         }
@@ -106,24 +107,17 @@ public override async Task<ICredential> GenerateCredentialAsync(InputArguments i
 
             AuthenticationModes authModes = GetSupportedAuthenticationModes(remoteUri);
 
-            AuthenticationPromptResult promptResult = _GitLabAuth.GetAuthentication(remoteUri, input.UserName, authModes);
+            AuthenticationPromptResult promptResult = _gitLabAuth.GetAuthentication(remoteUri, input.UserName, authModes);
 
             switch (promptResult.AuthenticationMode)
             {
                 case AuthenticationModes.Basic:
+                case AuthenticationModes.Pat:
                     return promptResult.Credential;
 
                 case AuthenticationModes.Browser:
                     return await GenerateOAuthCredentialAsync(remoteUri);
 
-                case AuthenticationModes.Pat:
-                    string token = promptResult.Credential.Password;
-
-                    // GitLab accepts any username https://gitlab.com/gitlab-org/gitlab/-/issues/212953
-                    string userName = promptResult.Credential.Account ?? "pat";
-
-                    return new GitCredential(userName, token);
-
                 default:
                     throw new ArgumentOutOfRangeException(nameof(promptResult));
             }
@@ -148,32 +142,27 @@ internal AuthenticationModes GetSupportedAuthenticationModes(Uri targetUri)
                 }
             }
 
-            AuthenticationModes modes = AuthenticationModes.Basic | AuthenticationModes.Pat;
-
-            try
-            {
-                GitLabOAuth2Client.GetClientId(Context.Settings, targetUri);
-            }
-            catch (Exception e)
+            if (GitLabConstants.IsGitLabDotCom(targetUri))
             {
-                Context.Streams.Error.WriteLine(e.Message);
-                return modes;
+                return AuthenticationModes.All;
             }
-            modes |= AuthenticationModes.Browser;
-            return modes;
+
+            Context.Streams.Error.WriteLine($"Missing OAuth configuration for {targetUri.Host}, see https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/gitlab.md.");
+            // Would like to query password_authentication_enabled_for_git, but can't unless logged in https://gitlab.com/gitlab-org/gitlab/-/issues/349463
+            return AuthenticationModes.Basic | AuthenticationModes.Pat;
         }
 
         private async Task<GitCredential> GenerateOAuthCredentialAsync(Uri targetUri)
         {
-            OAuth2TokenResult result = await _GitLabAuth.GetOAuthTokenViaBrowserAsync(targetUri, GitLabOAuthScopes);
+            OAuth2TokenResult result = await _gitLabAuth.GetOAuthTokenViaBrowserAsync(targetUri, GitLabOAuthScopes);
 
             // username oauth2 https://gitlab.com/gitlab-org/gitlab/-/issues/349461
             return new GitCredential("oauth2", result.AccessToken);
         }
 
         protected override void ReleaseManagedResources()
         {
-            _GitLabAuth.Dispose();
+            _gitLabAuth.Dispose();
             base.ReleaseManagedResources();
         }
     }

src/shared/GitLab/GitLabOAuth2Client.cs

@@ -9,7 +9,7 @@ public class GitLabOAuth2Client : OAuth2Client
     {
         public GitLabOAuth2Client(HttpClient httpClient, ISettings settings, Uri baseUri)
             : base(httpClient, CreateEndpoints(baseUri),
-                GetClientId(settings, baseUri), GetRedirectUri(settings), GetClientSecret(settings, baseUri))
+                GetClientId(settings), GetRedirectUri(settings), GetClientSecret(settings))
         { }
 
         private static OAuth2ServerEndpoints CreateEndpoints(Uri baseUri)
@@ -34,7 +34,7 @@ private static Uri GetRedirectUri(ISettings settings)
             return GitLabConstants.OAuthRedirectUri;
         }
 
-        internal static string GetClientId(ISettings settings, Uri baseUri)
+        internal static string GetClientId(ISettings settings)
         {
             // Check for developer override value
             if (settings.TryGetSetting(
@@ -45,15 +45,10 @@ internal static string GetClientId(ISettings settings, Uri baseUri)
                 return clientId;
             }
 
-            GitLabApplication instance;
-            if (GitLabConstants.GitLabApplicationsByHost.TryGetValue(baseUri.Host, out instance))
-            {
-                return instance.OAuthClientId;
-            }
-            throw new ArgumentException($"Missing OAuth configuration for {baseUri.Host}, see https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/gitlab.md.");
+            return GitLabConstants.OAuthClientId;
         }
 
-        private static string GetClientSecret(ISettings settings, Uri baseUri)
+        private static string GetClientSecret(ISettings settings)
         {
             // Check for developer override value
             if (settings.TryGetSetting(
@@ -64,12 +59,7 @@ private static string GetClientSecret(ISettings settings, Uri baseUri)
                 return clientSecret;
             }
 
-            GitLabApplication instance;
-            if (GitLabConstants.GitLabApplicationsByHost.TryGetValue(baseUri.Host, out instance))
-            {
-                return instance.OAuthClientSecret;
-            }
-            throw new ArgumentException($"Missing OAuth configuration for {baseUri.Host}, see https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/gitlab.md.");
+            return GitLabConstants.OAuthClientSecret;
         }
     }
 }

src/shared/GitLab/InternalsVisibleTo.cs

@@ -1,3 +1,3 @@
 using System.Runtime.CompilerServices;
 
-[assembly:InternalsVisibleTo("GitLab.Tests")]
+[assembly: InternalsVisibleTo("GitLab.Tests")]