devdocs/todo.txt

@@ -10,7 +10,13 @@ todo: check attachment NOTES property is actually supported
 
 //todo: search tables in schema, I think there is a missing index here, need to look at the search query section again as it was changed several times from the original schema creation
 
-
+todo: log failed
+	- Download attempts with wrong key
+		- Add delay for failed download
+	- authentication attempts (or would this cause a possible DOS?)
+todo: some kind of ops specific event log or system that triggers alerts to ops people, I guess notifications would be the word I'm looking for...
+	- Once that exists a lot of backfilling needs to be done for example attachment files missing notification etc etc etc
+	
 todo: can a user be locked out from the server end even though they posess a valid token?
 	- and prevent download of images etc?
 

server/AyaNova/Controllers/AttachmentController.cs

@@ -36,7 +36,6 @@ namespace AyaNova.Api.Controllers
     [ApiVersion("8.0")]
     [Route("api/v{version:apiVersion}/[controller]")]
     [Produces("application/json")]
-    [Authorize]
     public class AttachmentController : ControllerBase
     {
         private readonly AyContext ct;
@@ -118,6 +117,7 @@ namespace AyaNova.Api.Controllers
         /// 
         /// </summary>
         /// <returns>NameValue list of filenames and attachment id's</returns>
+        [Authorize]
         [HttpPost]
         [DisableFormValueModelBinding]
         [RequestSizeLimit(10737418241)]//10737418240 = 10gb  https://github.com/aspnet/Announcements/issues/267
@@ -269,6 +269,7 @@ namespace AyaNova.Api.Controllers
         /// </summary>
         /// <param name="id"></param>
         /// <returns>Ok</returns>
+        [Authorize]
         [HttpDelete("{id}")]
         public async Task<IActionResult> DeleteAttachmentAsync([FromRoute] long id)
         {
@@ -313,11 +314,14 @@ namespace AyaNova.Api.Controllers
         /// Download a file attachment
         /// </summary>
         /// <param name="id"></param>
-        /// <param name="dlkey"></param>
+        /// <param name="t">download token</param>
         /// <returns></returns>
         [HttpGet("download/{id}")]
-        public async Task<IActionResult> DownloadAsync([FromRoute] long id, [FromQuery] string dlkey)
+        public async Task<IActionResult> DownloadAsync([FromRoute] long id, [FromQuery] string t)
         {
+            int nFailedAuthDelay = 3000;//should be just long enough to make brute force a hassle but short enough to not annoy people who just mistyped their creds to login
+
+            //NOTE this is the only unauthorized route as it needs to work with wiki url links and relies on the dlkey to work
             //copied from Rockfish
             //https://dotnetcoretutorials.com/2017/03/12/uploading-files-asp-net-core/
             //https://stackoverflow.com/questions/45763149/asp-net-core-jwt-in-uri-query-parameter/45811270#45811270
@@ -325,47 +329,57 @@ namespace AyaNova.Api.Controllers
                 return StatusCode(503, new ApiErrorResponse(serverState.ApiErrorCode, null, serverState.Reason));
 
 
-            if (string.IsNullOrWhiteSpace(dlkey))
+            //NOTE: this is a potentially dangerous route since it's not Authorized so we need to treat it like Auth route and not leak any 
+            //useful information to bad actors and also ensure a delay to avoid brute force or DOS attacks
+
+            if (string.IsNullOrWhiteSpace(t))
             {
-                return NotFound();
+                await Task.Delay(nFailedAuthDelay);//DOS protection
+                return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
             }
 
+
+
+
             //get user by key, if not found then reject
             //If user dlkeyexp has not expired then return file
-            var dlkeyUser = await ct.User.SingleOrDefaultAsync(m => m.DlKey == dlkey);
+            var dlkeyUser = await ct.User.SingleOrDefaultAsync(m => m.DlKey == t && m.Active == true);
             if (dlkeyUser == null)
             {
-                //don't want to leak information so just say not found
-                //return BadRequest(new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, "dlkey", "Download token not valid"));
+                await Task.Delay(nFailedAuthDelay);//DOS protection
                 return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
             }
 
             //Make sure the token provided is for the current user
+            //bugbug: Is this not a bug, there's no way to set this properly is there?
             long UserId = UserIdFromContext.Id(HttpContext.Items);
             if (UserId != dlkeyUser.Id)
             {
-               // return BadRequest(new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, "dlkey", "Download token not valid"));
-               return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
+                await Task.Delay(nFailedAuthDelay);//DOS protection
+                return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
             }
 
 
             var utcNow = new DateTimeOffset(DateTime.Now.ToUniversalTime(), TimeSpan.Zero);
             if (dlkeyUser.DlKeyExpire < utcNow.DateTime)
             {
-               // return BadRequest(new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, "dlkey", "Download token has expired"));
-               return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
+
+                await Task.Delay(nFailedAuthDelay);//DOS protection
+                return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
             }
 
             //Ok, user has a valid download key and it's not expired yet so get the attachment record 
             var dbObj = await ct.FileAttachment.SingleOrDefaultAsync(m => m.Id == id);
             if (dbObj == null)
             {
+                await Task.Delay(nFailedAuthDelay);//fishing protection
                 return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND));
             }
 
             //is this allowed?
             if (!Authorized.HasReadFullRole(HttpContext.Items, dbObj.AttachToObjectType))
             {
+                await Task.Delay(nFailedAuthDelay);//DOS protection
                 return StatusCode(403, new ApiNotAuthorizedResponse());
             }
 
@@ -376,7 +390,11 @@ namespace AyaNova.Api.Controllers
             {
                 //TODO: this should trigger some kind of notification to the ops people
                 //and a red light on the dashboard
-                return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND, null, $"Physical file {dbObj.StoredFileName} not found despite attachment record, this file is missing"));
+
+                var errText = $"Physical file {dbObj.StoredFileName} not found despite attachment record, this file is missing";
+                log.LogError(errText);
+
+                return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND, null, errText));
             }
 
             //Log

server/AyaNova/Controllers/AuthController.cs

@@ -177,16 +177,6 @@ namespace AyaNova.Api.Controllers
                     }
 
 
-                    // //If the user is inactive they may not login
-                    // if (!u.Active)
-                    // {
-                    //     //This is leaking information, instead just act like bad creds
-                    //     //return StatusCode(401, new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, null, "User deactivated"));
-                    //     return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
-                    // }
-
-
-
                     //build the key (JWT set in startup.cs)
                     byte[] secretKey = System.Text.Encoding.ASCII.GetBytes(ServerBootConfig.AYANOVA_JWT_SECRET);