Added RequireSSL property to FormsAuthentication. Issue #425 #760

Merged
merged 5 commits into from Oct 4, 2012

Conversation

Projects
None yet
3 participants
Contributor

andreichuk commented Sep 24, 2012

No description provided.

@thecodejunkie thecodejunkie commented on an outdated diff Sep 24, 2012

...hentication.Forms/FormsAuthenticationConfiguration.cs
/// Gets or sets the cryptography configuration
/// </summary>
- public CryptographyConfiguration CryptographyConfiguration { get; set; }
+ public CryptographyConfiguration CryptographyConfiguration { get; set; }
@thecodejunkie

thecodejunkie Sep 24, 2012

Owner

Looks like you've added a tab at the end here

Owner

thecodejunkie commented Sep 24, 2012

Could you please add tests for this? There is a FormsAuthenticationFixture and FormsAuthenticationConfigurationFixture where the rest of the tests are.

@thecodejunkie thecodejunkie and 1 other commented on an outdated diff Sep 24, 2012

...hentication.Forms/FormsAuthenticationConfiguration.cs
@@ -42,9 +42,14 @@ public FormsAuthenticationConfiguration(CryptographyConfiguration cryptographyCo
public IUserMapper UserMapper { get; set; }
/// <summary>
+ /// Gets or sets the flag that indicates whether SSL is required
+ /// </summary>
+ public bool RequireSSL { get; set; }
@thecodejunkie

thecodejunkie Sep 24, 2012

Owner

MIssing <value> element

@prabirshrestha

prabirshrestha Sep 26, 2012

Contributor

should it be RequiresSSL instead? this would match with #702 for RequiresHttps.

@thecodejunkie

thecodejunkie Sep 27, 2012

Owner

Good call. Actually RequiresSSL and RequiresHttps are two ways of saying the same thing. So it should probably be RequiresHttps, for symmetry, or something like Secure

Owner

thecodejunkie commented Sep 24, 2012

Could you please add tests for this? The other tests for forms auth are in FormsAuthenticationFixture and FormsAuthenticationConfigurationFixture

Owner

thecodejunkie commented Sep 24, 2012

Am I right in assuming that it's not overlapping #691 ? I believe they're complementing eachother

Contributor

andreichuk commented Sep 29, 2012

done

@thecodejunkie thecodejunkie added a commit that referenced this pull request Oct 4, 2012

@thecodejunkie thecodejunkie Merge pull request #760 from andreichuk/FormsAuthentication
Added RequireSSL property to FormsAuthentication. Issue #425
c8be5d4

@thecodejunkie thecodejunkie merged commit c8be5d4 into NancyFx:master Oct 4, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment