Added guard to FormsAuthentication as per issue #1755 #1778

Worthaboutapig · 2015-01-08T12:01:05Z

Created guards on the four login/out methods.

Created tests for the changes- unfortunately all the FormsAuthentication stuff is static, so it can't really be tested properly. I've added an internal method for the test assembly to allow disabling FormsAuthentication so it can be tested. Not ideal, but with a static FormsAuthentication it's not really unit-testable.
Can't see another simple way to provide unit-testing, however, it introduces code only for testing, which is not great...

Believed I followed the correct procedures- read how to contribute, so I hope it's correctly setup.

…Enable' has been called. Added tests- a bit hacky, as FormsAuthentication is static.

albertjan · 2015-01-08T14:50:33Z

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


please use 4 spaces instead of tabs.

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

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

Can we please get rid of the region? Thanks 😋

Worthaboutapig · 2015-01-16T17:16:47Z

Errr... so what happens now? Can a committer have a look at this and decide how to resolve? Leave it as is, or accept it's not testable?

albertjan · 2015-01-16T17:28:37Z

Leave it as it is for now. You can go to the chat and ask one of the commiters to have a look at it.

Sent from my iPhone

On 16 jan. 2015, at 18:18, "Worthaboutapig" notifications@github.com wrote:

Errr... so what happens now? Can a committer have a look at this and decide how to resolve? Leave it as is, or accept it's not testable?

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

…/Worthaboutapig/Nancy into NancyFx#1755-FormsAuthentication

thecodejunkie · 2015-08-26T10:59:26Z

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

+			var result = Record.Exception(() => FormsAuthentication.UserLoggedInResponse(userGuid));
+
+			// Then
+			result.ShouldBeOfType(typeof(InvalidOperationException));


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

thecodejunkie · 2015-08-26T11:02:12Z

Need to convert tabs to spaces in all affected files + verify that the indentation is correct (currently it's not)

thecodejunkie · 2015-08-26T11:03:57Z

src/Nancy.Testing/Nancy.Testing.csproj

@@ -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">


Please do not change the version of our dependencies 😄

thecodejunkie · 2015-08-26T11:11:37Z

This pull-request does seem to touch a lot of unrelated file for no good reason

Updating nuget versions
Changing reference paths
Razor View Engine related files
MaterialisingResponse
SharedAssemblyInfo

All of these things needs to be resolved if we are going to pull this in. @Worthaboutapig are you still interested in working with us to update this pull-request or would you rather we close it and apply a fix ourselves ? Looking at resolving #1755 very soon

Worthaboutapig · 2015-08-26T12:23:25Z

I'm happy to do the fix- tbh I thought it had been ignored. I've numerous other fixes/ suggestions.
I attempted to follow the rules, but clearly failed 😉. I'll fix it up as best I can and resubmit.

thecodejunkie · 2015-08-26T12:28:11Z

@Worthaboutapig I do sincerely apologies that this was left hanging for so long without any feedback from us. We're trying to improve on that now and we've closed nearly 100 issues in 2 weeks :)

thecodejunkie · 2015-08-26T12:28:51Z

The one thing that I do not like is the Disable method that's been added for testing purposes only. Let us give it some thought and see if we can come up with a better approach for that one

Worthaboutapig · 2015-08-31T09:03:58Z

Clearly I can fix up all the unnecessary changes, but this last point you raise was the main reason I hadn't. FormsAuthentication and other similar plug-ins are all designed and configured statically. Due to this, I couldn't see any other way of testing it without the Disable method. Clearly it's not ideal, but once any test is run, there's no way to change anything... I'm not personally convinced that these things should be static in the first place.

Presumably you don't want to change that, but as I said originally, adding a Disable method was the least invasive way of making things testable... so either this, redesign FormsAuthentication to allow more testing, or don't have testing for this. :)

If someone has a better approach, that would be good.

I'm happy to fix up my code, but then until you have a design you're happy with, you won't merge it, so I also don't want to waste my time. What do you suggest?

khellang · 2015-08-31T09:05:47Z

@Worthaboutapig All the config stuff is changing from static to instance based in 2.0. See #1999 and #2003.

thecodejunkie · 2015-09-02T08:46:36Z

