From 396632dbcec6c7a0dbfb875520deaeccbd9a7acc Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Mon, 7 Feb 2011 17:26:00 -0800 Subject: [PATCH] #17276: Fixing email validation --HG-- branch : dev --- src/Orchard.Specs/Users.feature | 61 +++++++++---- src/Orchard.Specs/Users.feature.cs | 88 ++++++++++++++++++- .../Controllers/AccountControllerTests.cs | 37 +++++++- .../Controllers/AccountController.cs | 8 +- .../Controllers/AdminController.cs | 12 ++- .../Modules/Orchard.Users/Models/UserPart.cs | 2 + .../ViewModels/UserCreateViewModel.cs | 2 +- .../ViewModels/UserEditViewModel.cs | 1 - 8 files changed, 181 insertions(+), 30 deletions(-) diff --git a/src/Orchard.Specs/Users.feature b/src/Orchard.Specs/Users.feature index 8128b632d..1e3d598b6 100644 --- a/src/Orchard.Specs/Users.feature +++ b/src/Orchard.Specs/Users.feature @@ -1,17 +1,17 @@ -Feature: Users - In order to prevent users module regressions - As a site owner - I want to create, search and modify user accounts - -@management -Scenario: There is only one user by default +Feature: Users + In order to prevent users module regressions + As a site owner + I want to create, search and modify user accounts + +@management +Scenario: There is only one user by default Given I have installed Orchard When I go to "admin/users" Then I should see "Manage Users" And I should see "]*>admin" -@management -Scenario: I can create a new user +@management +Scenario: I can create a new user Given I have installed Orchard When I go to "admin/users" Then I should see "Manage Users" @@ -74,8 +74,8 @@ Scenario: I can create a new user And I hit "Save" Then I should see "Password confirmation must match" -@management -Scenario: I can edit an existing user +@management +Scenario: I can edit an existing user Given I have installed Orchard When I go to "admin/users" And I follow "Add a new user" @@ -104,8 +104,8 @@ Scenario: I can edit an existing user And I should see "]*>user2" And I should see "user2@domain.com" -@management -Scenario: I should not be able to reuse an existing username or email +@management +Scenario: I should not be able to reuse an existing username or email Given I have installed Orchard When I go to "admin/users" # create user1 @@ -150,8 +150,8 @@ Scenario: I should not be able to reuse an existing username or email And I hit "Save" Then I should see "User with that username and/or email already exists." -@filtering -Scenario: I should not be able to filter users by name +@filtering +Scenario: I should not be able to filter users by name Given I have installed Orchard When I go to "admin/users" # create user1 @@ -198,8 +198,8 @@ Scenario: I should not be able to filter users by name And I should see "]*>user2" And I should not see "]*>admin" -@filtering -Scenario: I should be able to filter users by status +@filtering +Scenario: I should be able to filter users by status Given I have installed Orchard When I go to "admin/users" # create user1 @@ -260,3 +260,30 @@ Scenario: I should be able to filter users by status Then I should see "]*>user1" And I should see "]*>user2" And I should see "]*>admin" +@email +Scenario: I should not be able to add users with invalid email addresses + Given I have installed Orchard + When I go to "admin/users" + And I follow "Add a new user" + And I fill in + | name | value | + | UserName | user1 | + | Email | NotAnEmail | + | Password | a12345! | + | ConfirmPassword | a12345! | + And I hit "Save" + Then I should see "You must specify a valid email address." +@email +Scenario: I should be able to add users with valid email addresses + Given I have installed Orchard + When I go to "admin/users" + And I follow "Add a new user" + And I fill in + | name | value | + | UserName | user1 | + | Email | user1@domain.com | + | Password | a12345! | + | ConfirmPassword | a12345! | + And I hit "Save" + And I am redirected + Then I should see "User created" diff --git a/src/Orchard.Specs/Users.feature.cs b/src/Orchard.Specs/Users.feature.cs index 8555b6616..54579ba16 100644 --- a/src/Orchard.Specs/Users.feature.cs +++ b/src/Orchard.Specs/Users.feature.cs @@ -30,8 +30,8 @@ namespace Orchard.Specs public virtual void FeatureSetup() { testRunner = TechTalk.SpecFlow.TestRunnerManager.GetTestRunner(); - TechTalk.SpecFlow.FeatureInfo featureInfo = new TechTalk.SpecFlow.FeatureInfo(new System.Globalization.CultureInfo("en-US"), "Users", "In order to prevent users module regressions\nAs a site owner\nI want to create, se" + - "arch and modify user accounts", GenerationTargetLanguage.CSharp, ((string[])(null))); + TechTalk.SpecFlow.FeatureInfo featureInfo = new TechTalk.SpecFlow.FeatureInfo(new System.Globalization.CultureInfo("en-US"), "Users", "In order to prevent users module regressions\r\nAs a site owner\r\nI want to create, " + + "search and modify user accounts", GenerationTargetLanguage.CSharp, ((string[])(null))); testRunner.OnFeatureStart(featureInfo); } @@ -707,6 +707,90 @@ this.ScenarioSetup(scenarioInfo); testRunner.And("I should see \"]*>user2\""); #line 262 testRunner.And("I should see \"]*>admin\""); +#line hidden + testRunner.CollectScenarioErrors(); + } + + [NUnit.Framework.TestAttribute()] + [NUnit.Framework.DescriptionAttribute("I should not be able to add users with invalid email addresses")] + [NUnit.Framework.CategoryAttribute("email")] + public virtual void IShouldNotBeAbleToAddUsersWithInvalidEmailAddresses() + { + TechTalk.SpecFlow.ScenarioInfo scenarioInfo = new TechTalk.SpecFlow.ScenarioInfo("I should not be able to add users with invalid email addresses", new string[] { + "email"}); +#line 264 +this.ScenarioSetup(scenarioInfo); +#line 265 + testRunner.Given("I have installed Orchard"); +#line 266 + testRunner.When("I go to \"admin/users\""); +#line 267 + testRunner.And("I follow \"Add a new user\""); +#line hidden + TechTalk.SpecFlow.Table table25 = new TechTalk.SpecFlow.Table(new string[] { + "name", + "value"}); + table25.AddRow(new string[] { + "UserName", + "user1"}); + table25.AddRow(new string[] { + "Email", + "NotAnEmail"}); + table25.AddRow(new string[] { + "Password", + "a12345!"}); + table25.AddRow(new string[] { + "ConfirmPassword", + "a12345!"}); +#line 268 + testRunner.And("I fill in", ((string)(null)), table25); +#line 274 + testRunner.And("I hit \"Save\""); +#line 275 + testRunner.Then("I should see \"You must specify a valid email address.\""); +#line hidden + testRunner.CollectScenarioErrors(); + } + + [NUnit.Framework.TestAttribute()] + [NUnit.Framework.DescriptionAttribute("I should be able to add users with valid email addresses")] + [NUnit.Framework.CategoryAttribute("email")] + public virtual void IShouldBeAbleToAddUsersWithValidEmailAddresses() + { + TechTalk.SpecFlow.ScenarioInfo scenarioInfo = new TechTalk.SpecFlow.ScenarioInfo("I should be able to add users with valid email addresses", new string[] { + "email"}); +#line 277 +this.ScenarioSetup(scenarioInfo); +#line 278 + testRunner.Given("I have installed Orchard"); +#line 279 + testRunner.When("I go to \"admin/users\""); +#line 280 + testRunner.And("I follow \"Add a new user\""); +#line hidden + TechTalk.SpecFlow.Table table26 = new TechTalk.SpecFlow.Table(new string[] { + "name", + "value"}); + table26.AddRow(new string[] { + "UserName", + "user1"}); + table26.AddRow(new string[] { + "Email", + "user1@domain.com"}); + table26.AddRow(new string[] { + "Password", + "a12345!"}); + table26.AddRow(new string[] { + "ConfirmPassword", + "a12345!"}); +#line 281 + testRunner.And("I fill in", ((string)(null)), table26); +#line 287 + testRunner.And("I hit \"Save\""); +#line 288 + testRunner.And("I am redirected"); +#line 289 + testRunner.Then("I should see \"User created\""); #line hidden testRunner.CollectScenarioErrors(); } diff --git a/src/Orchard.Tests.Modules/Users/Controllers/AccountControllerTests.cs b/src/Orchard.Tests.Modules/Users/Controllers/AccountControllerTests.cs index bf360411c..b5d4cff1a 100644 --- a/src/Orchard.Tests.Modules/Users/Controllers/AccountControllerTests.cs +++ b/src/Orchard.Tests.Modules/Users/Controllers/AccountControllerTests.cs @@ -154,7 +154,20 @@ namespace Orchard.Tests.Modules.Users.Controllers { } [Test] - public void UsersShouldNotBeAbleToRegisterIfInvalidEmail() { + + public void UsersShouldNotBeAbleToRegisterIfInvalidEmail( + [Values( + @"NotAnEmail", + @"@NotAnEmail", + @"""test\blah""@example.com", + "\"test\rblah\"@example.com", + @"""test""blah""@example.com", + @".wooly@example.com", + @"wo..oly@example.com", + @"pootietang.@example.com", + @".@example.com", + @"Ima Fool@example.com")] + string email) { var registrationSettings = _container.Resolve().GetContext().CurrentSite.As(); registrationSettings.UsersCanRegister = true; @@ -164,13 +177,29 @@ namespace Orchard.Tests.Modules.Users.Controllers { _session.Flush(); _controller.ModelState.Clear(); - var result = _controller.Register("bar", "notanemailaddress", "66554321", "66554321"); + var result = _controller.Register("bar", email, "66554321", "66554321"); Assert.That(((ViewResult)result).ViewData.ModelState.Count == 1,"Invalid email address."); } [Test] - public void UsersShouldBeAbleToRegisterIfValidEmail() { + public void UsersShouldBeAbleToRegisterIfValidEmail( + [Values( + @"""test\\blah""@example.com", + "\"test\\\rblah\"@example.com", + @"""test\""blah""@example.com", + @"customer/department@example.com", + @"$A12345@example.com", + @"!def!xyz%abc@example.com", + @"_Yosemite.Sam@example.com", + @"~@example.com", + @"""Austin@Powers""@example.com", + @"Ima.Fool@example.com", + @"""Ima.Fool""@example.com", + @"""Ima Fool""@example.com" + )] + string email) + { var registrationSettings = _container.Resolve().GetContext().CurrentSite.As(); registrationSettings.UsersCanRegister = true; @@ -180,7 +209,7 @@ namespace Orchard.Tests.Modules.Users.Controllers { _session.Flush(); _controller.ModelState.Clear(); - var result = _controller.Register("bar", "t@t.com", "password", "password"); + var result = _controller.Register("bar", email, "password", "password"); Assert.That(result, Is.TypeOf()); Assert.That(((RedirectResult)result).Url, Is.EqualTo("~/")); diff --git a/src/Orchard.Web/Modules/Orchard.Users/Controllers/AccountController.cs b/src/Orchard.Web/Modules/Orchard.Users/Controllers/AccountController.cs index c24a6046d..ac3868217 100644 --- a/src/Orchard.Web/Modules/Orchard.Users/Controllers/AccountController.cs +++ b/src/Orchard.Web/Modules/Orchard.Users/Controllers/AccountController.cs @@ -22,6 +22,7 @@ namespace Orchard.Users.Controllers { private readonly IMembershipService _membershipService; private readonly IUserService _userService; private readonly IOrchardServices _orchardServices; + public AccountController( IAuthenticationService authenticationService, @@ -320,18 +321,17 @@ namespace Orchard.Users.Controllers { private bool ValidateRegistration(string userName, string email, string password, string confirmPassword) { bool validate = true; - Regex isValidEmail = new Regex("^[a-z0-9_\\+-]+(\\.[a-z0-9_\\+-]+)*@[a-z0-9-]+(\\.[a-z0-9-]+)*\\.([a-z]{2,4})$"); - if (String.IsNullOrEmpty(userName)) { ModelState.AddModelError("username", T("You must specify a username.")); validate = false; } + if (String.IsNullOrEmpty(email)) { ModelState.AddModelError("email", T("You must specify an email address.")); validate = false; } - - if (!isValidEmail.IsMatch(email)) { + else if (!Regex.IsMatch(email, UserPart.EmailPattern, RegexOptions.IgnoreCase)) { + // http://haacked.com/archive/2007/08/21/i-knew-how-to-validate-an-email-address-until-i.aspx ModelState.AddModelError("email", T("You must specify a valid email address.")); validate = false; } diff --git a/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs b/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs index 9cff17ea0..f54c58d89 100644 --- a/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs +++ b/src/Orchard.Web/Modules/Orchard.Users/Controllers/AdminController.cs @@ -1,5 +1,6 @@ using System.Collections.Generic; using System.Linq; +using System.Text.RegularExpressions; using System.Web.Mvc; using System.Web.Routing; using Orchard.ContentManagement; @@ -169,7 +170,12 @@ namespace Orchard.Users.Controllers { AddModelError("NotUniqueUserName", T("User with that username and/or email already exists.")); } } - + + if (!Regex.IsMatch(createModel.Email ?? "", UserPart.EmailPattern, RegexOptions.IgnoreCase)) { + // http://haacked.com/archive/2007/08/21/i-knew-how-to-validate-an-email-address-until-i.aspx + ModelState.AddModelError("Email", T("You must specify a valid email address.")); + } + if (createModel.Password != createModel.ConfirmPassword) { AddModelError("ConfirmPassword", T("Password confirmation must match")); } @@ -229,6 +235,10 @@ namespace Orchard.Users.Controllers { if (!_userService.VerifyUserUnicity(id, editModel.UserName, editModel.Email)) { AddModelError("NotUniqueUserName", T("User with that username and/or email already exists.")); } + else if (!Regex.IsMatch(editModel.Email ?? "", UserPart.EmailPattern, RegexOptions.IgnoreCase)) { + // http://haacked.com/archive/2007/08/21/i-knew-how-to-validate-an-email-address-until-i.aspx + ModelState.AddModelError("Email", T("You must specify a valid email address.")); + } else { // also update the Super user if this is the renamed account if (String.Equals(Services.WorkContext.CurrentSite.SuperUser, previousName, StringComparison.OrdinalIgnoreCase)) { diff --git a/src/Orchard.Web/Modules/Orchard.Users/Models/UserPart.cs b/src/Orchard.Web/Modules/Orchard.Users/Models/UserPart.cs index 2f969cd9f..4825dd2f9 100644 --- a/src/Orchard.Web/Modules/Orchard.Users/Models/UserPart.cs +++ b/src/Orchard.Web/Modules/Orchard.Users/Models/UserPart.cs @@ -3,6 +3,8 @@ using Orchard.Security; namespace Orchard.Users.Models { public sealed class UserPart : ContentPart, IUser { + public const string EmailPattern = @"^(?!\.)(""([^""\r\\]|\\[""\r\\])*""|([-a-z0-9!#$%&'*+/=?^_`{|}~]|(?().Record.Email; } set { User.As().Record.Email = value; }