From 3a04ec8be3dfa0de24de2d6a274d4e3b11c0d1be Mon Sep 17 00:00:00 2001 From: Dylan Beattie Date: Thu, 3 Dec 2015 12:33:48 +0000 Subject: [PATCH] Fixed bug whereby the presence of the Basic authorizer contributor would cause security negotiation even for resources not decorated with the required attribute. The AuthorizerContributor now just attempts to set the User.Identity - but will never return Unauthorized. There's now an explict RequiresBasicAuthenticationAttribute(realm) attribute that will trigger the WWW-Authenticate negotation if an unauthorized request is received for a secured resource. --- .../OpenRasta.Tests.Unit.csproj | 1 + .../BasicAuthorizer_Specification.cs | 56 --------------- ...AuthenticationInterceptor_Specification.cs | 3 +- ...AuthenticationInterceptor_Specification.cs | 28 ++++++++ src/OpenRasta/OpenRasta.csproj | 2 + .../BasicAuthorizerContributor.cs | 69 +++++-------------- .../Security/BasicAuthorizationHeader.cs | 10 +-- .../RequiresAuthenticationInterceptor.cs | 9 ++- .../RequiresBasicAuthenticationAttribute.cs | 57 +++++++++++++++ .../RequiresBasicAuthenticationInterceptor.cs | 25 +++++++ 10 files changed, 143 insertions(+), 117 deletions(-) create mode 100644 src/OpenRasta.Tests.Unit/Security/RequiresBasicAuthenticationInterceptor_Specification.cs create mode 100644 src/OpenRasta/Security/RequiresBasicAuthenticationAttribute.cs create mode 100644 src/OpenRasta/Security/RequiresBasicAuthenticationInterceptor.cs diff --git a/src/OpenRasta.Tests.Unit/OpenRasta.Tests.Unit.csproj b/src/OpenRasta.Tests.Unit/OpenRasta.Tests.Unit.csproj index f8f43a24..3422284e 100644 --- a/src/OpenRasta.Tests.Unit/OpenRasta.Tests.Unit.csproj +++ b/src/OpenRasta.Tests.Unit/OpenRasta.Tests.Unit.csproj @@ -127,6 +127,7 @@ + diff --git a/src/OpenRasta.Tests.Unit/Pipeline/Contributors/BasicAuthorizer_Specification.cs b/src/OpenRasta.Tests.Unit/Pipeline/Contributors/BasicAuthorizer_Specification.cs index 26905b8f..923f5e50 100644 --- a/src/OpenRasta.Tests.Unit/Pipeline/Contributors/BasicAuthorizer_Specification.cs +++ b/src/OpenRasta.Tests.Unit/Pipeline/Contributors/BasicAuthorizer_Specification.cs @@ -36,61 +36,5 @@ public void BasicAuthorizer_Succeeds_For_Valid_Credentials(string username, stri result.ShouldBe(PipelineContinuation.Continue); Context.User.Identity.Name.ShouldBe(username); } - - [Test] - public void BasicAuthorizer_Returns_Authenticate_Header_For_Invalid_Credentials() - { - var mockAuthenticationProvider = new Mock(); - mockAuthenticationProvider.Setup(ap => ap.GetByUsername(It.IsAny())).Returns(new Credentials()); - mockAuthenticationProvider.Setup(ap => ap.ValidatePassword(It.IsAny(), It.IsAny())).Returns(false); - given_dependency(mockAuthenticationProvider.Object); - given_pipeline_contributor(); - Context.Request.Headers.Add("Authorization", "Basic " + Convert.ToBase64String(Encoding.ASCII.GetBytes("username:password"))); - var result = when_sending_notification(); - result.ShouldBe(PipelineContinuation.RenderNow); - Context.OperationResult.ShouldBeOfType(); - Context.User.ShouldBe(null); - } - - [Test] - [TestCase("Basic THIS_IS_NOT_A_BASE64_ENCODED_CREDENTIAL")] - [TestCase("Invalid Header")] - public void BasicAuthorizer_Returns_Unauthorized_When_Header_Is_Invalid(string header) - { - var mockAuthenticationProvider = new Mock(); - given_dependency(mockAuthenticationProvider.Object); - given_pipeline_contributor(); - - Context.Request.Headers.Add("Authorization", header); - - var result = when_sending_notification(); - result.ShouldBe(PipelineContinuation.RenderNow); - Context.OperationResult.ShouldBeOfType(); - } - - [Test] - public void BasicAuthorizer_Sends_WwwAuthenticate_For_Missing_Credentials() - { - var mockAuthenticationProvider = new Mock(); - given_dependency(mockAuthenticationProvider.Object); - given_pipeline_contributor(); - - when_sending_notification(); - when_sending_notification(); - var header = String.Format("Basic realm=\"{0}\"", BasicAuthenticationRequiredHeader.DEFAULT_REALM); - Context.Response.Headers["WWW-Authenticate"].ShouldBe(header); - } - - [Test] - public void BasicAuthorizer_Returns_NotAuthorized_For_Missing_Credentials() - { - var mockAuthenticationProvider = new Mock(); - given_dependency(mockAuthenticationProvider.Object); - given_pipeline_contributor(); - - var result = when_sending_notification(); - result.ShouldBe(PipelineContinuation.RenderNow); - Context.OperationResult.ShouldBeOfType(); - } } } diff --git a/src/OpenRasta.Tests.Unit/Security/RequiresAuthenticationInterceptor_Specification.cs b/src/OpenRasta.Tests.Unit/Security/RequiresAuthenticationInterceptor_Specification.cs index faf9ab82..be66e819 100644 --- a/src/OpenRasta.Tests.Unit/Security/RequiresAuthenticationInterceptor_Specification.cs +++ b/src/OpenRasta.Tests.Unit/Security/RequiresAuthenticationInterceptor_Specification.cs @@ -38,4 +38,5 @@ public void execution_is_allowed() .ShouldBeTrue(); } } -} \ No newline at end of file +} + diff --git a/src/OpenRasta.Tests.Unit/Security/RequiresBasicAuthenticationInterceptor_Specification.cs b/src/OpenRasta.Tests.Unit/Security/RequiresBasicAuthenticationInterceptor_Specification.cs new file mode 100644 index 00000000..734b3fd2 --- /dev/null +++ b/src/OpenRasta.Tests.Unit/Security/RequiresBasicAuthenticationInterceptor_Specification.cs @@ -0,0 +1,28 @@ +using System; +using System.Diagnostics; +using System.Security.Principal; +using Moq; +using NUnit.Framework; +using OpenRasta.Hosting.InMemory; +using OpenRasta.OperationModel; +using OpenRasta.Security; +using OpenRasta.Testing; +using OpenRasta.Web; + +namespace RequiresBasicAuthenticationInterceptor_Specification +{ + public class when_the_user_is_not_authenticated : context + { + [Test] + public void execution_is_denied() + { + var context = new InMemoryCommunicationContext(); + const string REALM = "Test Realm"; + var authenticationInterceptor = new RequiresBasicAuthenticationInterceptor(context, REALM); + authenticationInterceptor.BeforeExecute(new Mock().Object).ShouldBeFalse(); + context.OperationResult.ShouldBeOfType(); + var expectedHeader = String.Format("Basic realm=\"{0}\"", REALM); + context.Response.Headers["WWW-Authenticate"].ShouldBe(expectedHeader); + } + } +} \ No newline at end of file diff --git a/src/OpenRasta/OpenRasta.csproj b/src/OpenRasta/OpenRasta.csproj index d06a17f2..75730479 100644 --- a/src/OpenRasta/OpenRasta.csproj +++ b/src/OpenRasta/OpenRasta.csproj @@ -329,6 +329,8 @@ + + diff --git a/src/OpenRasta/Pipeline/Contributors/BasicAuthorizerContributor.cs b/src/OpenRasta/Pipeline/Contributors/BasicAuthorizerContributor.cs index d5473525..ece0cd49 100644 --- a/src/OpenRasta/Pipeline/Contributors/BasicAuthorizerContributor.cs +++ b/src/OpenRasta/Pipeline/Contributors/BasicAuthorizerContributor.cs @@ -26,8 +26,6 @@ namespace OpenRasta.Pipeline.Contributors { public class BasicAuthorizerContributor : IPipelineContributor { - public const string REALM = "Basic Authentication"; - private readonly IDependencyResolver _resolver; private IAuthenticationProvider _authentication; @@ -43,72 +41,39 @@ public void Initialize(IPipeline pipelineRunner) .After() .And .Before(); - - pipelineRunner.Notify(WriteCredentialRequest) - .After() - .And - .Before(); } public PipelineContinuation ReadCredentials(ICommunicationContext context) { - if (!_resolver.HasDependency(typeof(IAuthenticationProvider))) - { - return NotAuthorized(context); - } - - _authentication = _resolver.Resolve(); - - try + if (_resolver.HasDependency(typeof(IAuthenticationProvider))) { + _authentication = _resolver.Resolve(); var header = ReadBasicAuthHeader(context); - - if (header == null) - { - return NotAuthorized(context); - } - - var credentials = _authentication.GetByUsername(header.Username); - - if (credentials == null) + if (header != null) { - return NotAuthorized(context); - } + var credentials = _authentication.GetByUsername(header.Username); - if (!_authentication.ValidatePassword(credentials, header.Password)) - { - return NotAuthorized(context); + if (_authentication.ValidatePassword(credentials, header.Password)) + { + IIdentity id = new GenericIdentity(credentials.Username, "Basic"); + context.User = new GenericPrincipal(id, credentials.Roles); + } } - IIdentity id = new GenericIdentity(credentials.Username, "Basic"); - context.User = new GenericPrincipal(id, credentials.Roles); - return PipelineContinuation.Continue; - } - catch (ArgumentException ex) - { - return NotAuthorized(context); } - } - - private static BasicAuthorizationHeader ReadBasicAuthHeader(ICommunicationContext context) - { - var header = context.Request.Headers["Authorization"]; - return string.IsNullOrEmpty(header) ? null : BasicAuthorizationHeader.Parse(header); - } - private static PipelineContinuation NotAuthorized(ICommunicationContext context) - { - context.OperationResult = new OperationResult.Unauthorized(); - return PipelineContinuation.RenderNow; + return PipelineContinuation.Continue; } - private static PipelineContinuation WriteCredentialRequest(ICommunicationContext context) + private static BasicAuthorizationHeader ReadBasicAuthHeader(ICommunicationContext context) { - if (context.OperationResult is OperationResult.Unauthorized) + try + { + var header = context.Request.Headers["Authorization"]; + return string.IsNullOrEmpty(header) ? null : BasicAuthorizationHeader.Parse(header); + } catch (ArgumentException ex) { - var header = new BasicAuthenticationRequiredHeader().ServerResponseHeader; - context.Response.Headers["WWW-Authenticate"] = header; + return (null); } - return PipelineContinuation.Continue; } } } diff --git a/src/OpenRasta/Security/BasicAuthorizationHeader.cs b/src/OpenRasta/Security/BasicAuthorizationHeader.cs index f32050a0..de2a697d 100644 --- a/src/OpenRasta/Security/BasicAuthorizationHeader.cs +++ b/src/OpenRasta/Security/BasicAuthorizationHeader.cs @@ -30,13 +30,9 @@ private BasicAuthorizationHeader(string username, string password) public static BasicAuthorizationHeader Parse(string header) { var tokens = header.Split(' '); - if (tokens.Length != 2) + if (tokens.Length != 2 || tokens[0] != "Basic") { - throw (new ArgumentException("Supplied header is not in the format Basic {base64-encoded credential pair}", "header")); - } - if (tokens[0] != "Basic") - { - throw (new ArgumentException("Supplied header is not an HTTP Basic authorization header", header)); + return (null); } try { @@ -46,7 +42,7 @@ public static BasicAuthorizationHeader Parse(string header) return (new BasicAuthorizationHeader(credentials[0], credentials[1])); } catch (FormatException ex) { - throw (new ArgumentException("Supplied header doesn't contain valid base64 encoded credentials", ex)); + return (null); } } } diff --git a/src/OpenRasta/Security/RequiresAuthenticationInterceptor.cs b/src/OpenRasta/Security/RequiresAuthenticationInterceptor.cs index 5cac10c7..64ef4d85 100644 --- a/src/OpenRasta/Security/RequiresAuthenticationInterceptor.cs +++ b/src/OpenRasta/Security/RequiresAuthenticationInterceptor.cs @@ -17,11 +17,18 @@ public override bool BeforeExecute(IOperation operation) { if (_context.User == null || _context.User.Identity == null || !_context.User.Identity.IsAuthenticated) { - _context.OperationResult = new OperationResult.Unauthorized(); + DenyAuthorization(_context); return false; } return true; } + + protected virtual void DenyAuthorization(ICommunicationContext context) + { + _context.OperationResult = new OperationResult.Unauthorized(); + } } + + } \ No newline at end of file diff --git a/src/OpenRasta/Security/RequiresBasicAuthenticationAttribute.cs b/src/OpenRasta/Security/RequiresBasicAuthenticationAttribute.cs new file mode 100644 index 00000000..34fb642c --- /dev/null +++ b/src/OpenRasta/Security/RequiresBasicAuthenticationAttribute.cs @@ -0,0 +1,57 @@ +#region License +/* Authors: + * Sebastien Lambla (seb@serialseb.com) + * Copyright: + * (C) 2007-2009 Caffeine IT & naughtyProd Ltd (http://www.caffeine-it.com) + * License: + * This file is distributed under the terms of the MIT License found at the end of this file. + */ +#endregion + +using System; +using System.Collections.Generic; +using OpenRasta.DI; +using OpenRasta.OperationModel; +using OpenRasta.OperationModel.Interceptors; +using OpenRasta.Web; + +namespace OpenRasta.Security +{ + [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false, Inherited = true)] + public class RequiresBasicAuthenticationAttribute : InterceptorProviderAttribute + { + private readonly string realm; + + public RequiresBasicAuthenticationAttribute(string realm) + { + this.realm = realm; + } + + public override IEnumerable GetInterceptors(IOperation operation) + { + return new[] + { + new RequiresBasicAuthenticationInterceptor(DependencyManager.GetService(),realm) + }; + } + } +} + +#region Full license +// Permission is hereby granted, free of charge, to any person obtaining +// a copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to +// permit persons to whom the Software is furnished to do so, subject to +// the following conditions: +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE +// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION +// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION +// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +#endregion \ No newline at end of file diff --git a/src/OpenRasta/Security/RequiresBasicAuthenticationInterceptor.cs b/src/OpenRasta/Security/RequiresBasicAuthenticationInterceptor.cs new file mode 100644 index 00000000..aac3e68b --- /dev/null +++ b/src/OpenRasta/Security/RequiresBasicAuthenticationInterceptor.cs @@ -0,0 +1,25 @@ +using OpenRasta.OperationModel; +using OpenRasta.OperationModel.Interceptors; +using OpenRasta.Pipeline; +using OpenRasta.Web; + +namespace OpenRasta.Security +{ + public class RequiresBasicAuthenticationInterceptor : RequiresAuthenticationInterceptor + { + private readonly string realm; + + public RequiresBasicAuthenticationInterceptor(ICommunicationContext context, string realm) + : base(context) + { + this.realm = realm; + } + + protected override void DenyAuthorization(ICommunicationContext context) + { + base.DenyAuthorization(context); + var header = new BasicAuthenticationRequiredHeader(realm).ServerResponseHeader; + context.Response.Headers["WWW-Authenticate"] = header; + } + } +}