fix: incorrect check for Windows OS in FileDataStoreFactory #927

diesieben07 · 2019-12-28T22:17:45Z

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

elharo

Any way to test this?

elharo · 2019-12-28T22:27:21Z

google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java

@@ -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());


This appears to depend on the default character set.

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.

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.

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.

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.

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

diesieben07 · 2019-12-28T22:55:12Z

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.

elharo

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)

…ctory

diesieben07 · 2019-12-29T21:26:16Z

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.

MartijnWoudstra · 2020-03-24T17:02:53Z

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

elharo

" 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 · 2020-03-25T09:36:03Z

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.

ghost · 2020-03-26T06:02:11Z

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.

* docs(samples): added client code for idtoken, adc and metadata server * docs(samples): added authexplicit and copyright * docs(samples): add auth with metadata server * docs(samples): minor refactoring and added tests * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * refactored acc to review comments * refactored acc to review comments * refactored acc to review comments * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * minor comment update * modified google id token verification and removed third party dependency * removed third party deps from pom * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * included comment about verifying Google ID tokens * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Shabir Mohamed Abdul Samadh <7249208+Shabirmean@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [1.10.0](googleapis/google-auth-library-java@v1.9.0...v1.10.0) (2022-08-05) ### Features * workforce identity federation for pluggable auth ([googleapis#959](googleapis/google-auth-library-java#959)) ([7f2c535](googleapis/google-auth-library-java@7f2c535)) ### Bug Fixes * updates executable response spec for executable-sourced credentials ([googleapis#955](googleapis/google-auth-library-java#955)) ([48ff83d](googleapis/google-auth-library-java@48ff83d)) ### Documentation * **samples:** added auth samples and tests ([googleapis#927](googleapis/google-auth-library-java#927)) ([32c717f](googleapis/google-auth-library-java@32c717f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

diesieben07 requested a review from a team as a code owner December 28, 2019 22:17

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 28, 2019

fix: incorrect check for Windows OS in FileDataStoreFactory

7a3613a

diesieben07 force-pushed the filedatastorefactory-windows-check branch from 779af17 to 7a3613a Compare December 28, 2019 22:19

elharo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2019

elharo reviewed Dec 28, 2019

View reviewed changes

fix: properly get file owner on windows in FileDataStoreFactory

c97c34e

diesieben07 changed the title ~~Fix incorrect check for Windows OS in FileDataStoreFactory~~ fix: incorrect check for Windows OS in FileDataStoreFactory Dec 28, 2019

elharo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2019

elharo reviewed Dec 29, 2019

View reviewed changes

refactor: use toLowerCase instead of regionMatches in FileDataStoreFa…

6bc4da2

…ctory

elharo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 29, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 29, 2019

elharo reviewed Mar 24, 2020

View reviewed changes

elharo mentioned this pull request Mar 24, 2020

FileDataStoreFactory fails inconsistently when setting permissions #1011

Closed

elharo approved these changes Mar 25, 2020

View reviewed changes

elharo merged commit 8b4eabe into googleapis:master Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: incorrect check for Windows OS in FileDataStoreFactory #927

fix: incorrect check for Windows OS in FileDataStoreFactory #927

diesieben07 commented Dec 28, 2019 •

edited by chingor13

elharo left a comment

elharo Dec 28, 2019

diesieben07 Dec 28, 2019

diesieben07 Dec 28, 2019

diesieben07 Dec 28, 2019

elharo Dec 28, 2019

diesieben07 Dec 28, 2019 •

edited

elharo Dec 29, 2019

diesieben07 commented Dec 28, 2019 •

edited

elharo left a comment

diesieben07 commented Dec 29, 2019

MartijnWoudstra commented Mar 24, 2020

elharo left a comment

diesieben07 commented Mar 25, 2020

ghost commented Mar 26, 2020

fix: incorrect check for Windows OS in FileDataStoreFactory #927

fix: incorrect check for Windows OS in FileDataStoreFactory #927

Conversation

diesieben07 commented Dec 28, 2019 • edited by chingor13

elharo left a comment

Choose a reason for hiding this comment

elharo Dec 28, 2019

Choose a reason for hiding this comment

diesieben07 Dec 28, 2019

Choose a reason for hiding this comment

diesieben07 Dec 28, 2019

Choose a reason for hiding this comment

diesieben07 Dec 28, 2019

Choose a reason for hiding this comment

elharo Dec 28, 2019

Choose a reason for hiding this comment

diesieben07 Dec 28, 2019 • edited

Choose a reason for hiding this comment

elharo Dec 29, 2019

Choose a reason for hiding this comment

diesieben07 commented Dec 28, 2019 • edited

elharo left a comment

Choose a reason for hiding this comment

diesieben07 commented Dec 29, 2019

MartijnWoudstra commented Mar 24, 2020

elharo left a comment

Choose a reason for hiding this comment

diesieben07 commented Mar 25, 2020

ghost commented Mar 26, 2020

diesieben07 commented Dec 28, 2019 •

edited by chingor13

diesieben07 Dec 28, 2019 •

edited

diesieben07 commented Dec 28, 2019 •

edited