Updated: added (working) unit tests and an explanation asto how I'd speed up the password check
This commit is contained in:
parent
3cd3d97139
commit
b8afed8ac1
11 changed files with 10256 additions and 8 deletions
6
Server/.idea/.idea.BackEnd/.idea/vcs.xml
Normal file
6
Server/.idea/.idea.BackEnd/.idea/vcs.xml
Normal file
|
@ -0,0 +1,6 @@
|
||||||
|
<?xml version="1.0" encoding="UTF-8"?>
|
||||||
|
<project version="4">
|
||||||
|
<component name="VcsDirectoryMappings">
|
||||||
|
<mapping directory="$PROJECT_DIR$/.." vcs="Git" />
|
||||||
|
</component>
|
||||||
|
</project>
|
|
@ -1,8 +1,9 @@
|
||||||
|
|
||||||
Microsoft Visual Studio Solution File, Format Version 12.00
|
Microsoft Visual Studio Solution File, Format Version 12.00
|
||||||
|
#
|
||||||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BackEnd", "BackEnd\BackEnd.csproj", "{936595E0-B2A6-42B9-84F8-AA8D52466E5E}"
|
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BackEnd", "BackEnd\BackEnd.csproj", "{936595E0-B2A6-42B9-84F8-AA8D52466E5E}"
|
||||||
EndProject
|
EndProject
|
||||||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Test.BackEnd", "Test.BackEnd\Test.BackEnd.csproj", "{911B1058-1BDB-46E7-8365-CB6CEC864C54}"
|
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Backend.Tests", "Backend.Tests\Backend.Tests.csproj", "{2C7E25E6-5C68-4F23-96C1-67F953719CA9}"
|
||||||
EndProject
|
EndProject
|
||||||
Global
|
Global
|
||||||
GlobalSection(SolutionConfigurationPlatforms) = preSolution
|
GlobalSection(SolutionConfigurationPlatforms) = preSolution
|
||||||
|
@ -14,9 +15,9 @@ Global
|
||||||
{936595E0-B2A6-42B9-84F8-AA8D52466E5E}.Debug|Any CPU.Build.0 = Debug|Any CPU
|
{936595E0-B2A6-42B9-84F8-AA8D52466E5E}.Debug|Any CPU.Build.0 = Debug|Any CPU
|
||||||
{936595E0-B2A6-42B9-84F8-AA8D52466E5E}.Release|Any CPU.ActiveCfg = Release|Any CPU
|
{936595E0-B2A6-42B9-84F8-AA8D52466E5E}.Release|Any CPU.ActiveCfg = Release|Any CPU
|
||||||
{936595E0-B2A6-42B9-84F8-AA8D52466E5E}.Release|Any CPU.Build.0 = Release|Any CPU
|
{936595E0-B2A6-42B9-84F8-AA8D52466E5E}.Release|Any CPU.Build.0 = Release|Any CPU
|
||||||
{911B1058-1BDB-46E7-8365-CB6CEC864C54}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
|
{2C7E25E6-5C68-4F23-96C1-67F953719CA9}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
|
||||||
{911B1058-1BDB-46E7-8365-CB6CEC864C54}.Debug|Any CPU.Build.0 = Debug|Any CPU
|
{2C7E25E6-5C68-4F23-96C1-67F953719CA9}.Debug|Any CPU.Build.0 = Debug|Any CPU
|
||||||
{911B1058-1BDB-46E7-8365-CB6CEC864C54}.Release|Any CPU.ActiveCfg = Release|Any CPU
|
{2C7E25E6-5C68-4F23-96C1-67F953719CA9}.Release|Any CPU.ActiveCfg = Release|Any CPU
|
||||||
{911B1058-1BDB-46E7-8365-CB6CEC864C54}.Release|Any CPU.Build.0 = Release|Any CPU
|
{2C7E25E6-5C68-4F23-96C1-67F953719CA9}.Release|Any CPU.Build.0 = Release|Any CPU
|
||||||
EndGlobalSection
|
EndGlobalSection
|
||||||
EndGlobal
|
EndGlobal
|
||||||
|
|
|
@ -14,8 +14,7 @@ public class PasswordController : ControllerBase
|
||||||
public PasswordController(ILogger<PasswordController> logger, IPasswordService passwordService)
|
public PasswordController(ILogger<PasswordController> logger, IPasswordService passwordService)
|
||||||
{
|
{
|
||||||
_logger = logger;
|
_logger = logger;
|
||||||
_passwordService = passwordService;
|
_passwordService = new PasswordService("Data/common-passwords.txt");
|
||||||
_passwordService.LoadCommonPasswords("Data/common-passwords.txt");
|
|
||||||
}
|
}
|
||||||
|
|
||||||
[HttpPost("change")]
|
[HttpPost("change")]
|
||||||
|
|
|
@ -4,8 +4,81 @@ namespace back_end.Services;
|
||||||
|
|
||||||
public class PasswordService : IPasswordService
|
public class PasswordService : IPasswordService
|
||||||
{
|
{
|
||||||
|
|
||||||
|
|
||||||
|
private class PasswordTreeNode {
|
||||||
|
|
||||||
|
public Dictionary<char, PasswordTreeNode> children { get; set; }
|
||||||
|
public bool endOfWord { get; set; }
|
||||||
|
|
||||||
|
public PasswordTreeNode()
|
||||||
|
{
|
||||||
|
children = new Dictionary<char, PasswordTreeNode>();
|
||||||
|
endOfWord = false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
private class PasswordTree
|
||||||
|
{
|
||||||
|
private readonly PasswordTreeNode _root;
|
||||||
|
|
||||||
|
public PasswordTree()
|
||||||
|
{
|
||||||
|
_root = new PasswordTreeNode();
|
||||||
|
}
|
||||||
|
|
||||||
|
public void Insert(string password)
|
||||||
|
{
|
||||||
|
var currentNode = _root;
|
||||||
|
foreach (var c in password)
|
||||||
|
{
|
||||||
|
if (!currentNode.children.ContainsKey(c))
|
||||||
|
{
|
||||||
|
currentNode.children.Add(c, new PasswordTreeNode());
|
||||||
|
}
|
||||||
|
currentNode = currentNode.children[c];
|
||||||
|
}
|
||||||
|
currentNode.endOfWord = true;
|
||||||
|
}
|
||||||
|
|
||||||
|
public bool Search(string password)
|
||||||
|
{
|
||||||
|
var currentNode = _root;
|
||||||
|
foreach (var c in password)
|
||||||
|
{
|
||||||
|
// This is where the logic gets a bit confusing
|
||||||
|
// if we want to just check whether the password the user gives 'passwords' EXACTLY
|
||||||
|
// then this traversal is easy, we just do the following
|
||||||
|
if (!currentNode.children.ContainsKey(c)) return false;
|
||||||
|
currentNode = currentNode.children[c];
|
||||||
|
}
|
||||||
|
return currentNode.endOfWord;
|
||||||
|
/* However, I had the idea that instead of just checking the tree like this, I'd want it to loop through
|
||||||
|
* the password given:
|
||||||
|
*
|
||||||
|
* For example
|
||||||
|
* 123!passwords should loop through the first four characters (123!)
|
||||||
|
* and just continue looping through the above once it gets to p and return true for the common check
|
||||||
|
* I think this would just be done by popping off the first character, so we get down to just 'passwords'
|
||||||
|
* for the example which would work just fine, but for something like '123!someotherpasswords' it would
|
||||||
|
* not
|
||||||
|
* For this to work, I'd have to really slice up the stirng in multiple (if not all) of the possible spaces
|
||||||
|
* and then run this check in parallel since now I'd be giving the program a LOT more work
|
||||||
|
* I believe that that would be overengineering this however, as someotherpasswords is (marginally) more
|
||||||
|
* secure than the common password 'passwords', but still fun to think about!
|
||||||
|
*/
|
||||||
|
}
|
||||||
|
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
private readonly List<string> _commonPasswords = new List<string>();
|
private readonly List<string> _commonPasswords = new List<string>();
|
||||||
|
|
||||||
|
public PasswordService(string commonPasswordFilepath)
|
||||||
|
{
|
||||||
|
LoadCommonPasswords(commonPasswordFilepath);
|
||||||
|
}
|
||||||
|
|
||||||
// load common passwords on start up to save us from having to re-load the file over and over
|
// load common passwords on start up to save us from having to re-load the file over and over
|
||||||
// if the list would change, then this is a bad idea
|
// if the list would change, then this is a bad idea
|
||||||
public void LoadCommonPasswords(string filepath)
|
public void LoadCommonPasswords(string filepath)
|
||||||
|
@ -25,7 +98,28 @@ public class PasswordService : IPasswordService
|
||||||
{
|
{
|
||||||
// RegEx feels like cheating it's so good
|
// RegEx feels like cheating it's so good
|
||||||
Regex regex = new Regex("^(?=.*?[a-zA-Z])(?=.*?[0-9])(?=.*?[!£$^*#])[a-zA-Z0-9!£$^*#]{7,14}$");
|
Regex regex = new Regex("^(?=.*?[a-zA-Z])(?=.*?[0-9])(?=.*?[!£$^*#])[a-zA-Z0-9!£$^*#]{7,14}$");
|
||||||
return !regex.IsMatch(password);
|
return !(regex.IsMatch(password)
|
||||||
|
&& this.IsPasswordLengthValid(password)
|
||||||
|
&& this.IsPasswordContainingMinimumCharacters(password)
|
||||||
|
&& this.IsPasswordContainingOnlyLegalCharacters(password));
|
||||||
|
}
|
||||||
|
|
||||||
|
public bool IsPasswordLengthValid(string password) {
|
||||||
|
// check if all the characters between the start and end of the password add to 7-14 characters
|
||||||
|
Regex regex = new Regex("^.{7,14}$");
|
||||||
|
return regex.IsMatch(password);
|
||||||
|
}
|
||||||
|
|
||||||
|
public bool IsPasswordContainingMinimumCharacters(string password) {
|
||||||
|
// look ahead for the special characters and digits and match them at least once
|
||||||
|
Regex regex = new Regex("^(?=.*?[!£$^*#])(?=.*?[0-9]).*$");
|
||||||
|
return regex.IsMatch(password);
|
||||||
|
}
|
||||||
|
|
||||||
|
public bool IsPasswordContainingOnlyLegalCharacters(string password) {
|
||||||
|
// check for any characters not in the allowed list
|
||||||
|
Regex regex = new Regex("^[a-zA-Z0-9!£$^*#]*$");
|
||||||
|
return regex.IsMatch(password);
|
||||||
}
|
}
|
||||||
|
|
||||||
public bool IsPasswordCommon(string? password)
|
public bool IsPasswordCommon(string? password)
|
||||||
|
|
35
Server/Backend.Tests/Backend.Tests.csproj
Normal file
35
Server/Backend.Tests/Backend.Tests.csproj
Normal file
|
@ -0,0 +1,35 @@
|
||||||
|
<Project Sdk="Microsoft.NET.Sdk">
|
||||||
|
|
||||||
|
<PropertyGroup>
|
||||||
|
<TargetFramework>net7.0</TargetFramework>
|
||||||
|
<ImplicitUsings>enable</ImplicitUsings>
|
||||||
|
<Nullable>enable</Nullable>
|
||||||
|
|
||||||
|
<IsPackable>false</IsPackable>
|
||||||
|
<IsTestProject>true</IsTestProject>
|
||||||
|
</PropertyGroup>
|
||||||
|
|
||||||
|
<ItemGroup>
|
||||||
|
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.7.1" />
|
||||||
|
<PackageReference Include="xunit" Version="2.4.2" />
|
||||||
|
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.5">
|
||||||
|
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
|
||||||
|
<PrivateAssets>all</PrivateAssets>
|
||||||
|
</PackageReference>
|
||||||
|
<PackageReference Include="coverlet.collector" Version="3.2.0">
|
||||||
|
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
|
||||||
|
<PrivateAssets>all</PrivateAssets>
|
||||||
|
</PackageReference>
|
||||||
|
</ItemGroup>
|
||||||
|
|
||||||
|
<ItemGroup>
|
||||||
|
<ProjectReference Include="..\BackEnd\BackEnd.csproj" />
|
||||||
|
</ItemGroup>
|
||||||
|
|
||||||
|
<ItemGroup>
|
||||||
|
<None Update="common-passwords.txt">
|
||||||
|
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
|
||||||
|
</None>
|
||||||
|
</ItemGroup>
|
||||||
|
|
||||||
|
</Project>
|
1
Server/Backend.Tests/GlobalUsings.cs
Normal file
1
Server/Backend.Tests/GlobalUsings.cs
Normal file
|
@ -0,0 +1 @@
|
||||||
|
global using Xunit;
|
76
Server/Backend.Tests/PasswordTests.cs
Normal file
76
Server/Backend.Tests/PasswordTests.cs
Normal file
|
@ -0,0 +1,76 @@
|
||||||
|
using back_end.Services;
|
||||||
|
|
||||||
|
namespace Backend.Tests;
|
||||||
|
|
||||||
|
public class PasswordTests
|
||||||
|
{
|
||||||
|
private readonly PasswordService _passwordService;
|
||||||
|
|
||||||
|
public PasswordTests()
|
||||||
|
{
|
||||||
|
_passwordService = new PasswordService(Path.Combine(AppDomain.CurrentDomain.BaseDirectory,"common-passwords.txt"));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
[InlineData("abcdefg",true)]
|
||||||
|
[InlineData("abcdef",false)]
|
||||||
|
[InlineData("abcdefghijklmn",true)]
|
||||||
|
[InlineData("abcdefghijklmno",false)]
|
||||||
|
public void PasswordLengthValid(string password, bool valid)
|
||||||
|
{
|
||||||
|
var check = _passwordService.IsPasswordLengthValid(password);
|
||||||
|
Assert.Equal(valid, check);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
[InlineData("password", false)]
|
||||||
|
[InlineData("password1", false)]
|
||||||
|
[InlineData("password!", false)]
|
||||||
|
[InlineData("password1!", true)]
|
||||||
|
public void PasswordContainsMinimumCharacters(string password, bool valid)
|
||||||
|
{
|
||||||
|
var check = _passwordService.IsPasswordContainingMinimumCharacters(password);
|
||||||
|
Assert.Equal(valid, check);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
[InlineData("password",true)]
|
||||||
|
[InlineData("password(",false)]
|
||||||
|
[InlineData("password\u00a9",false)]
|
||||||
|
public void PasswordContainsOnlyLegalCharacters(string password, bool valid)
|
||||||
|
{
|
||||||
|
var check = _passwordService.IsPasswordContainingOnlyLegalCharacters(password);
|
||||||
|
Assert.Equal(valid, check);
|
||||||
|
}
|
||||||
|
|
||||||
|
// not sure why, however, 'password' on it's own is not considered a 'common' password
|
||||||
|
// As explained in the comments for the PasswordTree, for the '123!passwords' test, I feel like it should count
|
||||||
|
// as a common password but doesnt in this implementation!
|
||||||
|
[Theory]
|
||||||
|
[InlineData("123!passwords",false)]
|
||||||
|
[InlineData("password123!",true)]
|
||||||
|
[InlineData("123!haslo",false)]
|
||||||
|
[InlineData("haslo123!",false)]
|
||||||
|
[InlineData("passwords",true)]
|
||||||
|
[InlineData("haslo",false)]
|
||||||
|
public void PasswordIsCommon(string password, bool valid)
|
||||||
|
{
|
||||||
|
var check = _passwordService.IsPasswordCommon(password);
|
||||||
|
Assert.Equal(valid, check);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
[InlineData("123!passwords",true)]
|
||||||
|
[InlineData("password123!",true)] // this is a valid password, regardless of it being common
|
||||||
|
[InlineData("123!haslo",true)]
|
||||||
|
[InlineData("haslo123!",true)]
|
||||||
|
[InlineData("passwords",false)]
|
||||||
|
[InlineData("haslo",false)]
|
||||||
|
[InlineData("password\u00a9",false)]
|
||||||
|
public void PasswordIsValid(string password, bool valid)
|
||||||
|
{
|
||||||
|
// The logic here is hard to read, the test checks whether I am passing in a VALID password
|
||||||
|
var check = !(_passwordService.IsPasswordInvalid(password));
|
||||||
|
Assert.Equal(valid, check);
|
||||||
|
}
|
||||||
|
}
|
10000
Server/Backend.Tests/common-passwords.txt
Normal file
10000
Server/Backend.Tests/common-passwords.txt
Normal file
File diff suppressed because it is too large
Load diff
1
Server/PasswordTests/GlobalUsings.cs
Normal file
1
Server/PasswordTests/GlobalUsings.cs
Normal file
|
@ -0,0 +1 @@
|
||||||
|
global using Xunit;
|
25
Server/PasswordTests/PasswordTests.csproj
Normal file
25
Server/PasswordTests/PasswordTests.csproj
Normal file
|
@ -0,0 +1,25 @@
|
||||||
|
<Project Sdk="Microsoft.NET.Sdk">
|
||||||
|
|
||||||
|
<PropertyGroup>
|
||||||
|
<TargetFramework>net7.0</TargetFramework>
|
||||||
|
<ImplicitUsings>enable</ImplicitUsings>
|
||||||
|
<Nullable>enable</Nullable>
|
||||||
|
|
||||||
|
<IsPackable>false</IsPackable>
|
||||||
|
<IsTestProject>true</IsTestProject>
|
||||||
|
</PropertyGroup>
|
||||||
|
|
||||||
|
<ItemGroup>
|
||||||
|
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.7.1" />
|
||||||
|
<PackageReference Include="xunit" Version="2.4.2" />
|
||||||
|
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.5">
|
||||||
|
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
|
||||||
|
<PrivateAssets>all</PrivateAssets>
|
||||||
|
</PackageReference>
|
||||||
|
<PackageReference Include="coverlet.collector" Version="3.2.0">
|
||||||
|
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
|
||||||
|
<PrivateAssets>all</PrivateAssets>
|
||||||
|
</PackageReference>
|
||||||
|
</ItemGroup>
|
||||||
|
|
||||||
|
</Project>
|
10
Server/PasswordTests/UnitTest1.cs
Normal file
10
Server/PasswordTests/UnitTest1.cs
Normal file
|
@ -0,0 +1,10 @@
|
||||||
|
namespace PasswordTests;
|
||||||
|
|
||||||
|
public class UnitTest1
|
||||||
|
{
|
||||||
|
[Fact]
|
||||||
|
public void Test1()
|
||||||
|
{
|
||||||
|
|
||||||
|
}
|
||||||
|
}
|
Loading…
Reference in a new issue