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

fix: JSON parsing of S2A addresses. #1589

Merged
merged 4 commits into from
Dec 11, 2024
Merged

fix: JSON parsing of S2A addresses. #1589

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57ab3c7
fix: JSON parsing of S2A addresses.
rmehta19 Dec 9, 2024
f98b1ba
extract extra parsing logic to calling method.
rmehta19 Dec 10, 2024
8c39dcb
cast once + use defined constant.
rmehta19 Dec 10, 2024
f1c15d6
Merge branch 'main' into fix-s2a-addr-parse
rmehta19 Dec 11, 2024
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
4 changes: 2 additions & 2 deletions oauth2_http/java/com/google/auth/oauth2/OAuth2Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ class OAuth2Utils {

static final JsonFactory JSON_FACTORY = GsonFactory.getDefaultInstance();

private static String VALUE_NOT_FOUND_MESSAGE = "%sExpected value %s not found.";
private static String VALUE_WRONG_TYPE_MESSAGE = "%sExpected %s value %s of wrong type.";
static final String VALUE_NOT_FOUND_MESSAGE = "%sExpected value %s not found.";
static final String VALUE_WRONG_TYPE_MESSAGE = "%sExpected %s value %s of wrong type.";

static final String BEARER_PREFIX = AuthHttpConstants.BEARER + " ";

Expand Down
26 changes: 23 additions & 3 deletions oauth2_http/java/com/google/auth/oauth2/SecureSessionAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import java.io.InputStream;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Map;
import java.util.ServiceLoader;
import java.util.Set;
import javax.annotation.concurrent.ThreadSafe;
Expand All @@ -59,6 +60,7 @@
*/
@ThreadSafe
public class SecureSessionAgent {
static final String S2A_JSON_KEY = "s2a";
static final String S2A_PLAINTEXT_ADDRESS_JSON_KEY = "plaintext_address";
static final String S2A_MTLS_ADDRESS_JSON_KEY = "mtls_address";
static final String S2A_CONFIG_ENDPOINT_POSTFIX =
Expand Down Expand Up @@ -190,15 +192,14 @@ private SecureSessionAgentConfig getSecureSessionAgentConfigFromMDS() {
String mtlsS2AAddress = "";
try {
plaintextS2AAddress =
OAuth2Utils.validateString(responseData, S2A_PLAINTEXT_ADDRESS_JSON_KEY, PARSE_ERROR_S2A);
validateString(responseData, S2A_PLAINTEXT_ADDRESS_JSON_KEY, PARSE_ERROR_S2A);
} catch (IOException ignore) {
/*
* Do not throw error because of parsing error, just leave the address as empty in {@link SecureSessionAgentConfig}.
*/
}
try {
mtlsS2AAddress =
OAuth2Utils.validateString(responseData, S2A_MTLS_ADDRESS_JSON_KEY, PARSE_ERROR_S2A);
mtlsS2AAddress = validateString(responseData, S2A_MTLS_ADDRESS_JSON_KEY, PARSE_ERROR_S2A);
} catch (IOException ignore) {
/*
* Do not throw error because of parsing error, just leave the address as empty in {@link SecureSessionAgentConfig}.
Expand All @@ -210,4 +211,23 @@ private SecureSessionAgentConfig getSecureSessionAgentConfigFromMDS() {
.setMtlsAddress(mtlsS2AAddress)
.build();
}

private static String validateString(Map<String, Object> map, String key, String errorPrefix)
throws IOException {
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));
}
Copy link
Contributor

@lqiu96 lqiu96 Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
4 changes: 1 addition & 3 deletions oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,7 @@ public LowLevelHttpResponse execute() throws IOException {
GenericJson content = new GenericJson();
content.setFactory(OAuth2Utils.JSON_FACTORY);
if (requestStatusCode == 200) {
for (Map.Entry<String, String> entrySet : s2aContentMap.entrySet()) {
content.put(entrySet.getKey(), entrySet.getValue());
}
content.put("s2a", s2aContentMap);
Copy link
Contributor

@lqiu96 lqiu96 Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Can you use the constant from above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the latest commit.

}
String contentText = content.toPrettyString();

Expand Down
Loading