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

fix: incorrect check for Windows OS in FileDataStoreFactory #927

Merged

Conversation

diesieben07
Copy link
Contributor

@diesieben07 diesieben07 commented Dec 28, 2019

The "Windows" part of os.name is not always uppercase. On Windows 10 for example I get "Windows 10", which does not pass the current check. This leads to setPermissionsToOwnerOnly instead of setPermissionsToOwnerOnlyWindows being executed, which then issues the following warning (and does nothing): Unable to set permissions for C:\example, because you are running on a non-POSIX file system

Fixes #953

@diesieben07 diesieben07 requested a review from as a code owner Dec 28, 2019
@googlebot googlebot added the cla: yes label Dec 28, 2019
@diesieben07 diesieben07 force-pushed the filedatastorefactory-windows-check branch from 779af17 to 7a3613a Compare Dec 28, 2019
@elharo elharo added the kokoro:force-run label Dec 28, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Dec 28, 2019
Copy link
Collaborator

@elharo elharo left a comment

Any way to test this?

@@ -53,7 +53,7 @@
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
Collaborator

@elharo elharo Dec 28, 2019

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

@diesieben07 diesieben07 Dec 28, 2019

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

@diesieben07 diesieben07 Dec 28, 2019

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

@diesieben07 diesieben07 Dec 28, 2019

Choose a reason for hiding this comment

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

See: #557

Copy link
Collaborator

@elharo elharo Dec 28, 2019

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

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
Collaborator

@elharo elharo Dec 29, 2019

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.

@diesieben07
Copy link
Contributor Author

@diesieben07 diesieben07 commented Dec 28, 2019

