Add ModelState validation checks for all controller actions

Introduced validation checks for `ModelState.IsValid` at the beginning of controller actions. If the model state is invalid, the actions now return appropriate HTTP responses such as `BadRequest()`, `NotFound()`, `Forbid()`, `NoContent()`, or redirect to specific views or actions. This enhances the robustness and security of the application by ensuring only valid data is processed.
axuno · Oct 8, 2024 · 732f969 · 732f969
1 parent 4d34af8
commit 732f969
Show file tree

Hide file tree

Showing 13 changed files with 92 additions and 8 deletions.
diff --git a/League.Demo/Controllers/TenantContent.cs b/League.Demo/Controllers/TenantContent.cs
@@ -34,6 +34,8 @@ public TenantContent(ICompositeViewEngine viewEngine, IActionContextAccessor act
     [HttpGet]
     public IActionResult Index(string category = "home", string topic = "index")
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         // Note: Indicate the current controller-specific tenant directory with the "./" prefix
         var view = $"./{_tenantContext.SiteContext.FolderName}/{category}_{topic}";
         var result = _viewEngine.FindView(_actionContext, view, false);

diff --git a/League/Areas/Admin/Controllers/Impersonation.cs b/League/Areas/Admin/Controllers/Impersonation.cs
@@ -30,6 +30,8 @@ public Impersonation(SignInManager<ApplicationUser> signInManager, ITenantContex
     [HttpGet("")]
     public async Task<IActionResult> Index(string search, int limit = 20)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         limit = Math.Abs(limit);
         var users = new List<UserEntity>();
         if (!string.IsNullOrWhiteSpace(search))
@@ -49,6 +51,8 @@ public async Task<IActionResult> Index(string search, int limit = 20)
     [HttpGet("[action]/{id}")]
     public async Task<IActionResult> Start(long id)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         var currentUser = User;
         var targetUser = await _signInManager.UserManager.FindByIdAsync(id.ToString());
 

diff --git a/League/Controllers/Account.cs b/League/Controllers/Account.cs
@@ -114,6 +114,8 @@ public Account(
     [AllowAnonymous]
     public IActionResult SignIn(string? returnUrl = null)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         ViewData[ReturnUrl] = returnUrl;
         return View();
     }
@@ -181,8 +183,10 @@ public async Task<IActionResult> SignIn(SignInViewModel model, string? returnUrl
     [AllowAnonymous]
     public IActionResult CreateAccount(string? returnUrl = null)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         ViewData[ReturnUrl] = returnUrl;
-        return View();
+        return View(); // return the view to create a new account
     }
 
     [HttpPost("create")]
@@ -216,6 +220,8 @@ public async Task<IActionResult> CreateAccount(CreateAccountViewModel model, str
     [AllowAnonymous]
     public IActionResult Register(string? code = null, string? returnUrl = null)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         if (!_dataProtector.TryDecrypt(code?.Base64UrlDecode() ?? string.Empty, out var email, out var expiration))
         {
             return Redirect(TenantLink.Action(nameof(Message), nameof(Account), new { messageTypeText = MessageType.ConfirmEmailError })!);
@@ -295,6 +301,8 @@ public async Task<IActionResult> Register(RegisterViewModel model, CancellationT
     [ValidateAntiForgeryToken]
     public IActionResult ExternalSignIn(string provider, string? returnUrl = null)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         // Request a redirect to the external login provider.
         var redirectUrl = TenantLink.Action(nameof(ExternalSignInCallback), nameof(Account), new { ReturnUrl = returnUrl });
         var properties = _signInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl);
@@ -306,7 +314,7 @@ public IActionResult ExternalSignIn(string provider, string? returnUrl = null)
     [AllowAnonymous]
     public async Task<IActionResult> ExternalSignInCallback(string? returnUrl = null, string? remoteError = null)
     {
-        if (remoteError != null)
+        if (remoteError != null || !ModelState.IsValid)
         {
             _logger.LogInformation("{Method} failed. Remote error was supplied.", nameof(ExternalSignInCallback));
             return SocialMediaSignInFailure(_localizer["Failed to sign-in with the social network account"].Value + $" ({remoteError})");
@@ -447,6 +455,8 @@ public async Task<IActionResult> ForgotPassword(ForgotPasswordViewModel model)
     [AllowAnonymous]
     public IActionResult ResetPassword(string? code = null)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         var model = new ResetPasswordViewModel {Code = code};
         if (code == null)
         {
@@ -460,7 +470,7 @@ public IActionResult ResetPassword(string? code = null)
     [ValidateAntiForgeryToken]
     public async Task<IActionResult> ResetPassword(ResetPasswordViewModel model)
     {
-        if (model.Code == null)
+        if (!ModelState.IsValid || model.Code is null)
         {
             return Redirect(TenantLink.Action(nameof(ResetPassword), nameof(Account))!);
         }
@@ -510,6 +520,8 @@ public async Task<IActionResult> ResetPassword(ResetPasswordViewModel model)
     [AllowAnonymous]
     public IActionResult Message(string messageTypeText)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         if (Enum.TryParse(messageTypeText, true, out MessageType messageType))
         {
             switch (messageType)

diff --git a/League/Controllers/Error.cs b/League/Controllers/Error.cs
@@ -24,6 +24,8 @@ public Error(ILogger<Error> logger, IStringLocalizer<Error> localizer, ILoggerFa
     [HttpGet]
     public IActionResult Index(string? id)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         ViewBag.TitleTagText = $"{_tenantContext.OrganizationContext.ShortName} - {_localizer["Error"]}";
         id ??= string.Empty;
         id = id.Trim();
@@ -64,6 +66,8 @@ public IActionResult Index(string? id)
     [HttpGet]
     public IActionResult AccessDenied(string returnUrl)
     {
+        if (!ModelState.IsValid) return View(Views.ViewNames.Error.AccessDenied); ;
+
         ViewData["ReturnUrl"] = returnUrl;
         return View(Views.ViewNames.Error.AccessDenied);
     }

diff --git a/League/Controllers/Language.cs b/League/Controllers/Language.cs
@@ -32,6 +32,8 @@ public Language(IOptions<RequestLocalizationOptions> requestLocalizationOptions,
     [HttpGet("")]
     public IActionResult Index(string? culture, string? uiCulture, string? returnUrl)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         returnUrl ??= "/";
         // QueryStringRequestCultureProvider processes arguments and requires one of both
         if (culture == null && uiCulture == null)

diff --git a/League/Controllers/Manage.cs b/League/Controllers/Manage.cs
@@ -189,6 +189,8 @@ public async Task<IActionResult> ChangeEmail(ChangeEmailViewModel model)
     [HttpGet("[action]/{id}")]
     public async Task<IActionResult> ConfirmNewPrimaryEmail(string id, string code, string e)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         var user = await _userManager.FindByIdAsync(id);
         if (user == null)
         {
@@ -609,7 +611,7 @@ public async Task<IActionResult> ManageLogins()
     public async Task<IActionResult> RemoveLogin(RemoveLoginViewModel account)
     {
         var user = await GetCurrentUserAsync();
-        if (user != null)
+        if (ModelState.IsValid && user != null)
         {
             var result = await _userManager.RemoveLoginAsync(user, account.LoginProvider, account.ProviderKey);
             if (result.Succeeded)
@@ -629,6 +631,8 @@ public async Task<IActionResult> RemoveLogin(RemoveLoginViewModel account)
     [ValidateAntiForgeryToken]
     public IActionResult LinkLogin(string provider)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         // Request a redirect to the external login provider to link a login for the current user
         var redirectUrl = TenantLink.Action(nameof(LinkLoginCallback), nameof(Manage));
         var properties = _signInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl, _userManager.GetUserId(User));
@@ -639,7 +643,7 @@ public IActionResult LinkLogin(string provider)
     public async Task<ActionResult> LinkLoginCallback()
     {
         var user = await GetCurrentUserAsync();
-        if (user == null)
+        if (!ModelState.IsValid || user == null)
         {
             TempData.Put<ManageMessage>(nameof(ManageMessage), new ManageMessage { AlertType = SiteAlertTagHelper.AlertType.Danger, MessageId = MessageId.AddLoginFailure });
             return Redirect(GeneralLink.GetUriByAction(HttpContext, nameof(Index))!);

diff --git a/League/Controllers/Map.cs b/League/Controllers/Map.cs
@@ -48,6 +48,8 @@ public async Task<IActionResult> Index(CancellationToken cancellationToken)
     [HttpGet]
     public async Task<IActionResult> Venue(long id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         var venue = (await _appDb.VenueRepository.GetVenueTeamRowsAsync(
             new PredicateExpression(VenueTeamFields.VenueId == id & VenueTeamFields.TournamentId == _tenantContext.TournamentContext.MapTournamentId), cancellationToken)).FirstOrDefault();
 

diff --git a/League/Controllers/Match.cs b/League/Controllers/Match.cs
@@ -119,6 +119,8 @@ public async Task<IActionResult> Fixtures(CancellationToken cancellationToken)
     [HttpGet("[action]")]
     public async Task<IActionResult> Calendar(long id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NoContent();
+
         var matches = await _appDb.MatchRepository.GetMatchCalendarAsync(_tenantContext.TournamentContext.MatchPlanTournamentId, id, null, null, cancellationToken);
         if (matches.Count != 1) return NoContent();
 
@@ -163,6 +165,8 @@ public async Task<IActionResult> Calendar(long id, CancellationToken cancellatio
     [HttpGet("[action]")]
     public async Task<IActionResult> PublicCalendar(long? team, long? round, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NoContent();
+
         var matches = await _appDb.MatchRepository.GetMatchCalendarAsync(_tenantContext.TournamentContext.MatchPlanTournamentId, null, team, round, cancellationToken);
         if (matches.Count == 0) return NoContent();
 
@@ -241,6 +245,8 @@ public async Task<IActionResult> Results(CancellationToken cancellationToken)
     [HttpGet("overrule-result/{id:long}")]
     public async Task<IActionResult> OverruleResult(long id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return Forbid();
+
         var match = await _appDb.MatchRepository.GetMatchWithSetsAsync(id, cancellationToken);
         if (!(await _authorizationService.AuthorizeAsync(User, match, Authorization.MatchOperations.OverruleResult)).Succeeded)
         {
@@ -261,6 +267,8 @@ public async Task<IActionResult> OverruleResult(long id, CancellationToken cance
     [HttpGet("enter-result/{id:long}")]
     public async Task<IActionResult> EnterResult(long id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return Forbid();
+
         try
         {
             var match = await _appDb.MatchRepository.GetMatchWithSetsAsync(id, cancellationToken);
@@ -322,6 +330,8 @@ public async Task<IActionResult> EnterResult(long id, CancellationToken cancella
     [ValidateAntiForgeryToken]
     public async Task<IActionResult> EnterResult([FromForm] EnterResultViewModel? model, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         MatchEntity? match;
         try
         {
@@ -427,8 +437,7 @@ public async Task<IActionResult> EnterResult([FromForm] EnterResultViewModel? mo
     public async Task<IActionResult> RemoveResult([FromForm] EnterResultViewModel? model,
         CancellationToken cancellationToken)
     {
-        ArgumentNullException.ThrowIfNull(model);
-        ArgumentNullException.ThrowIfNull(model.Id);
+        if (!ModelState.IsValid || model?.Id is null) return NotFound();
 
         try
         {
@@ -467,6 +476,8 @@ public async Task<IActionResult> RemoveResult([FromForm] EnterResultViewModel? m
     [HttpGet("edit-fixture/{id:long}")]
     public async Task<IActionResult> EditFixture(long id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         var model = new EditFixtureViewModel(await GetPlannedMatchFromDatabase(id, cancellationToken), _timeZoneConverter)
         {
             Tournament = await GetPlanTournament(cancellationToken)
@@ -506,6 +517,8 @@ public async Task<IActionResult> EditFixture([FromForm] EditFixtureViewModel mod
         // [FromBody] => 'content-type': 'application/json'
         // [FromForm] => 'content-type': 'application/x-www-form-urlencoded'
 
+        if (!ModelState.IsValid) return NotFound();
+
         model = new EditFixtureViewModel(await GetPlannedMatchFromDatabase(model.Id, cancellationToken), _timeZoneConverter)
         {
             Tournament = await GetPlanTournament(cancellationToken)

diff --git a/League/Controllers/Ranking.cs b/League/Controllers/Ranking.cs
@@ -79,6 +79,8 @@ await _appDb.TournamentRepository.GetTournamentAsync(new PredicateExpression(Tou
     [HttpGet("all-time/tournament/{id?}")]
     public async Task<IActionResult> AllTimeTournament(long?id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) id = null;
+
         try
         {
             var rankingList = await GetRankingListCached(cancellationToken);
@@ -101,6 +103,8 @@ public async Task<IActionResult> AllTimeTournament(long?id, CancellationToken ca
     [HttpGet("all-time/team/{id?}")]
     public async Task<IActionResult> AllTimeTeam(long? id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) id = null;
+
         try
         {
             var rankingList = await GetRankingListCached(cancellationToken);

diff --git a/League/Controllers/Role.cs b/League/Controllers/Role.cs
@@ -37,6 +37,8 @@ public Role(ITenantContext tenantContext,
     public async Task<IActionResult> Remove(string roleName, long uid, long tid, string un,
         string returnUrl, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return BadRequest();
+
         var model = new RoleRemoveModel
         {
             TeamId = tid,
@@ -62,6 +64,9 @@ public async Task<IActionResult> Remove(string roleName, long uid, long tid, str
     [ValidateAntiForgeryToken]
     public async Task<IActionResult> Remove([FromForm] RoleRemoveModel model, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return JsonResponseRedirect(Url.Action(nameof(Error.AccessDenied), nameof(Error),
+            new { _defaultReturnUrl }));
+
         model.ClaimType = Identity.Constants.ClaimType.ManagesTeam.ToLowerInvariant() ==
                           model.ClaimType?.ToLowerInvariant()
             ? Identity.Constants.ClaimType.ManagesTeam
@@ -106,6 +111,9 @@ public async Task<IActionResult> Remove([FromForm] RoleRemoveModel model, Cancel
     [HttpGet("[action]/{tid:long}")]
     public async Task<IActionResult> Add(long tid, string returnUrl, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return JsonResponseRedirect(Url.Action(nameof(Error.AccessDenied), nameof(Error),
+            new { _defaultReturnUrl }));
+
         var model = new RoleAddModel
         {
             TeamId = tid,

diff --git a/League/Controllers/Team.cs b/League/Controllers/Team.cs
@@ -69,6 +69,8 @@ public async Task<IActionResult> List(CancellationToken cancellationToken)
     [HttpGet("my-team/{id:long?}")]
     public async Task<IActionResult> MyTeam(long? id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return Redirect(TenantLink.Action(nameof(MyTeam), nameof(Team), new { id = default(long?) })!);
+
         // make sure that any changes are immediately reflected in the user's application cookie
         var appUser = await _signInManager.UserManager.GetUserAsync(User);
         if (appUser != null)
@@ -126,6 +128,8 @@ public async Task<IActionResult> MyTeam(long? id, CancellationToken cancellation
     [HttpGet("{id:long}")]
     public async Task<IActionResult> Single(long id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         var model = new TeamSingleModel(_timeZoneConverter)
         {
             Tournament = await _appDb.TournamentRepository.GetTournamentAsync(
@@ -154,6 +158,8 @@ public async Task<IActionResult> Single(long id, CancellationToken cancellationT
     [HttpGet("[action]/{teamId:long}")]
     public async Task<IActionResult> Edit(long teamId, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         var team = await _appDb.TeamRepository.GetTeamEntityAsync(new PredicateExpression(TeamFields.Id == teamId), cancellationToken);
         if (team == null)
         {
@@ -180,8 +186,10 @@ public async Task<IActionResult> Edit(long teamId, CancellationToken cancellatio
     [ValidateAntiForgeryToken]
     public async Task<IActionResult> Edit([FromForm] TeamEditModel model, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         TeamEntity? team = null;
-        if (model.Team != null && !model.Team.IsNew)
+        if (model.Team is { IsNew: false })
         {
             team = await _appDb.TeamRepository.GetTeamEntityAsync(new PredicateExpression(TeamFields.Id == model.Team.Id), cancellationToken);
             if (team == null)
@@ -259,6 +267,8 @@ public async Task<IActionResult> Edit([FromForm] TeamEditModel model, Cancellati
     [HttpGet("[action]/{tid}")]
     public async Task<IActionResult> ChangeVenue(long tid, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         var teamEntity = await
             _appDb.TeamRepository.GetTeamEntityAsync(new PredicateExpression(TeamFields.Id == tid),
                 cancellationToken);
@@ -273,6 +283,8 @@ public async Task<IActionResult> ChangeVenue(long tid, CancellationToken cancell
     [HttpGet("[action]/{tid}")]
     public async Task<IActionResult> SelectVenue(long tid, string returnUrl, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         var teamEntity = await
             _appDb.TeamRepository.GetTeamEntityAsync(new PredicateExpression(TeamFields.Id == tid),
                 cancellationToken);
@@ -305,6 +317,9 @@ public async Task<IActionResult> SelectVenue([FromForm][Bind("TeamId, VenueId")]
     {
         // model binding is not case-sensitive
 
+        if (!ModelState.IsValid) return JsonResponseRedirect(Url.Action(nameof(Error.AccessDenied), nameof(Error),
+            new { ReturnUrl = TenantLink.Action(nameof(MyTeam), nameof(Team)) }));
+
         var teamEntity = await
             _appDb.TeamRepository.GetTeamEntityAsync(new PredicateExpression(TeamFields.Id == model.TeamId),
                 cancellationToken);

diff --git a/League/Controllers/Upload.cs b/League/Controllers/Upload.cs
@@ -35,6 +35,8 @@ public Upload(ITenantContext tenantContext, IWebHostEnvironment webHostingEnviro
     [HttpGet("team-photo/{id:long}")]
     public async Task<IActionResult> TeamPhoto(long id, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return NotFound();
+
         if (!(await _authorizationService.AuthorizeAsync(User, new TeamEntity(id),
                 Authorization.TeamOperations.ChangePhoto)).Succeeded)
         {
@@ -78,6 +80,8 @@ public async Task<IActionResult> TeamPhoto(long id, CancellationToken cancellati
     [ValidateAntiForgeryToken]
     public async Task<IActionResult> TeamPhoto([FromForm] IFormFile file, [FromForm] long teamId, CancellationToken cancellationToken)
     {
+        if (!ModelState.IsValid) return Forbid();
+
         if (!(await _authorizationService.AuthorizeAsync(User, new TeamEntity(teamId),
                 Authorization.TeamOperations.ChangePhoto)).Succeeded)
         {
@@ -148,6 +152,8 @@ await _tenantContext.DbContext.AppDb.TeamRepository.GetTeamEntityAsync(
     [HttpGet("remove-team-photo/{id:long}")]
     public async Task<IActionResult> RemoveTeamPhoto(long id)
     {
+        if (!ModelState.IsValid) return Forbid();
+
         if (!(await _authorizationService.AuthorizeAsync(User, new TeamEntity(id),
                 Authorization.TeamOperations.ChangePhoto)).Succeeded)
         {