fix: JSON parsing of S2A addresses. #1589

rmehta19 · 2024-12-09T23:51:06Z

Autoconfig endpoint returns

{
  s2a:
  {
    plaintext_address: ""
    mtls_address: "" 
  }
}

not

{
  plaintext_address: ""
  mtls_address: "" 
}

rmehta19 · 2024-12-09T23:54:15Z

@lqiu96 , @zhumin8, please review this PR, thanks!

lqiu96 · 2024-12-10T16:15:53Z

oauth2_http/java/com/google/auth/oauth2/SecureSessionAgent.java

+    Object value = map.get(S2A_JSON_KEY);
+    if (value == null) {
+      throw new IOException(
+          String.format(OAuth2Utils.VALUE_NOT_FOUND_MESSAGE, errorPrefix, S2A_JSON_KEY));
+    }
+    if (!(value instanceof Map)) {
+      throw new IOException(
+          String.format(OAuth2Utils.VALUE_WRONG_TYPE_MESSAGE, errorPrefix, "Map", S2A_JSON_KEY));
+    }


Can retrieving the map be moved out to the calling method? It's shared by when parsing both plaintextS2AAdress and mtlsS2AAddress? If we do need validateString, then I think it should only be validating the inputted string.

Yes, we can do this, I've moved it out to the calling function in the latest commit.

lqiu96 · 2024-12-10T16:16:51Z

oauth2_http/java/com/google/auth/oauth2/SecureSessionAgent.java

+    if (!(value instanceof Map)) {
+      throw new IOException(
+          String.format(OAuth2Utils.VALUE_WRONG_TYPE_MESSAGE, errorPrefix, "Map", S2A_JSON_KEY));
+    }


qq, is this check necessary? Is it possible that it may be returned as an array? I was under the assumption that we had control over this API. Or is this just intended as a defensive check?

This is not really necessary, as you mention we own the MDS autoconfig endpoint. I have gotten rid of this check in the latest commit

lqiu96 · 2024-12-10T16:21:46Z

oauth2_http/java/com/google/auth/oauth2/SecureSessionAgent.java

+    Object address = ((Map<String, Object>) value).get(key);
+    if (!(address instanceof String)) {
+      throw new IOException(
+          String.format(OAuth2Utils.VALUE_WRONG_TYPE_MESSAGE, errorPrefix, "string", key));
+    }
+    return (String) address;
+  }


If we can extract the above out, can we just revert back to using Oauth2Utils.validateString(...) and remove this method? This implementation looks to be the same as

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/OAuth2Utils.java

Lines 151 to 158 in cc9826e

Object value = map.get(key);

if (value == null) {

throw new IOException(String.format(VALUE_NOT_FOUND_MESSAGE, errorPrefix, key));

}

if (!(value instanceof String)) {

throw new IOException(String.format(VALUE_WRONG_TYPE_MESSAGE, errorPrefix, "string", key));

}

return (String) value;

Yes, it is the same implementation, I extracted out the additional parsing logic into the calling function and reverted back to using Oauth2Utils.validateString(...) in the latest commit

rmehta19 · 2024-12-10T18:07:42Z

Thanks for the review @lqiu96 !

lqiu96 · 2024-12-10T20:13:26Z

oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java

-          for (Map.Entry<String, String> entrySet : s2aContentMap.entrySet()) {
-            content.put(entrySet.getKey(), entrySet.getValue());
-          }
+          content.put("s2a", s2aContentMap);


nit: Can you use the constant from above?

Done in the latest commit.

lqiu96 · 2024-12-10T20:13:51Z

oauth2_http/java/com/google/auth/oauth2/SecureSessionAgent.java

