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

[WFCORE-7102]: AccessDeniedException on Windows when using a read-only #6287

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ehsavoie
Copy link
Contributor

configuration dir.

  • Replacing java.io.File.canWrite() by java.nio.file.FIles.isWritable(Path).

Jira: https://issues.redhat.com/browse/WFCORE-7102

@yersan
Copy link
Collaborator

yersan commented Dec 17, 2024

thanks @ehsavoie , Ideally, we should enable this test for Windows with this PR: https://github.com/wildfly/wildfly-core/blob/main/testsuite/manualmode/src/test/java/org/jboss/as/test/manualmode/management/ReadOnlyModeTestCase.java#L72-L74

But I know there are currently issues setting a read-only directory in the code since the CI won't be able to delete it. We can discuss it with @luck3y if there is any way to enable that permissions for Windows OS

@wildfly-ci

This comment was marked as off-topic.

@luck3y
Copy link
Contributor

luck3y commented Dec 17, 2024 via email

@wildfly-ci

This comment was marked as off-topic.

@wildfly-ci

This comment was marked as outdated.

@ehsavoie ehsavoie force-pushed the WFCORE-7102 branch 2 times, most recently from bd7f05f to eb173c8 Compare December 18, 2024 17:54
@ehsavoie
Copy link
Contributor Author

@yersan This is properly fixed now and it works

@ehsavoie ehsavoie added the 27.x label Dec 18, 2024
@ehsavoie ehsavoie force-pushed the WFCORE-7102 branch 4 times, most recently from d0b7664 to 6f5d3fa Compare December 19, 2024 17:57
@wildfly-ci

This comment was marked as off-topic.

@wildfly-ci

This comment was marked as off-topic.

@yersan yersan added 28.x and removed 27.x labels Dec 23, 2024
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
@wildfly wildfly deleted a comment from wildfly-ci Jan 14, 2025
Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

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

I've made some trivial change requests. I've not tried to understand the meat of this, so please don't regard fixing my requests as an approval; rely on the others who are reviewing.

OTOH, mergers please don't block on my re-reviewing my trivial items before merging.

@bstansberry bstansberry requested a review from luck3y January 14, 2025 20:45
@bstansberry
Copy link
Contributor

@luck3y Please review.


Files.getFileAttributeView(roConfigDir, PosixFileAttributeView.class).setPermissions(perms);

if (TestSuiteEnvironment.isWindows()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems odd to me, for Windows an ACL is constructed that denies the listed permissions, which seems correct, but on all other OSes are added as allowed. I'm probably missing something here though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ehsavoie FYI ^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well 1st, I only looked at Windows but the problem is that the permissions are quite complex with a lot of principals so I reduced the change to the owner only and removing a permission isn't sufficient in Windows you have to DENY it.

…y configuration dir.

* Replacing java.io.File.canWrite() by java.nio.file.Files.isWritable(Path) in:
   - ConfigurationFile
   - FilePersistenceUtils
   - ConfigurationFilePersistenceResource
* Enabling the test for Windows and chaning the temp folder creation

Jira: https://issues.redhat.com/browse/WFCORE-7102

Signed-off-by: Emmanuel Hugonnet <[email protected]>
@bstansberry bstansberry dismissed their stale review January 17, 2025 20:34

The trivial items I mentioned have been addressed.

Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

LGTM, @luck3y Do you have any additional review from your side?

@luck3y
Copy link
Contributor

luck3y commented Jan 28, 2025

LGTM, @luck3y Do you have any additional review from your side?

LGTM too, I looked at the code again, its only adding read and execute, no write, so I must have misread it the first time.

@luck3y luck3y self-requested a review January 28, 2025 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants