Skip to content

Commit 269fb53

Browse files
pauld-msftpauld-msft
authored
Handle unauthorized files / dirs for cleanup (#1328)
* handle unauthorized files / dirs for cleanup * add comments and unit tests
1 parent 91378b6 commit 269fb53

File tree

2 files changed

+108
-56
lines changed

2 files changed

+108
-56
lines changed

‎src/Microsoft.ComponentDetection.Contracts/FileComponentDetectorWithCleanup.cs

+69-54
Original file line numberDiff line numberDiff line change
@@ -56,80 +56,80 @@ protected async Task WithCleanupAsync(
5656
// determining the files that exist as there will be no subsequent cleanup process.
5757
if (this.FileUtilityService == null
5858
|| this.DirectoryUtilityService == null
59-
|| !this.TryGetCleanupFileDirectory(processRequest, out var fileParentDirectory))
59+
|| !this.TryGetCleanupFileDirectory(processRequest, out var fileParentDirectory)
60+
|| !cleanupCreatedFiles)
6061
{
6162
await process(processRequest, detectorArgs, cancellationToken).ConfigureAwait(false);
6263
return;
6364
}
6465

6566
// Get the files and directories that match the cleanup pattern and exist before the process runs.
66-
var (preExistingFiles, preExistingDirs) = this.DirectoryUtilityService.GetFilesAndDirectories(fileParentDirectory, this.CleanupPatterns, DefaultCleanDepth);
67-
try
67+
var (preSuccess, preExistingFiles, preExistingDirs) = this.TryGetFilesAndDirectories(fileParentDirectory, this.CleanupPatterns, DefaultCleanDepth);
68+
69+
await process(processRequest, detectorArgs, cancellationToken).ConfigureAwait(false);
70+
if (!preSuccess)
6871
{
69-
await process(processRequest, detectorArgs, cancellationToken).ConfigureAwait(false);
72+
// return early if we failed to get the pre-existing files and directories, since no need for cleanup
73+
return;
7074
}
71-
finally
75+
76+
// Ensure that only one cleanup process is running at a time, helping to prevent conflicts
77+
await this.cleanupSemaphore.WaitAsync(cancellationToken).ConfigureAwait(false);
78+
79+
try
7280
{
73-
// Ensure that only one cleanup process is running at a time, helping to prevent conflicts
74-
await this.cleanupSemaphore.WaitAsync(cancellationToken).ConfigureAwait(false);
81+
// Clean up any new files or directories created during the scan that match the clean up patterns.
82+
var (postSuccess, latestFiles, latestDirs) = this.TryGetFilesAndDirectories(fileParentDirectory, this.CleanupPatterns, DefaultCleanDepth);
83+
if (!postSuccess)
84+
{
85+
// return early if we failed to get the latest files and directories, since we will not be able
86+
// to determine what to clean up
87+
return;
88+
}
7589

76-
try
90+
var createdFiles = latestFiles.Except(preExistingFiles).ToList();
91+
var createdDirs = latestDirs.Except(preExistingDirs).ToList();
92+
93+
foreach (var createdDir in createdDirs)
7794
{
78-
// Clean up any new files or directories created during the scan that match the clean up patterns.
79-
// If the cleanupCreatedFiles flag is set to false, this will be a dry run and will just log the files that it would clean.
80-
var dryRun = !cleanupCreatedFiles;
81-
var dryRunStr = dryRun ? "[DRYRUN] " : string.Empty;
82-
var (latestFiles, latestDirs) = this.DirectoryUtilityService.GetFilesAndDirectories(fileParentDirectory, this.CleanupPatterns, DefaultCleanDepth);
83-
var createdFiles = latestFiles.Except(preExistingFiles).ToList();
84-
var createdDirs = latestDirs.Except(preExistingDirs).ToList();
85-
86-
foreach (var createdDir in createdDirs)
95+
if (createdDir is null || !this.DirectoryUtilityService.Exists(createdDir))
8796
{
88-
if (createdDir is null || !this.DirectoryUtilityService.Exists(createdDir))
89-
{
90-
continue;
91-
}
92-
93-
try
94-
{
95-
this.Logger.LogDebug("{DryRun}Cleaning up directory {Dir}", dryRunStr, createdDir);
96-
if (!dryRun)
97-
{
98-
this.DirectoryUtilityService.Delete(createdDir, true);
99-
}
100-
}
101-
catch (Exception e)
102-
{
103-
this.Logger.LogDebug(e, "{DryRun}Failed to delete directory {Dir}", dryRunStr, createdDir);
104-
}
97+
continue;
10598
}
10699

107-
foreach (var createdFile in createdFiles)
100+
try
101+
{
102+
this.Logger.LogDebug("Cleaning up directory {Dir}", createdDir);
103+
this.DirectoryUtilityService.Delete(createdDir, true);
104+
}
105+
catch (Exception e)
108106
{
109-
if (createdFile is null || !this.FileUtilityService.Exists(createdFile))
110-
{
111-
continue;
112-
}
113-
114-
try
115-
{
116-
this.Logger.LogDebug("{DryRun}Cleaning up file {File}", dryRunStr, createdFile);
117-
if (!dryRun)
118-
{
119-
this.FileUtilityService.Delete(createdFile);
120-
}
121-
}
122-
catch (Exception e)
123-
{
124-
this.Logger.LogDebug(e, "{DryRun}Failed to delete file {File}", dryRunStr, createdFile);
125-
}
107+
this.Logger.LogDebug(e, "Failed to delete directory {Dir}", createdDir);
126108
}
127109
}
128-
finally
110+
111+
foreach (var createdFile in createdFiles)
129112
{
130-
_ = this.cleanupSemaphore.Release();
113+
if (createdFile is null || !this.FileUtilityService.Exists(createdFile))
114+
{
115+
continue;
116+
}
117+
118+
try
119+
{
120+
this.Logger.LogDebug("Cleaning up file {File}", createdFile);
121+
this.FileUtilityService.Delete(createdFile);
122+
}
123+
catch (Exception e)
124+
{
125+
this.Logger.LogDebug(e, "Failed to delete file {File}", createdFile);
126+
}
131127
}
132128
}
129+
finally
130+
{
131+
_ = this.cleanupSemaphore.Release();
132+
}
133133
}
134134

