Skip to content

Fix: Pass Credential to SMB copy functions via New-PSDrive - #624

Open
Adam Rudell (arudell) with Copilot wants to merge 5 commits into
mainfrom
copilot/add-smb-support-credential
Open

Fix: Pass Credential to SMB copy functions via New-PSDrive#624
Adam Rudell (arudell) with Copilot wants to merge 5 commits into
mainfrom
copilot/add-smb-support-credential

Conversation

Copilot AI commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Description

Copy-FileToRemoteComputer and Copy-FileFromRemoteComputer did not pass -Credential to their SMB helper functions, causing "Access Denied" failures and unnecessary WinRM fallback.

Since Copy-Item doesn't support -Credential for filesystem paths, the fix uses New-PSDrive to establish an authenticated SMB session before copying:

  • Pass -Credential from parent functions to Copy-FileToRemoteComputerSMB / Copy-FileFromRemoteComputerSMB
  • Create a temporary PSDrive mapped to the remote admin share when credentials are provided
  • Derive the UNC share root from the actual path (handles non-C: drives)
  • Clean up the PSDrive in the end block
# In the SMB copy functions' begin block:
if ($Credential -ne [System.Management.Automation.PSCredential]::Empty -and $null -ne $Credential) {
    $driveRoot = [System.IO.Path]::GetPathRoot($Path[0]).Replace(':','$')
    $uncRoot = "\\{0}\{1}" -f $ComputerName, $driveRoot
    $tempDriveName = "osPsDrive_{0}" -f [guid]::NewGuid().ToString()
    $null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop
    $psDriveName = $tempDriveName
}

Change type

  • Bug fix (non-breaking change)
  • Code style update (formatting, local variables)
  • New Feature (non-breaking change that adds new functionality without impacting existing)
  • Breaking change (fix or feature that may cause functionality impact)
  • Other

Checklist:

  • My code follows the style and contribution guidelines of this project.
  • I have tested and validated my code changes.

Copilot AI balanced review requested due to automatic review settings August 13, 2026 15:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…thentication

Fixes #269. Copy-FileToRemoteComputer and Copy-FileFromRemoteComputer now
pass the -Credential parameter to their respective SMB helper functions.
The SMB functions use New-PSDrive to establish an authenticated SMB session
when credentials are provided, since Copy-Item does not natively support
-Credential for filesystem operations. The PSDrive is cleaned up in the
end block.

Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 13, 2026 15:54
Address code review feedback:
- Derive the admin share UNC root from the actual path instead of
  hardcoding c$
- Assign $psDriveName only after New-PSDrive succeeds to avoid
  spurious Remove-PSDrive calls on failure

Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>
Copilot AI changed the title [WIP] Add support for -Credential in SMB file copy Fix: Pass Credential to SMB copy functions via New-PSDrive Aug 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/modules/SdnDiag.Utilities.psm1:852

  • Issue #269 requires handling an existing SMB mapping, but this always creates a new PSDrive without checking connections to the target. If the process already has an SMB session to this computer under another identity, New-PSDrive fails with a credential-conflict error and the requested credential still falls back to WinRM. Detect and deliberately reuse or replace a compatible existing connection before creating the drive.
        [System.String]$psDriveName = $null
        if ($Credential -ne [System.Management.Automation.PSCredential]::Empty -and $null -ne $Credential) {
            $driveRoot = [System.IO.Path]::GetPathRoot($Destination.FullName).Replace(':','$')

src/modules/SdnDiag.Utilities.psm1:885

  • The end block is not guaranteed to run when Copy-Item raises a terminating error (the caller supplies -ErrorAction Stop), so the temporary drive and authenticated SMB session can leak on the failure path. Put drive removal in cleanup that runs when processing terminates, such as a try/finally around the mapped-drive copy lifecycle.
            }
        }
    }

src/modules/SdnDiag.Utilities.psm1:763

  • This fixes the credential-forwarding bug for local-to-remote copies, but tests/offline/Utilities.Tests.ps1 has no regression coverage for this path. Add an offline Pester test that supplies a credential, verifies the SMB helper receives it, and covers PSDrive creation and cleanup.
                # try SMB Copy first and fallback to WinRM

src/modules/SdnDiag.Utilities.psm1:605

  • Issue #269 requires handling an existing SMB mapping, but this always creates a new PSDrive without checking connections to the target. If the process already has an SMB session to this computer under another identity, New-PSDrive fails with a credential-conflict error and the requested credential still falls back to WinRM. Detect and deliberately reuse or replace a compatible existing connection before creating the drive.

This issue also appears on line 850 of the same file.

            $driveRoot = [System.IO.Path]::GetPathRoot($Path[0]).Replace(':','$')
            $uncRoot = "\\{0}\{1}" -f $ComputerName, $driveRoot
            $tempDriveName = "osPsDrive_{0}" -f [guid]::NewGuid().ToString()

src/modules/SdnDiag.Utilities.psm1:642

  • The end block is not guaranteed to run when Test-Path or Copy-Item raises a terminating error (the caller supplies -ErrorAction Stop), so the temporary drive and authenticated SMB session can leak on the failure path. Put drive removal in cleanup that runs when processing terminates, such as a try/finally around the mapped-drive copy lifecycle.