@khellang can you think of a decent workaround for the Disable call, or should we move this to 2.x ?

Worthaboutapig · 2015-09-02T08:56:49Z

It would seem sensible to me to move it to 2.x, I can't think of a workaround and presumably the problem won't exist with an instance-based system and it's not a critical problem.

I'll take a look at implementing it into 2.x, if that's ok with you?

thecodejunkie · 2015-09-24T19:37:12Z

Yep, we'll put this in 2.x and migrate it over to the new configuration API instread 👍

Added guard to FormsAuthentication login/out methods to ensure that '…

394922c

…Enable' has been called. Added tests- a bit hacky, as FormsAuthentication is static.

albertjan reviewed Jan 8, 2015
View reviewed changes

Worthaboutapig added 4 commits January 30, 2015 12:56

Merge branch 'master' into NancyFx#1755-FormsAuthentication

a4b8796

Support for runtime view errors

e21534e

Merge branch 'master' into NancyFx#1755-FormsAuthentication

3a656fc

Merge branch 'NancyFx#1755-FormsAuthentication' of https://github.com…

08cfc39

…/Worthaboutapig/Nancy into NancyFx#1755-FormsAuthentication

thecodejunkie added this to the 1.3 milestone Aug 26, 2015

thecodejunkie reviewed Aug 26, 2015
View reviewed changes

khellang mentioned this pull request Aug 26, 2015

Added .editorconfig #2035

Merged

jchannon modified the milestones: 1.4, 1.3 Sep 14, 2015

thecodejunkie modified the milestones: 2.0, 1.4 Sep 24, 2015

thecodejunkie self-assigned this Sep 24, 2015

khellang modified the milestones: 2.0, 2.0-beta Mar 22, 2016

thecodejunkie modified the milestones: 2.0-barneyrubble, 2.1 Jul 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added guard to FormsAuthentication as per issue #1755 #1778

Added guard to FormsAuthentication as per issue #1755 #1778

Worthaboutapig commented Jan 8, 2015

albertjan Jan 8, 2015

Worthaboutapig Jan 9, 2015

06b Jan 9, 2015

Worthaboutapig Jan 9, 2015

thecodejunkie Aug 26, 2015

Worthaboutapig commented Jan 16, 2015

albertjan commented Jan 16, 2015

thecodejunkie Aug 26, 2015

thecodejunkie commented Aug 26, 2015

thecodejunkie Aug 26, 2015

thecodejunkie commented Aug 26, 2015

Worthaboutapig commented Aug 26, 2015

thecodejunkie commented Aug 26, 2015

thecodejunkie commented Aug 26, 2015

Worthaboutapig commented Aug 31, 2015

khellang commented Aug 31, 2015

thecodejunkie commented Sep 2, 2015

Worthaboutapig commented Sep 2, 2015

thecodejunkie commented Sep 24, 2015

Added guard to FormsAuthentication as per issue #1755 #1778

Are you sure you want to change the base?

Added guard to FormsAuthentication as per issue #1755 #1778

Conversation

Worthaboutapig commented Jan 8, 2015

albertjan Jan 8, 2015

Choose a reason for hiding this comment

Worthaboutapig Jan 9, 2015

Choose a reason for hiding this comment

06b Jan 9, 2015

Choose a reason for hiding this comment

Worthaboutapig Jan 9, 2015

Choose a reason for hiding this comment

thecodejunkie Aug 26, 2015

Choose a reason for hiding this comment

Worthaboutapig commented Jan 16, 2015

albertjan commented Jan 16, 2015

thecodejunkie Aug 26, 2015

Choose a reason for hiding this comment

thecodejunkie commented Aug 26, 2015

thecodejunkie Aug 26, 2015

Choose a reason for hiding this comment

thecodejunkie commented Aug 26, 2015

Worthaboutapig commented Aug 26, 2015

thecodejunkie commented Aug 26, 2015

thecodejunkie commented Aug 26, 2015

Worthaboutapig commented Aug 31, 2015

khellang commented Aug 31, 2015

thecodejunkie commented Sep 2, 2015

Worthaboutapig commented Sep 2, 2015

thecodejunkie commented Sep 24, 2015