135135
protected override async Task OnFileFoundAsync(ProcessRequest processRequest, IDictionary<string, string> detectorArgs, bool cleanupCreatedFiles, CancellationToken cancellationToken = default) =>
@@ -151,4 +151,19 @@ private bool TryGetCleanupFileDirectory(ProcessRequest processRequest, out strin
151151

152152
return false;
153153
}
154+
155+
private (bool Success, HashSet<string> Files, HashSet<string> Directories) TryGetFilesAndDirectories(string root, IList<string> patterns, int depth)
156+
{
157+
try
158+
{
159+
var (files, directories) = this.DirectoryUtilityService.GetFilesAndDirectories(root, patterns, depth);
160+
return (true, files, directories);
161+
}
162+
catch (UnauthorizedAccessException e)
163+
{
164+
// log and return false if we are unauthorized to get files and directories
165+
this.Logger.LogDebug(e, "Unauthorized to get files and directories for {Root}", root);
166+
return (false, new HashSet<string>(), new HashSet<string>());
167+
}
168+
}
154169
}

‎test/Microsoft.ComponentDetection.Contracts.Tests/FileComponentDetectorWithCleanupTests.cs

+39-2
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,6 @@ public async Task WithCleanupAsync_CleansUpCreatedFile()
111111
var fileComponentDetector = new TestFileComponentDetector(["*.pyc"], this.loggerMock.Object, fileUtilityService, directoryUtilityService);
112112
var createdFilePath = Path.Combine(this.testDirectory, "todelete.pyc");
113113

114-
// Add files/dirs to the mock file system
115-
116114
// Act
117115
await fileComponentDetector.TestCleanupAsync(
118116
(process, args, token) =>
@@ -198,6 +196,11 @@ await fileComponentDetector.TestCleanupAsync(
198196
fileUtilityService.Exists(this.existingFilePath).Should().BeTrue();
199197
directoryUtilityService.Exists(createdDirectory).Should().BeTrue();
200198
fileUtilityService.Exists(createdFilePath).Should().BeTrue();
199+
200+
// Assert we don't even try to read items
201+
this.directoryUtilityServiceMock.Verify(
202+
d => d.GetFilesAndDirectories(It.IsAny<string>(), It.IsAny<List<string>>(), It.IsAny<int>()),
203+
Times.Never);
201204
}
202205

203206
[TestMethod]
@@ -292,6 +295,40 @@ await fileComponentDetector.TestCleanupAsync(
292295
fileUtilityService.Exists(createdFilePath).Should().BeTrue();
293296
}
294297

298+
[TestMethod]
299+
public async Task WithCleanupAsync_NoCleanup_WhenUnauthorized()
300+
{
301+
// Arrange
302+
var fileUtilityService = this.fileUtilityServiceMock.Object;
303+
var directoryUtilityService = this.directoryUtilityServiceMock.Object;
304+
CancellationToken cancellationToken = default;
305+
var detectorArgs = new Dictionary<string, string>();
306+
var cleanupCreatedFiles = true;
307+
var fileComponentDetector = new TestFileComponentDetector(["*.pyc"], this.loggerMock.Object, fileUtilityService, directoryUtilityService);
308+
var createdFilePath = Path.Combine(this.testDirectory, "todelete.pyc");
309+
this.directoryUtilityServiceMock
310+
.Setup(d => d.GetFilesAndDirectories(It.IsAny<string>(), It.IsAny<List<string>>(), It.IsAny<int>()))
311+
.Throws(new UnauthorizedAccessException("Unauthorized"));
312+
313+
// Act
314+
await fileComponentDetector.TestCleanupAsync(
315+
(process, args, token) =>
316+
{
317+
// creates a single file
318+
this.fileSystemMockFiles.Add(createdFilePath);
319+
return Task.CompletedTask;
320+
},
321+
this.processRequest,
322+
detectorArgs,
323+
cleanupCreatedFiles,
324+
cancellationToken).ConfigureAwait(false);
325+
326+
// Assert
327+
directoryUtilityService.Exists(this.testDirectory).Should().BeTrue();
328+
fileUtilityService.Exists(this.existingFilePath).Should().BeTrue();
329+
fileUtilityService.Exists(createdFilePath).Should().BeTrue();
330+
}
331+
295332
private bool IsPatternMatch(string path, string pattern)
296333
{
297334
return Regex.IsMatch(path, "^" + Regex.Escape(pattern).Replace("\\*", ".*") + "$");

0 commit comments

Comments
 (0)