From 238fdf24e8b0e3c5e84cd8dc7f7324502a439429 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 3 Nov 2015 12:37:35 -0800 Subject: [PATCH] Further improve error handling for OAuth providers --- .../FacebookHandler.cs | 2 +- .../OAuthHandler.cs | 30 +++++++-- .../OAuthTokenResponse.cs | 19 +++++- .../RemoteAuthenticationHandler.cs | 3 +- .../Google/GoogleMiddlewareTests.cs | 64 +++++++++---------- .../Twitter/TwitterMiddlewareTests.cs | 8 +-- 6 files changed, 80 insertions(+), 46 deletions(-) diff --git a/src/Microsoft.AspNet.Authentication.Facebook/FacebookHandler.cs b/src/Microsoft.AspNet.Authentication.Facebook/FacebookHandler.cs index cd6c2d1f73..e25fef4967 100644 --- a/src/Microsoft.AspNet.Authentication.Facebook/FacebookHandler.cs +++ b/src/Microsoft.AspNet.Authentication.Facebook/FacebookHandler.cs @@ -46,7 +46,7 @@ namespace Microsoft.AspNet.Authentication.Facebook } // The refresh token is not available. - return new OAuthTokenResponse(payload); + return OAuthTokenResponse.Success(payload); } protected override async Task CreateTicketAsync(ClaimsIdentity identity, AuthenticationProperties properties, OAuthTokenResponse tokens) diff --git a/src/Microsoft.AspNet.Authentication.OAuth/OAuthHandler.cs b/src/Microsoft.AspNet.Authentication.OAuth/OAuthHandler.cs index 5db46991bb..c0f05aa0dc 100644 --- a/src/Microsoft.AspNet.Authentication.OAuth/OAuthHandler.cs +++ b/src/Microsoft.AspNet.Authentication.OAuth/OAuthHandler.cs @@ -7,6 +7,7 @@ using System.Net.Http; using System.Net.Http.Headers; using System.Security.Claims; using System.Security.Cryptography; +using System.Text; using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Http.Authentication; @@ -62,9 +63,14 @@ namespace Microsoft.AspNet.Authentication.OAuth var tokens = await ExchangeCodeAsync(code, BuildRedirectUri(Options.CallbackPath)); + if (tokens.Error != null) + { + return AuthenticateResult.Failed(tokens.Error); + } + if (string.IsNullOrEmpty(tokens.AccessToken)) { - return AuthenticateResult.Failed("Access token was not found."); + return AuthenticateResult.Failed("Failed to retrieve access token."); } var identity = new ClaimsIdentity(Options.ClaimsIssuer); @@ -113,9 +119,25 @@ namespace Microsoft.AspNet.Authentication.OAuth requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); requestMessage.Content = requestContent; var response = await Backchannel.SendAsync(requestMessage, Context.RequestAborted); - response.EnsureSuccessStatusCode(); - var payload = JObject.Parse(await response.Content.ReadAsStringAsync()); - return new OAuthTokenResponse(payload); + if (response.IsSuccessStatusCode) + { + var payload = JObject.Parse(await response.Content.ReadAsStringAsync()); + return OAuthTokenResponse.Success(payload); + } + else + { + var error = "OAuth token endpoint failure: " + await Display(response); + return OAuthTokenResponse.Failed(new Exception(error)); + } + } + + private static async Task Display(HttpResponseMessage response) + { + var output = new StringBuilder(); + output.Append("Status: " + response.StatusCode + ";"); + output.Append("Headers: " + response.Headers.ToString() + ";"); + output.Append("Body: " + await response.Content.ReadAsStringAsync() + ";"); + return output.ToString(); } protected virtual async Task CreateTicketAsync(ClaimsIdentity identity, AuthenticationProperties properties, OAuthTokenResponse tokens) diff --git a/src/Microsoft.AspNet.Authentication.OAuth/OAuthTokenResponse.cs b/src/Microsoft.AspNet.Authentication.OAuth/OAuthTokenResponse.cs index f796669553..7a45120b4a 100644 --- a/src/Microsoft.AspNet.Authentication.OAuth/OAuthTokenResponse.cs +++ b/src/Microsoft.AspNet.Authentication.OAuth/OAuthTokenResponse.cs @@ -1,13 +1,14 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using Newtonsoft.Json.Linq; namespace Microsoft.AspNet.Authentication.OAuth { public class OAuthTokenResponse { - public OAuthTokenResponse(JObject response) + private OAuthTokenResponse(JObject response) { Response = response; AccessToken = response.Value("access_token"); @@ -16,10 +17,26 @@ namespace Microsoft.AspNet.Authentication.OAuth ExpiresIn = response.Value("expires_in"); } + private OAuthTokenResponse(Exception error) + { + Error = error; + } + + public static OAuthTokenResponse Success(JObject response) + { + return new OAuthTokenResponse(response); + } + + public static OAuthTokenResponse Failed(Exception error) + { + return new OAuthTokenResponse(error); + } + public JObject Response { get; set; } public string AccessToken { get; set; } public string TokenType { get; set; } public string RefreshToken { get; set; } public string ExpiresIn { get; set; } + public Exception Error { get; set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Authentication/RemoteAuthenticationHandler.cs b/src/Microsoft.AspNet.Authentication/RemoteAuthenticationHandler.cs index 7e382edd06..0851b1b5ce 100644 --- a/src/Microsoft.AspNet.Authentication/RemoteAuthenticationHandler.cs +++ b/src/Microsoft.AspNet.Authentication/RemoteAuthenticationHandler.cs @@ -36,8 +36,7 @@ namespace Microsoft.AspNet.Authentication return false; } - Context.Response.StatusCode = 500; - return true; + throw new AggregateException("Unhandled remote error.", errorContext.Error); } // We have a ticket if we get here diff --git a/test/Microsoft.AspNet.Authentication.Test/Google/GoogleMiddlewareTests.cs b/test/Microsoft.AspNet.Authentication.Test/Google/GoogleMiddlewareTests.cs index beda30bbdf..ab822206de 100644 --- a/test/Microsoft.AspNet.Authentication.Test/Google/GoogleMiddlewareTests.cs +++ b/test/Microsoft.AspNet.Authentication.Test/Google/GoogleMiddlewareTests.cs @@ -244,8 +244,8 @@ namespace Microsoft.AspNet.Authentication.Google options.ClientId = "Test Id"; options.ClientSecret = "Test Secret"; }); - var transaction = await server.SendAsync("https://example.com/signin-google?code=TestCode"); - Assert.Equal(HttpStatusCode.InternalServerError, transaction.Response.StatusCode); + var error = await Assert.ThrowsAnyAsync(() => server.SendAsync("https://example.com/signin-google?code=TestCode")); + Assert.Equal("The oauth state was missing or invalid.", error.Message); } [Theory] @@ -270,15 +270,16 @@ namespace Microsoft.AspNet.Authentication.Google }; } }); - var transaction = await server.SendAsync("https://example.com/signin-google?error=OMG"); if (redirect) { + var transaction = await server.SendAsync("https://example.com/signin-google?error=OMG"); Assert.Equal(HttpStatusCode.Redirect, transaction.Response.StatusCode); Assert.Equal("/error?ErrorMessage=OMG", transaction.Response.Headers.GetValues("Location").First()); } else { - Assert.Equal(HttpStatusCode.InternalServerError, transaction.Response.StatusCode); + var error = await Assert.ThrowsAnyAsync(() => server.SendAsync("https://example.com/signin-google?error=OMG")); + Assert.Equal("OMG", error.Message); } } @@ -379,7 +380,8 @@ namespace Microsoft.AspNet.Authentication.Google { Sender = req => { - return new HttpResponseMessage(HttpStatusCode.BadRequest); + return ReturnJsonResponse(new { Error = "Error" }, + HttpStatusCode.BadRequest); } }; if (redirect) @@ -402,24 +404,21 @@ namespace Microsoft.AspNet.Authentication.Google properties.RedirectUri = "/me"; var state = stateFormat.Protect(properties); - - await Assert.ThrowsAsync(() => server.SendAsync( + var sendTask = server.SendAsync( "https://example.com/signin-google?code=TestCode&state=" + UrlEncoder.Default.Encode(state), - correlationKey + "=" + correlationValue)); - - //var transaction = await server.SendAsync( - // "https://example.com/signin-google?code=TestCode&state=" + UrlEncoder.Default.Encode(state), - // correlationKey + "=" + correlationValue); - //if (redirect) - //{ - // Assert.Equal(HttpStatusCode.Redirect, transaction.Response.StatusCode); - // Assert.Equal("/error?ErrorMessage=" + UrlEncoder.Default.Encode("Access token was not found."), - // transaction.Response.Headers.GetValues("Location").First()); - //} - //else - //{ - // Assert.Equal(HttpStatusCode.InternalServerError, transaction.Response.StatusCode); - //} + correlationKey + "=" + correlationValue); + if (redirect) + { + var transaction = await sendTask; + Assert.Equal(HttpStatusCode.Redirect, transaction.Response.StatusCode); + Assert.Equal("/error?ErrorMessage=" + UrlEncoder.Default.Encode("OAuth token endpoint failure: Status: BadRequest;Headers: ;Body: {\"Error\":\"Error\"};"), + transaction.Response.Headers.GetValues("Location").First()); + } + else + { + var error = await Assert.ThrowsAnyAsync(() => sendTask); + Assert.Equal("OAuth token endpoint failure: Status: BadRequest;Headers: ;Body: {\"Error\":\"Error\"};", error.Message); + } } [Theory] @@ -459,18 +458,20 @@ namespace Microsoft.AspNet.Authentication.Google properties.Items.Add(correlationKey, correlationValue); properties.RedirectUri = "/me"; var state = stateFormat.Protect(properties); - var transaction = await server.SendAsync( + var sendTask = server.SendAsync( "https://example.com/signin-google?code=TestCode&state=" + UrlEncoder.Default.Encode(state), correlationKey + "=" + correlationValue); if (redirect) { + var transaction = await sendTask; Assert.Equal(HttpStatusCode.Redirect, transaction.Response.StatusCode); - Assert.Equal("/error?ErrorMessage=" + UrlEncoder.Default.Encode("Access token was not found."), + Assert.Equal("/error?ErrorMessage=" + UrlEncoder.Default.Encode("Failed to retrieve access token."), transaction.Response.Headers.GetValues("Location").First()); } else { - Assert.Equal(HttpStatusCode.InternalServerError, transaction.Response.StatusCode); + var error = await Assert.ThrowsAnyAsync(() => sendTask); + Assert.Equal("Failed to retrieve access token.", error.Message); } } @@ -712,9 +713,8 @@ namespace Microsoft.AspNet.Authentication.Google } [Fact] - public async Task NoStateCauses500() + public async Task NoStateCausesException() { - var stateFormat = new PropertiesDataFormat(new EphemeralDataProtectionProvider().CreateProtector("GoogleTest")); var server = CreateServer(options => { options.ClientId = "Test Id"; @@ -722,10 +722,8 @@ namespace Microsoft.AspNet.Authentication.Google }); //Post a message to the Google middleware - var transaction = await server.SendAsync( - "https://example.com/signin-google?code=TestCode"); - - Assert.Equal(HttpStatusCode.InternalServerError, transaction.Response.StatusCode); + var error = await Assert.ThrowsAnyAsync(() => server.SendAsync("https://example.com/signin-google?code=TestCode")); + Assert.Equal("The oauth state was missing or invalid.", error.Message); } [Fact] @@ -756,9 +754,9 @@ namespace Microsoft.AspNet.Authentication.Google transaction.Response.Headers.GetValues("Location").First()); } - private static HttpResponseMessage ReturnJsonResponse(object content) + private static HttpResponseMessage ReturnJsonResponse(object content, HttpStatusCode code = HttpStatusCode.OK) { - var res = new HttpResponseMessage(HttpStatusCode.OK); + var res = new HttpResponseMessage(code); var text = JsonConvert.SerializeObject(content); res.Content = new StringContent(text, Encoding.UTF8, "application/json"); return res; diff --git a/test/Microsoft.AspNet.Authentication.Test/Twitter/TwitterMiddlewareTests.cs b/test/Microsoft.AspNet.Authentication.Test/Twitter/TwitterMiddlewareTests.cs index aa060482b6..e66090144c 100644 --- a/test/Microsoft.AspNet.Authentication.Test/Twitter/TwitterMiddlewareTests.cs +++ b/test/Microsoft.AspNet.Authentication.Test/Twitter/TwitterMiddlewareTests.cs @@ -64,7 +64,7 @@ namespace Microsoft.AspNet.Authentication.Twitter } [Fact] - public async Task BadSignInWill500() + public async Task BadSignInWillThrow() { var server = CreateServer(options => { @@ -73,10 +73,8 @@ namespace Microsoft.AspNet.Authentication.Twitter }); // Send a bogus sign in - var transaction = await server.SendAsync( - "https://example.com/signin-twitter"); - - Assert.Equal(HttpStatusCode.InternalServerError, transaction.Response.StatusCode); + var error = await Assert.ThrowsAnyAsync(() => server.SendAsync("https://example.com/signin-twitter")); + Assert.Equal("Invalid state cookie.", error.Message); } [Fact]