Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/UnitTests/UnitTests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="MongoDB.Analyzer" Version="1.1.0" />
<PackageReference Include="Moq" Version="4.18.4" />
<PackageReference Include="Shouldly" Version="4.1.0" />
<PackageReference Include="xunit" Version="2.4.2" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.5">
Expand Down
55 changes: 55 additions & 0 deletions backend/UnitTests/WebApi/Auth/ProjectAuthorizationHandlerTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
using LanguageForge.Api.Entities;
using LanguageForge.WebApi.Auth;
using Microsoft.AspNetCore.Authorization;
using static LanguageForge.UnitTests.WebApi.ContextHelpers;

namespace LanguageForge.UnitTests.WebApi.Auth;

public class ProjectAuthorizationHandlerTest
{
[Fact]
public async Task FailsIfUserIsNotAuthorized()
{
// GIVEN a user that is not authorized
var user = new LfUser("test@testeroolaboom.fun", LfId<User>.Parse("User:6359f8855e3dc273d4662f2a"),
UserRole.User,
new[] {
new UserProjectRole("fun-language", ProjectRole.Manager),
new UserProjectRole("spooky-language", ProjectRole.Contributor),
});
var webContext = WebContext(user);
var projectContext = ProjectContext("missing-language");

// WHEN the handler is invoked
var handler = new ProjectAuthorizationHandler(webContext, projectContext);
var req = new ProjectAuthorizationRequirement();
var context = new AuthorizationHandlerContext(new IAuthorizationRequirement[] { req }, null, null);
await handler.HandleAsync(context);

// THEN the handler fails
context.HasFailed.ShouldBeTrue();
}

[Fact]
public async Task SucceedsIfUserIsAdmin()
Comment thread
myieye marked this conversation as resolved.
{
// GIVEN an admin user
var user = new LfUser("test@testeroolaboom.fun", LfId<User>.Parse("User:6359f8855e3dc273d4662f2a"),
UserRole.SystemAdmin,
new[] {
new UserProjectRole("fun-language", ProjectRole.Manager),
new UserProjectRole("spooky-language", ProjectRole.Contributor),
});
var webContext = WebContext(user);
var projectContext = ProjectContext("missing-language");

// WHEN the handler is invoked
var handler = new ProjectAuthorizationHandler(webContext, projectContext);
var req = new ProjectAuthorizationRequirement();
var context = new AuthorizationHandlerContext(new IAuthorizationRequirement[] { req }, null, null);
await handler.HandleAsync(context);

// THEN the handler succeeds
context.HasSucceeded.ShouldBeTrue();
}
}
22 changes: 22 additions & 0 deletions backend/UnitTests/WebApi/ContextHelpers.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using LanguageForge.WebApi;
using LanguageForge.WebApi.Auth;
using Moq;

namespace LanguageForge.UnitTests.WebApi;

public static class ContextHelpers
{
public static ILfWebContext WebContext(LfUser user)
{
var contextMock = new Mock<ILfWebContext>();
contextMock.SetupGet(c => c.User).Returns(user);
return contextMock.Object;
}

public static ILfProjectContext ProjectContext(string projectCode)
{
var contextMock = new Mock<ILfProjectContext>();
contextMock.SetupGet(c => c.ProjectCode).Returns(projectCode);
return contextMock.Object;
}
}
6 changes: 5 additions & 1 deletion backend/WebApi/Auth/AuthSetup.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.AspNetCore.Authorization;
using Microsoft.IdentityModel.Logging;
using Microsoft.OpenApi.Models;

