hierynomus / smbj

Server Message Block (SMB2, SMB3) implementation in Java
Other
707 stars 180 forks source link

Unsetting Read-Only File Attribute failing #234

Open ronchalant opened 6 years ago

ronchalant commented 6 years ago

Note: This is with a PureStorage SMBv2 share.

When issuing a DiskShare.setFileInformation call to unset a read-only flag for a given file it indicating "access denied":

com.hierynomus.mssmb2.SMBApiException: STATUS_ACCESS_DENIED(3221225506/3221225506): Create failed for {file}

This is a code snippet that replicates the issue:

        SMBClient client = new SMBClient();
        try (Connection connection = client.connect(host)) {
            AuthenticationContext ac = new AuthenticationContext(user, password.toCharArray(), domain);
            Session session = connection.authenticate(ac);

            // Connect to Share
            try (DiskShare share = (DiskShare) session.connectShare(shareName)) {
                FileBasicInformation current = share.getFileInformation(path, FileBasicInformation.class);
                long isReadOnly = current.getFileAttributes() & FileAttributes.FILE_ATTRIBUTE_READONLY.getValue();
                if (isReadOnly == FileAttributes.FILE_ATTRIBUTE_READONLY.getValue()) {
                    long newAttrs = current.getFileAttributes() ^ FileAttributes.FILE_ATTRIBUTE_READONLY.getValue();
                    FileBasicInformation update = new FileBasicInformation(
                            current.getCreationTime(),
                            current.getLastAccessTime(),
                            current.getLastWriteTime(),
                            current.getChangeTime(),
                            newAttrs
                    );
                    share.setFileInformation(path, update);
                }
            }
        }

This was the only way I could find to update file attributes, if there is a better approach I'm all ears - just started using SMBJ this week.

The work-around I found that works for this is to open the file with an AccessMask of FILE_WRITE_ATTRIBUTES only:

        SMBClient client = new SMBClient();
        try (Connection connection = client.connect(host)) {
            AuthenticationContext ac = new AuthenticationContext(user, password.toCharArray(), domain);
            Session session = connection.authenticate(ac);

            // Connect to Share
            try (DiskShare share = (DiskShare) session.connectShare(shareName)) {
                FileBasicInformation current = share.getFileInformation(path, FileBasicInformation.class);
                long isReadOnly = current.getFileAttributes() & FileAttributes.FILE_ATTRIBUTE_READONLY.getValue();
                if (isReadOnly == FileAttributes.FILE_ATTRIBUTE_READONLY.getValue()) {
                    long newAttrs = current.getFileAttributes() ^ FileAttributes.FILE_ATTRIBUTE_READONLY.getValue();
                    FileBasicInformation update = new FileBasicInformation(
                            current.getCreationTime(),
                            current.getLastAccessTime(),
                            current.getLastWriteTime(),
                            current.getChangeTime(),
                            newAttrs
                    );

                    DiskEntry e = share.open(path,
                            EnumSet.of(AccessMask.FILE_WRITE_ATTRIBUTES),
                            (Set) null,
                            SMB2ShareAccess.ALL,
                            SMB2CreateDisposition.FILE_OPEN,
                            (Set) null);

                    e.setFileInformation(update);
                }
            }
        }

Without changing the existing setFileInformation in a way that might impact other setFileInformation use cases, the only way I can see to correct this would be to add a setFileAttributes(String path, long attrs) method to the DiskShare class, or overloading the setFileInformation to allow for overriding AccessMask value(s).

Alternatively resolving the AccessMask set required by the FileSettableInformation implementation is possible, but that may be heavy handed.

ricardo1604 commented 4 years ago

Hi @ronchalant
Do you close the open file at finished? Regards

Ricardo

ronchalant commented 4 years ago

It's been a few years :)

The connection and diskshare objects should both be closing by being in the try () block.

However in my sample/workaround, the disk entry opening here:

DiskEntry e = share.open(path,
                            EnumSet.of(AccessMask.FILE_WRITE_ATTRIBUTES),
                            (Set) null,
                            SMB2ShareAccess.ALL,
                            SMB2CreateDisposition.FILE_OPEN,
                            (Set) null);

appears to not actually be closed. Looks like I messed up there; should be closed explicitly or in a try () block.

The above snippets were from a test case I put together to demonstrate the issue and a workaround, not from my prod code - I'll take a look to ensure that it's being closed.

ricardo1604 commented 4 years ago

Thanks @ronchalant You're right the connection and diskshare should be closed in a try() block. whether you find anything else in your prod code, I will be grateful that you share it

ronchalant commented 4 years ago

My production code is pretty similar to the above functionally. I have not run into issues in production with this, though the DiskEntry looks like it also should be closed.

My guess is that closing the accompanying share will flush and close any open DiskEntry operations.

ricardo1604 commented 4 years ago

Thanks ! Regards

ndimitry commented 3 years ago

I added the code to remove read-only recently. By the way, this thread helped me. The main thing I see is (as you mentioned): it is required to open file before changing its attributes (openning with write-attribute rights is essential). It is not a workaround, it is the way it works.

I would also replace bitwise xor ^ by bitwise not combined with and to ensure setting the flag to false (proposed solution with ^ will set true in case originally read-only flag was false, i.e. it will change the value whatever it was):

// this will ensure the flag is set to 0
long newAttrs = current.getFileAttributes() & (~FileAttributes.FILE_ATTRIBUTE_READONLY.getValue());
ManuelQS commented 1 month ago

Using this code to remove read-only flags, had to add the following special-case:

// from the FileAttributes javadoc: "a value of 0x00000000 in the FileAttributes
// field means that the file attributes for this file MUST NOT be changed"
if (newAttrs == 0l)
    newAttrs = FileAttributes.FILE_ATTRIBUTE_NORMAL.getValue();