Turns out the implementation in setPermissionsToOwnerOnlyWindows did not actually work (as indicated by the test failures that occurred on Windows).
To me this means that when the original PR happened (#557) it just never got tested, because the IS_WINDOWS check never worked. This caused the Windows test runs to also use setPermissionsToOwnerOnly, which does not fail the tests, it simply logs a warning.

I've fixed obtaining the owning UserPrinicipal, which should also fix the tests on windows.

@diesieben07 diesieben07 changed the title Fix incorrect check for Windows OS in FileDataStoreFactory fix: incorrect check for Windows OS in FileDataStoreFactory Dec 28, 2019
@elharo elharo added the kokoro:force-run label Dec 28, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Dec 28, 2019
Copy link
Collaborator

@elharo elharo left a comment

This is probably just unrelated flakiness but there is a test failure on Java 8:

[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.115 s - in com.google.api.client.http.javanet.NetHttpRequestTest
[INFO] Running com.google.api.client.http.javanet.NetHttpTransportTest
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.002 s - in com.google.api.client.http.javanet.NetHttpTransportTest
[INFO] Running com.google.api.client.http.HttpRequestTracingTest
Dec 28, 2019 11:05:15 PM io.opencensus.implcore.trace.RecordEventsSpanImpl finalize
SEVERE: Span Sent.com.google.api.client.http.HttpRequest.execute is GC'ed without being ended.
Dec 28, 2019 11:05:15 PM io.opencensus.implcore.trace.RecordEventsSpanImpl finalize
SEVERE: Span Sent.com.google.api.client.http.HttpRequest.execute is GC'ed without being ended.
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 8.528 s <<< FAILURE! - in com.google.api.client.http.HttpRequestTracingTest
[ERROR] executeExceptionCreatesSpan(com.google.api.client.http.HttpRequestTracingTest) Time elapsed: 3.528 s <<< FAILURE!
java.lang.AssertionError: expected:<1> but was:<2>
at com.google.api.client.http.HttpRequestTracingTest.executeExceptionCreatesSpan(HttpRequestTracingTest.java:119)

@diesieben07
Copy link
Contributor Author

@diesieben07 diesieben07 commented Dec 29, 2019

I've changed the code to use toLowerCase(Locale.ENGLISH) instead to be more explicit about localization rules.

Not sure why that test failed, it seems to be "unrelated flakyness", like you said.

@elharo elharo added the kokoro:force-run label Dec 29, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Dec 29, 2019
@MartijnWoudstra
Copy link

@MartijnWoudstra MartijnWoudstra commented Mar 24, 2020

I'd still love to have this in, since this is blocking my current development

Copy link
Collaborator

@elharo elharo left a comment

" This caused the Windows test runs to also use setPermissionsToOwnerOnly, which does not fail the tests, it simply logs a warning."

Good detective work. Should this warning be upgraded to an error in the tests somehow?

Also, am I understanding you that the first change in line 56 by itself caused existing tests to fail, and then the additional change in line 160 caused the tests to pass again?

@diesieben07
Copy link
Contributor Author

@diesieben07 diesieben07 commented Mar 25, 2020

Also, am I understanding you that the first change in line 56 by itself caused existing tests to fail, and then the additional change in line 160 caused the tests to pass again?

Correct. The change in line 56 changes Windows to actually use setPermissionsToOwnerOnlyWindows. The original implementation of that method was broken and threw an exception, causing test failures on Windows to now happen.
The change in line 160 fixes setPermissionsToOwnerOnlyWindows to actually work and no longer throw exceptions.

elharo
elharo approved these changes Mar 25, 2020
@elharo elharo merged commit 8b4eabe into googleapis:master Mar 25, 2020
9 checks passed
@ghost
Copy link

@ghost ghost commented Mar 26, 2020

As I'm not used to Github bare with me if this is inappropriate:
Although I understand the reason behind this check and change it's not common in the Windows world to do such specific permission stuff. Usually on a Windows system the NTFS does a good job with its ACLs to "guard" a users home directory from access by other non-Administrators (same as on Unix: if you're Administrator (root) permissions doesn't apply to you) and any other directory is usual on a somewhat semi-read-only. This all started when all that privilege stuff was introduced to normal people when XP was released, wich was based on Win2k and its NT based security model. The "normal user" didn't wanted to struggle about permissions and rights/access management - so Microsoft itself always had it rather loosely and since UAC came with Vista it was, and still is, common practice to just modify ACLs so that just anyone has full access (pretty much like a 777 on Unix) as most stuff struggle with permissions is used by normal home users on a home version of Windows.
TLDR: I don't see that windows specific check and modification useful at all - aside from the fact that when it was implemented at first no Windows box was available for testing. This may will cause more issues in the future as the broken one just because it's done right now.

gcf-merge-on-green bot pushed a commit that referenced this issue Apr 27, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.35.0](https://www.github.com/googleapis/google-http-java-client/compare/v1.34.2...v1.35.0) (2020-04-27)


### Features

* add logic for verifying ES256 JsonWebSignatures ([#1033](https://www.github.com/googleapis/google-http-java-client/issues/1033)) ([bb4227f](https://www.github.com/googleapis/google-http-java-client/commit/bb4227f9daec44fc2976fa9947e2ff5ee07ed21a))


### Bug Fixes

* add linkage monitor plugin ([#1000](https://www.github.com/googleapis/google-http-java-client/issues/1000)) ([027c227](https://www.github.com/googleapis/google-http-java-client/commit/027c227e558164f77be204152fb47023850b543f))
* Correctly handling chunked response streams with gzip ([#990](https://www.github.com/googleapis/google-http-java-client/issues/990)) ([1ba2197](https://www.github.com/googleapis/google-http-java-client/commit/1ba219743e65c89bc3fdb196acc5d2042e01f542)), closes [#367](https://www.github.com/googleapis/google-http-java-client/issues/367)
* FileDataStoreFactory will throw IOException for any permissions errors ([#1012](https://www.github.com/googleapis/google-http-java-client/issues/1012)) ([fd33073](https://www.github.com/googleapis/google-http-java-client/commit/fd33073da3674997897d7a9057d1d0e9d42d7cd4))
* include request method and URL into HttpResponseException message ([#1002](https://www.github.com/googleapis/google-http-java-client/issues/1002)) ([15111a1](https://www.github.com/googleapis/google-http-java-client/commit/15111a1001d6f72cb92cd2d76aaed6f1229bc14a))
* incorrect check for Windows OS in FileDataStoreFactory ([#927](https://www.github.com/googleapis/google-http-java-client/issues/927)) ([8b4eabe](https://www.github.com/googleapis/google-http-java-client/commit/8b4eabe985794fc64ad6a4a53f8f96201cf73fb8))
* reuse reference instead of calling getter twice ([#983](https://www.github.com/googleapis/google-http-java-client/issues/983)) ([1f66222](https://www.github.com/googleapis/google-http-java-client/commit/1f662224d7bee6e27e8d66975fda39feae0c9359)), closes [#982](https://www.github.com/googleapis/google-http-java-client/issues/982)
* **android:** set minimum API level to 19 a.k.a. 4.4 Kit Kat ([#1016](https://www.github.com/googleapis/google-http-java-client/issues/1016)) ([b9a8023](https://www.github.com/googleapis/google-http-java-client/commit/b9a80232c9c8b16a3c3277458835f72e346f6b2c)), closes [#1015](https://www.github.com/googleapis/google-http-java-client/issues/1015)


### Documentation

* android 4.4 or later is required ([#1008](https://www.github.com/googleapis/google-http-java-client/issues/1008)) ([bcc41dd](https://www.github.com/googleapis/google-http-java-client/commit/bcc41dd615af41ae6fb58287931cbf9c2144a075))
* libraries-bom 4.0.1 ([#976](https://www.github.com/googleapis/google-http-java-client/issues/976)) ([fc21dc4](https://www.github.com/googleapis/google-http-java-client/commit/fc21dc412566ef60d23f1f82db5caf3cfd5d447b))
* libraries-bom 4.1.1 ([#984](https://www.github.com/googleapis/google-http-java-client/issues/984)) ([635c813](https://www.github.com/googleapis/google-http-java-client/commit/635c81352ae383b3abfe6d7c141d987a6944b3e9))
* libraries-bom 5.2.0 ([#1032](https://www.github.com/googleapis/google-http-java-client/issues/1032)) ([ca34202](https://www.github.com/googleapis/google-http-java-client/commit/ca34202bfa077adb70313b6c4562c7a5d904e064))
* require Android 4.4 ([#1007](https://www.github.com/googleapis/google-http-java-client/issues/1007)) ([f9d2bb0](https://www.github.com/googleapis/google-http-java-client/commit/f9d2bb030398fe09e3c47b84ea468603355e08e9))


### Dependencies

* httpclient 4.5.12 ([#991](https://www.github.com/googleapis/google-http-java-client/issues/991)) ([79bc1c7](https://www.github.com/googleapis/google-http-java-client/commit/79bc1c76ebd48d396a080ef715b9f07cd056b7ef))
* update to Guava 29 ([#1024](https://www.github.com/googleapis/google-http-java-client/issues/1024)) ([ca9520f](https://www.github.com/googleapis/google-http-java-client/commit/ca9520f2da4babc5bbd28c828da1deb7dbdc87e5))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants