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.

Sign up for GitHub

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

Jump to bottom

fix: incorrect check for Windows OS in FileDataStoreFactory #927

Merged
merged 3 commits into from
Mar 25, 2020
Merged

fix: incorrect check for Windows OS in FileDataStoreFactory #927

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a3613a
fix: incorrect check for Windows OS in FileDataStoreFactory
diesieben07 Dec 28, 2019
c97c34e
fix: properly get file owner on windows in FileDataStoreFactory
diesieben07 Dec 28, 2019
6bc4da2
refactor: use toLowerCase instead of regionMatches in FileDataStoreFa…
diesieben07 Dec 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.nio.file.attribute.AclEntryPermission;
import java.nio.file.attribute.AclEntryType;
import java.nio.file.attribute.AclFileAttributeView;
import java.nio.file.attribute.FileOwnerAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.UserPrincipal;
import java.util.HashSet;
Expand All @@ -53,7 +54,7 @@ public class FileDataStoreFactory extends AbstractDataStoreFactory {
private static final Logger LOGGER = Logger.getLogger(FileDataStoreFactory.class.getName());

private static final boolean IS_WINDOWS = StandardSystemProperty.OS_NAME.value()
.startsWith("WINDOWS");
.regionMatches(true, 0, "WINDOWS", 0, "WINDOWS".length());
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to depend on the default character set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are just dealing with strings here, they are always in utf-16. Do you mean default locale? If so, I don't think so. The Javadoc for regionMatches does not mention anything about locale and the source code also does not do anything locale dependent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for testing...
The PR that introduced this check mentions waiting for a windows test machine, which was then apparently addressed in a different PR. I am not familiar with the automated testing setup for this library, but through searching the web I have not been able to find an instance where os.name ever contained "WINDOWS" (all uppercase). So I am not sure how the initial PR even passed testing on any kind of Windows system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See: #557

Copy link
Contributor

Choose a reason for hiding this comment

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

Case insensitive comparison is fundamentally locale dependent. E.g. I is not the upper case of i in Turkish locales. UTF-16 is a character set, not a locale.

Copy link
Contributor Author

@diesieben07 diesieben07 Dec 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

UTF-16 is a character set, not a locale.

Yes. Which is why your original message saying "default character set" confused me.

Case insensitive comparison is fundamentally locale dependent. E.g. I is not the upper case of i in Turkish locales.

I know. regionMatches does not take this into account. regionMatches uses Character.toLowerCase, which specifically mentions:

{@code String} case mapping methods can perform locale-sensitive mappings, context-sensitive mappings, and 1:M character mappings, whereas the {@code Character} case mapping methods cannot.

If you prefer i'll change it to a toLowerCase(Locale.ENGLISH) version though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be good. I still need to think about how to test this.


/** Directory to store data. */
private final File dataDirectory;
Expand Down Expand Up @@ -155,8 +156,8 @@ static void setPermissionsToOwnerOnly(File file) throws IOException {

static void setPermissionsToOwnerOnlyWindows(File file) throws IOException {
Path path = Paths.get(file.getAbsolutePath());
UserPrincipal owner = path.getFileSystem().getUserPrincipalLookupService()
.lookupPrincipalByName("OWNER@");
FileOwnerAttributeView fileAttributeView = Files.getFileAttributeView(path, FileOwnerAttributeView.class);
UserPrincipal owner = fileAttributeView.getOwner();

// get view
AclFileAttributeView view = Files.getFileAttributeView(path, AclFileAttributeView.class);
Expand Down