Skip to content
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

[Import-Excel] Don't request ReadWrite access to the file #1601

Closed

Conversation

edwardmiller-mesirow
Copy link
Contributor

@edwardmiller-mesirow edwardmiller-mesirow commented May 17, 2024

Write access can cause locking issues, and I don't think it is really needed for this cmdlet.

Tested this locally and it works

This can cause locking issues, and I don't think it is really needed for this cmdlet.
@edwardmiller-mesirow edwardmiller-mesirow changed the title Don't request Write access to the file for Import-Excel [Import-Excel] Don't request ReadWrite access to the file May 18, 2024
@dfinke dfinke self-assigned this May 18, 2024
@dfinke
Copy link
Owner

dfinke commented May 18, 2024

Interesting, have not had a locking issue. Will investigate.

@edwardmiller-mesirow
Copy link
Contributor Author

edwardmiller-mesirow commented May 18, 2024

I switched to this module because there was some terrible existing OLEDB-based powershell code we had that was suffering from driver issues after migrating it to another server with an intentionally obsolete Office version.

Switching to this library actually solved that for the import step, but then a subsequent archive step was then running into locking issues from a file on a network drive.

It could literally be the case that the file was just open somewhere. But I also wanted to eliminate any possibility that a temporary file lock is not released.

I can see by searching this library for ReadWrite that the original code for this was added precisely to circumvent locking issues.

But, the original PR author seemed to think the ReadWrite permissions were what fixed that, and I think the usage of the stream is what fixed it and ReadWrite permissions are not necessary.

In terms of the principle of least privilege it just doesn't make sense to grant this process write permission to the file since it is not writing to it.

@edwardmiller-mesirow
Copy link
Contributor Author

In fairness, with the existing code I cannot locally replicate a locking issue either with or without this change. But, it does work with less privileges and that feels like a win.

@edwardmiller-mesirow
Copy link
Contributor Author

Here was the relevant related PR that I mentioned.

#35

@nlsdg
Copy link

nlsdg commented Jun 26, 2024

The ReadWrite in the code is for the FileShare enum. This parameter is to control how other processes can access the file.
It doesn't mean we request write access to the file. That is done through the FileAccess parameter (but it is set to Read).

If you go ahead with this change, you could run into problems.

  1. Requests to open the file for writing by this or another process will fail until the FileStream object has been closed, but read attempts will succeed.
  2. If another process already has the file open for writing the call will fail, because it tries to gain exclusive access. (which was the reason to change it in Allow reading a file already open in Excel #35.)
    Check the documentation on the System.IO.Filestream constructors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants