Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Added guard to FormsAuthentication as per issue #1755 #1778

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Added guard to FormsAuthentication as per issue #1755 #1778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
394922c
Added guard to FormsAuthentication login/out methods to ensure that '…
Worthaboutapig Jan 8, 2015
a4b8796
Merge branch 'master' into #1755-FormsAuthentication
Worthaboutapig Jan 30, 2015
e21534e
Support for runtime view errors
Worthaboutapig Feb 6, 2015
3a656fc
Merge branch 'master' into #1755-FormsAuthentication
Worthaboutapig Apr 30, 2015
08cfc39
Merge branch '#1755-FormsAuthentication' of https://github.com/Wortha…
Worthaboutapig Apr 30, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 65 additions & 2 deletions src/Nancy.Authentication.Forms.Tests/FormsAuthenticationFixture.cs
View file
Open in desktop
Expand Up @@ -8,7 +8,6 @@ namespace Nancy.Authentication.Forms.Tests
using FakeItEasy;
using Fakes;
using Helpers;
using Nancy.Security;
using Nancy.Tests;
using Nancy.Tests.Fakes;
using Xunit;
Expand Down Expand Up @@ -162,7 +161,71 @@ public void Should_return_ok_response_when_user_logs_in_without_redirect()
result.StatusCode.ShouldEqual(HttpStatusCode.OK);
}

[Fact]
#region Throw helpful exception when the configuration is not enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

please use 4 spaces instead of tabs.

Copy link
Author

Choose a reason for hiding this comment

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

Ug. Yuck. Spaces. Ok. There isn't a style-guide, is there? There seems to be a space for one, but no link.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/NancyFx/Nancy/blob/master/CONTRIBUTING.md#style-guidelines

Copy link
Author

Choose a reason for hiding this comment

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

thx

Copy link
Member

Choose a reason for hiding this comment

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

Can we please get rid of the region? Thanks 😋


[Fact]
public void Should_throw_helpful_exception_message_when_user_logs_in_without_redirect_and_forms_authentication_not_enabled()
{
// Given
const string expectedMessage = "The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper";
FormsAuthentication.Disable();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's neccesairy to disable the FormsAuthentication if you don't enable it it's disabled.

Copy link
Author

Choose a reason for hiding this comment

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

But as I explained in the comment, I can't see how else to have unit tests for the feature(possibly creating a new AppDomain, but that's no fun). Everything for FormsAuthentication is static and the unit tests don't/shouldn't run in a any particular order, so once a test has enabled FormsAuthentication, it's enabled for the whole test-run. Hence the whether the test will pass depends on state created by other tests. It will fail nearly all of the time and it's not testable without some way of disabling FormsAuthentication. It still would fail if the tests can run in parallel, but the existing tests would as well.

FormsAuthentication could be redesigned not to be static- I don't think it should be static, it maintains a global state- however, that's a non-trivial breaking-change, so I suppose it wouldn't be approved.

TL;DR- Accept that it's not testable, or leave the hack in and sweep the design under the carpet.

Let me know what you'd like to do and I'll fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand but when FormsAuthentication.Enable isn't called the configuration is null. note: xUnit creates a new instance of this class for every test.

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps I'm misunderstanding. xUnit can't create a new instance of FormsAuthentication, it's a static class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes sorry my bad :)

Sent from my iPhone

On 9 jan. 2015, at 12:06, "Worthaboutapig" notifications@github.com wrote:

In src/Nancy.Authentication.Forms.Tests/FormsAuthenticationFixture.cs:

@@ -162,7 +161,71 @@ public void Should_return_ok_response_when_user_logs_in_without_redirect()
result.StatusCode.ShouldEqual(HttpStatusCode.OK);
}

  •    [Fact]

  •   #region Throw helpful exception when the configuration is not enabled

  •   [Fact]

  •   public void Should_throw_helpful_exception_message_when_user_logs_in_without_redirect_and_forms_authentication_not_enabled()

  •   {

  •       // Given

  •       const string expectedMessage = "The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper";

  •       FormsAuthentication.Disable();
    Perhaps I'm misunderstanding. xUnit can't create a new instance of FormsAuthentication, it's a static class.

—
Reply to this email directly or view it on GitHub.


// When
var result = Record.Exception(() => FormsAuthentication.UserLoggedInResponse(userGuid));

// Then
result.ShouldBeOfType(typeof(InvalidOperationException));
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to use the generic overload result.ShouldBeOfType<InvalidOperationException>();

result.Message.ShouldBeSameAs(expectedMessage);
}

[Fact]
public void Should_throw_helpful_exception_message_when_user_logs_in_with_redirect_and_forms_authentication_not_enabled()
{
// Given
const string expectedMessage = "The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper";
FormsAuthentication.Disable();

// When
var result = Record.Exception(() => FormsAuthentication.UserLoggedInRedirectResponse(context, userGuid));

// Then
result.ShouldBeOfType(typeof(InvalidOperationException));
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to use the generic overload result.ShouldBeOfType<InvalidOperationException>();

result.Message.ShouldBeSameAs(expectedMessage);
}

[Fact]
public void Should_throw_helpful_exception_message_when_user_logs_out_with_redirect_and_forms_authentication_not_enabled()
{
// Given
const string expectedMessage = "The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper";
FormsAuthentication.Disable();

// When
var result = Record.Exception(() => FormsAuthentication.LogOutAndRedirectResponse(context, "/"));

// Then
result.ShouldBeOfType(typeof(InvalidOperationException));
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to use the generic overload result.ShouldBeOfType<InvalidOperationException>();

result.Message.ShouldBeSameAs(expectedMessage);
}

[Fact]
public void Should_throw_helpful_exception_message_when_user_logs_out_without_redirect_and_forms_authentication_not_enabled()
{
// Given
const string expectedMessage = "The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper";
FormsAuthentication.Disable();

// When
var result = Record.Exception(() => FormsAuthentication.LogOutResponse());

// Then
result.ShouldBeOfType(typeof(InvalidOperationException));
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to use the generic overload result.ShouldBeOfType<InvalidOperationException>();

result.Message.ShouldBeSameAs(expectedMessage);
}

#endregion

[Fact]
public void Should_have_authentication_cookie_in_login_response_when_logging_in_with_redirect()
{
FormsAuthentication.Enable(A.Fake<IPipelines>(), this.config);
Expand Down
46 changes: 35 additions & 11 deletions src/Nancy.Authentication.Forms/FormsAuthentication.cs
View file
Open in desktop
Expand Up @@ -34,7 +34,15 @@ public static string FormsAuthenticationCookieName
}
}

/// <summary>
/// <summary>
/// To support testing, necessary as everying is static, but not ideal
/// </summary>
internal static void Disable()
{
currentConfiguration = null;
}

/// <summary>
/// Enables forms authentication for the application
/// </summary>
/// <param name="pipelines">Pipelines to add handlers to (usually "this")</param>
Expand Down Expand Up @@ -110,7 +118,12 @@ public static void Enable(INancyModule module, FormsAuthenticationConfiguration
/// <returns>Nancy response with redirect.</returns>
public static Response UserLoggedInRedirectResponse(NancyContext context, Guid userIdentifier, DateTime? cookieExpiry = null, string fallbackRedirectUrl = null)
{
var redirectUrl = fallbackRedirectUrl;
if (currentConfiguration == null)
{
throw new InvalidOperationException("The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper");
}

var redirectUrl = fallbackRedirectUrl;

if (string.IsNullOrEmpty(redirectUrl))
{
Expand Down Expand Up @@ -149,11 +162,14 @@ public static Response UserLoggedInRedirectResponse(NancyContext context, Guid u
/// <returns>Nancy response with status <see cref="HttpStatusCode.OK"/></returns>
public static Response UserLoggedInResponse(Guid userIdentifier, DateTime? cookieExpiry = null)
{
var response =
(Response)HttpStatusCode.OK;
if (currentConfiguration == null)
{
throw new InvalidOperationException("The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper");
}

var response = (Response)HttpStatusCode.OK;

var authenticationCookie =
BuildCookie(userIdentifier, cookieExpiry, currentConfiguration);
var authenticationCookie = BuildCookie(userIdentifier, cookieExpiry, currentConfiguration);

response.AddCookie(authenticationCookie);

Expand All @@ -168,7 +184,12 @@ public static Response UserLoggedInResponse(Guid userIdentifier, DateTime? cooki
/// <returns>Nancy response</returns>
public static Response LogOutAndRedirectResponse(NancyContext context, string redirectUrl)
{
var response = context.GetRedirect(redirectUrl);
if (currentConfiguration == null)
{
throw new InvalidOperationException("The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper");
}

var response = context.GetRedirect(redirectUrl);
var authenticationCookie = BuildLogoutCookie(currentConfiguration);
response.AddCookie(authenticationCookie);

Expand All @@ -181,11 +202,14 @@ public static Response LogOutAndRedirectResponse(NancyContext context, string re
/// <returns>Nancy response</returns>
public static Response LogOutResponse()
{
var response =
(Response)HttpStatusCode.OK;
if (currentConfiguration == null)
{
throw new InvalidOperationException("The internal FormsAuthenticationConfiguration has not been set. Ensure that FormsAuthentication has been enabled in the bootstrapper");
}

var response = (Response)HttpStatusCode.OK;

var authenticationCookie =
BuildLogoutCookie(currentConfiguration);
var authenticationCookie = BuildLogoutCookie(currentConfiguration);

response.AddCookie(authenticationCookie);

Expand Down
8 changes: 5 additions & 3 deletions src/Nancy.Testing/Nancy.Testing.csproj
View file
Open in desktop
Expand Up @@ -85,9 +85,9 @@
<DocumentationFile>bin\MonoRelease\Nancy.Testing.XML</DocumentationFile>
</PropertyGroup>
<ItemGroup>
<Reference Include="CsQuery, Version=1.3.3.5, Culture=neutral, processorArchitecture=MSIL">
<Reference Include="CsQuery, Version=1.3.3.249, Culture=neutral, processorArchitecture=MSIL">
Copy link
Member

Choose a reason for hiding this comment

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

Please do not change the version of our dependencies 😄

<SpecificVersion>False</SpecificVersion>
<HintPath>..\packages\CsQuery.1.3.3\lib\net40\CsQuery.dll</HintPath>
<HintPath>..\..\..\..\savetrees\code\references\packages\CsQuery.1.3.4\lib\net40\CsQuery.dll</HintPath>
Copy link
Member

Choose a reason for hiding this comment

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

Nope.. this can't happen

</Reference>
<Reference Include="System">
</Reference>
Expand Down Expand Up @@ -171,7 +171,6 @@
</BootstrapperPackage>
</ItemGroup>
<ItemGroup>
<None Include="packages.config" />
<EmbeddedResource Include="Resources\Nancy Testing Cert.pfx" />
</ItemGroup>
<ItemGroup>
Expand All @@ -180,6 +179,9 @@
<LastGenOutput>Resources.Designer.cs</LastGenOutput>
</EmbeddedResource>
</ItemGroup>
<ItemGroup>
<None Include="packages.config" />
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<!-- To modify your build process, add your task inside one of the targets below and uncomment it.
Other similar extension points exist, see Microsoft.Common.targets.
Expand Down
2 changes: 1 addition & 1 deletion src/Nancy.Testing/packages.config
View file
Open in desktop
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="CsQuery" version="1.3.3" targetFramework="net40" />
<package id="CsQuery" version="1.3.4" targetFramework="net40" />
Copy link
Member

Choose a reason for hiding this comment

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

Please do not change the version of our dependencies 😄

</packages>
4 changes: 2 additions & 2 deletions src/Nancy.ViewEngines.Razor.BuildProviders/Nancy.ViewEngines.Razor.BuildProviders.csproj
View file
Open in desktop
Expand Up @@ -97,8 +97,8 @@
<Reference Include="System.Web" />
<Reference Include="Microsoft.CSharp" />
<Reference Include="System.Web.Razor, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<SpecificVersion>False</SpecificVersion>
<HintPath>..\packages\Microsoft.AspNet.Razor.2.0.30506.0\lib\net40\System.Web.Razor.dll</HintPath>
<Private>True</Private>
Copy link
Member

Choose a reason for hiding this comment

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

Nope :)

<HintPath>..\..\..\..\savetrees\code\references\packages\Microsoft.AspNet.Razor.2.0.30506.0\lib\net40\System.Web.Razor.dll</HintPath>
Copy link
Member

Choose a reason for hiding this comment

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

Nope :)

</Reference>
</ItemGroup>
<ItemGroup>
Expand Down
30 changes: 22 additions & 8 deletions src/Nancy.ViewEngines.Razor/HtmlHelpers.cs
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@
{
using System;
using System.IO;
using Nancy.Extensions;
using Nancy.Security;

/// <summary>
Expand Down Expand Up @@ -48,6 +49,8 @@ public HtmlHelpers(RazorViewEngine engine, IRenderContext renderContext, TModel
/// <returns>An <see cref="IHtmlString"/> representation of the partial.</returns>
public IHtmlString Partial(string viewName)
{
if (string.IsNullOrWhiteSpace(viewName)) throw new ArgumentNullException("viewName", "Attempted to render a non-existent view");
Copy link
Member

Choose a reason for hiding this comment

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

No single-line if-statements. Wrap in curly-braces


return this.Partial(viewName, null);
}

Expand All @@ -59,18 +62,29 @@ public IHtmlString Partial(string viewName)
/// <returns>An <see cref="IHtmlString"/> representation of the partial.</returns>
public IHtmlString Partial(string viewName, dynamic modelForPartial)
{
var view = this.RenderContext.LocateView(viewName, modelForPartial);
if (string.IsNullOrWhiteSpace(viewName)) throw new ArgumentNullException("viewName", "Attempted to render a non-existent view");

var response = this.Engine.RenderView(view, modelForPartial, this.RenderContext, true);
Action<Stream> action = response.Contents;
var mem = new MemoryStream();
//this.RenderContext.Context.WriteTraceLog(sb => sb.AppendFormat("Rendered partial view {0} with type {1}", viewName, response));
try
{
var view = this.RenderContext.LocateView(viewName, modelForPartial);

action.Invoke(mem);
mem.Position = 0;
var response = this.Engine.RenderView(view, modelForPartial, this.RenderContext, true);
Action<Stream> action = response.Contents;
var mem = new MemoryStream();

var reader = new StreamReader(mem);
action.Invoke(mem);
mem.Position = 0;

return new NonEncodedHtmlString(reader.ReadToEnd());
var reader = new StreamReader(mem);

return new NonEncodedHtmlString(reader.ReadToEnd());
}
catch (Exception exception)
{
this.RenderContext.Context.WriteTraceLog(sb => sb.AppendFormat("Rendering partial view {0} threw exception {1}", viewName, exception.Message));
throw;
}
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Nancy.ViewEngines.Razor/Nancy.ViewEngines.Razor.csproj
View file
Open in desktop
Expand Up @@ -94,7 +94,7 @@
</Reference>
<Reference Include="System.Web.Razor, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<Private>True</Private>
<HintPath>..\packages\Microsoft.AspNet.Razor.2.0.30506.0\lib\net40\System.Web.Razor.dll</HintPath>
<HintPath>..\..\..\..\savetrees\code\references\packages\Microsoft.AspNet.Razor.2.0.30506.0\lib\net40\System.Web.Razor.dll</HintPath>
</Reference>
<Reference Include="System.Xml.Linq">
</Reference>
Expand Down
15 changes: 8 additions & 7 deletions src/Nancy.ViewEngines.Razor/NancyRazorViewBase.cs
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text;
using Nancy.Helpers;

Expand Down Expand Up @@ -395,25 +396,25 @@ public void ExecuteView(string body, IDictionary<string, string> sectionContents
}
catch (NullReferenceException e)
{
throw new ViewRenderException("Unable to render the view. Most likely the Model, or a property on the Model, is null", e);
throw new ViewRenderException("Unable to render the view. Most likely the Model, or a property on the Model, is null", e);
}

this.Body = this.contents.ToString();

this.SectionContents = new Dictionary<string, string>(this.Sections.Count);
this.SectionContents = new Dictionary<string, string>(this.Sections.Count);
foreach (var section in this.Sections)
{
this.contents.Clear();
this.contents.Clear();
try
{
section.Value.Invoke();
section.Value.Invoke();
}
catch (NullReferenceException e)
{
throw new ViewRenderException(string.Format("A null reference was encountered while rendering the section {0}. Does the section require a model? (maybe it wasn't passed in)", section.Key), e);
throw new ViewRenderException(string.Format("A null reference was encountered while rendering the section {0}. Does the section require a model? (maybe it wasn't passed in)", section.Key), e);
}
this.SectionContents.Add(section.Key, this.contents.ToString());
}
this.SectionContents.Add(section.Key, this.contents.ToString());
}
}

/// <summary>
Expand Down