refact: return detailed error messages for WebApi integration

Closes: Brightspace#40
omsmith · Dec 5, 2018 · 2bccfda · 2bccfda
1 parent fd44b02
commit 2bccfda
Show file tree

Hide file tree

Showing 16 changed files with 260 additions and 65 deletions.
diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/AuthenticationAttribute.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/AuthenticationAttribute.cs
@@ -1,5 +1,8 @@
 using System;
+using System.Net;
+using System.Net.Http;
 using System.Web.Http.Controllers;
+using D2L.Security.OAuth2.Authorization.Exceptions;
 using D2L.Security.OAuth2.Principal;
 
 namespace D2L.Security.OAuth2.Authorization {
@@ -46,10 +49,24 @@ protected override bool IsAuthorizedInternal( HttpActionContext context ) {
 					return false;
 
 				case PrincipalType.User:
-					return m_allowUsers;
+					if( m_allowUsers ) {
+						return true;
+					}
+
+					throw new OAuth2Exception(
+						error: OAuth2Exception.Type.invalid_token,
+						errorDescription: "Users are not allowed to access this API."
+					);
 
 				case PrincipalType.Service:
-					return m_allowServices;
+					if( m_allowServices ) {
+						return true;
+					}
+
+					throw new OAuth2Exception(
+						error: OAuth2Exception.Type.invalid_token,
+						errorDescription: "Services are not allowed to access this API."
+					);
 
 				default:
 					throw new NotImplementedException();

diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/Exceptions/InsufficientScopeException.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/Exceptions/InsufficientScopeException.cs
@@ -0,0 +1,18 @@
+using System;
+using D2L.Security.OAuth2.Scopes;
+
+namespace D2L.Security.OAuth2.Authorization.Exceptions {
+	internal sealed class InsufficientScopeException : OAuth2Exception {
+
+		internal InsufficientScopeException( Scope scope, Exception innerException = null ) : base(
+			error: OAuth2Exception.Type.insufficient_scope,
+			errorDescription: $"Required scope: '{ scope }'",
+			innerException: innerException
+		) {
+			Scope = scope;
+		}
+
+		internal Scope Scope { get; }
+
+	}
+}
diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/Exceptions/OAuth2Exception.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/Exceptions/OAuth2Exception.cs
@@ -0,0 +1,32 @@
+using System;
+using System.Net;
+
+namespace D2L.Security.OAuth2.Authorization.Exceptions {
+	internal class OAuth2Exception : Exception {
+		// Note: the naming style of these enum values matches the codes from RFC6750
+		public enum Type {
+			invalid_request = HttpStatusCode.BadRequest,
+			invalid_token = HttpStatusCode.Unauthorized,
+			insufficient_scope = HttpStatusCode.Forbidden
+		}
+
+		internal OAuth2Exception(
+			Type error,
+			string errorDescription,
+			Exception innerException = null
+		) : base(
+			message: $"{ error }: { errorDescription }",
+			innerException: innerException
+		) {
+			Error = error;
+			ErrorDescription = errorDescription;
+
+			if( errorDescription.Contains( "\"" ) ) {
+				throw new ArgumentException( nameof( errorDescription ), "Must not contain '\"' character" );
+			}
+		}
+
+		public Type Error { get; }
+		public string ErrorDescription { get; }
+	}
+}
diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/NoImpersonationAttribute.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/NoImpersonationAttribute.cs
@@ -1,7 +1,6 @@
 using System;
-using System.Net;
-using System.Net.Http;
 using System.Web.Http.Controllers;
+using D2L.Security.OAuth2.Authorization.Exceptions;
 using D2L.Security.OAuth2.Principal;
 
 namespace D2L.Security.OAuth2.Authorization {
@@ -21,27 +20,21 @@ protected override bool IsAuthorizedInternal( HttpActionContext actionContext )
 				.Principal as ID2LPrincipal;
 
 			if( principal == null ) {
-				return true;
+				return false;
 			}
 
 			if( principal.Type != PrincipalType.User ) {
 				return true;
 			}
 
-			return principal.ActualUserId == principal.UserId;
-		}
-
-		protected override void HandleUnauthorizedRequestInternal(
-			HttpActionContext actionContext
-		) {
-			var response = actionContext
-				.Request
-				.CreateErrorResponse(
-					HttpStatusCode.Forbidden,
-					"This API is not usable while impersonating. This error message indicates a bug in the client application which is responsible for knowing this."
-				);
+			if( principal.UserId == principal.ActualUserId ) {
+				return true;
+			}
 
-			actionContext.Response = response;
+			throw new OAuth2Exception(
+				error: OAuth2Exception.Type.invalid_token,
+				errorDescription: "This API is not usable while impersonating. This error message indicates a bug in the client application which is responsible for knowing this."
+			);
 		}
 	}
 }
diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/OAuth2AuthorizeAttribute.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/OAuth2AuthorizeAttribute.cs
@@ -1,12 +1,18 @@
 using System.Linq;
+using System.Net;
+using System.Net.Http;
+using System.Net.Http.Formatting;
+using System.Web;
 using System.Web.Http;
 using System.Web.Http.Controllers;
+using D2L.Security.OAuth2.Authorization.Exceptions;
+using Newtonsoft.Json;
 
 namespace D2L.Security.OAuth2.Authorization {
 	public abstract class OAuth2AuthorizeAttribute : AuthorizeAttribute {
 
 		private const string AUTH_HAS_RUN = "D2L.Security.OAuth2.Authorization.OAuth2AuthorizeAttribute_Auth_Has_Run";
-		private const string FAILING_ATTR_PROP = "D2L.Security.OAuth2.Authorization.OAuth2AuthorizeAttribute_Failing_Attribute";
+		private const string FAILED_EXCEPTION_PROP = "D2L.Security.OAuth2.Authorization.OAuth2AuthorizeAttribute_Failed_Exception";
 
 		protected override bool IsAuthorized( HttpActionContext actionContext ) {
 			var props = actionContext.Request.Properties;
@@ -32,8 +38,12 @@ protected override bool IsAuthorized( HttpActionContext actionContext ) {
 				.ToArray();
 
 			foreach( var attr in oauth2Attributes ) {
-				if( !attr.IsAuthorizedInternal( actionContext ) ) {
-					props[ FAILING_ATTR_PROP ] = attr;
+				try {
+					if( !attr.IsAuthorizedInternal( actionContext ) ) {
+						return false;
+					}
+				} catch( OAuth2Exception e ) {
+					props[ FAILED_EXCEPTION_PROP ] = e;
 					return false;
 				}
 			}
@@ -44,9 +54,36 @@ protected override bool IsAuthorized( HttpActionContext actionContext ) {
 		protected override void HandleUnauthorizedRequest( HttpActionContext actionContext ) {
 			var props = actionContext.Request.Properties;
 
-			var attr = ( OAuth2AuthorizeAttribute )props[ FAILING_ATTR_PROP ];
+			if( !props.TryGetValue( FAILED_EXCEPTION_PROP, out object exceptionObj ) ) {
+				HandleNoAuth( actionContext );
+				return;
+			}
+
+			OAuth2Exception exception = exceptionObj as OAuth2Exception;
+			OAuth2ErrorResponse responseContent = new OAuth2ErrorResponse(
+				error: exception.Error.ToString(),
+				errorDescription: exception.ErrorDescription
+			);
+			string authenticateHeader = $"Bearer error=\"{ responseContent.Error }\", error_description=\"{ responseContent.ErrorDescription }\"";
+
+			if( exception is InsufficientScopeException insufficientScopeException ) {
+				responseContent.Scope = insufficientScopeException.Scope.ToString();
+				authenticateHeader += $", scope=\"{ responseContent.Scope }\"";
+			}
 
-			attr.HandleUnauthorizedRequestInternal( actionContext );
+			var response = new HttpResponseMessage( (HttpStatusCode)exception.Error ) {
+				Content = new ObjectContent<OAuth2ErrorResponse>(
+					responseContent,
+					new JsonMediaTypeFormatter() {
+						SerializerSettings = new JsonSerializerSettings() {
+							NullValueHandling = NullValueHandling.Ignore
+						}
+					}
+				)
+			};
+			response.Headers.Add( "WWW-Authenticate", authenticateHeader );
+
+			actionContext.Response = response;
 		}
 
 		protected virtual void HandleUnauthorizedRequestInternal( HttpActionContext actionContext ) {
@@ -57,5 +94,12 @@ protected virtual void HandleUnauthorizedRequestInternal( HttpActionContext acti
 
 		protected abstract bool IsAuthorizedInternal( HttpActionContext actionContext );
 
+		private static void HandleNoAuth( HttpActionContext actionContext ) {
+			var response = new HttpResponseMessage( HttpStatusCode.Unauthorized );
+			response.Headers.Add( "WWW-Authenticate", "Bearer" );
+
+			actionContext.Response = response;
+		}
+
 	}
 }
diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/OAuth2ErrorResponse.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/OAuth2ErrorResponse.cs
@@ -0,0 +1,24 @@
+using Newtonsoft.Json;
+
+namespace D2L.Security.OAuth2.Authorization {
+	internal sealed class OAuth2ErrorResponse {
+
+		public OAuth2ErrorResponse(
+			string error,
+			string errorDescription
+		) {
+			Error = error;
+			ErrorDescription = errorDescription;
+		}
+
+		[JsonProperty( "error", Required = Required.Always )]
+		public string Error { get; }
+
+		[JsonProperty( "error_description", Required = Required.Always )]
+		public string ErrorDescription { get; }
+
+		[JsonProperty( "scope" )]
+		public string Scope { get; set; }
+
+	}
+}
diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/RequireClaimAttribute.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/RequireClaimAttribute.cs
@@ -1,7 +1,6 @@
 using System;
-using System.Net;
-using System.Net.Http;
 using System.Web.Http.Controllers;
+using D2L.Security.OAuth2.Authorization.Exceptions;
 using D2L.Security.OAuth2.Principal;
 using D2L.Services;
 
@@ -30,25 +29,14 @@ protected override bool IsAuthorizedInternal( HttpActionContext actionContext )
 				return false;
 			}
 
-			bool hasClaim = principal
-				.AccessToken
-				.Claims
-				.HasClaim( m_claimType );
-
-			return hasClaim;
-		}
-
-		protected override void HandleUnauthorizedRequestInternal(
-			HttpActionContext actionContext
-		) {
-			var response = actionContext
-				.Request
-				.CreateErrorResponse(
-					HttpStatusCode.Forbidden,
-					String.Format( "Missing claim: '{0}'", m_claimType )
-				);
+			if( principal.AccessToken.Claims.HasClaim( m_claimType ) ) {
+				return true;
+			}
 
-			actionContext.Response = response;
+			throw new OAuth2Exception(
+				error: OAuth2Exception.Type.invalid_token,
+				errorDescription: $"Missing claim: '{ m_claimType }'"
+			);
 		}
 	}
 }
diff --git a/src/D2L.Security.OAuth2.WebApi/Authorization/RequireScopeAttribute.cs b/src/D2L.Security.OAuth2.WebApi/Authorization/RequireScopeAttribute.cs
@@ -1,7 +1,6 @@
 using System;
-using System.Net;
-using System.Net.Http;
 using System.Web.Http.Controllers;
+using D2L.Security.OAuth2.Authorization.Exceptions;
 using D2L.Security.OAuth2.Principal;
 using D2L.Security.OAuth2.Scopes;
 
@@ -39,16 +38,11 @@ protected override bool IsAuthorizedInternal( HttpActionContext actionContext )
 				return false;
 			}
 
-			bool isAuthorized = ScopeAuthorizer.IsAuthorized( principal.Scopes, m_requiredScope );
-
-			return isAuthorized;
-		}
-
-		protected override void HandleUnauthorizedRequestInternal( HttpActionContext actionContext ) {
-			var response = actionContext.Request.CreateErrorResponse( HttpStatusCode.Forbidden, "insufficient_scope" );
-			response.Headers.Add( "WWW-Authenticate", "Bearer error=\"insufficient_scope\"" );
+			if( ScopeAuthorizer.IsAuthorized( principal.Scopes, m_requiredScope ) ) {
+				return true;
+			}
 
-			actionContext.Response = response;
+			throw new InsufficientScopeException( m_requiredScope );
 		}
 	}
 }
