Skip to content

Commit

Permalink
Validate signer client options based on authtype (#3579)
Browse files Browse the repository at this point in the history 
This fixes an issue with the client option validation where the client builder
is incorrectly validating that a value for the SIGNER option is present, even
when it's not required (because the service does not use AWS auth).

This change only does the appropriate validations based on the authtypes present
in the service model.
  • Loading branch information
@dagnir
dagnir committed Dec 1, 2022
1 parent 46e49ba commit c119790
Show file tree
Hide file tree
Showing 27 changed files with 1,110 additions and 42 deletions.
6 changes: 6 additions & 0 deletions .changes/next-release/bugfix-AWSSDKforJavav2-7cb7fcf.json
View file
@@ -0,0 +1,6 @@
{
"type": "bugfix",
"category": "AWS SDK for Java v2",
"contributor": "",
"description": "This fixes an issue with the client option validation where the client builder is incorrectly validating that a value for the SIGNER option is present, even when it's not required (because the service does not use AWS auth).\n \n This change only does the appropriate validations based on the authtypes present in the service model."
}
9 changes: 6 additions & 3 deletions ...en/src/main/java/software/amazon/awssdk/codegen/poet/builder/AsyncClientBuilderClass.java
View file
Expand Up @@ -27,7 +27,8 @@
import software.amazon.awssdk.codegen.poet.ClassSpec;
import software.amazon.awssdk.codegen.poet.PoetUtils;
import software.amazon.awssdk.codegen.poet.rules.EndpointRulesSpecUtils;
import software.amazon.awssdk.codegen.utils.BearerAuthUtils;
import software.amazon.awssdk.codegen.utils.AuthUtils;
import software.amazon.awssdk.core.client.config.SdkClientConfiguration;
import software.amazon.awssdk.core.client.config.SdkClientOption;

public class AsyncClientBuilderClass implements ClassSpec {
Expand Down Expand Up @@ -72,7 +73,7 @@ public TypeSpec poetSpec() {
builder.addMethod(endpointProviderMethod());
}

if (BearerAuthUtils.usesBearerAuth(model)) {
if (AuthUtils.usesBearerAuth(model)) {
builder.addMethod(bearerTokenProviderMethod());
}

Expand Down Expand Up @@ -120,7 +121,9 @@ private MethodSpec buildClientMethod() {
.addAnnotation(Override.class)
.addModifiers(Modifier.PROTECTED, Modifier.FINAL)
.returns(clientInterfaceName)
.addCode("return new $T(super.asyncClientConfiguration());", clientClassName)
.addStatement("$T clientConfiguration = super.asyncClientConfiguration()", SdkClientConfiguration.class)
.addStatement("this.validateClientOptions(clientConfiguration)")
.addCode("return new $T(clientConfiguration);", clientClassName)
.build();
}

Expand Down
71 changes: 51 additions & 20 deletions ...gen/src/main/java/software/amazon/awssdk/codegen/poet/builder/BaseClientBuilderClass.java
View file
Expand Up @@ -48,7 +48,7 @@
import software.amazon.awssdk.codegen.poet.ClassSpec;
import software.amazon.awssdk.codegen.poet.PoetUtils;
import software.amazon.awssdk.codegen.poet.rules.EndpointRulesSpecUtils;
import software.amazon.awssdk.codegen.utils.BearerAuthUtils;
import software.amazon.awssdk.codegen.utils.AuthUtils;
import software.amazon.awssdk.core.client.config.SdkAdvancedClientOption;
import software.amazon.awssdk.core.client.config.SdkClientConfiguration;
import software.amazon.awssdk.core.client.config.SdkClientOption;
Expand Down Expand Up @@ -83,15 +83,15 @@ public BaseClientBuilderClass(IntermediateModel model) {
@Override
public TypeSpec poetSpec() {
TypeSpec.Builder builder =
PoetUtils.createClassBuilder(builderClassName)
.addModifiers(Modifier.ABSTRACT)
.addAnnotation(SdkInternalApi.class)
.addTypeVariable(PoetUtils.createBoundedTypeVariableName("B", builderInterfaceName, "B", "C"))
.addTypeVariable(TypeVariableName.get("C"))
.superclass(PoetUtils.createParameterizedTypeName(AwsDefaultClientBuilder.class, "B", "C"))
.addJavadoc("Internal base class for {@link $T} and {@link $T}.",
ClassName.get(basePackage, model.getMetadata().getSyncBuilder()),
ClassName.get(basePackage, model.getMetadata().getAsyncBuilder()));
PoetUtils.createClassBuilder(builderClassName)
.addModifiers(Modifier.ABSTRACT)
.addAnnotation(SdkInternalApi.class)
.addTypeVariable(PoetUtils.createBoundedTypeVariableName("B", builderInterfaceName, "B", "C"))
.addTypeVariable(TypeVariableName.get("C"))
.superclass(PoetUtils.createParameterizedTypeName(AwsDefaultClientBuilder.class, "B", "C"))
.addJavadoc("Internal base class for {@link $T} and {@link $T}.",
ClassName.get(basePackage, model.getMetadata().getSyncBuilder()),
ClassName.get(basePackage, model.getMetadata().getAsyncBuilder()));

// Only services that require endpoint discovery for at least one of their operations get a default value of
// 'true'
Expand Down Expand Up @@ -126,13 +126,15 @@ public TypeSpec poetSpec() {
.addMethod(beanStyleSetServiceConfigurationMethod());
}

if (BearerAuthUtils.usesBearerAuth(model)) {
if (AuthUtils.usesBearerAuth(model)) {
builder.addMethod(defaultBearerTokenProviderMethod());
builder.addMethod(defaultTokenAuthSignerMethod());
}

addServiceHttpConfigIfNeeded(builder, model);

builder.addMethod(validateClientOptionsMethod());


return builder.build();
}
Expand Down Expand Up @@ -205,7 +207,7 @@ private MethodSpec mergeServiceDefaultsMethod() {
SdkClientOption.class, ClassName.bestGuess(clientConfigClassName));
}

if (BearerAuthUtils.usesBearerAuth(model)) {
if (AuthUtils.usesBearerAuth(model)) {
builder.addCode(".option($T.TOKEN_PROVIDER, defaultTokenProvider())\n", AwsClientOption.class);
builder.addCode(".option($T.TOKEN_SIGNER, defaultTokenSigner())", SdkAdvancedClientOption.class);
}
Expand All @@ -217,7 +219,7 @@ private MethodSpec mergeServiceDefaultsMethod() {
private Optional<MethodSpec> mergeInternalDefaultsMethod() {
String userAgent = model.getCustomizationConfig().getUserAgent();
RetryMode defaultRetryMode = model.getCustomizationConfig().getDefaultRetryMode();

// If none of the options are customized, then we do not need to bother overriding the method
if (userAgent == null && defaultRetryMode == null) {
return Optional.empty();
Expand Down Expand Up @@ -246,10 +248,10 @@ private MethodSpec finalizeServiceConfigurationMethod() {
String requestHandlerPath = String.format("%s/execution.interceptors", requestHandlerDirectory);

MethodSpec.Builder builder = MethodSpec.methodBuilder("finalizeServiceConfiguration")
.addAnnotation(Override.class)
.addModifiers(PROTECTED, FINAL)
.returns(SdkClientConfiguration.class)
.addParameter(SdkClientConfiguration.class, "config");
.addAnnotation(Override.class)
.addModifiers(PROTECTED, FINAL)
.returns(SdkClientConfiguration.class)
.addParameter(SdkClientConfiguration.class, "config");

// Initialize configuration values

Expand Down Expand Up @@ -289,7 +291,7 @@ private MethodSpec finalizeServiceConfigurationMethod() {
CollectionUtils.class);

builder.addCode("interceptors = $T.mergeLists(interceptors, config.option($T.EXECUTION_INTERCEPTORS));\n",
CollectionUtils.class, SdkClientOption.class);
CollectionUtils.class, SdkClientOption.class);

if (model.getMetadata().isQueryProtocol()) {
TypeName listType = ParameterizedTypeName.get(List.class, ExecutionInterceptor.class);
Expand Down Expand Up @@ -488,7 +490,7 @@ private void mergeServiceConfiguration(MethodSpec.Builder builder, String client

private MethodSpec setServiceConfigurationMethod() {
ClassName serviceConfiguration = ClassName.get(basePackage,
model.getCustomizationConfig().getServiceConfig().getClassName());
model.getCustomizationConfig().getServiceConfig().getClassName());
return MethodSpec.methodBuilder("serviceConfiguration")
.addModifiers(Modifier.PUBLIC)
.returns(TypeVariableName.get("B"))
Expand All @@ -501,7 +503,7 @@ private MethodSpec setServiceConfigurationMethod() {

private MethodSpec beanStyleSetServiceConfigurationMethod() {
ClassName serviceConfiguration = ClassName.get(basePackage,
model.getCustomizationConfig().getServiceConfig().getClassName());
model.getCustomizationConfig().getServiceConfig().getClassName());
return MethodSpec.methodBuilder("setServiceConfiguration")
.addModifiers(Modifier.PUBLIC)
.addParameter(serviceConfiguration, "serviceConfiguration")
Expand Down Expand Up @@ -623,4 +625,33 @@ private boolean hasClientContextParams() {
Map<String, ClientContextParam> clientContextParams = model.getClientContextParams();
return clientContextParams != null && !clientContextParams.isEmpty();
}

private MethodSpec validateClientOptionsMethod() {
MethodSpec.Builder builder = MethodSpec.methodBuilder("validateClientOptions")
.addModifiers(PROTECTED, Modifier.STATIC)
.addParameter(SdkClientConfiguration.class, "c")
.returns(void.class);

if (AuthUtils.usesAwsAuth(model)) {
builder.addStatement("$T.notNull(c.option($T.SIGNER), $S)",
Validate.class,
SdkAdvancedClientOption.class,
"The 'overrideConfiguration.advancedOption[SIGNER]' must be configured in the client builder.");
}

if (AuthUtils.usesBearerAuth(model)) {
builder.addStatement("$T.notNull(c.option($T.TOKEN_SIGNER), $S)",
Validate.class,
SdkAdvancedClientOption.class,
"The 'overrideConfiguration.advancedOption[TOKEN_SIGNER]' "
+ "must be configured in the client builder.");
builder.addStatement("$T.notNull(c.option($T.TOKEN_PROVIDER), $S)",
Validate.class,
AwsClientOption.class,
"The 'overrideConfiguration.advancedOption[TOKEN_PROVIDER]' "
+ "must be configured in the client builder.");
}

return builder.build();
}
}
4 changes: 2 additions & 2 deletions ...src/main/java/software/amazon/awssdk/codegen/poet/builder/BaseClientBuilderInterface.java
View file
Expand Up @@ -34,7 +34,7 @@
import software.amazon.awssdk.codegen.poet.ClassSpec;
import software.amazon.awssdk.codegen.poet.PoetUtils;
import software.amazon.awssdk.codegen.poet.rules.EndpointRulesSpecUtils;
import software.amazon.awssdk.codegen.utils.BearerAuthUtils;
import software.amazon.awssdk.codegen.utils.AuthUtils;
import software.amazon.awssdk.core.client.config.SdkAdvancedClientOption;
import software.amazon.awssdk.utils.internal.CodegenNamingUtils;

Expand Down Expand Up @@ -164,7 +164,7 @@ private MethodSpec clientContextParamSetter(String name, ClientContextParam para
}

private boolean generateTokenProviderMethod() {
return BearerAuthUtils.usesBearerAuth(model);
return AuthUtils.usesBearerAuth(model);
}

private MethodSpec tokenProviderMethod() {
Expand Down
10 changes: 7 additions & 3 deletions ...gen/src/main/java/software/amazon/awssdk/codegen/poet/builder/SyncClientBuilderClass.java
View file
Expand Up @@ -27,7 +27,8 @@
import software.amazon.awssdk.codegen.poet.ClassSpec;
import software.amazon.awssdk.codegen.poet.PoetUtils;
import software.amazon.awssdk.codegen.poet.rules.EndpointRulesSpecUtils;
import software.amazon.awssdk.codegen.utils.BearerAuthUtils;
import software.amazon.awssdk.codegen.utils.AuthUtils;
import software.amazon.awssdk.core.client.config.SdkClientConfiguration;
import software.amazon.awssdk.core.client.config.SdkClientOption;

public class SyncClientBuilderClass implements ClassSpec {
Expand Down Expand Up @@ -72,7 +73,7 @@ public TypeSpec poetSpec() {
builder.addMethod(endpointProviderMethod());
}

if (BearerAuthUtils.usesBearerAuth(model)) {
if (AuthUtils.usesBearerAuth(model)) {
builder.addMethod(tokenProviderMethodImpl());
}

Expand Down Expand Up @@ -120,7 +121,10 @@ private MethodSpec buildClientMethod() {
.addAnnotation(Override.class)
.addModifiers(Modifier.PROTECTED, Modifier.FINAL)
.returns(clientInterfaceName)
.addCode("return new $T(super.syncClientConfiguration());", clientClassName)
.addStatement("$T clientConfiguration = super.syncClientConfiguration()",
SdkClientConfiguration.class)
.addStatement("this.validateClientOptions(clientConfiguration)")
.addCode("return new $T(clientConfiguration);", clientClassName)
.build();
}

Expand Down
4 changes: 2 additions & 2 deletions codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/specs/ProtocolSpec.java
View file
Expand Up @@ -33,7 +33,7 @@
import software.amazon.awssdk.codegen.model.intermediate.ShapeType;
import software.amazon.awssdk.codegen.model.service.AuthType;
import software.amazon.awssdk.codegen.poet.PoetExtension;
import software.amazon.awssdk.codegen.utils.BearerAuthUtils;
import software.amazon.awssdk.codegen.utils.AuthUtils;
import software.amazon.awssdk.core.CredentialType;
import software.amazon.awssdk.core.client.handler.SyncClientHandler;
import software.amazon.awssdk.core.runtime.transform.AsyncStreamingRequestMarshaller;
Expand Down Expand Up @@ -114,7 +114,7 @@ default String discoveredEndpoint(OperationModel opModel) {

default CodeBlock credentialType(OperationModel opModel, IntermediateModel model) {

if (BearerAuthUtils.isOpBearerAuth(model, opModel)) {
if (AuthUtils.isOpBearerAuth(model, opModel)) {
return CodeBlock.of(".credentialType($T.TOKEN)\n", CredentialType.class);
} else {
return CodeBlock.of("");
Expand Down
34 changes: 32 additions & 2 deletions ...awssdk/codegen/utils/BearerAuthUtils.java → ...mazon/awssdk/codegen/utils/AuthUtils.java
View file
Expand Up @@ -19,8 +19,8 @@
import software.amazon.awssdk.codegen.model.intermediate.OperationModel;
import software.amazon.awssdk.codegen.model.service.AuthType;

public final class BearerAuthUtils {
private BearerAuthUtils() {
public final class AuthUtils {
private AuthUtils() {
}

/**
Expand All @@ -36,6 +36,16 @@ public static boolean usesBearerAuth(IntermediateModel model) {
.anyMatch(authType -> authType == AuthType.BEARER);
}

public static boolean usesAwsAuth(IntermediateModel model) {
if (isServiceAwsAuthType(model)) {
return true;
}

return model.getOperations().values().stream()
.map(OperationModel::getAuthType)
.anyMatch(AuthUtils::isAuthTypeAws);
}

/**
* Returns {@code true} if the operation should use bearer auth.
*/
Expand All @@ -50,6 +60,26 @@ private static boolean isServiceBearerAuth(IntermediateModel model) {
return model.getMetadata().getAuthType() == AuthType.BEARER;
}

private static boolean isServiceAwsAuthType(IntermediateModel model) {
AuthType authType = model.getMetadata().getAuthType();
return isAuthTypeAws(authType);
}

private static boolean isAuthTypeAws(AuthType authType) {
if (authType == null) {
return false;
}

switch (authType) {
case V4:
case S3:
case S3V4:
return true;
default:
return false;
}
}

private static boolean hasNoAuthType(OperationModel opModel) {
return opModel.getAuthType() == null || opModel.getAuthType() == AuthType.NONE;
}
Expand Down
26 changes: 23 additions & 3 deletions ...dk/codegen/utils/BearerAuthUtilsTest.java → ...n/awssdk/codegen/utils/AuthUtilsTest.java
View file
Expand Up @@ -31,7 +31,7 @@
import software.amazon.awssdk.codegen.model.intermediate.OperationModel;
import software.amazon.awssdk.codegen.model.service.AuthType;

public class BearerAuthUtilsTest {
public class AuthUtilsTest {

@ParameterizedTest
@MethodSource("serviceValues")
Expand All @@ -40,7 +40,7 @@ public void testIfServiceHasBearerAuth(AuthType serviceAuthType,
Boolean expectedResult) {
IntermediateModel model = modelWith(serviceAuthType);
model.setOperations(createOperations(opAuthTypes));
assertThat(BearerAuthUtils.usesBearerAuth(model)).isEqualTo(expectedResult);
assertThat(AuthUtils.usesBearerAuth(model)).isEqualTo(expectedResult);
}

private static Stream<Arguments> serviceValues() {
Expand All @@ -53,12 +53,32 @@ private static Stream<Arguments> serviceValues() {
Arguments.of(AuthType.S3V4, oneBearerOp, true));
}

@ParameterizedTest
@MethodSource("awsAuthServiceValues")
public void testIfServiceHasAwsAuthAuth(AuthType serviceAuthType,
List<AuthType> opAuthTypes,
Boolean expectedResult) {
IntermediateModel model = modelWith(serviceAuthType);
model.setOperations(createOperations(opAuthTypes));
assertThat(AuthUtils.usesAwsAuth(model)).isEqualTo(expectedResult);
}

private static Stream<Arguments> awsAuthServiceValues() {
List<AuthType> oneAwsAuthOp = Arrays.asList(AuthType.V4, AuthType.BEARER, AuthType.NONE);
List<AuthType> noAwsAuthOp = Arrays.asList(AuthType.BEARER, AuthType.NONE);

return Stream.of(Arguments.of(AuthType.BEARER, oneAwsAuthOp, true),
Arguments.of(AuthType.BEARER, noAwsAuthOp, false),
Arguments.of(AuthType.V4, oneAwsAuthOp, true),
Arguments.of(AuthType.V4, noAwsAuthOp, true));
}

@ParameterizedTest
@MethodSource("opValues")
public void testIfOperationIsBearerAuth(AuthType serviceAuthType, AuthType opAuthType, Boolean expectedResult) {
IntermediateModel model = modelWith(serviceAuthType);
OperationModel opModel = opModelWith(opAuthType);
assertThat(BearerAuthUtils.isOpBearerAuth(model, opModel)).isEqualTo(expectedResult);
assertThat(AuthUtils.isOpBearerAuth(model, opModel)).isEqualTo(expectedResult);
}

private static Stream<Arguments> opValues() {
Expand Down
5 changes: 4 additions & 1 deletion ...esources/software/amazon/awssdk/codegen/poet/builder/test-async-client-builder-class.java
View file
Expand Up @@ -4,6 +4,7 @@
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.auth.token.credentials.SdkTokenProvider;
import software.amazon.awssdk.awscore.client.config.AwsClientOption;
import software.amazon.awssdk.core.client.config.SdkClientConfiguration;

/**
* Internal implementation of {@link JsonAsyncClientBuilder}.
Expand All @@ -20,6 +21,8 @@ public DefaultJsonAsyncClientBuilder tokenProvider(SdkTokenProvider tokenProvide

@Override
protected final JsonAsyncClient buildClient() {
return new DefaultJsonAsyncClient(super.asyncClientConfiguration());
SdkClientConfiguration clientConfiguration = super.asyncClientConfiguration();
this.validateClientOptions(clientConfiguration);
return new DefaultJsonAsyncClient(clientConfiguration);
}
}
10 changes: 9 additions & 1 deletion ...es/software/amazon/awssdk/codegen/poet/builder/test-bearer-auth-client-builder-class.java
View file
Expand Up @@ -16,6 +16,7 @@
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
import software.amazon.awssdk.core.signer.Signer;
import software.amazon.awssdk.utils.CollectionUtils;
import software.amazon.awssdk.utils.Validate;

/**
* Internal base class for {@link DefaultJsonClientBuilder} and {@link DefaultJsonAsyncClientBuilder}.
Expand Down Expand Up @@ -65,4 +66,11 @@ private SdkTokenProvider defaultTokenProvider() {
private Signer defaultTokenSigner() {
return BearerTokenSigner.create();
}
}

protected static void validateClientOptions(SdkClientConfiguration c) {
Validate.notNull(c.option(SdkAdvancedClientOption.TOKEN_SIGNER),
"The 'overrideConfiguration.advancedOption[TOKEN_SIGNER]' must be configured in the client builder.");
Validate.notNull(c.option(AwsClientOption.TOKEN_PROVIDER),
"The 'overrideConfiguration.advancedOption[TOKEN_PROVIDER]' must be configured in the client builder.");
}
}

0 comments on commit c119790

Please sign in to comment.