From bc7830fc6ef02a9f0b38ee8254d21cdfbb36b984 Mon Sep 17 00:00:00 2001 From: Ryan Gribble Date: Fri, 5 Jan 2018 07:02:26 +1000 Subject: [PATCH] Fix OAuthClient to work correctly with GitHub Enterprise instances (#1726) * Add more test cases for GetGitHubLoginUrl tests * Fix enterprise authorize Urls by replacing the existing relative Uri segment (/api/v3) instead of appending to it via an extension method * Add tests for new ReplaceRelativeUri extension method * add test for GHE URL on oauth CreateAccessToken * move away from class level consts * Oauth client can store stripped base URL so it works with GHE --- Octokit.Tests/Clients/OauthClientTests.cs | 41 +++++++++++++++++++-- Octokit.Tests/Helpers/UriExtensionsTests.cs | 17 +++++++++ Octokit/Clients/OAuthClient.cs | 6 ++- Octokit/Helpers/ApiUrls.cs | 8 ++-- Octokit/Helpers/UriExtensions.cs | 24 ++++++++++++ 5 files changed, 86 insertions(+), 10 deletions(-) diff --git a/Octokit.Tests/Clients/OauthClientTests.cs b/Octokit.Tests/Clients/OauthClientTests.cs index 7f723b26e0..b4155f9abf 100644 --- a/Octokit.Tests/Clients/OauthClientTests.cs +++ b/Octokit.Tests/Clients/OauthClientTests.cs @@ -24,12 +24,13 @@ public class TheGetGitHubLoginUrlMethod [Theory] [InlineData("https://api.github.com", "https://github.com/login/oauth/authorize?client_id=secret")] [InlineData("https://github.com", "https://github.com/login/oauth/authorize?client_id=secret")] - [InlineData("https://example.com", "https://example.com/login/oauth/authorize?client_id=secret")] - [InlineData("https://api.example.com", "https://api.example.com/login/oauth/authorize?client_id=secret")] + [InlineData("https://example.com/api/v3", "https://example.com/login/oauth/authorize?client_id=secret")] + [InlineData("https://api.example.com/any/path/really", "https://api.example.com/login/oauth/authorize?client_id=secret")] + [InlineData(null, "https://github.com/login/oauth/authorize?client_id=secret")] public void ReturnsProperAuthorizeUrl(string baseAddress, string expectedUrl) { var connection = Substitute.For(); - connection.BaseAddress.Returns(new Uri(baseAddress)); + connection.BaseAddress.Returns(baseAddress == null ? null : new Uri(baseAddress)); var client = new OauthClient(connection); var result = client.GetGitHubLoginUrl(new OauthLoginRequest("secret")); @@ -93,6 +94,40 @@ public async Task PostsWithCorrectBodyAndContentType() await calledBody.ReadAsStringAsync()); } + [Fact] + public async Task PostsWithCorrectBodyAndContentTypeForGHE() + { + var responseToken = new OauthToken(null, null, null); + var response = Substitute.For>(); + response.Body.Returns(responseToken); + var connection = Substitute.For(); + connection.BaseAddress.Returns(new Uri("https://example.com/api/v3")); + Uri calledUri = null; + FormUrlEncodedContent calledBody = null; + Uri calledHostAddress = null; + connection.Post( + Arg.Do(uri => calledUri = uri), + Arg.Do(body => calledBody = body as FormUrlEncodedContent), + "application/json", + null, + Arg.Do(uri => calledHostAddress = uri)) + .Returns(_ => Task.FromResult(response)); + var client = new OauthClient(connection); + + var token = await client.CreateAccessToken(new OauthTokenRequest("secretid", "secretsecret", "code") + { + RedirectUri = new Uri("https://example.com/foo") + }); + + Assert.Same(responseToken, token); + Assert.Equal("login/oauth/access_token", calledUri.ToString()); + Assert.NotNull(calledBody); + Assert.Equal("https://example.com/", calledHostAddress.ToString()); + Assert.Equal( + "client_id=secretid&client_secret=secretsecret&code=code&redirect_uri=https%3A%2F%2Fexample.com%2Ffoo", + await calledBody.ReadAsStringAsync()); + } + [Fact] public async Task DeserializesOAuthScopeFormat() { diff --git a/Octokit.Tests/Helpers/UriExtensionsTests.cs b/Octokit.Tests/Helpers/UriExtensionsTests.cs index 7effa276c5..1463ffb604 100644 --- a/Octokit.Tests/Helpers/UriExtensionsTests.cs +++ b/Octokit.Tests/Helpers/UriExtensionsTests.cs @@ -6,6 +6,23 @@ namespace Octokit.Tests.Helpers { public class UriExtensionsTests { + public class TheReplaceRelativeUriMethod + { + [Theory] + [InlineData("https://api.github.com", "my/new/path", "https://api.github.com/my/new/path")] + [InlineData("https://api.github.com", "/my/new/path", "https://api.github.com/my/new/path")] + [InlineData("https://example.com/api/v3", "my/new/path", "https://example.com/my/new/path")] + [InlineData("https://example.com/api/v3", "/my/new/path", "https://example.com/my/new/path")] + public void ReplacesRelativeUrisCorrectly(string baseUri, string relativeUri, string expectedUri) + { + var uri = new Uri(baseUri); + + var newUri = uri.ReplaceRelativeUri(relativeUri.FormatUri()); + + Assert.Equal(newUri.ToString(), expectedUri); + } + } + public class TheApplyParametersMethod { [Fact] diff --git a/Octokit/Clients/OAuthClient.cs b/Octokit/Clients/OAuthClient.cs index e6e17fb82c..d163ae3bdd 100644 --- a/Octokit/Clients/OAuthClient.cs +++ b/Octokit/Clients/OAuthClient.cs @@ -23,10 +23,12 @@ public OauthClient(IConnection connection) this.connection = connection; var baseAddress = connection.BaseAddress ?? GitHubClient.GitHubDotComUrl; - // The Oauth login stuff uses https://github.com and not the https://api.github.com URLs. + // The Oauth login stuff uses the main website and not the API URLs + // For https://api.github.com we use https://github.com + // For any other address (presumably a GitHub Enterprise address) we need to strip any relative Uri such as /api/v3 hostAddress = baseAddress.Host.Equals("api.github.com") ? new Uri("https://github.com") - : baseAddress; + : baseAddress.StripRelativeUri(); } /// diff --git a/Octokit/Helpers/ApiUrls.cs b/Octokit/Helpers/ApiUrls.cs index 24120b3165..a9c18556ce 100644 --- a/Octokit/Helpers/ApiUrls.cs +++ b/Octokit/Helpers/ApiUrls.cs @@ -18,9 +18,7 @@ public static partial class ApiUrls static readonly Uri _currentUserNotificationsEndpoint = new Uri("notifications", UriKind.Relative); static readonly Uri _currentUserAllIssues = new Uri("issues", UriKind.Relative); static readonly Uri _currentUserOwnedAndMemberIssues = new Uri("user/issues", UriKind.Relative); - static readonly Uri _oauthAuthorize = new Uri("login/oauth/authorize", UriKind.Relative); - static readonly Uri _oauthAccessToken = new Uri("login/oauth/access_token", UriKind.Relative); - + /// /// Returns the that returns all public repositories in /// response to a GET request. @@ -2164,7 +2162,7 @@ public static Uri User(string login) /// public static Uri OauthAuthorize() { - return _oauthAuthorize; + return "login/oauth/authorize".FormatUri(); } /// @@ -2173,7 +2171,7 @@ public static Uri OauthAuthorize() /// public static Uri OauthAccessToken() { - return _oauthAccessToken; + return "login/oauth/access_token".FormatUri(); } /// diff --git a/Octokit/Helpers/UriExtensions.cs b/Octokit/Helpers/UriExtensions.cs index 7317b9b90b..7be15802ad 100644 --- a/Octokit/Helpers/UriExtensions.cs +++ b/Octokit/Helpers/UriExtensions.cs @@ -9,6 +9,30 @@ namespace Octokit /// public static class UriExtensions { + /// + /// Returns a Uri where any existing relative Uri component is stripped + /// eg https://example.com/some/path becomes https://example.com + /// + /// Base Uri + /// + public static Uri StripRelativeUri(this Uri uri) + { + return new Uri(uri, "/"); + } + + /// + /// Returns a Uri where any existing relative Uri component is replaced with the respective value + /// eg https://example.com/some/path becomes https://example.com/replacement/path + /// + /// Base Uri + /// Relative Uri to add to the base Uri, replacing any existing relative Uri component + /// + public static Uri ReplaceRelativeUri(this Uri uri, Uri relativeUri) + { + // Prepending a forward slash to the relative Uri causes it to replace any that is existing + return new Uri(StripRelativeUri(uri), relativeUri); + } + /// /// Merge a dictionary of values with an existing ///