Skip to content
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

Add explicit PAT authentication mode for the GitHub Provider #236

Merged
merged 3 commits into from
Nov 30, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/shared/GitHub.Tests/GitHubHostProviderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ public async Task GitHubHostProvider_GenerateCredentialAsync_Basic_1FAOnly_Retur

var ghAuthMock = new Mock<IGitHubAuthentication>(MockBehavior.Strict);
ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, null, It.IsAny<AuthenticationModes>()))
.ReturnsAsync(new AuthenticationPromptResult(new GitCredential(expectedUserName, expectedPassword)));
.ReturnsAsync(new AuthenticationPromptResult(
AuthenticationModes.Basic, new GitCredential(expectedUserName, expectedPassword)));

var ghApiMock = new Mock<IGitHubRestApi>(MockBehavior.Strict);
ghApiMock.Setup(x => x.CreatePersonalAccessTokenAsync(expectedTargetUri, expectedUserName, expectedPassword, null, It.IsAny<IEnumerable<string>>()))
Expand Down Expand Up @@ -270,7 +271,8 @@ public async Task GitHubHostProvider_GenerateCredentialAsync_Basic_2FARequired_R

var ghAuthMock = new Mock<IGitHubAuthentication>(MockBehavior.Strict);
ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, null, It.IsAny<AuthenticationModes>()))
.ReturnsAsync(new AuthenticationPromptResult(new GitCredential(expectedUserName, expectedPassword)));
.ReturnsAsync(new AuthenticationPromptResult(
AuthenticationModes.Basic, new GitCredential(expectedUserName, expectedPassword)));
ghAuthMock.Setup(x => x.GetTwoFactorCodeAsync(expectedTargetUri, false))
.ReturnsAsync(expectedAuthCode);

Expand Down
88 changes: 61 additions & 27 deletions src/shared/GitHub/GitHubAuthentication.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ public AuthenticationPromptResult(AuthenticationModes mode)
AuthenticationMode = mode;
}

public AuthenticationPromptResult(ICredential basicCredential)
: this(AuthenticationModes.Basic)
public AuthenticationPromptResult(AuthenticationModes mode, ICredential credential)
: this(mode)
{
BasicCredential = basicCredential;
Credential = credential;
}

public AuthenticationModes AuthenticationMode { get; }

public ICredential BasicCredential { get; set; }
public ICredential Credential { get; set; }
}

[Flags]
Expand All @@ -45,6 +45,9 @@ public enum AuthenticationModes
None = 0,
Basic = 1,
OAuth = 1 << 1,
Pat = 1 << 2,

All = Basic | OAuth | Pat
}