This issue also appears on line 882 of the same file.

    }

    end {
        if ($psDriveName) {

src/modules/SdnDiag.Utilities.psm1:518

  • This fixes the credential-forwarding bug for remote-to-local copies, but tests/offline/Utilities.Tests.ps1 has no regression coverage for this path. Add an offline Pester test that supplies a credential, verifies the SMB helper receives it, and covers PSDrive creation and cleanup.

This issue also appears on line 763 of the same file.

                    Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop

Copilot AI review requested due to automatic review settings August 13, 2026 15:57
Comment thread src/modules/SdnDiag.Utilities.psm1 Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/modules/SdnDiag.Utilities.psm1:855

  • The linked issue explicitly requires handling an existing SMB connection, but this always creates another mapping. On Windows, a connection to the same server under a different identity makes New-PSDrive fail with error 1219, so this path still falls back to WinRM instead of supporting the supplied credential. Detect an existing connection and reuse it when compatible, or explicitly handle the credential conflict before creating the temporary drive.
            $null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:765

  • This bug fix changes credential-based copy-to behavior, but tests/offline/Utilities.Tests.ps1 has no regression coverage for it. Please add offline Pester tests that verify the credential reaches the SMB helper, the destination drive determines the admin share, and the temporary drive is removed.
                    Copy-FileToRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:606

  • The linked issue explicitly requires handling an existing SMB connection, but this always creates another mapping. On Windows, a connection to the same server under a different identity makes New-PSDrive fail with error 1219, so this path still falls back to WinRM instead of supporting the supplied credential. Detect an existing connection and reuse it when compatible, or explicitly handle the credential conflict before creating the temporary drive.

This issue also appears on line 855 of the same file.

            $null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:518

  • This bug fix changes credential-based copy-from behavior, but tests/offline/Utilities.Tests.ps1 has no regression coverage for it. Please add offline Pester tests that verify the credential reaches the SMB helper, the expected admin-share drive is created (including a non-C: path), and the temporary drive is removed.

This issue also appears on line 765 of the same file.

                    Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop

Comment thread src/modules/SdnDiag.Utilities.psm1
Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>
Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/modules/SdnDiag.Utilities.psm1:606

  • The linked issue explicitly requires detecting an existing SMB mapping, but this always creates a new drive. If the process already has a connection to this server under different credentials, Windows rejects the new mapping (typically error 1219), so this still falls back to WinRM. Check existing FileSystem/SMB connections for the server and define whether to reuse or replace them before calling New-PSDrive; apply the same handling to the upload helper.
            $null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:770

  • The corresponding upload behavior is also untested: tests/offline/Utilities.Tests.ps1 contains no coverage for the exported Copy-SdnFileToComputer path. Add an offline Pester test that verifies credential forwarding, mapping the destination's actual drive root (including a non-C: case), and removing the temporary drive after copying.
                try {

src/modules/SdnDiag.Utilities.psm1:518

  • This bug fix changes credential forwarding for the exported Copy-SdnFileFromComputer path, but tests/offline/Utilities.Tests.ps1 has no regression coverage for it. Add an offline Pester test that mocks the SMB dependencies and verifies the credential reaches New-PSDrive, the source drive root is used, and the temporary drive is removed after copying.

This issue also appears in the following locations of the same file:

  • line 606
  • line 770
                    Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop

Copilot AI review requested due to automatic review settings August 13, 2026 16:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/modules/SdnDiag.Utilities.psm1:896

  • end is skipped when a terminating copy error exits process, so the temporary PSDrive remains mounted precisely when this helper throws and the caller falls back to WinRM. Put the removal on a guaranteed failure path, such as a try/finally covering the mapping and copy lifecycle.
    end {
        if ($psDriveName) {
            try {
                Remove-PSDrive -Name $psDriveName -Force -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:861

  • This unconditionally creates another mapping without detecting an existing SMB/PSDrive connection. An existing connection to the same server under different credentials causes Windows error 1219, so this fix still takes the WinRM fallback in that required scenario. Reuse a compatible connection or explicitly handle a credential conflict before calling New-PSDrive.
        if ($Credential -ne [System.Management.Automation.PSCredential]::Empty -and $null -ne $Credential) {
            $driveRoot = [System.IO.Path]::GetPathRoot($Destination.FullName).Replace(':','$')
            $uncRoot = "\\{0}\{1}" -f $ComputerName, $driveRoot
            $tempDriveName = "osPsDrive_{0}" -f [guid]::NewGuid().ToString()
            $null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:771

  • The corresponding exported Copy-SdnFileToComputer credential path is also changed without a regression test. Add offline Pester coverage that verifies credential forwarding, authenticated drive creation for the destination drive, and cleanup on both success and failure.
                    Copy-FileToRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:645

  • end is not a finally block: if Test-Path, Copy-Item, or the explicit rethrow terminates the process block, PowerShell skips this cleanup and leaves the temporary drive/credentialed SMB session mounted. Ensure removal runs on the failure path before rethrowing (or restructure the mapping and copy under try/finally) so the WinRM fallback does not leave stale drives behind.

This issue also appears on line 893 of the same file.

    end {
        if ($psDriveName) {
            try {
                Remove-PSDrive -Name $psDriveName -Force -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:606

  • This always creates a new mapping without checking existing SMB/PSDrive connections to the host, despite issue #269 requiring detection. If the process already has a connection to this server under another identity, Windows rejects this with error 1219 and the operation still falls back to WinRM. Detect and reuse a compatible existing connection, and handle an incompatible connection explicitly before creating a drive.

This issue also appears on line 857 of the same file.

        if ($Credential -ne [System.Management.Automation.PSCredential]::Empty -and $null -ne $Credential) {
            $driveRoot = [System.IO.Path]::GetPathRoot($Path[0]).Replace(':','$')
            $uncRoot = "\\{0}\{1}" -f $ComputerName, $driveRoot
            $tempDriveName = "osPsDrive_{0}" -f [guid]::NewGuid().ToString()
            $null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop

src/modules/SdnDiag.Utilities.psm1:518

  • This bug fix changes credential handling for the exported Copy-SdnFileFromComputer path, but tests/offline/Utilities.Tests.ps1 has no regression coverage for forwarding the credential, creating the authenticated drive, and removing it on success/failure. Please add mocked offline Pester tests that would have caught #269.

This issue also appears on line 771 of the same file.

                    Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop

@arudell
Adam Rudell (arudell) marked this pull request as ready for review August 13, 2026 16:14
@arudell
Adam Rudell (arudell) requested a review from a team as a code owner August 13, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copy using SMB files as -Credential is not passed

3 participants