ansh0014 commented on code in PR #6886:
URL: https://github.com/apache/camel-k/pull/6886#discussion_r4290458536
##########
pkg/cmd/source/util_test.go:
##########
@@ -42,34 +43,45 @@ func isWindows() bool {
return runtime.GOOS == "windows"
}
-func TestPermissionDenied(t *testing.T) {
-
+func denyAccess(t *testing.T, dir string) func() {
+ t.Helper()
if isWindows() {
- t.Skip("Test not reliably producing a result on a windows OS")
+ out, err := exec.Command("icacls", dir, "/deny",
"*S-1-1-0:(OI)(CI)(F)").CombinedOutput()
Review Comment:
Thanks for the feedback @squakez !
Totally agree with you on not executing external shell commands like
`icacls` in unit tests for security reasons.
To explain why this was tricky and why the test originally failed on Windows:
According to the official [Go `os.Chmod`
documentation](https://pkg.go.dev/os#Chmod):
> *"On Windows, only the 0o200 bit (owner writable) of mode is used; it
controls whether the file's read-only attribute is set or cleared. The other
bits are currently unused. For directories, the write permission has no effect
on access or creation of files within the directory."*
Because of this Go standard library behavior on Windows:
- Running `os.Chmod(dir, 0000)` does not restrict directory traversal or
read access on Windows NTFS.
- As a result, `os.Stat(filename)` succeeds with `err == nil`, so the OS
never raises a permission error to the business logic to begin with.
However, we **did fix a real business logic bug** in
`pkg/cmd/source/util.go`:
Previously on Windows, `if _, ok := err.(*fs.PathError); ok && runtime.GOOS
== "windows"` was catching and swallowing **all** errors from `os.Stat` as
`return false, nil`, masking genuine permission errors. Adding `errors.Is(err,
fs.ErrPermission)` before that fallback ensures permission errors are properly
reported on all operating systems.
If you like this approach, I will gladly update the PR shortly to remove
`icacls` and keep the test as pure Go (with an updated comment explaining why
the test is skipped on Windows due to Go's `os.Chmod` limitation).
Let me know what you think!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
