-
Notifications
You must be signed in to change notification settings - Fork 8
Fixing a few issues #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
9741952
8f3054a
c33961e
5fbc307
5679637
ee8bc93
57e174c
7f92ef2
63b789c
a9c554a
0647bde
808e856
21fc159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| using Microsoft.AspNetCore.StaticFiles; | ||
| using System.Text.RegularExpressions; | ||
| using WebDavServer.Application.Contracts.Cache; | ||
| using WebDavServer.Application.Contracts.FileStorage; | ||
| using WebDavServer.Application.Contracts.FileStorage.Enums; | ||
|
|
@@ -354,7 +353,7 @@ private async Task<ErrorType> CreateDirectoryAsync(string path, CancellationToke | |
| { | ||
| var pathInfo = await _pathService.GetDestinationPathInfoAsync(path, cancellationToken); | ||
|
|
||
| if (!Regex.IsMatch(pathInfo.ResourceName, @"^[a-zA-Z0-9_]+$", RegexOptions.Compiled)) | ||
| if (HasInvalidChars(pathInfo.ResourceName)) | ||
| { | ||
| return ErrorType.PartResourcePathNotExists; | ||
| } | ||
|
|
@@ -447,5 +446,9 @@ private string GetContentType(string fileName) | |
|
|
||
| return "text/plain"; | ||
| } | ||
| private bool HasInvalidChars(string directoryName) | ||
| { | ||
| return (!string.IsNullOrEmpty(directoryName) && directoryName.IndexOfAny(Path.GetInvalidPathChars()) >= 0); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove extra parentheses "(" ")" |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,12 +27,12 @@ public async Task<PathInfo> GetDestinationPathInfoAsync(string relativePath, Can | |
|
|
||
| if (directories.Any()) | ||
| { | ||
| var nextDirectory = directories.First(); | ||
| var nextDirectory = directories[0]; | ||
| var otherDirectories = directories.Skip(1).ToList(); | ||
|
|
||
| var directoryInfo = await GetItemAsync(null, string.Empty, nextDirectory, otherDirectories, cancellationToken); | ||
|
|
||
| directory = GetLastChild(directoryInfo); | ||
| directory = GetLastChild(directoryInfo!); | ||
| } | ||
|
|
||
| return new PathInfo | ||
|
|
@@ -50,17 +50,20 @@ public async Task<PathInfo> GetDestinationPathInfoAsync(string relativePath, Can | |
|
|
||
| var directories = relativePathTrim.Split("/").Where(x => !string.IsNullOrEmpty(x)).ToList(); | ||
|
|
||
| var isDirectory = relativePathTrim.EndsWith("/"); | ||
| var isDirectory = relativePathTrim == "/" || !Path.HasExtension(relativePathTrim); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The file name does not necessarily contain extensions (for example, "my_file_name"), you need to check for the presence of a "/" at the end.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All path names, even directories via WebDAV comss through with no "/" at the end of the name, which meant that all directories other than root were seen as files, and took the wrong code-path
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats problem concrete client, by specification required http://www.webdav.org/specs/rfc4918.html#rfc.section.5.2.p.8
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not from what I observed. URL does end with "/" (even files), but path via GetPath doesnt. Might be the way Microsoft implemented WebDAV via the map network drive functionality, which would be strange not to follow RFC (but not unlike Microsoft). Other WebDAV server we are running follows RFC and works with that though. Maybe I am missing something. |
||
|
|
||
| directories.Insert(0, RootDirectory); | ||
|
|
||
| var resourceName = directories.Last(); | ||
| directories.RemoveAt(directories.Count - 1); | ||
| var resourceName = directories[directories.Count - 1]; | ||
| if (!isDirectory) | ||
| { | ||
| directories.RemoveAt(directories.Count - 1); | ||
| } | ||
|
|
||
| return (resourceName, directories, isDirectory); | ||
| } | ||
|
|
||
| private async Task<PathInfo> GetItemAsync( | ||
| private async Task<PathInfo?> GetItemAsync( | ||
| long? parentDirectoryId, | ||
| string relativePath, | ||
| string directoryName, | ||
|
|
@@ -77,14 +80,14 @@ private async Task<PathInfo> GetItemAsync( | |
|
|
||
| if (item == null) | ||
| { | ||
| throw new FileStorageException(ErrorCodes.PartOfPathNotExists); | ||
| return default; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use http://www.webdav.org/neon/litmus/ for testing all changes, especially this place |
||
| } | ||
|
|
||
| var virtualPath = $"{relativePath}/{directoryName}"; | ||
|
|
||
| if (nextDirectories.Any()) | ||
| { | ||
| var nextDirectory = nextDirectories.First(); | ||
| var nextDirectory = nextDirectories[0]; | ||
| var otherDirectories = nextDirectories.Skip(1).ToList(); | ||
|
|
||
| child = await GetItemAsync(item.Id, virtualPath, nextDirectory, otherDirectories, cancellationToken); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,12 +87,12 @@ public async Task<List<Item>> GetDirectoryInfoAsync(PathInfo pathInfo, bool with | |
| var directory = await _dbContext.Set<Item>() | ||
| .Where(x => x.IsDirectory) | ||
| .Where(x => x.Title == pathInfo.ResourceName) | ||
| .Where(x => x.DirectoryId == directoryId) | ||
| .FirstAsync(cancellationToken); | ||
| var result = new List<Item> { directory }; | ||
| .Where(x => x.Id == directoryId) | ||
| .FirstOrDefaultAsync(cancellationToken); | ||
|
|
||
| var result = directory != null ? new List<Item> { directory } : new List<Item>(); | ||
|
|
||
| if (withContent) | ||
| if (directory is not null && withContent) | ||
| { | ||
| var contents = await GetDirectoryAsync(directory.Id, cancellationToken); | ||
| result.AddRange(contents); | ||
|
|
@@ -140,7 +140,7 @@ public async Task MoveDirectoryAsync(PathInfo srcPath, PathInfo dstPath, Cancell | |
| var item = await _dbContext.Set<Item>() | ||
| .Where(x => x.IsDirectory) | ||
| .Where(x => x.Title == srcPath.ResourceName) | ||
| .Where(x => x.DirectoryId == directoryId) | ||
| .Where(x => x.Id == directoryId) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in this filter, the item is searched by the parent directory in which it should be located and the name of the items, and not by its id. If we had an id here, it would be enough to do Find(id)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my observations, in sub-directories, that is not in root, this results in not finding any information from the database. Works fine if all directories will always be in root |
||
| .FirstOrDefaultAsync(cancellationToken); | ||
|
|
||
| if (item is null) | ||
|
|
@@ -150,6 +150,7 @@ public async Task MoveDirectoryAsync(PathInfo srcPath, PathInfo dstPath, Cancell | |
|
|
||
| item.DirectoryId = dstPath.Directory.Id; | ||
| item.Title = dstPath.ResourceName; | ||
| item.Name = dstPath.ResourceName; | ||
|
|
||
| await _dbContext.SaveChangesAsync(cancellationToken); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,8 +48,17 @@ public async Task GetAsync(string? path, CancellationToken cancellationToken = d | |
|
|
||
| [ApiExplorerSettings(IgnoreApi = true)] | ||
| [AcceptVerbs("PROPFIND")] | ||
| public async Task<string> PropfindAsync(string? path, CancellationToken cancellationToken) | ||
| public async Task<IActionResult> PropfindAsync(string? path, CancellationToken cancellationToken) | ||
| { | ||
| if (path is not null && (path.Contains("desktop.ini") || | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do these files need to be hidden?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Annoying windows explorer-only stuff that gets checked on a file system that would never contain them *shrug :)
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Place the list of files in a separate class, something like this |
||
| path.Contains("folder.gif") || | ||
| path.Contains("folder.jpg") || | ||
| path.Contains("thumbs.db"))) | ||
| { | ||
|
|
||
| return StatusCode((int)HttpStatusCode.NotFound); | ||
| } | ||
|
|
||
| var returnXml = await _webDavService.PropfindAsync(new PropfindRequest | ||
| { | ||
| Url = $"{Request.GetDisplayUrl().TrimEnd('/')}/", | ||
|
|
@@ -59,7 +68,7 @@ public async Task<string> PropfindAsync(string? path, CancellationToken cancella | |
|
|
||
| Response.StatusCode = (int)HttpStatusCode.MultiStatus; | ||
|
|
||
| return returnXml; | ||
| return Content(returnXml, "application/xml", Encoding.UTF8); | ||
| } | ||
|
|
||
| [HttpHead] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why it should be static? Im trying to learn clean architecture as I go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not about
clean architecture, but aboutclean code. Since the method does not use class members, it should be defined as statichttps://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/static#example---static-field-and-method