public class GitHubAuthentication : AuthenticationBase, IGitHubAuthentication
Expand All @@ -63,16 +66,24 @@ public async Task<AuthenticationPromptResult> GetAuthenticationAsync(Uri targetU

if (modes == AuthenticationModes.None)
{
throw new ArgumentException($"Must specify at least one {nameof(AuthenticationModes)}", nameof(modes));
throw new ArgumentException(@$"Must specify at least one {nameof(AuthenticationModes)}", nameof(modes));
}

if (TryFindHelperExecutablePath(out string helperPath))
{
var promptArgs = new StringBuilder("prompt");
if ((modes & AuthenticationModes.Basic) != 0) promptArgs.Append(" --basic");
if ((modes & AuthenticationModes.OAuth) != 0) promptArgs.Append(" --oauth");
if (!GitHubHostProvider.IsGitHubDotCom(targetUri)) promptArgs.AppendFormat(" --enterprise-url {0}", targetUri);
if (!string.IsNullOrWhiteSpace(userName)) promptArgs.AppendFormat(" --username {0}", userName);
if (modes == AuthenticationModes.All)
{
promptArgs.Append(" --all");
}
else
{
if ((modes & AuthenticationModes.Basic) != 0) promptArgs.Append(" --basic");
if ((modes & AuthenticationModes.OAuth) != 0) promptArgs.Append(" --oauth");
if ((modes & AuthenticationModes.Pat) != 0) promptArgs.Append(" --pat");
}
if (!GitHubHostProvider.IsGitHubDotCom(targetUri)) promptArgs.AppendFormat(" --enterprise-url \"{0}\"", targetUri);
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
if (!string.IsNullOrWhiteSpace(userName)) promptArgs.AppendFormat(" --username \"{0}\"", userName);

IDictionary<string, string> resultDict = await InvokeHelperAsync(helperPath, promptArgs.ToString(), null);

Expand All @@ -83,6 +94,15 @@ public async Task<AuthenticationPromptResult> GetAuthenticationAsync(Uri targetU

switch (responseMode.ToLowerInvariant())
{
case "pat":
if (!resultDict.TryGetValue("pat", out string pat))
{
throw new Exception("Missing 'pat' in response");
}

return new AuthenticationPromptResult(
AuthenticationModes.Pat, new GitCredential(userName, pat));

case "oauth":
return new AuthenticationPromptResult(AuthenticationModes.OAuth);

Expand All @@ -97,7 +117,8 @@ public async Task<AuthenticationPromptResult> GetAuthenticationAsync(Uri targetU
throw new Exception("Missing 'password' in response");
}

return new AuthenticationPromptResult(new GitCredential(userName, password));
return new AuthenticationPromptResult(
AuthenticationModes.Basic, new GitCredential(userName, password));

default:
throw new Exception($"Unknown mode value in response '{responseMode}'");
Expand All @@ -109,21 +130,6 @@ public async Task<AuthenticationPromptResult> GetAuthenticationAsync(Uri targetU

switch (modes)
{
case AuthenticationModes.Basic | AuthenticationModes.OAuth:
var menuTitle = $"Select an authentication method for '{targetUri}'";
var menu = new TerminalMenu(Context.Terminal, menuTitle)
{
new TerminalMenuItem(1, "Web browser", isDefault: true),
new TerminalMenuItem(2, "Username/password")
};

int option = menu.Show();

if (option == 1) goto case AuthenticationModes.OAuth;
if (option == 2) goto case AuthenticationModes.Basic;

throw new Exception();

case AuthenticationModes.Basic:
Context.Terminal.WriteLine("Enter GitHub credentials for '{0}'...", targetUri);

Expand All @@ -138,13 +144,41 @@ public async Task<AuthenticationPromptResult> GetAuthenticationAsync(Uri targetU

string password = Context.Terminal.PromptSecret("Password");

return new AuthenticationPromptResult(new GitCredential(userName, password));
return new AuthenticationPromptResult(
AuthenticationModes.Basic, new GitCredential(userName, password));

case AuthenticationModes.OAuth:
return new AuthenticationPromptResult(AuthenticationModes.OAuth);

case AuthenticationModes.Pat:
Context.Terminal.WriteLine("Enter GitHub 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");

default:
throw new ArgumentOutOfRangeException(nameof(modes), $"Unknown {nameof(AuthenticationModes)} value");
var menuTitle = $"Select an authentication method for '{targetUri}'";
var menu = new TerminalMenu(Context.Terminal, menuTitle);

TerminalMenuItem oauthItem = null;
TerminalMenuItem basicItem = null;
TerminalMenuItem patItem = null;

if ((modes & AuthenticationModes.OAuth) != 0) oauthItem = menu.Add("Web browser");
if ((modes & AuthenticationModes.Pat) != 0) patItem = menu.Add("Personal access token");
if ((modes & AuthenticationModes.Basic) != 0) basicItem = menu.Add("Username/password");

// Default to the 'first' choice in the menu
TerminalMenuItem choice = menu.Show(0);

if (choice == oauthItem) goto case AuthenticationModes.OAuth;
if (choice == basicItem) goto case AuthenticationModes.Basic;
if (choice == patItem) goto case AuthenticationModes.Pat;

throw new Exception();
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/shared/GitHub/GitHubConstants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ public static class GitHubConstants
/// <summary>
/// Supported authentication modes for GitHub.com.
/// </summary>
public const AuthenticationModes DotComAuthenticationModes = AuthenticationModes.OAuth;
/// <remarks>
/// As of 13th November 2020, GitHub.com does not support username/password (basic) authentication to the APIs.
/// See https://developer.github.com/changes/2020-02-14-deprecating-oauth-auth-endpoint for more information.
/// </remarks>
public const AuthenticationModes DotComAuthenticationModes = AuthenticationModes.OAuth | AuthenticationModes.Pat;

public static class TokenScopes
{
Expand Down
27 changes: 23 additions & 4 deletions src/shared/GitHub/GitHubHostProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public override async Task<ICredential> GenerateCredentialAsync(InputArguments i
switch (promptResult.AuthenticationMode)
{
case AuthenticationModes.Basic:
GitCredential patCredential = await GeneratePersonalAccessTokenAsync(remoteUri, promptResult.BasicCredential);
GitCredential patCredential = await GeneratePersonalAccessTokenAsync(remoteUri, promptResult.Credential);

// HACK: Store the PAT immediately in case this PAT is not valid for SSO.
// We don't know if this PAT is valid for SAML SSO and if it's not Git will fail
Expand All @@ -111,6 +111,22 @@ public override async Task<ICredential> GenerateCredentialAsync(InputArguments i
case AuthenticationModes.OAuth:
return await GenerateOAuthCredentialAsync(remoteUri);

case AuthenticationModes.Pat:
// The token returned by the user should be good to use directly as the password for Git
string token = promptResult.Credential.Password;

// Resolve the GitHub user handle if we don't have a specific username already from the
// initial request. The reason for this is GitHub requires a (any?) value for the username
// when Git makes calls to GitHub.
string userName = promptResult.Credential.Account;
if (userName is null)
{
GitHubUserInfo userInfo = await _gitHubApi.GetUserInfoAsync(remoteUri, token);
userName = userInfo.Login;
}

return new GitCredential(userName, token);

default:
throw new ArgumentOutOfRangeException(nameof(promptResult));
}
Expand Down Expand Up @@ -186,15 +202,16 @@ internal async Task<AuthenticationModes> GetSupportedAuthenticationModesAsync(Ur
}
}

// GitHub.com should use OAuth authentication only
// GitHub.com should use OAuth or manual PAT based authentication only, never basic auth as of 13th November 2020
// https://developer.github.com/changes/2020-02-14-deprecating-oauth-auth-endpoint
if (IsGitHubDotCom(targetUri))
{
Context.Trace.WriteLine($"{targetUri} is github.com - authentication schemes: '{GitHubConstants.DotComAuthenticationModes}'");
return GitHubConstants.DotComAuthenticationModes;
}

// For GitHub Enterprise we must do some detection of supported modes
Context.Trace.WriteLine($"{targetUri} is GitHub Enterprise - checking for supporting authentication schemes...");
Context.Trace.WriteLine($"{targetUri} is GitHub Enterprise - checking for supported authentication schemes...");

try
{
Expand All @@ -219,7 +236,9 @@ internal async Task<AuthenticationModes> GetSupportedAuthenticationModesAsync(Ur
Context.Trace.WriteException(ex);

Context.Terminal.WriteLine($"warning: failed to query '{targetUri}' for supported authentication schemes.");
return AuthenticationModes.Basic | AuthenticationModes.OAuth;

// Fall-back to offering all modes so the user is never blocked from authenticating by at least one mode
return AuthenticationModes.All;
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
using System;
using Xunit;

namespace Microsoft.Git.CredentialManager.Tests
{
public class EnsureArgumentTests
{
[Theory]
[InlineData(5, 0, 10, true, true)]
[InlineData(0, 0, 10, true, true)]
[InlineData(10, 0, 10, true, true)]
[InlineData(0, -10, 0, true, true)]
[InlineData(-10, -10, 0, true, true)]
public void EnsureArgument_InRange_DoesNotThrow(int x, int lower, int upper, bool lowerInc, bool upperInc)
{
EnsureArgument.InRange(x, nameof(x), lower, upper, lowerInc, upperInc);
}

[Theory]
[InlineData(-3, 0, 10, true, true)]
[InlineData(13, 0, 10, true, true)]
[InlineData(10, 0, 10, true, false)]
[InlineData(0, 0, 10, false, true)]
[InlineData(-10, -10, 0, false, true)]
[InlineData(0, -10, 0, true, false)]
public void EnsureArgument_InRange_ThrowsException(int x, int lower, int upper, bool lowerInc, bool upperInc)
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
{
Assert.Throws<ArgumentOutOfRangeException>(
() => EnsureArgument.InRange(x, nameof(x), lower, upper, lowerInc, upperInc)
);
}
}
}
23 changes: 23 additions & 0 deletions src/shared/Microsoft.Git.CredentialManager/EnsureArgument.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,28 @@ public static void Negative(int arg, string name)
throw new ArgumentOutOfRangeException(name, "Argument must be negative.");
}
}

public static void InRange(int arg, string name, int lower, int upper, bool lowerInclusive = true, bool upperInclusive = true)
{
if (lowerInclusive && arg < lower)
{
throw new ArgumentOutOfRangeException(name, $"Argument must be greater than or equal to {lower}.");
}

if (!lowerInclusive && arg <= lower)
{
throw new ArgumentOutOfRangeException(name, $"Argument must be strictly greater than {lower}.");
}

if (upperInclusive && arg > upper)
{
throw new ArgumentOutOfRangeException(name, $"Argument must be less than or equal to {upper}.");
}

if (!upperInclusive && arg >= upper)
{
throw new ArgumentOutOfRangeException(name, $"Argument must be strictly less than {upper}.");
}
}
}
}
Loading