Skip to content

Add support for OAuth with GitHub AE #343

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.

Sign up for GitHub

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

Merged
mjcheetham merged 1 commit into from
May 13, 2021
Merged

Add support for OAuth with GitHub AE #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions src/shared/GitHub/GitHubConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ public static class GitHubConstants
// TODO: update this with a real version number once the GCM OAuth application has been deployed to GHE
public static readonly Version MinimumEnterpriseOAuthVersion = new Version("99.99.99");
Copy link
Contributor Author

@mjcheetham mjcheetham May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we know which version of GHES has the OAuth app deployed, we can update this constant too.

Copy link
Contributor

@vtbassmatt vtbassmatt May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever cause confusion since "Enterprise" is a part of the name of three different GitHub products? MinimumOnPremOAuthVersion maybe? (This may be pointless bikeshedding on my part - the question is genuine but not particularly important.)

Copy link
Contributor Author

@mjcheetham mjcheetham May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to submit the rename PR to that OnPrem suggestion ;)

Copy link
Contributor

@vtbassmatt vtbassmatt May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha 😁

Copy link
Contributor

@vtbassmatt vtbassmatt May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, can't resolve, since you had left this as a note to yourself.


/// <summary>
/// The version string returned from the meta API endpoint for GitHub AE instances.
/// </summary>
public const string GitHubAeVersionString = "GitHub AE";

/// <summary>
/// Supported authentication modes for GitHub.com.
/// </summary>
Expand Down
9 changes: 8 additions & 1 deletion src/shared/GitHub/GitHubHostProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,15 @@ internal async Task<AuthenticationModes> GetSupportedAuthenticationModesAsync(Ur
{
modes |= AuthenticationModes.Basic;
}
if (Version.TryParse(metaInfo.InstalledVersion, out var version) && version >= GitHubConstants.MinimumEnterpriseOAuthVersion)

if (StringComparer.OrdinalIgnoreCase.Equals(metaInfo.InstalledVersion, GitHubConstants.GitHubAeVersionString))
{
// Assume all GHAE instances have the GCM OAuth application deployed
modes |= AuthenticationModes.OAuth;
}
else if (Version.TryParse(metaInfo.InstalledVersion, out var version) && version >= GitHubConstants.MinimumEnterpriseOAuthVersion)
{
// Only GHES versions beyond the minimum version have the GCM OAuth application deployed
modes |= AuthenticationModes.OAuth;
}

Expand Down