@@ -188,17 +190,28 @@ private SecureSessionAgentConfig getSecureSessionAgentConfigFromMDS() {

    String plaintextS2AAddress = "";
    String mtlsS2AAddress = "";
+    Object s2aAddressConfig = responseData.get(S2A_JSON_KEY);


nit: Can you cast to Map<String, Object> here so it doesn't need to be done twice below?

Done in the latest commit

lqiu96

Added a few nits if you could resolve prior to merging

rmehta19 · 2024-12-10T22:27:26Z

Thanks @lqiu96, I've addressed them, and I think the PR should be good to merge now.

rmehta19 · 2024-12-10T23:11:23Z

I see the SonarCloud / Build Github action test failed. From looking at the log, I don't think it's related to this PR. WDYT @lqiu96 ?

lqiu96 · 2024-12-11T01:33:31Z

Should be fixed in a separate PR. Feel free to merge

rmehta19 · 2024-12-11T18:30:41Z

Thanks @lqiu96 -- I sycned the branch with main. I don't think I have permissions to merge, would you be able to?

| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [com.google.http-client:google-http-client-jackson2](https://github.com/googleapis/google-http-java-client) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.45.2` -> `1.45.3` | | [com.google.http-client:google-http-client](https://github.com/googleapis/google-http-java-client) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.45.2` -> `1.45.3` | | [com.google.auth:google-auth-library-oauth2-http](https://github.com/googleapis/google-auth-library-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.30.0` -> `1.30.1` | | [com.google.auth:google-auth-library-credentials](https://github.com/googleapis/google-auth-library-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.30.0` -> `1.30.1` | | [org.flywaydb:flyway-gradle-plugin](https://flywaydb.org) ([source](https://github.com/flyway/flyway)) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `11.0.1` -> `11.1.0` | | [com.github.docker-java:docker-java-transport](https://github.com/docker-java/docker-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `3.4.0` -> `3.4.1` | | [com.github.docker-java:docker-java-transport-httpclient5](https://github.com/docker-java/docker-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `3.4.0` -> `3.4.1` | | [com.github.docker-java:docker-java-core](https://github.com/docker-java/docker-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `3.4.0` -> `3.4.1` | | [com.github.docker-java:docker-java-api](https://github.com/docker-java/docker-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `3.4.0` -> `3.4.1` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.29.31` -> `2.29.32` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.29.31` -> `2.29.32` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.29.31` -> `2.29.32` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.29.31` -> `2.29.32` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.29.31` -> `2.29.32` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.29.31` -> `2.29.32` | --- > ⚠️ **Warning** > > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>googleapis/google-http-java-client (com.google.http-client:google-http-client-jackson2)</summary> ### [`v1.45.3`](https://github.com/googleapis/google-http-java-client/blob/HEAD/CHANGELOG.md#1453-2024-12-11) ##### Dependencies - Update dependency io.grpc:grpc-context to v1.69.0 ([#2050](googleapis/google-http-java-client#2050)) ([9f4f6ab](googleapis/google-http-java-client@9f4f6ab)) - Update github/codeql-action action to v3.27.7 ([#2049](googleapis/google-http-java-client#2049)) ([9190382](googleapis/google-http-java-client@9190382)) </details> <details> <summary>googleapis/google-auth-library-java (com.google.auth:google-auth-library-oauth2-http)</summary> ### [`v1.30.1`](https://github.com/googleapis/google-auth-library-java/blob/HEAD/CHANGELOG.md#1301-2024-12-11) ##### Bug Fixes - JSON parsing of S2A addresses. ([#1589](googleapis/google-auth-library-java#1589)) ([9d5ebfe](googleapis/google-auth-library-java@9d5ebfe)) </details> <details> <summary>docker-java/docker-java (com.github.docker-java:docker-java-transport)</summary> ### [`v3.4.1`](https://github.com/docker-java/docker-java/releases/tag/3.4.1) [Compare Source](docker-java/docker-java@3.4.0...3.4.1) ##### Changes - Fix restart test [@eddumelendez](https://github.com/eddumelendez) ([#2375](docker-java/docker-java#2375)) ##### 📈 Enhancements - Add support for CgroupVersion and CgroupDriver [@LarsSven](https://github.com/LarsSven) ([#2360](docker-java/docker-java#2360)) ##### 🧰 Maintenance - Don't swallow IOException caused by opening socket [@Sineaggi](https://github.com/Sineaggi) ([#2041](docker-java/docker-java#2041)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 6eb3a8d6c07d3090499286e2624f648323e96355

fix: JSON parsing of S2A addresses.

57ab3c7

rmehta19 requested review from a team as code owners December 9, 2024 23:51

product-auto-label bot added the size: s Pull request size is small. label Dec 9, 2024

lqiu96 reviewed Dec 10, 2024

View reviewed changes

extract extra parsing logic to calling method.

f98b1ba

lqiu96 reviewed Dec 10, 2024

View reviewed changes

lqiu96 approved these changes Dec 10, 2024

View reviewed changes

cast once + use defined constant.

8c39dcb

Merge branch 'main' into fix-s2a-addr-parse

f1c15d6

lqiu96 merged commit 9d5ebfe into googleapis:main Dec 11, 2024
14 of 15 checks passed

release-please bot mentioned this pull request Dec 11, 2024

chore(main): release 1.30.1 #1593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: JSON parsing of S2A addresses. #1589

fix: JSON parsing of S2A addresses. #1589

rmehta19 commented Dec 9, 2024 •

edited

Loading

rmehta19 commented Dec 9, 2024

lqiu96 Dec 10, 2024 •

edited

Loading

rmehta19 Dec 10, 2024

lqiu96 Dec 10, 2024

rmehta19 Dec 10, 2024

lqiu96 Dec 10, 2024

rmehta19 Dec 10, 2024

rmehta19 commented Dec 10, 2024

lqiu96 Dec 10, 2024 •

edited

Loading

rmehta19 Dec 10, 2024

lqiu96 Dec 10, 2024 •

edited

Loading

rmehta19 Dec 10, 2024

lqiu96 left a comment

rmehta19 commented Dec 10, 2024

rmehta19 commented Dec 10, 2024

lqiu96 commented Dec 11, 2024

rmehta19 commented Dec 11, 2024

	Object value = map.get(key);
	if (value == null) {
	throw new IOException(String.format(VALUE_NOT_FOUND_MESSAGE, errorPrefix, key));
	}
	if (!(value instanceof String)) {
	throw new IOException(String.format(VALUE_WRONG_TYPE_MESSAGE, errorPrefix, "string", key));
	}
	return (String) value;

fix: JSON parsing of S2A addresses. #1589

fix: JSON parsing of S2A addresses. #1589

Conversation

rmehta19 commented Dec 9, 2024 • edited Loading

rmehta19 commented Dec 9, 2024

lqiu96 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

rmehta19 Dec 10, 2024

Choose a reason for hiding this comment

lqiu96 Dec 10, 2024

Choose a reason for hiding this comment

rmehta19 Dec 10, 2024

Choose a reason for hiding this comment

lqiu96 Dec 10, 2024

Choose a reason for hiding this comment

rmehta19 Dec 10, 2024

Choose a reason for hiding this comment

rmehta19 commented Dec 10, 2024

lqiu96 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

rmehta19 Dec 10, 2024

Choose a reason for hiding this comment

lqiu96 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

rmehta19 Dec 10, 2024

Choose a reason for hiding this comment

lqiu96 left a comment

Choose a reason for hiding this comment

rmehta19 commented Dec 10, 2024

rmehta19 commented Dec 10, 2024

lqiu96 commented Dec 11, 2024

rmehta19 commented Dec 11, 2024

rmehta19 commented Dec 9, 2024 •

edited

Loading

lqiu96 Dec 10, 2024 •

edited

Loading

lqiu96 Dec 10, 2024 •

edited

Loading

lqiu96 Dec 10, 2024 •

edited

Loading