Skip to content

Commit

Permalink
Backend refactor/cleanup (#892)
Browse files Browse the repository at this point in the history 
* Remove redundant parenthesis and use async

* Fix typo

* Fix private naming convention

* Fix HasProjectPermission mock type signature

* Make RoundTripObj immutable

* Remove extra parens and reduce nesting

* More refactor

* Use ??= operator to simplify expression

* Remove redundant else blocks

* Make PasswordResetData attributes readonly

* Make EmailInviteData immutable

* Remove null check that cannot happen

* Properly sanitize filenames in Audio Controller

* Add unit tests for SanitizeId

* Add switch expression

* Allow ( ) , and space file name characters

* Fix LGTM issue
  • Loading branch information
@johnthagen
johnthagen authored Jan 1, 2021
1 parent 9a28ce2 commit aa6cfa2
Show file tree
Hide file tree
Showing 20 changed files with 228 additions and 139 deletions.
28 changes: 14 additions & 14 deletions Backend.Tests/Controllers/LiftControllerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,20 +130,20 @@ public string RandomLiftFile(string path)

private static Word RandomWord(string projId)
{
var word = new Word { Senses = new List<Sense>() { new Sense(), new Sense(), new Sense() } };
var word = new Word { Senses = new List<Sense> { new Sense(), new Sense(), new Sense() } };

foreach (var sense in word.Senses)
{
sense.Accessibility = State.Active;
sense.Glosses = new List<Gloss>() { new Gloss(), new Gloss(), new Gloss() };
sense.Glosses = new List<Gloss> { new Gloss(), new Gloss(), new Gloss() };

foreach (var gloss in sense.Glosses)
{
gloss.Def = Util.RandString();
gloss.Language = Util.RandString(3);
}

sense.SemanticDomains = new List<SemanticDomain>()
sense.SemanticDomains = new List<SemanticDomain>
{
new SemanticDomain(), new SemanticDomain(), new SemanticDomain()
};
Expand Down Expand Up @@ -186,12 +186,12 @@ private static string ExtractZipFileContents(byte[] fileContents)

public class RoundTripObj
{
public string Filename { get; set; }
public string Language { get; set; }
public List<string> AudioFiles { get; set; }
public int NumOfWords { get; set; }
public string EntryGuid { get; set; }
public string SenseGuid { get; set; }
public string Filename { get; }
public string Language { get; }
public List<string> AudioFiles { get; }
public int NumOfWords { get; }
public string EntryGuid { get; }
public string SenseGuid { get; }

public RoundTripObj(
string filename, string language, List<string> audio,
Expand Down Expand Up @@ -238,7 +238,7 @@ public async Task TestExportDeleted()

// Read contents.
byte[] contents;
using (var fileStream = result.FileStream)
await using (var fileStream = result.FileStream)
{
contents = ReadAllBytes(fileStream);
}
Expand All @@ -247,7 +247,7 @@ public async Task TestExportDeleted()
var extractedExportDir = ExtractZipFileContents(contents);
var exportPath = Path.Combine(extractedExportDir,
Path.Combine("Lift", "NewLiftFile.lift"));
var text = File.ReadAllText(exportPath, Encoding.UTF8);
var text = await File.ReadAllTextAsync(exportPath, Encoding.UTF8);
//TODO: Add SIL or other XML assertion library and verify with xpath that the correct entries are kept vs deleted
// Make sure we exported 2 live and one dead entry
Assert.That(Regex.Matches(text, "<entry").Count, Is.EqualTo(3));
Expand All @@ -260,7 +260,7 @@ public async Task TestExportDeleted()
Assert.NotNull(notFoundResult);
}

private static RoundTripObj[] RoundTripCases =
private static RoundTripObj[] _roundTripCases =
{
new RoundTripObj("Gusillaay.zip", "gsl-Qaaa-x-orth", new List<string>(), 8045),
new RoundTripObj("Lotud.zip", "dtr", new List<string>(), 5400),
Expand All @@ -278,7 +278,7 @@ public async Task TestExportDeleted()
"e44420dd-a867-4d71-a43f-e472fd3a8f82" /*id of its first sense*/)
};

[TestCaseSource(nameof(RoundTripCases))]
[TestCaseSource(nameof(_roundTripCases))]
public void TestRoundtrip(RoundTripObj roundTripObj)
{
// This test assumes you have the starting .zip (Filename) included in your project files.
Expand Down Expand Up @@ -328,7 +328,7 @@ public void TestRoundtrip(RoundTripObj roundTripObj)
var exportedFilePath = _liftController.CreateLiftExport(proj1.Id).Result;
var exportedDirectory = FileOperations.ExtractZipFile(exportedFilePath, null, false);

// Assert the file was created with desired heirarchy.
// Assert the file was created with desired hierarchy.
Assert.That(Directory.Exists(exportedDirectory));
Assert.That(Directory.Exists(Path.Combine(exportedDirectory, "Lift", "audio")));
foreach (var audioFile in roundTripObj.AudioFiles)
Expand Down
6 changes: 3 additions & 3 deletions Backend.Tests/Controllers/ProjectControllerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,23 @@ private static Project RandomProject()

for (var i = 1; i < 4; i++)
{
project.SemanticDomains.Add(new SemanticDomain()
project.SemanticDomains.Add(new SemanticDomain
{
Id = $"{i}",
Name = Util.RandString(),
Description = Util.RandString()
});
for (var j = 1; j < 4; j++)
{
project.SemanticDomains.Add(new SemanticDomain()
project.SemanticDomains.Add(new SemanticDomain
{
Id = $"{i}.{j}",
Name = Util.RandString(),
Description = Util.RandString()
});
for (var k = 1; k < 4; k++)
{
project.SemanticDomains.Add(new SemanticDomain()
project.SemanticDomains.Add(new SemanticDomain
{
Id = $"{i}.{j}.{k}",
Name = Util.RandString(),
Expand Down
2 changes: 1 addition & 1 deletion Backend.Tests/Controllers/UserEditControllerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private UserEdit RandomUserEdit()
var edit = new Edit
{
GoalType = count,
StepData = new List<string>() { Util.RandString() }
StepData = new List<string> { Util.RandString() }
};
userEdit.ProjectId = _projId;
userEdit.Edits.Add(edit);
Expand Down
2 changes: 1 addition & 1 deletion Backend.Tests/Controllers/UserRoleControllerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ private UserRole RandomUserRole()
var userRole = new UserRole
{
ProjectId = _projId,
Permissions = new List<int>()
Permissions = new List<int>
{
(int)Permission.DeleteEditSettingsAndUsers,
(int)Permission.ImportExport,
Expand Down
6 changes: 3 additions & 3 deletions Backend.Tests/Controllers/WordControllerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,22 @@ private Word RandomWord()
Audio = new List<string>(),
EditedBy = new List<string> { Util.RandString(), Util.RandString() },
ProjectId = _projId,
Senses = new List<Sense>() { new Sense(), new Sense(), new Sense() },
Senses = new List<Sense> { new Sense(), new Sense(), new Sense() },
Note = new Note { Language = Util.RandString(), Text = Util.RandString() }
};

foreach (var sense in word.Senses)
{
sense.Accessibility = State.Active;
sense.Glosses = new List<Gloss>() { new Gloss(), new Gloss(), new Gloss() };
sense.Glosses = new List<Gloss> { new Gloss(), new Gloss(), new Gloss() };

foreach (var gloss in sense.Glosses)
{
gloss.Def = Util.RandString();
gloss.Language = Util.RandString(3);
}

sense.SemanticDomains = new List<SemanticDomain>()
sense.SemanticDomains = new List<SemanticDomain>
{
new SemanticDomain(), new SemanticDomain(), new SemanticDomain()
};
Expand Down
106 changes: 106 additions & 0 deletions Backend.Tests/Helper/Sanitization.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
using System.Collections.Generic;
using NUnit.Framework;

using BackendFramework.Helper;

namespace Backend.Tests.Helper
{
public class SanitizationTests
{
private static List<string> _validIds = new List<string>
{
"a",
"1",
"a-5",
};

[TestCaseSource(nameof(_validIds))]
public void TestValidIds(string id)
{
Assert.That(Sanitization.SanitizeId(id));
}

private static List<string> _invalidIds = new List<string>
{
"_",
"a_1",
".",
"a.1",
"/",
"\\",
"../a",
"..\\a",
"../1",
"..\\1",
"!",
"@",
"#",
"$",
"%",
"^",
"&",
"*",
"+",
"<",
">",
":",
"|",
"?"
};
[TestCaseSource(nameof(_invalidIds))]
public void TestInvalidIds(string id)
{
Assert.False(Sanitization.SanitizeId(id));
}

private static List<string> _validFileNames = new List<string>
{
"a",
"1",
"ab555.webm",
"a-5.webm",
"a-5.jpg",
"a-5.png",
"a_5.png",
"a-5.png",
"a(5).png",
"a 5.png",
"a,5.png"
};

[TestCaseSource(nameof(_validFileNames))]
public void TestValidFileNames(string fileName)
{
Assert.That(Sanitization.SanitizeFileName(fileName));
}

private static List<string> _invalidFileNames = new List<string>
{
"/",
"\\",
"../a",
"..\\a",
"../1",
"..\\1",
"!",
"@",
"#",
"$",
"%",
"^",
"&",
"*",
"+",
"<",
">",
":",
"|",
"?"
};
[TestCaseSource(nameof(_invalidFileNames))]
public void TestInvalidFileNames(string fileName)
{
Assert.False(Sanitization.SanitizeFileName(fileName));
}
}
}
7 changes: 6 additions & 1 deletion Backend.Tests/Mocks/PermissionServiceMock.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,13 @@ public bool IsUserIdAuthorized(HttpContext request, string userId)

/// <summary>
/// By default this will return true, unless the test passes in an <see cref="UnauthorizedHttpContext"/>.
///
/// <param name="request">
/// Note this parameter is nullable in the mock implementation even though the real implementation it is not
/// to support unit testing when `HttpContext`s are not available.
/// </param>
/// </summary>
public Task<bool> HasProjectPermission(HttpContext request, Permission permission)
public Task<bool> HasProjectPermission(HttpContext? request, Permission permission)
{
return Task.FromResult(request is null || request.Request.Headers["Authorization"] != UnauthorizedHeader);
}
Expand Down
8 changes: 2 additions & 6 deletions Backend/Controllers/AudioController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,13 @@ public IActionResult DownloadAudioFile(string projectId, string wordId, string f
//}

// Sanitize user input
if (!Sanitization.SanitizeId(projectId) || !Sanitization.SanitizeId(wordId))
if (!Sanitization.SanitizeId(projectId) || !Sanitization.SanitizeId(wordId) ||
!Sanitization.SanitizeFileName(fileName))
{
return new UnsupportedMediaTypeResult();
}

var filePath = FileStorage.GenerateAudioFilePath(projectId, fileName);
if (filePath is null)
{
return new BadRequestObjectResult("There was more than one subDir of the extracted zip");
}

var file = System.IO.File.OpenRead(filePath);
if (file is null)
{
Expand Down
61 changes: 23 additions & 38 deletions Backend/Controllers/ProjectController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,7 @@ public async Task<IActionResult> Post([FromBody] Project project)
userRole = await _userRoleService.Create(userRole);

// Update user with userRole
if (currentUser.ProjectRoles is null)
{
currentUser.ProjectRoles = new Dictionary<string, string>();
}
currentUser.ProjectRoles ??= new Dictionary<string, string>();

// Generate the userRoles and update the user
currentUser.ProjectRoles.Add(project.Id, userRole.Id);
Expand Down Expand Up @@ -157,18 +154,12 @@ public async Task<IActionResult> Put(string projectId, [FromBody] Project projec
}

var result = await _projectService.Update(projectId, project);
if (result == ResultOfUpdate.NotFound)
{
return new NotFoundObjectResult(projectId);
}
else if (result == ResultOfUpdate.Updated)
return result switch
{
return new OkObjectResult(projectId);
}
else
{
return new StatusCodeResult(304);
}
ResultOfUpdate.NotFound => new NotFoundObjectResult(projectId),
ResultOfUpdate.Updated => new OkObjectResult(projectId),
_ => new StatusCodeResult(304)
};
}

/// <summary> Updates <see cref="Project"/> with specified id with a new list of chars </summary>
Expand Down Expand Up @@ -274,17 +265,12 @@ public async Task<IActionResult> UpdateUserRole(string projectId, string userId,
userRole.Permissions = new List<int>(permissions);

var result = await _userRoleService.Update(userRoleId, userRole);

if (result == ResultOfUpdate.NotFound)
return result switch
{
return new NotFoundObjectResult(userId);
}
if (result == ResultOfUpdate.Updated)
{
return new OkObjectResult(userId);
}

return new StatusCodeResult(304);
ResultOfUpdate.NotFound => new NotFoundObjectResult(userId),
ResultOfUpdate.Updated => new OkObjectResult(userId),
_ => new StatusCodeResult(304)
};
}

/// <summary> Check if lift import has already happened for this project </summary>
Expand Down Expand Up @@ -370,26 +356,25 @@ public async Task<IActionResult> ValidateToken(string projectId, string token)
{
return new OkObjectResult(status);
}
else if (activeTokenExists && userIsRegistered
&& !currentUser.ProjectRoles.ContainsKey(projectId)
&& await _projectService.RemoveTokenAndCreateUserRole(project, currentUser, tokenObj))
{
return new OkObjectResult(status);
}
else

if (activeTokenExists && userIsRegistered
&& !currentUser.ProjectRoles.ContainsKey(projectId)
&& await _projectService.RemoveTokenAndCreateUserRole(project, currentUser, tokenObj))
{
status[0] = false;
status[1] = false;
return new OkObjectResult(status);
}

status[0] = false;
status[1] = false;
return new OkObjectResult(status);
}

public class EmailInviteData
{
public string EmailAddress;
public string Message;
public string ProjectId;
public string Domain;
public readonly string EmailAddress;
public readonly string Message;
public readonly string ProjectId;
public readonly string Domain;

public EmailInviteData()
{
Expand Down
Loading

0 comments on commit aa6cfa2

Please sign in to comment.