Expand All @@ -21,11 +22,14 @@ public static void SetupLfAuth(IServiceCollection services, IConfiguration confi
services.AddSingleton<JwtService>();
services.AddAuthorization(options =>
{
options.AddPolicy(nameof(ProjectAuthorizationRequirement), policy => policy.Requirements.Add(new ProjectAuthorizationRequirement()));

//fallback policy is used when there's no auth attribute.
//default policy is when there's no parameters specified on the auth attribute
//this will make sure that all endpoints require auth unless they have the AllowAnonymous attribute
options.FallbackPolicy = options.DefaultPolicy;
options.FallbackPolicy = AuthorizationPolicy.Combine(options.DefaultPolicy, options.GetPolicy(nameof(ProjectAuthorizationRequirement)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this modifies the fallback policy. But not the default, so if we were to put the authorize attribute somewhere then we would no longer be applying the project auth requirement without any trigger that this was happening.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Then, I'll change the Default-Policy.

});
services.AddScoped<IAuthorizationHandler, ProjectAuthorizationHandler>();
services.AddOptions<JwtOptions>()
.BindConfiguration("Authentication:Jwt")
.Validate(options => options.GoogleClientId != "==== replace ====",
Expand Down
4 changes: 2 additions & 2 deletions backend/WebApi/Auth/JwtService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public static TokenValidationParameters TokenValidationParameters(JwtOptions jwt
};
}

public static LfUser ExtractLfUser(ClaimsPrincipal user)
public static LfUser? ExtractLfUser(ClaimsPrincipal user)
{
var emailClaim = user.FindFirstValue(EmailClaimType);
var idClaim = user.FindFirstValue(JwtRegisteredClaimNames.Sub);
Expand All @@ -115,7 +115,7 @@ public static LfUser ExtractLfUser(ClaimsPrincipal user)
if (string.IsNullOrEmpty(emailClaim) || string.IsNullOrEmpty(idClaim) ||
string.IsNullOrEmpty(roleClaim) || string.IsNullOrEmpty(projectRolesClaim))
{
throw new ArgumentException($"User is missing required claims. Claims: {user.Claims}.");
return null;
Copy link
Copy Markdown
Contributor Author

@myieye myieye Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hahn-kev I decided this makes more sense in the end, because when we get requests from unauthenticated users there can still be a WebContext, it just doesn't include a user.

Ultimately I made this change, because (to my surprise) my ProjectAuthorizationHandler gets instantiated for requests that don't need it (i.e. requests where AllowAnonymous is present). So, I either had to call ExtractLfUser from the authorization handler or make this change. I don't have a super strong opinion either way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first thought was to just lazily get the user - and leave this code as is.

But that means we could miss bugs where we access the user from some code outside of an authorized context, in this case that code has to decide what to do if the user is null now.

}

var userId = LfId<User>.Parse(idClaim);
Expand Down
48 changes: 48 additions & 0 deletions backend/WebApi/Auth/ProjectAuthorizationHandler.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
using LanguageForge.Api.Entities;
using Microsoft.AspNetCore.Authorization;

namespace LanguageForge.WebApi.Auth;

public class ProjectAuthorizationRequirement : IAuthorizationRequirement { }

public class ProjectAuthorizationHandler : AuthorizationHandler<ProjectAuthorizationRequirement>
{
private readonly ILfWebContext _lfWebContext;
private readonly ILfProjectContext _lfProjectContext;

public ProjectAuthorizationHandler(ILfWebContext lfWebContext, ILfProjectContext lfProjectContext)
{
_lfWebContext = lfWebContext;
_lfProjectContext = lfProjectContext;
}

protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, ProjectAuthorizationRequirement requirement)
{
var projectCode = _lfProjectContext.ProjectCode;
if (string.IsNullOrWhiteSpace(projectCode))
{
context.Succeed(requirement);
return Task.CompletedTask;
}

var lfUser = _lfWebContext.User;

if (lfUser == null)
{
context.Fail(new AuthorizationFailureReason(this, "User is not authenticated"));
return Task.CompletedTask;
}

if (lfUser.Role == UserRole.SystemAdmin
|| lfUser.Projects.Any(p => p.ProjectCode == projectCode))
{
context.Succeed(requirement);
}
else
{
context.Fail(new AuthorizationFailureReason(this, $"User is not authorized for project: {projectCode}"));
}

return Task.CompletedTask;
}
}
3 changes: 2 additions & 1 deletion backend/WebApi/Controllers/CommentController.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
using Microsoft.AspNetCore.Mvc;
using static LanguageForge.WebApi.Controllers.PathConstants;

namespace LanguageForge.WebApi.Controllers;

[ApiController]
[Route("api/[controller]/{projectCode}/{entryId}")]
[Route($"api/[controller]/{{{ProjectCode}}}/{{entryId}}")]
public class CommentController : ControllerBase
{
[HttpGet("{fieldName}/{inputSystem}")]
Expand Down
6 changes: 3 additions & 3 deletions backend/WebApi/Controllers/EntryController.cs
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
using LanguageForge.WebApi.Dtos;
using Microsoft.AspNetCore.Mvc;
using static LanguageForge.WebApi.Controllers.PathConstants;

namespace LanguageForge.WebApi.Controllers;

[ApiController]
[Route("api/[controller]/{projectCode}")]
[Route($"api/[controller]/{{{ProjectCode}}}")]
public class EntryController : ControllerBase
{
// GET: api/Entry/{projectCode}
[HttpGet]
public List<EntryDto> GetEntries(string projectCode)
public List<EntryDto> GetEntries()
{
return new() {
new() {
Expand Down
6 changes: 6 additions & 0 deletions backend/WebApi/Controllers/PathConstants.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace LanguageForge.WebApi.Controllers;

public static class PathConstants
{
public const string ProjectCode = "projectCode";
}
15 changes: 8 additions & 7 deletions backend/WebApi/Controllers/ProjectController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using LanguageForge.WebApi.Dtos;
using LanguageForge.WebApi.Services;
using Microsoft.AspNetCore.Mvc;
using static LanguageForge.WebApi.Controllers.PathConstants;

namespace LanguageForge.WebApi.Controllers;

Expand Down Expand Up @@ -35,10 +36,10 @@ public async Task<List<ProjectDto>> GetAllProjects()
}

// GET: api/Project/5
[HttpGet("{projectCode}")]
public string GetProject(string projectCode)
[HttpGet($"{{{ProjectCode}}}")]
public async Task<ProjectDto?> GetProject(string projectCode)
{
return "value";
return await _projectService.GetProject(projectCode);
}

// POST: api/Project
Expand All @@ -48,14 +49,14 @@ public void PostProject([FromBody] string value)
}

// PUT: api/Project/5
[HttpPut("{projectCode}")]
public void PutProject(string projectCode, [FromBody] string value)
[HttpPut($"{{{ProjectCode}}}")]
public void PutProject([FromBody] string value)
{
}

// DELETE: api/Project/5
[HttpDelete("{projectCode}")]
public void DeleteProject(string projectCode)
[HttpDelete($"{{{ProjectCode}}}")]
public void DeleteProject()
{
}
}
23 changes: 20 additions & 3 deletions backend/WebApi/LfWebContext.cs
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
using LanguageForge.WebApi.Auth;
using LanguageForge.WebApi.Controllers;

namespace LanguageForge.WebApi;

public interface ILfWebContext
{
/// <summary>
/// Authenticated user details
/// Authenticated user details. Null if the current user is not authenticated.
/// </summary>
LfUser User { get; }
LfUser? User { get; }
}

public class LfWebContext : ILfWebContext
{
public LfUser User { get; }
public LfUser? User { get; }

public LfWebContext(IHttpContextAccessor httpContextAccessor)
{
Expand All @@ -23,5 +24,21 @@ public LfWebContext(IHttpContextAccessor httpContextAccessor)
}
User = JwtService.ExtractLfUser(httpContext.User);
}
}

public interface ILfProjectContext
{
public string? ProjectCode { get; }
}

public class LfProjectContext : ILfProjectContext
{
public string? ProjectCode { get; }

public LfProjectContext(IHttpContextAccessor httpContextAccessor)
{
object? projectCode = null;
httpContextAccessor.HttpContext?.Request.RouteValues.TryGetValue(PathConstants.ProjectCode, out projectCode);
ProjectCode = projectCode?.ToString();
}
}
8 changes: 8 additions & 0 deletions backend/WebApi/Services/ProjectService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,12 @@ public async Task<List<ProjectDto>> ListProjects(IEnumerable<string> projectCode
.Project(_projectToDto)
.ToListAsync();
}

public async Task<ProjectDto?> GetProject(string projectCode)
{
return await _systemDbContext.Projects
.Find(p => p.ProjectCode == projectCode)
.Project(_projectToDto)
.SingleOrDefaultAsync();
}
}
1 change: 1 addition & 0 deletions backend/WebApi/WebApiKernel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ public static void Setup(IServiceCollection services)
services.AddSingleton<UserService>();
services.AddHttpContextAccessor();
services.AddScoped<ILfWebContext, LfWebContext>();
services.AddScoped<ILfProjectContext, LfProjectContext>();
}
}