diff --git a/src/D2L.Security.OAuth2.WebApi/D2L.Security.OAuth2.WebApi.csproj b/src/D2L.Security.OAuth2.WebApi/D2L.Security.OAuth2.WebApi.csproj
@@ -87,9 +87,12 @@
     <Compile Include="Authentication\OAuth2AuthenticationFilter.cs" />
     <Compile Include="Authorization\AuthenticationAttribute.cs" />
     <Compile Include="Authorization\DefaultAuthorizationAttribute.cs" />
+    <Compile Include="Authorization\Exceptions\InsufficientScopeException.cs" />
     <Compile Include="Authorization\NoImpersonationAttribute.cs" />
     <Compile Include="Authorization\NoRequiredScopeAttribute.cs" />
     <Compile Include="Authorization\OAuth2AuthorizeAttribute.cs" />
+    <Compile Include="Authorization\Exceptions\OAuth2Exception.cs" />
+    <Compile Include="Authorization\OAuth2ErrorResponse.cs" />
     <Compile Include="Authorization\RequireClaimAttribute.cs" />
     <Compile Include="Authorization\RequireScopeAttribute.cs" />
     <Compile Include="Extensions\System.Security.Claims.ClaimsPrincipal.Extensions.cs" />

diff --git a/...curity.OAuth2.WebApi.IntegrationTests/Authorization/DefaultAuthorizationAttributeTests.cs b/...curity.OAuth2.WebApi.IntegrationTests/Authorization/DefaultAuthorizationAttributeTests.cs
@@ -24,7 +24,7 @@ await TestUtilities.RunBasicAuthTest( "/authorization/unspecifiedprincipaltype",
 
 
 		[Test]
-		public async Task Basic_NoAuthentication_403() {
+		public async Task Basic_NoAuthentication_401() {
 			await TestUtilities.RunBasicAuthTest( "/authorization/basic", HttpStatusCode.Unauthorized )
 				.SafeAsync();
 		}

diff --git a/...2L.Security.OAuth2.WebApi.IntegrationTests/Authorization/NoImpersonationAttributeTests.cs b/...2L.Security.OAuth2.WebApi.IntegrationTests/Authorization/NoImpersonationAttributeTests.cs
@@ -29,14 +29,24 @@ await TestUtilities
 		}
 
 		[Test]
-		public async Task ImpersonationToken_Forbidden() {
+		public async Task ImpersonationToken_Unauthorized() {
 			string jwt = await TestUtilities
 				.GetAccessTokenValidForAMinute( userId: 1234, actualUserId: 1235 )
 				.SafeAsync();
 
-			await TestUtilities
-				.RunBasicAuthTest( "/authorization/imp", jwt, HttpStatusCode.Forbidden )
+			var response = await TestUtilities
+				.RunBasicAuthTest<OAuth2ErrorResponse>( "/authorization/imp", jwt, HttpStatusCode.Unauthorized )
 				.SafeAsync();
+
+			string expectedError = "invalid_token";
+			string expectedErrorDescription = "This API is not usable while impersonating. This error message indicates a bug in the client application which is responsible for knowing this.";
+
+			Assert.AreEqual( expectedError, response.Body.Error );
+			Assert.AreEqual( expectedErrorDescription, response.Body.ErrorDescription );
+
+			string challengeHeader = response.Headers.WwwAuthenticate.ToString();
+			StringAssert.Contains( expectedError, challengeHeader );
+			StringAssert.Contains( expectedErrorDescription, challengeHeader );
 		}
 	}
 }
diff --git a/test/D2L.Security.OAuth2.WebApi.IntegrationTests/Authorization/RequireClaimAttributeTests.cs b/test/D2L.Security.OAuth2.WebApi.IntegrationTests/Authorization/RequireClaimAttributeTests.cs
@@ -15,16 +15,24 @@ await TestUtilities
 		}
 
 		[Test]
-		public async Task ServiceToken_Forbidden() {
+		public async Task ServiceToken_Unauthorized() {
 			string jwt = await TestUtilities
 				.GetAccessTokenValidForAMinute( userId: null )
 				.SafeAsync();
 
-			string body = await TestUtilities
-				.RunBasicAuthTest( RequireClaimAttributeTestsController.ROUTE, jwt, HttpStatusCode.Forbidden )
+			var response = await TestUtilities
+				.RunBasicAuthTest<OAuth2ErrorResponse>( RequireClaimAttributeTestsController.ROUTE, jwt, HttpStatusCode.Unauthorized )
 				.SafeAsync();
 
-			StringAssert.Contains( Constants.Claims.USER_ID, body );
+			string expectedError = "invalid_token";
+			string expectedErrorDescription = $"Missing claim: '{ Constants.Claims.USER_ID }'";
+
+			Assert.AreEqual( expectedError, response.Body.Error );
+			Assert.AreEqual( expectedErrorDescription, response.Body.ErrorDescription );
+
+			string challengeHeader = response.Headers.WwwAuthenticate.ToString();
+			StringAssert.Contains( expectedError, challengeHeader );
+			StringAssert.Contains( expectedErrorDescription, challengeHeader );
 		}
 
 		[Test]