chore: Refactor x509Provider to create a shared Utils class for Mtls#1907
chore: Refactor x509Provider to create a shared Utils class for Mtls#1907vverman wants to merge 10 commits intogoogleapis:mainfrom
Conversation
vverman
force-pushed
the
refactor-x509-provider
branch
from
1500bea to
f78ed98
Compare
| * | ||
| * <p>For internal use only. | ||
| */ | ||
| public class SystemEnvironmentProvider implements EnvironmentProvider, Serializable { |
There was a problem hiding this comment.
qq, what is the difference between SystemEnvironmentProvider and EnvironmentProvider?
There was a problem hiding this comment.
An EnvProvider is an interface which can be implemented by test-classes to pass in their own env variables.
The SystemEnvProvider implements this EnvProvider and implements the associated functions such that the env-variables are fetched from the system env.
There was a problem hiding this comment.
Got it. Can you add @internalapi to this class and for SystemPropertyProvider?
Since this is a refactor, I'm going to change the title to |
lqiu96
changed the title
|
E
El martes, 24 de marzo de 2026, ***@***.***> escribió:
… ***@***.**** commented on this pull request.
------------------------------
In oauth2_http/java/com/google/auth/mtls/X509Provider.java
<#1907 (comment)>
:
> + public X509Provider(
+ EnvironmentProvider envProvider,
+ PropertyProvider propProvider,
+ String certConfigPathOverride) {
Thanks for the suggestion.
I think we can leave this one out for now because I don't think the
argument list is big enough to justify the Builder's inclusion and we don't
anticipate any changes to X509Provider (in the short-term).
In case this changes, we can implement it then.
—
Reply to this email directly, view it on GitHub
<#1907?email_source=notifications&email_token=B5MKPO3RH26N37TDJ35VCOL4SL3TJA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBQGIZTKMBWGUYKM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r2984325705>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B5MKPOZWHPYIKAJ5GNPMM5L4SL3TJAVCNFSM6AAAAACW43RKUSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMBSGM2TANRVGA>
.
You are receiving this because you are subscribed to this thread.Message
ID: <googleapis/google-auth-library-java/pull/1907/review/4002350650@
github.com>
|
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class TestPropertyProvider implements PropertyProvider { |
There was a problem hiding this comment.
qq, is this TestPropertyProvider used anywhere in the tests? If not, can we remove it?
There was a problem hiding this comment.
Since this is used to test the System Properties, added in a test for the windows OS default file path in X509ProviderTest.
| } else { | ||
| String osName = propProvider.getProperty("os.name", "").toLowerCase(Locale.US); | ||
| if (osName.indexOf("windows") >= 0) { | ||
| File appDataPath = new File(envProvider.getEnv("APPDATA")); |
There was a problem hiding this comment.
If APPDATA is unset on Windows, envProvider.getEnv("APPDATA") returns null. Creating a new File(null) throws a NullPointerException.
We can align with how Linux handles its user.home fallback by checking user.home as a fallback when APPDATA is missing.
There was a problem hiding this comment.
I believe this is the same logic to detect the default 'directory' where the ADC file is, within the GoogleAuthUtils.getWellKnownCredentialsFile
I believe this should stay as is since the discovery for ADC and cert config should be the same and having a default for windows might lead to an unexpected outcome for cert_config as compared to ADC.
Do lmk your thoughts.
4 participants
Fixes #1745
Addressed a concern raised by Andy refer.
Now X509Provider only exposes the necessary methods needed by the MtlsProvider interface