From be5d650433fed05343e632e7af3adb1fd1652f51 Mon Sep 17 00:00:00 2001 From: Ted Wollman <25165500+TheTedder@users.noreply.github.com> Date: Tue, 16 Jul 2024 20:50:39 -0400 Subject: [PATCH] Fix Anon Endpoints (#215) * Remove fallback policy. * Add authorization headers. * Return 401 when authentication fails. * Correctly annotate endpoints that return 403. * Require authentication to create a run. * Update openapi.json. * Require mod status to create a category. * Don't use moderator policy. * Remove old comments. * Fix faulty LB test. * Don't explicitly fail auth. --- LeaderboardBackend.Test/Leaderboards.cs | 8 +-- .../Authorization/MiddlewareResultHandler.cs | 4 +- .../UserTypeAuthorizationHandler.cs | 60 ++++++------------- .../Controllers/AccountController.cs | 1 + .../Controllers/CategoriesController.cs | 9 +-- .../Controllers/LeaderboardsController.cs | 2 +- .../Controllers/RunsController.cs | 1 + .../Controllers/UsersController.cs | 1 + LeaderboardBackend/Program.cs | 28 +++------ LeaderboardBackend/openapi.json | 6 +- 10 files changed, 40 insertions(+), 80 deletions(-) diff --git a/LeaderboardBackend.Test/Leaderboards.cs b/LeaderboardBackend.Test/Leaderboards.cs index f8f1621b..215b2c4d 100644 --- a/LeaderboardBackend.Test/Leaderboards.cs +++ b/LeaderboardBackend.Test/Leaderboards.cs @@ -143,12 +143,8 @@ await _apiClient.Post( } CreateLeaderboardRequest reqForInexistentBoard = _createBoardReqFaker.Generate(); - - RequestFailureException e = Assert.ThrowsAsync( - () => _apiClient.Get($"/api/leaderboards/{reqForInexistentBoard.Slug}", new()) - )!; - - e.Response.StatusCode.Should().Be(HttpStatusCode.NotFound); + Func> act = async () => await _apiClient.Get($"/api/leaderboards/{reqForInexistentBoard.Slug}", new()); + await act.Should().ThrowAsync().Where(e => e.Response.StatusCode == HttpStatusCode.NotFound); } private static string ListToQueryString(IEnumerable list, string key) diff --git a/LeaderboardBackend/Authorization/MiddlewareResultHandler.cs b/LeaderboardBackend/Authorization/MiddlewareResultHandler.cs index e1242655..1d5ff711 100644 --- a/LeaderboardBackend/Authorization/MiddlewareResultHandler.cs +++ b/LeaderboardBackend/Authorization/MiddlewareResultHandler.cs @@ -15,9 +15,9 @@ public async Task HandleAsync( PolicyAuthorizationResult policyAuthorizationResult ) { - if (policyAuthorizationResult.Forbidden) + if (policyAuthorizationResult.AuthorizationFailure?.FailureReasons.Any(fr => fr.Handler is UserTypeAuthorizationHandler) ?? false) { - httpContext.Response.StatusCode = (int)HttpStatusCode.NotFound; + httpContext.Response.StatusCode = (int)HttpStatusCode.Unauthorized; return; } diff --git a/LeaderboardBackend/Authorization/UserTypeAuthorizationHandler.cs b/LeaderboardBackend/Authorization/UserTypeAuthorizationHandler.cs index d6cc5628..836a9d59 100644 --- a/LeaderboardBackend/Authorization/UserTypeAuthorizationHandler.cs +++ b/LeaderboardBackend/Authorization/UserTypeAuthorizationHandler.cs @@ -1,5 +1,6 @@ using System.Diagnostics.CodeAnalysis; using LeaderboardBackend.Models.Entities; +using LeaderboardBackend.Result; using LeaderboardBackend.Services; using Microsoft.AspNetCore.Authorization; using Microsoft.Extensions.Options; @@ -7,22 +8,13 @@ namespace LeaderboardBackend.Authorization; -public class UserTypeAuthorizationHandler : AuthorizationHandler +public class UserTypeAuthorizationHandler( + IOptions config, + IUserService userService + ) : AuthorizationHandler { - private readonly IAuthService _authService; - private readonly TokenValidationParameters _jwtValidationParams; - private readonly IUserService _userService; - - public UserTypeAuthorizationHandler( - IAuthService authService, - IOptions config, - IUserService userService - ) - { - _authService = authService; - _jwtValidationParams = Jwt.ValidationParameters.GetInstance(config.Value); - _userService = userService; - } + private readonly TokenValidationParameters _jwtValidationParams = Jwt.ValidationParameters.GetInstance(config.Value); + private readonly IUserService _userService = userService; protected override async Task HandleRequirementAsync( AuthorizationHandlerContext context, @@ -34,37 +26,23 @@ UserTypeRequirement requirement return; } - Guid? userId = _authService.GetUserIdFromClaims(context.User); + GetUserResult res = await _userService.GetUserFromClaims(context.User); - if (userId is null) + res.Switch(user => { - context.Fail(); - return; - } - - User? user = _userService.GetUserById(userId.Value).Result; - - if (user is null || !Handle(user, requirement)) - { - // FIXME: Work out how to fail as a ForbiddenResult. - context.Fail(); - return; - } - - context.Succeed(requirement); - - return; + if (Handle(user, requirement)) + { + context.Succeed(requirement); + } + }, badCredentials => context.Fail(new(this, "Bad Credentials")), notFound => context.Fail(new(this, "User Not Found"))); } - private bool Handle(User user, UserTypeRequirement requirement) + private static bool Handle(User user, UserTypeRequirement requirement) => requirement.Type switch { - return requirement.Type switch - { - UserTypes.ADMINISTRATOR => user.IsAdmin, - UserTypes.USER => true, - _ => false, - }; - } + UserTypes.ADMINISTRATOR => user.IsAdmin, + UserTypes.USER => true, + _ => false, + }; private static bool TryGetJwtFromHttpContext( AuthorizationHandlerContext context, [NotNullWhen(true)] out string? token diff --git a/LeaderboardBackend/Controllers/AccountController.cs b/LeaderboardBackend/Controllers/AccountController.cs index 111809e7..9ea54c78 100644 --- a/LeaderboardBackend/Controllers/AccountController.cs +++ b/LeaderboardBackend/Controllers/AccountController.cs @@ -136,6 +136,7 @@ public async Task> Login( ); } + [Authorize] [HttpPost("confirm")] [SwaggerOperation("Resends the account confirmation link.")] [SwaggerResponse(200, "A new confirmation link was generated.")] diff --git a/LeaderboardBackend/Controllers/CategoriesController.cs b/LeaderboardBackend/Controllers/CategoriesController.cs index 7f7e5716..a0ee02a9 100644 --- a/LeaderboardBackend/Controllers/CategoriesController.cs +++ b/LeaderboardBackend/Controllers/CategoriesController.cs @@ -1,3 +1,4 @@ +using LeaderboardBackend.Authorization; using LeaderboardBackend.Models.Entities; using LeaderboardBackend.Models.Requests; using LeaderboardBackend.Models.ViewModels; @@ -24,8 +25,6 @@ public CategoriesController(ICategoryService categoryService) [SwaggerResponse(404)] public async Task> GetCategory(long id) { - // NOTE: Should this use [AllowAnonymous]? - Ero - Category? category = await _categoryService.GetCategory(id); if (category == null) @@ -36,18 +35,16 @@ public async Task> GetCategory(long id) return Ok(CategoryViewModel.MapFrom(category)); } + [Authorize(Policy = UserTypes.ADMINISTRATOR)] [HttpPost] [SwaggerOperation("Creates a new Category. This request is restricted to Moderators.")] [SwaggerResponse(201)] - [SwaggerResponse(404)] + [SwaggerResponse(403)] [SwaggerResponse(422, Type = typeof(ValidationProblemDetails))] public async Task> CreateCategory( [FromBody] CreateCategoryRequest request ) { - // FIXME: Allow only moderators to call this! - Ero - // NOTE: Allow administrators to call this as well? - Ero - // NOTE: Check that body.PlayersMax > body.PlayersMin? - Ero Category category = new() diff --git a/LeaderboardBackend/Controllers/LeaderboardsController.cs b/LeaderboardBackend/Controllers/LeaderboardsController.cs index 2641277e..94e383b7 100644 --- a/LeaderboardBackend/Controllers/LeaderboardsController.cs +++ b/LeaderboardBackend/Controllers/LeaderboardsController.cs @@ -69,7 +69,7 @@ [FromQuery] long[] ids [SwaggerOperation("Creates a new leaderboard. This request is restricted to Administrators.")] [SwaggerResponse(201)] [SwaggerResponse(401)] - [SwaggerResponse(404, "The requesting `User` is unauthorized to create `Leaderboard`s.")] + [SwaggerResponse(403, "The requesting `User` is unauthorized to create `Leaderboard`s.")] [SwaggerResponse(422, Type = typeof(ValidationProblemDetails))] public async Task> CreateLeaderboard( [FromBody] CreateLeaderboardRequest request diff --git a/LeaderboardBackend/Controllers/RunsController.cs b/LeaderboardBackend/Controllers/RunsController.cs index 8270f6a7..9f9a434c 100644 --- a/LeaderboardBackend/Controllers/RunsController.cs +++ b/LeaderboardBackend/Controllers/RunsController.cs @@ -39,6 +39,7 @@ public async Task> GetRun(Guid id) return Ok(RunViewModel.MapFrom(run)); } + [Authorize] [HttpPost] [SwaggerOperation("Creates a new Run.")] [SwaggerResponse(201)] diff --git a/LeaderboardBackend/Controllers/UsersController.cs b/LeaderboardBackend/Controllers/UsersController.cs index 9f126718..52f0a9cb 100644 --- a/LeaderboardBackend/Controllers/UsersController.cs +++ b/LeaderboardBackend/Controllers/UsersController.cs @@ -37,6 +37,7 @@ public async Task> GetUserById( return Ok(UserViewModel.MapFrom(user)); } + [Authorize] [HttpGet("me")] [SwaggerOperation( "Gets the currently logged-in User.", diff --git a/LeaderboardBackend/Program.cs b/LeaderboardBackend/Program.cs index 04148802..ac166ffe 100644 --- a/LeaderboardBackend/Program.cs +++ b/LeaderboardBackend/Program.cs @@ -221,40 +221,26 @@ JsonWebTokenHandler.DefaultInboundClaimTypeMap.Clear(); builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme).AddJwtBearer(); -// Configure authorisation. -builder.Services.AddAuthorization(options => -{ - options.AddPolicy( - UserTypes.ADMINISTRATOR, - policy => +builder.Services.AddAuthorizationBuilder() + .AddPolicy(UserTypes.ADMINISTRATOR, policy => { policy.AuthenticationSchemes.Add(JwtBearerDefaults.AuthenticationScheme); policy.RequireAuthenticatedUser(); policy.Requirements.Add(new UserTypeRequirement(UserTypes.ADMINISTRATOR)); } - ); - options.AddPolicy( - UserTypes.MODERATOR, - policy => +) + .AddPolicy(UserTypes.MODERATOR, policy => { policy.AuthenticationSchemes.Add(JwtBearerDefaults.AuthenticationScheme); policy.RequireAuthenticatedUser(); policy.Requirements.Add(new UserTypeRequirement(UserTypes.MODERATOR)); } - ); - - // Handles empty [Authorize] attributes - options.DefaultPolicy = new AuthorizationPolicyBuilder() +) + .SetDefaultPolicy(new AuthorizationPolicyBuilder() .AddAuthenticationSchemes(new[] { JwtBearerDefaults.AuthenticationScheme }) .RequireAuthenticatedUser() .AddRequirements(new[] { new UserTypeRequirement(UserTypes.USER) }) - .Build(); - - options.FallbackPolicy = new AuthorizationPolicyBuilder() - .AddAuthenticationSchemes(new[] { JwtBearerDefaults.AuthenticationScheme }) - .RequireAuthenticatedUser() - .Build(); -}); + .Build()); builder.Services.AddSingleton(); builder.Services.AddFluentValidationAutoValidation(c => diff --git a/LeaderboardBackend/openapi.json b/LeaderboardBackend/openapi.json index 690a8113..83d7a44f 100644 --- a/LeaderboardBackend/openapi.json +++ b/LeaderboardBackend/openapi.json @@ -437,8 +437,8 @@ } } }, - "404": { - "description": "Not Found" + "403": { + "description": "Forbidden" }, "422": { "description": "Unprocessable Content", @@ -635,7 +635,7 @@ "401": { "description": "Unauthorized" }, - "404": { + "403": { "description": "The requesting `User` is unauthorized to create `Leaderboard`s." }, "422": {