Implemented RemotePathType for credential separation. #83
Conversation
Tests not working.
WalkthroughIntroduces RemotePathType to control when remote credentials are applied, replaces a boolean option, and refactors Move flow to use impersonation selectively for discovery, copy, directory ops, and cleanup. Adds ExecuteWithImpersonation API. Updates existing tests, and adds new Samba- and Windows-based impersonation test suites with docker-compose support. Adds Docker.DotNet test dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Flow as Caller
participant Files as Files.Move
participant Exec as ExecuteMoveAsync
participant Disc as Discovery
participant Copy as ExecuteCopyAsync
participant Imp as ExecuteWithImpersonation
Flow->>Files: Move(input, options)
Files->>Exec: ExecuteMoveAsync(input, options)
alt options.RemotePath == Source
Exec->>Imp: FindMatchingFilesWithImpersonation
Imp-->>Disc: run under impersonation
else
Exec->>Disc: FindMatchingFiles (local)
end
loop For each matched file
alt options.RemotePath == Target
Exec->>Imp: Create target dir / check target / delete existing
Imp-->>Exec: done
Exec->>Copy: Local -> Remote (impersonated writes)
else options.RemotePath == Source
Exec->>Copy: Remote -> Local (impersonated reads)
else options.RemotePath == None
Exec->>Copy: Local -> Local
end
end
alt Error occurs
Exec->>Imp: DeleteExistingFilesWithCredentials
Imp-->>Exec: cleanup done
end
Exec-->>Files: List<FileItem>
Files-->>Flow: Result(files)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj (1)
1-1: Strip BOM from the XML to avoid tooling glitches.There’s a Unicode BOM before the root element. Some parsers and diff tools choke on it. Remove it.
-<Project Sdk="Microsoft.NET.Sdk"> +<Project Sdk="Microsoft.NET.Sdk">Frends.Files.Move/Frends.Files.Move/Definitions/RemotePathType.cs (2)
3-6: Add a short block to aid task authors and UI rendering.A minimal usage example improves discoverability in Frends UI and satisfies the “ for public types” guideline.
/// <summary> /// Defines the types of remote path configurations for file operations. /// Used to identify which path requires remote credentials and impersonation. /// </summary> +/// <example> +/// <code> +/// var options = new Options { +/// RemotePath = RemotePathType.Source, +/// UserName = "DOMAIN\\user", +/// Password = Secret.FromEnvironment("MOVE_PASSWORD") +/// }; +/// </code> +/// </example> public enum RemotePathType
14-22: Clarify behavior when both ends are remote or out-of-domain.As-is, the enum can’t represent “both remote”. If that scenario is intentionally unsupported, state it in the XML docs of the enum and/or Options.RemotePath. If you plan to support it later (with sequential impersonation and memory buffer crossing), consider a future
SourceAndTargetvalue to make intent explicit.Frends.Files.Move/Frends.Files.Move.Tests/Samba/docker-compose.yml (5)
12-17: Reduce duplicated configuration and rely on the image’s native entrypoint instead of a long shell.You’re setting both environment variables and invoking
samba.shwith equivalent flags, then keeping the container alive viatail -f. Prefer a single approach that starts daemons in the foreground and exits on failure.- environment: - - USER=testuser;password123! - - SHARE=testshare;/mount/testshare;yes;no;no;testuser;testuser;testuser - - SMB=true - - NMBD=true - - WORKGROUP=WORKGROUP - command: > - sh -c " - /usr/bin/samba.sh -u 'testuser;password123!' -s 'testshare;/mount/testshare;yes;no;no;testuser' -p && - chmod 777 /mount/testshare && - chown testuser:testuser /mount/testshare && - tail -f /dev/null - " + environment: + - WORKGROUP=WORKGROUP + command: > + /usr/bin/samba.sh + -u 'testuser;password123!' + -s 'testshare;/mount/testshare;yes;no;no;testuser' + -p + ; chmod 777 /mount/testshare && chown testuser:testuser /mount/testshareIf the image supports direct
command: -u '...' -s '...' -pwithout explicit path, use that instead of/usr/bin/samba.sh. Keep the post-start permission fix as a separate chained command if necessary.Also applies to: 18-24
29-33: Healthcheck may fail if smbclient isn’t present; use a simpler liveness probe.Some samba images omit
smbclient. Consider checking the smbd process or the listening socket instead.- healthcheck: - test: ["CMD", "smbclient", "-L", "//localhost/", "-U", "testuser%password123!", "--option=client min protocol=SMB2"] - interval: 15s - timeout: 10s - retries: 5 - start_period: 30s + healthcheck: + test: ["CMD-SHELL", "pidof smbd >/dev/null 2>&1 && nc -z localhost 445"] + interval: 15s + timeout: 10s + retries: 5 + start_period: 30sIf
ncis not available, drop it and keeppidof smbdonly.
13-14: Externalize credentials via .env and environment expansion.Don’t hardcode test credentials in the compose file. Keep them in a
.envcommitted to CI with non-sensitive defaults (or injected in pipeline secrets) and reference those here.- - USER=testuser;password123! - - SHARE=testshare;/mount/testshare;yes;no;no;testuser;testuser;testuser + - USER=${SMB_USER:-testuser};${SMB_PASSWORD:-password123!} + - SHARE=${SMB_SHARE:-testshare};/mount/testshare;yes;no;no;${SMB_USER:-testuser}And add a
.envfile with SMB_USER, SMB_PASSWORD, SMB_SHARE for local runs.I can draft the
.envand update tests to load it before running.Also applies to: 20-21
41-41: Add a trailing newline.YAMLlint flagged a missing newline at EOF.
- driver: bridge + driver: bridge +
5-5: Pin dperson/samba image to an immutable digest for reproducibilityThe
latesttag is mutable and may change unexpectedly. Replace it with a specific SHA-256 digest to ensure your tests always run against the same image.• File: Frends.Files.Move/Frends.Files.Move.Tests/Samba/docker-compose.yml
Line 5Suggested change:
- image: dperson/samba:latest + image: dperson/samba@sha256:66088b78a19810dd1457a8f39340e95e663c728083efa5fe7dc0d40b2478e869This digest corresponds to the “latest” image pulled in January 2023 and will remain unchanged.
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs (1)
34-34: Good migration to RemotePathType.None; add explicit Source/Target coverage to exercise the new semantics.Replacing the old boolean with
RemotePath = RemotePathType.Noneis correct for local-only tests. To validate the credential-selection logic, add unit/integration cases forSourceandTarget(can be mocked file system or gated “Docker” category hitting the Samba fixture).I can add two targeted tests:
- RemotePathType.Source: moves from a remote UNC (mock/stub) to local, ensuring only reads are under impersonation.
- RemotePathType.Target: moves from local to remote UNC, ensuring only writes are under impersonation.
Also applies to: 62-62, 79-79
Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs (1)
20-21: Nullability cleanup for _input/_options or use null-forgiving to satisfy analyzers.They’re nullable but assigned in setup before use. Either drop
?or use!at call sites to avoid noise and potential misuse.- Input? _input; - Options? _options; + Input _input = default!; + Options _options = default!;Also applies to: 39-45, 69-72
Frends.Files.Move/Frends.Files.Move.Tests/WindowsServer/docker-compose.yml (1)
1-2: Clarify filename in comment.The comment on line 1 refers to
docker-compose-windows.ymlbut the actual filename isdocker-compose.yml.Apply this diff to fix the comment:
-# docker-compose-windows.yml +# docker-compose.ymlFrends.Files.Move/Frends.Files.Move.Tests/WindowsServer/WindowsImpersonationTests.cs (2)
26-28: Remove or configure commented test configuration.The commented hardcoded test configuration should either be removed or converted to environment-based configuration for better maintainability.
Consider replacing the hardcoded values with environment variables or configuration:
- // Simple manual setup - just set these values for your environment - //_remoteShare = @"\\172.24.187.181%3a1445\testshare"; - //_remoteShare = @"\\windows_fileserver\testshare"; - _remoteShare = @"\\127.0.0.1\testshare"; + // Configure the remote share path - defaults to localhost + _remoteShare = Environment.GetEnvironmentVariable("TEST_REMOTE_SHARE") ?? @"\\127.0.0.1\testshare";
14-17: Add XML documentation for test class fields.The private fields lack documentation explaining their purpose.
Add XML documentation to improve code readability:
+ /// <summary> + /// Local directory used as source for test files + /// </summary> private string _localSourceDir; + /// <summary> + /// Local directory used as target for moved files + /// </summary> private string _localTargetDir; + /// <summary> + /// UNC path to the remote share for testing + /// </summary> private string _remoteShare; + /// <summary> + /// Common options used across tests + /// </summary> private Options _options;Frends.Files.Move/Frends.Files.Move.Tests/Samba/ImpersonationSambaDockerTests.cs (1)
78-105: Consider adding timeout configuration.The
WaitForDockerContainermethod has a hardcoded timeout that might be insufficient in some environments.Consider making the timeout configurable:
- WaitForDockerContainer("impersonation_test_samba", TimeSpan.FromMinutes(2)); + var containerTimeout = TimeSpan.FromMinutes(int.Parse(Environment.GetEnvironmentVariable("DOCKER_CONTAINER_TIMEOUT_MINUTES") ?? "2")); + WaitForDockerContainer("impersonation_test_samba", containerTimeout);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj(2 hunks)Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs(2 hunks)Frends.Files.Move/Frends.Files.Move.Tests/Samba/ImpersonationSambaDockerTests.cs(1 hunks)Frends.Files.Move/Frends.Files.Move.Tests/Samba/docker-compose.yml(1 hunks)Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs(3 hunks)Frends.Files.Move/Frends.Files.Move.Tests/WindowsServer/WindowsImpersonationTests.cs(1 hunks)Frends.Files.Move/Frends.Files.Move.Tests/WindowsServer/docker-compose.yml(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/Options.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Definitions/RemotePathType.cs(1 hunks)Frends.Files.Move/Frends.Files.Move/Move.cs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Frends.*/Frends.*/*.csproj
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.csproj: Ensure the .csproj targets .NET 6 or 8, uses the MIT license, and includes the following fields:
= Frends
= true
= MIT
Follow Microsoft C# project file conventions.
Files:
Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj
Frends.*/Frends.*.Tests/*
⚙️ CodeRabbit configuration file
Frends.*/Frends.*.Tests/*: Confirm unit tests exist and provide at least 80% coverage.
Tests should:
- Load secrets via dotenv
- Use mocking where real systems can't be simulated
- Follow Microsoft unit testing naming and structuring conventions
Files:
Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csprojFrends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.csFrends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs
Frends.*/**/*.cs
⚙️ CodeRabbit configuration file
Frends.*/**/*.cs: Code must follow Microsoft C# coding standards, including:
- PascalCase for public members and task parameters
- Proper naming for abbreviations (Csv, Url, Api)
- Use of var only when type is obvious
- Clean structure and no unused code
Files:
Frends.Files.Move/Frends.Files.Move/Definitions/RemotePathType.csFrends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.csFrends.Files.Move/Frends.Files.Move.Tests/WindowsServer/WindowsImpersonationTests.csFrends.Files.Move/Frends.Files.Move.Tests/Samba/ImpersonationSambaDockerTests.csFrends.Files.Move/Frends.Files.Move.Tests/UnitTests.csFrends.Files.Move/Frends.Files.Move/Definitions/Options.csFrends.Files.Move/Frends.Files.Move/Move.cs
Frends.*/Frends.*/*.cs
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.cs: Ensure every public method and class:
- Has
and XML comments
- If the documentation is very long then it can also use element
- Follows Microsoft C# code conventions
- Uses semantic task result documentation (e.g., Success, Error, Data)
Frends.*/Frends.*/*.cs: Validate all task result classes include:
- Success (bool)
- Task-specific return value (e.g. Data, FilePaths)
- Error object with Message and AdditionalInfo
- Ensure result structure is flat, simple, and avoids 3rd-party types.
- Use dynamic or JToken only when the structure is unknown.
Files:
Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.csFrends.Files.Move/Frends.Files.Move.Tests/UnitTests.csFrends.Files.Move/Frends.Files.Move/Move.cs
🧬 Code graph analysis (1)
Frends.Files.Move/Frends.Files.Move/Move.cs (2)
Frends.Files.Delete/Frends.Files.Delete/Delete.cs (4)
Result(70-75)List(93-115)Files(23-157)Files(25-31)Frends.Files.Copy/Frends.Files.Copy/Copy.cs (7)
Task(31-37)Task(39-54)Task(56-111)Task(113-119)Files(21-190)AssertNoTargetFileConflicts(166-172)GetNonConflictingDestinationFilePath(179-189)
🪛 YAMLlint (1.37.1)
Frends.Files.Move/Frends.Files.Move.Tests/WindowsServer/docker-compose.yml
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 21-21: trailing spaces
(trailing-spaces)
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 27-27: trailing spaces
(trailing-spaces)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 60-60: no new line character at the end of file
(new-line-at-end-of-file)
Frends.Files.Move/Frends.Files.Move.Tests/Samba/docker-compose.yml
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: build / Build on windows-latest
🔇 Additional comments (7)
Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj (1)
11-16: Ensure a Single Test Framework Across All ProjectsIt looks like you’ve added NUnit to the Move.Tests project but left MSTest in other test suites (MoveDirectory.Tests, LocalBackup.Tests, CreateDirectory.Tests, DeleteDirectory.Tests), as evidenced by the presence of
[TestClass],[TestMethod],[TestInitialize], and[TestCleanup]attributes in those files. Mixing frameworks can lead to duplicate test discovery, slower CI, and confusion for future contributors.You have two paths forward—pick one and apply it consistently across the repo:
• Migrate everything to NUnit
- Remove all MSTest packages (
MSTest.TestFramework,MSTest.TestAdapter) from every test project’s.csproj.- Replace MSTest attributes with NUnit equivalents (
[TestFixture],[Test],[SetUp],[TearDown], etc.).- Keep
Microsoft.NET.Test.Sdk,nunit, andNUnit3TestAdapter.• Stick with MSTest everywhere
- Remove the NUnit packages from
Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj.- Ensure Move.Tests uses
[TestClass],[TestMethod], etc., matching the rest of the codebase.In either case, after aligning on one framework:
• Prune any redundant adapters.
• Optionally addDotNetEnvif your tests need to load.envfiles for secrets.Files needing attention:
- All
*.Tests.csprojfiles under each Frends.Files.* project.- All test classes in
Frends.Files.*.Tests/**/*.csthat currently use MSTest or NUnit attributes.Frends.Files.Move/Frends.Files.Move/Definitions/RemotePathType.cs (1)
3-23: Enum and XML docs look good; names are clear and aligned with the new semantics.The type and member naming are concise and follow conventions. The summary reads well.
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.cs (1)
48-55: Verify NUnit Parallel Execution ConfigurationI didn’t find any assembly-level
[Parallelizable]attributes or NUnit3TestAdapter add-ins in the codebase, so it’s unclear whether your test run configuration enables parallel execution. If parallelization is enabled (e.g. via a.runsettingsfile or an assembly attribute), the shared_TargetDiracross multiple tests can cause race conditions and intermittent failures.To eliminate flakiness, choose one of the following:
- Mark the fixture non-parallelizable
- [TestFixture] + [TestFixture, NonParallelizable] public class UnitTests- Isolate each test’s files by generating unique temporary directories instead of reusing
_TargetDirAffected tests in
Frends.Files.Move/Frends.Files.Move.Tests/UnitTests.csinclude the ranges:
- Lines 48–55
- Lines 58–66
- Lines 75–83
- Lines 95–108
- Lines 111–124
- Lines 141–153
- Lines 156–169
Please verify your NUnit execution settings and apply one of the above mitigations to prevent test interference.
Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs (1)
11-13: Enforce Windows-only and Administrator requirement for ImpersonationTestsI scanned the codebase for any existing helper or pattern to detect elevated privileges (e.g.
IsAdministrator(),WindowsIdentity.GetCurrent(),WindowsPrincipal) and found none. To prevent CI failures on Linux agents or non-admin Windows runs, please apply both an OS guard and an admin-check in your tests:• In Frends.Files.Move.Tests/ImpersonationTests.cs, add the
[Platform]attribute to limit the fixture to Windows:-[TestFixture] -class ImpersonationTests +[TestFixture] +[Platform(Include = "Win")] +class ImpersonationTests• Introduce an admin-check in a
[OneTimeSetUp](or before each test) and ignore tests if not running with Administrator rights. You can inline it or extract it into a new helper:[OneTimeSetUp] public void RequireAdministrator() { var identity = WindowsIdentity.GetCurrent(); var principal = new WindowsPrincipal(identity); if (!principal.IsInRole(WindowsBuiltInRole.Administrator)) Assert.Ignore("Impersonation tests require Administrator privileges."); }• (Optional) If you expect to reuse this logic elsewhere, extract it to a static helper method
TestHelper.IsAdministrator()in a shared test-utils class and call it here.Please verify that no existing helper covers this scenario—if one does, use it instead of duplicating the logic.
Frends.Files.Move/Frends.Files.Move/Definitions/Options.cs (1)
11-20: LGTM! Well-designed API change.The replacement of the boolean flag with the
RemotePathTypeenum is a good design improvement. It provides clearer semantics and better extensibility for handling different credential scenarios.Frends.Files.Move/Frends.Files.Move/Move.cs (2)
316-326: Consider resource cleanup in case of exceptions.In the remote copy scenarios, if an exception occurs after opening streams but before disposal, the streams might not be properly closed.
The current implementation uses
await usingwhich should handle disposal correctly even on exceptions. The code looks correct.Also applies to: 328-340
37-170: Well-structured refactoring of impersonation logic.The refactoring effectively separates concerns by isolating impersonation logic based on the
RemotePathType. The flow is clear and maintains proper error handling with cleanup operations.
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Docker.DotNet" Version="3.125.15" /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate Docker.DotNet version and gate Docker-driven tests when the daemon isn’t available.
Pin looks fine for net6.0, but please double-check the version against your CI images. Add a test category/assumption to skip Docker tests when the engine isn’t reachable to keep pipelines green on Windows/macOS agents without Docker.
Use this quick check to find tests already gated by category and confirm at least one suite depends on Docker:
If missing, wrap Docker-dependent fixtures/tests with [Category("Docker")] and add an environment/assumption check (e.g., try connecting via DockerClient and Assert.Ignore on failure).
🏁 Script executed:
#!/bin/bash
rg -nP '\[Category\("Docker"\)|DockerClient|Docker\.DotNet' -C2Length of output: 647
Validate Docker.DotNet version and gate Docker-driven tests when the daemon isn’t available
It looks like you’ve correctly pinned Docker.DotNet to version 3.125.15 in your test project, but we didn’t find any existing test gating or assumptions around Docker availability. To prevent CI failures on agents without Docker (e.g., Windows/macOS hosted runners), please:
-
Verify the Docker.DotNet version
Ensure v3.125.15 is compatible with the Docker engine version installed in your CI environments. If your images use an older or newer Docker API, you may need to bump or roll back this package reference in
Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj(Line 11). -
Annotate Docker-dependent tests
Wrap any tests or fixtures that rely on Docker with a Docker-specific category. For NUnit, add:[Category("Docker")] public class MyDockerTests { … }
Or for MSTest:
[TestCategory("Docker")] public class MyDockerTests { … }
-
Skip tests when Docker isn’t reachable
At the start of each Docker-dependent test or fixture setup, attempt a lightweight ping to the daemon and call the appropriate skip/ignore API on failure. For example (NUnit):var client = new DockerClientConfiguration().CreateClient(); try { client.System.PingAsync().Wait(); } catch { Assert.Ignore("Docker daemon is not available; skipping Docker tests."); }
This will keep your pipelines green on agents that can’t run Docker.
Once these changes are in place, your suite will correctly isolate and skip Docker-driven tests when the engine isn’t available.
🤖 Prompt for AI Agents
In Frends.Files.Move/Frends.Files.Move.Tests/Frends.Files.Move.Tests.csproj
around line 11, you've pinned Docker.DotNet to v3.125.15 but haven't handled CI
agents without Docker; verify that v3.125.15 matches the Docker Engine API used
in your CI and update the PackageReference if incompatible, and then annotate
all Docker-dependent tests/fixtures with a Docker-specific test category (e.g.,
"Docker") and add a lightweight Docker daemon availability check in each Docker
test setup that performs a ping/health call and calls the test framework's
skip/ignore API when the daemon is unreachable so those tests are skipped on
hosts without Docker.
| //UseGivenUserCredentialsForRemoteConnections = true, | ||
| UserName = $"{_domain}\\{_name}", | ||
| Password = _pwd | ||
| }; |
There was a problem hiding this comment.
This test no longer exercises impersonation; set RemotePath to trigger credential validation.
With the old flag commented out, Files.Move may treat the operation as local-only and skip user/domain validation. Assign RemotePath accordingly so the test asserts the credential path.
_options = new Options
{
- //UseGivenUserCredentialsForRemoteConnections = true,
+ RemotePath = RemotePathType.Source,
UserName = $"{_domain}\\{_name}",
Password = _pwd
};If the test intends to validate writing under impersonation, use RemotePathType.Target instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //UseGivenUserCredentialsForRemoteConnections = true, | |
| UserName = $"{_domain}\\{_name}", | |
| Password = _pwd | |
| }; | |
| _options = new Options | |
| { | |
| RemotePath = RemotePathType.Source, | |
| UserName = $"{_domain}\\{_name}", | |
| Password = _pwd | |
| }; |
🤖 Prompt for AI Agents
In Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs around lines
41–44, the test no longer triggers impersonation because
UseGivenUserCredentialsForRemoteConnections is commented out and the RemotePath
is not set for a remote operation; update the test to assign RemotePath to a
remote location (e.g. a UNC path) so credential validation runs, or set
RemotePathType to RemotePathType.Target to force remote credential use, and if
needed re-enable UseGivenUserCredentialsForRemoteConnections = true to ensure
the impersonation path is exercised.
| //UseGivenUserCredentialsForRemoteConnections = true, | ||
| UserName = "test", | ||
| Password = _pwd | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Domain-less username test should also set RemotePath to ensure the expected ArgumentException.
Same issue as above. Without RemotePath, the validation might not run. Update the options and keep the assertion.
var options = new Options
{
- //UseGivenUserCredentialsForRemoteConnections = true,
+ RemotePath = RemotePathType.Source,
UserName = "test",
Password = _pwd
};Also applies to: 88-90
🤖 Prompt for AI Agents
In Frends.Files.Move/Frends.Files.Move.Tests/ImpersonationTests.cs around lines
83-86 (and similarly for lines 88-90), the domain-less username test sets
UserName and Password but omits RemotePath so the validation that throws the
expected ArgumentException may not run; update the test options to include a
valid RemotePath value (e.g. a non-empty UNC or placeholder path) before
invoking the method and leave the existing assertion unchanged so the test
triggers and asserts the ArgumentException as intended.
| var psi = new ProcessStartInfo("docker", $"exec {containerName} smbclient -L //localhost/ -U testuser%password123! --option=\"client min protocol=SMB2\"") | ||
| { | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| UseShellExecute = false, | ||
| CreateNoWindow = true | ||
| }; |
There was a problem hiding this comment.
Security concern: Password visible in process arguments.
The password is passed directly in command-line arguments, which could be visible in process listings.
While this is test code, consider using environment variables or stdin for passing credentials to avoid exposing them in process arguments:
- var psi = new ProcessStartInfo("docker", $"exec {containerName} smbclient -L //localhost/ -U testuser%password123! --option=\"client min protocol=SMB2\"")
+ // Consider using environment variables or stdin to pass credentials
+ var psi = new ProcessStartInfo("docker", $"exec {containerName} sh -c 'echo password123! | smbclient -L //localhost/ -U testuser --option=\"client min protocol=SMB2\"'")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var psi = new ProcessStartInfo("docker", $"exec {containerName} smbclient -L //localhost/ -U testuser%password123! --option=\"client min protocol=SMB2\"") | |
| { | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, | |
| UseShellExecute = false, | |
| CreateNoWindow = true | |
| }; | |
| // Consider using environment variables or stdin to pass credentials | |
| var psi = new ProcessStartInfo("docker", $"exec {containerName} sh -c 'echo password123! | smbclient -L //localhost/ -U testuser --option=\"client min protocol=SMB2\"'") | |
| { | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, | |
| UseShellExecute = false, | |
| CreateNoWindow = true | |
| }; |
| - fileserver_data:C:\SharedData | ||
| networks: | ||
| - test-net | ||
| command: |
There was a problem hiding this comment.
Remove trailing spaces.
The YAML file contains trailing spaces on multiple lines that should be removed.
Apply this diff to remove the trailing spaces:
- command:
+ command:
- Write-Host 'Starting Windows file server setup...'
+ Write-Host 'Starting Windows file server setup...'
- net localgroup Administrators testfileuser /add
+ net localgroup Administrators testfileuser /add
- New-Item -Path 'C:\TestShare' -ItemType Directory -Force
+ New-Item -Path 'C:\TestShare' -ItemType Directory -Force
- net share testshare=C:\TestShare /GRANT:testfileuser,FULL /GRANT:Administrator,FULL
+ net share testshare=C:\TestShare /GRANT:testfileuser,FULL /GRANT:Administrator,FULL
- icacls 'C:\TestShare' /grant 'testfileuser:(OI)(CI)F' /T
+ icacls 'C:\TestShare' /grant 'testfileuser:(OI)(CI)F' /T
- Write-Host 'Setup complete - container ready'
+ Write-Host 'Setup complete - container ready'
- net share
+ net share
- while ($$true) {
+ while ($$true) {Also applies to: 17-17, 21-21, 24-24, 27-27, 30-30, 35-35, 37-37, 40-40, 42-42
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In Frends.Files.Move/Frends.Files.Move.Tests/WindowsServer/docker-compose.yml
around lines 12,17,21,24,27,30,35,37,40 and 42, several lines have trailing
spaces; remove the trailing whitespace at the end of each listed line (and any
other trailing spaces in the file), save the file so those lines end exactly at
the last meaningful character with no space after it, and re-run a YAML/linter
check to confirm no trailing-space warnings remain.
|
|
||
| networks: | ||
| test-net: | ||
| driver: nat No newline at end of file |
There was a problem hiding this comment.
Add newline at the end of file.
The YAML file is missing a newline character at the end.
Apply this diff to add the missing newline:
test-net:
- driver: nat
+ driver: nat
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| driver: nat | |
| test-net: | |
| driver: nat | |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 60-60: no new line character at the end of file
(new-line-at-end-of-file)
🤖 Prompt for AI Agents
Frends.Files.Move/Frends.Files.Move.Tests/WindowsServer/docker-compose.yml
around line 60: the file is missing a trailing newline; add a single newline
character at the end of the file so the final line ("driver: nat") is terminated
with a newline.
| }).Wait(); | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling in Setup method.
The empty catch block silently swallows exceptions during remote directory cleanup, which could hide test setup issues.
Apply this diff to improve error handling:
await Task.CompletedTask;
}).Wait();
}
- catch { }
+ catch (Exception ex)
+ {
+ // Log but don't fail setup if cleanup fails
+ TestContext.WriteLine($"Warning: Failed to clean remote directory during setup: {ex.Message}");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }).Wait(); | |
| } | |
| catch { } | |
| await Task.CompletedTask; | |
| }).Wait(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Log but don't fail setup if cleanup fails | |
| TestContext.WriteLine($"Warning: Failed to clean remote directory during setup: {ex.Message}"); | |
| } |
🤖 Prompt for AI Agents
In
Frends.Files.Move/Frends.Files.Move.Tests/WindowsServer/WindowsImpersonationTests.cs
around lines 73 to 75, the empty catch block in the Setup method swallows any
exceptions during remote directory cleanup; replace it with a catch(Exception
ex) that logs the exception details (e.g., Console.Error.WriteLine or
TestContext.Progress.WriteLine) and then rethrow or call Assert.Fail so setup
failures are surfaced; ensure the log includes contextual information about
which cleanup step failed and the exception message/stack trace.
| fileExists = File.Exists(targetFilePath); | ||
| await Task.CompletedTask; |
There was a problem hiding this comment.
Fix variable scope issue in impersonation block.
The fileExists variable is being set inside the lambda but referenced outside, which won't work as expected.
Apply this diff to fix the variable scope issue:
- bool fileExists;
+ bool fileExists = false;
if (needsImpersonationForCheck && !string.IsNullOrEmpty(options.UserName))
{
- fileExists = false;
await ExecuteWithImpersonation(options, async () =>
{
fileExists = File.Exists(targetFilePath);
await Task.CompletedTask;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fileExists = File.Exists(targetFilePath); | |
| await Task.CompletedTask; | |
| // Declare and initialize before use, so it's definitely assigned | |
| bool fileExists = false; | |
| if (needsImpersonationForCheck && !string.IsNullOrEmpty(options.UserName)) | |
| { | |
| await ExecuteWithImpersonation(options, async () => | |
| { | |
| fileExists = File.Exists(targetFilePath); | |
| await Task.CompletedTask; | |
| }); | |
| } | |
| else | |
| { | |
| fileExists = File.Exists(targetFilePath); | |
| } |
🤖 Prompt for AI Agents
In Frends.Files.Move/Frends.Files.Move/Move.cs around lines 140-141, the
fileExists variable is set inside the impersonation lambda but referenced
outside its scope; declare fileExists before entering the impersonation block
(or have the impersonation call return the boolean) and assign to that outer
variable inside the lambda so the value is visible after impersonation ends,
ensuring no local-only declaration inside the lambda hides the outer reference.
| { | ||
| try | ||
| { | ||
| var entries = Directory.GetFileSystemEntries(directoryPath); | ||
| Console.WriteLine($"[DEBUG] GetFileSystemEntries succeeded, found {entries.Length} items"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"[DEBUG] GetFileSystemEntries failed: {ex.Message}"); | ||
| } | ||
|
|
||
| throw new DirectoryNotFoundException($"Directory does not exist or you do not have read access. Tried to access directory '{directoryPath}'"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or conditionally compile debug code.
The debug output code should be removed or wrapped in conditional compilation for production builds.
Apply this diff to conditionally compile the debug code:
if (!Directory.Exists(directoryPath))
{
+#if DEBUG
try
{
var entries = Directory.GetFileSystemEntries(directoryPath);
Console.WriteLine($"[DEBUG] GetFileSystemEntries succeeded, found {entries.Length} items");
}
catch (Exception ex)
{
Console.WriteLine($"[DEBUG] GetFileSystemEntries failed: {ex.Message}");
}
+#endif
throw new DirectoryNotFoundException($"Directory does not exist or you do not have read access. Tried to access directory '{directoryPath}'");
}🤖 Prompt for AI Agents
In Frends.Files.Move/Frends.Files.Move/Move.cs around lines 226 to 238, the
debug Console.WriteLine block should not be present in production; either remove
those debug Console.WriteLine try/catch statements or wrap them in a conditional
compilation block (e.g. #if DEBUG ... #endif) so they compile only in debug
builds, keeping the DirectoryNotFoundException throw unchanged.
| public static async Task ExecuteWithImpersonation(Options options, Func<Task> action) | ||
| { | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| throw new PlatformNotSupportedException("Impersonation is only supported on Windows."); | ||
|
|
||
| var (domain, user) = GetDomainAndUsername(options.UserName); | ||
| UserCredentials credentials = new UserCredentials(domain, user, options.Password); | ||
|
|
||
| using SafeAccessTokenHandle userHandle = credentials.LogonUser(LogonType.NewCredentials); | ||
| await WindowsIdentity.RunImpersonated(userHandle, action); | ||
| } |
There was a problem hiding this comment.
Add XML documentation for the new public method.
The new public method ExecuteWithImpersonation lacks XML documentation, which is required according to the coding guidelines.
Add proper XML documentation:
+ /// <summary>
+ /// Executes an asynchronous action with Windows impersonation using the provided credentials.
+ /// </summary>
+ /// <param name="options">Options containing the username and password for impersonation.</param>
+ /// <param name="action">The asynchronous action to execute under impersonation.</param>
+ /// <exception cref="PlatformNotSupportedException">Thrown when called on non-Windows platforms.</exception>
+ /// <exception cref="ArgumentException">Thrown when username format is invalid.</exception>
+ /// <example>
+ /// <code>
+ /// var options = new Options
+ /// {
+ /// UserName = @"domain\user",
+ /// Password = "password"
+ /// };
+ /// await Files.ExecuteWithImpersonation(options, async () =>
+ /// {
+ /// // Perform file operations with impersonation
+ /// await File.WriteAllTextAsync(@"\\server\share\file.txt", "content");
+ /// });
+ /// </code>
+ /// </example>
public static async Task ExecuteWithImpersonation(Options options, Func<Task> action)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static async Task ExecuteWithImpersonation(Options options, Func<Task> action) | |
| { | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | |
| throw new PlatformNotSupportedException("Impersonation is only supported on Windows."); | |
| var (domain, user) = GetDomainAndUsername(options.UserName); | |
| UserCredentials credentials = new UserCredentials(domain, user, options.Password); | |
| using SafeAccessTokenHandle userHandle = credentials.LogonUser(LogonType.NewCredentials); | |
| await WindowsIdentity.RunImpersonated(userHandle, action); | |
| } | |
| /// <summary> | |
| /// Executes an asynchronous action with Windows impersonation using the provided credentials. | |
| /// </summary> | |
| /// <param name="options">Options containing the username and password for impersonation.</param> | |
| /// <param name="action">The asynchronous action to execute under impersonation.</param> | |
| /// <exception cref="PlatformNotSupportedException">Thrown when called on non-Windows platforms.</exception> | |
| /// <exception cref="ArgumentException">Thrown when username format is invalid.</exception> | |
| /// <example> | |
| /// <code> | |
| /// var options = new Options | |
| /// { | |
| /// UserName = @"domain\user", | |
| /// Password = "password" | |
| /// }; | |
| /// await Files.ExecuteWithImpersonation(options, async () => | |
| /// { | |
| /// // Perform file operations with impersonation | |
| /// await File.WriteAllTextAsync(@"\\server\share\file.txt", "content"); | |
| /// }); | |
| /// </code> | |
| /// </example> | |
| public static async Task ExecuteWithImpersonation(Options options, Func<Task> action) | |
| { | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | |
| throw new PlatformNotSupportedException("Impersonation is only supported on Windows."); | |
| var (domain, user) = GetDomainAndUsername(options.UserName); | |
| UserCredentials credentials = new UserCredentials(domain, user, options.Password); | |
| using SafeAccessTokenHandle userHandle = credentials.LogonUser(LogonType.NewCredentials); | |
| await WindowsIdentity.RunImpersonated(userHandle, action); | |
| } |
🤖 Prompt for AI Agents
In Frends.Files.Move/Frends.Files.Move/Move.cs around lines 347 to 357, the new
public method ExecuteWithImpersonation is missing XML documentation; add a
standard XML doc block above the method including <summary> describing what the
method does, <param name="options"> and <param name="action"> describing inputs,
<returns> noting it returns a Task, and <exception> tags for
PlatformNotSupportedException and any credential/logon related exceptions (e.g.,
when LogonUser fails); optionally include a <remarks> line clarifying that
impersonation is Windows-only and that the caller is responsible for supplying
credentials.
Implemented RemotePathType enum for credential separation in file operations. Added RemotePath property to Options class with three values: None (both local), Source (source is remote), Target (target is remote). Logic uses impersonation only for remote operations and service credentials for local operations, with memory buffer for secure transfer across credential boundaries.
Attempted Docker testing with Samba and Windows Server Core containers but hit issues.
Still need to update CHANGELOG.md and increment version.
Also commented out the old UseGivenUserCredentialsForRemoteConnections property in existing tests since it was replaced by the more specific RemotePathType enum, but haven't cleaned up all old tests yet.
[]# Frends Task Pull Request
Summary
Review Checklist
1. Frends Task Project Files
Frends.*/Frends.*/*.csproj<PackageLicenseExpression>MIT</PackageLicenseExpression>)<Version><Authors>Frends</Authors><Description><RepositoryUrl><GenerateDocumentationFile>true</GenerateDocumentationFile>2. File: FrendsTaskMetadata.json
Frends.*/Frends.*/FrendsTaskMetadata.json3. File: README.md
Frends.*/README.md4. File: CHANGELOG.md
Frends.*/CHANGELOG.md5. File: migration.json
Frends.*/Frends.*/migration.json6. Source Code Documentation
Frends.*/Frends.*/*.cs<summary>XML comments<example>XML comments<frendsdocs>XML comments, if needed7. GitHub Actions Workflows
.github/workflows/*.yml*_test.yml*_main.yml*_release.yml8. Task Result Object Structure
Frends.*/Frends.*/*.csSuccess(bool)Additional Notes
Summary by CodeRabbit