Add tests to assert signer and endpoint auth-scheme behaviors #4651

haydenbaker · 2023-10-31T17:43:29Z

Modifications

Add tests to assert behavior when endpoint auth-scheme sets properties for the selected auth-scheme, and the signer is overriden (to a non-SRA signer)

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

sugmanue · 2023-10-31T22:38:24Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+        try {
+            client.streamingInputOperation(r -> {}, RequestBody.fromString("test"));
+        } catch (Exception expected) {
+        }


Can we switch instead to use an interceptor and assert that the exception is the expected one?, See for instance the setup in the tests here. Rationale, this ensures that the exception thrown is exactly what we expect instead of something else failing in the middle.

For instance, the assertion will be instead

assertThatThrownBy(() -> client.streamingInputOperation(r -> { }, RequestBody.fromString("test"))) .hasMessageContaining("boom!");

Point taken. We don't need the interceptor, we can just use the mocked signer to do that.

sugmanue · 2023-10-31T22:42:59Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+            argThat(attrs ->
+                        "us-west-2".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNING_REGION).id()) &&
+                        "query-test-v4".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME)) &&
+                        !attrs.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE))


Nit, I think that being explicit with the expected value makes this test easier to read, e.g.,

Boolean.FALSE.equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE)))

sugmanue · 2023-10-31T22:45:30Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+    }
+
+    private static ProtocolQueryAuthSchemeProvider v4AuthSchemeProviderOverride() {
+        return __ -> {


Nit, can we use a letter here instead? (e.g., x -> {...})

It's unused, and this is a fairly common when the variable is unused.

Not in Java, and I can't find any other test in the project using it (didn't search thoroughly), but up to the team.

(c.f., the commonly used pattern on some other languages _ is not even possible in Java since a single underscore is reserved).

Yep, hence the usage of two underscores. Changed to 'x'.

You forgot this one and the other below.

sugmanue · 2023-10-31T22:58:12Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+            .authSchemeProvider(v4AuthSchemeProviderOverride())
+            .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "skid")))
+            .endpointProvider(v4EndpointProviderOverride())
+            .region(Region.US_WEST_2)


Nit, let's use here another region to make it clear that the override is working as expected

Suggested change

.region(Region.US_WEST_2)

.region(Region.AP_SOUTH_1)

sugmanue · 2023-10-31T22:59:41Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+    public Signer mockSigner = mock(Signer.class);
+
+    @Test
+    public void test_whenV4EndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {


Nit, SonarCloud have complained before about the test methods being public

Suggested change

public void test_whenV4EndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {

void test_whenV4EndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {

sugmanue · 2023-11-01T01:12:46Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+
+    @BeforeAll
+    static void setup() {
+        when(mockSigner.sign(any(), any())).thenThrow(new RuntimeException("boom!"));


The idea behind using the interceptor instead is two folded, it's the pattern used by the Java team and I been asked before to use it, and, the exceptions is thrown before transmission, IOW, you also validate that everything else in the execution pipeline runs as expected and it's not throwing somewhere else due to the setup you have here.

sugmanue · 2023-11-01T01:51:23Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+            .authSchemeProvider(v4AuthSchemeProviderOverride())
+            .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "skid")))
+            .endpointProvider(v4EndpointProviderOverride())
+            .region(Region.US_WEST_1)


The idea behind the ask to change the region here was that you can validate that the override takes place, given that the region here is the same as the override we cannot know whether is it coming from the client builder or from the override.

sugmanue · 2023-11-01T01:53:31Z

...asses-test/src/test/java/software/amazon/awssdk/services/SignerAndEndpointOverridesTest.java

+    }
+
+    private static ProtocolQueryAuthSchemeProvider v4AuthSchemeProviderOverride() {
+        return __ -> {


You forgot this one and the other below.

sonarcloud · 2023-11-01T18:41:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

94.0% Coverage
0.0% Duplication

gosar · 2023-12-14T22:52:10Z

...s-core/src/main/java/software/amazon/awssdk/awscore/internal/AwsExecutionContextBuilder.java

+        // Use auth scheme provider that the user specified at the request level with
+        // preference over that which is configured on the client.
+        AuthSchemeProvider authSchemeProvider =
+            executionAttributes.getOptionalAttribute(SdkInternalExecutionAttribute.AUTH_SCHEME_RESOLVER)


Directly setting AUTH_SCHEME_RESOLVER into ExecutionAttributes is not how users would specify request level override for it. These attributes are SdkInternalExecutionAttributes (and SdkProtectedApi) which customers are not supposed to use directly. Even if they can set ExecutionAttributes in RequestOverrideConfiguration. To resolve this TODO we need to support a first class setter somewhere in RequestOverrideConfiguration(or sub-class) for each of these properties.

We have other internal tasks tracking these TODOs, let's not try to address those in the context of this PR.

I think you made this change to have the tests below with sigv4a. The supported way to do that is putAuthScheme on the clientBuilder. (also note that's also at client level not request level which is what this TODO is about).

Add tests to assert signer and endpoint override behaviors

7768fe5

haydenbaker changed the title ~~Add tests to assert signer and endpoint override behaviors~~ Add tests to assert signer and endpoint auth-scheme behaviors Oct 31, 2023

haydenbaker marked this pull request as ready for review October 31, 2023 19:46

haydenbaker requested a review from a team as a code owner October 31, 2023 19:46

Handle TODOs that were causing test to fail

509c65f

sugmanue reviewed Oct 31, 2023

View reviewed changes

Address nits

2829700

sugmanue reviewed Nov 1, 2023

View reviewed changes

Use capturing-interceptor

5cf2ed1

haydenbaker force-pushed the haydenbaker/signer-override-auth-scheme-testing branch from 405fcf1 to 5cf2ed1 Compare November 1, 2023 16:08

haydenbaker added 2 commits November 1, 2023 09:16

Make parameterized for different signer type

343ab78

Add crt dep for test

f1c81e9

haydenbaker requested a review from millems November 1, 2023 18:09

gosar reviewed Dec 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests to assert signer and endpoint auth-scheme behaviors #4651

Add tests to assert signer and endpoint auth-scheme behaviors #4651

haydenbaker commented Oct 31, 2023 •

edited

Loading

sugmanue Oct 31, 2023

haydenbaker Oct 31, 2023

sugmanue Oct 31, 2023

sugmanue Oct 31, 2023

haydenbaker Oct 31, 2023

sugmanue Oct 31, 2023

sugmanue Oct 31, 2023

haydenbaker Oct 31, 2023

sugmanue Nov 1, 2023

sugmanue Oct 31, 2023

sugmanue Oct 31, 2023

sugmanue Nov 1, 2023

sugmanue Nov 1, 2023

sugmanue Nov 1, 2023

sonarcloud bot commented Nov 1, 2023

gosar Dec 14, 2023

	public void test_whenV4EndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {
	void test_whenV4EndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {

Add tests to assert signer and endpoint auth-scheme behaviors #4651

Are you sure you want to change the base?

Add tests to assert signer and endpoint auth-scheme behaviors #4651

Conversation

haydenbaker commented Oct 31, 2023 • edited Loading

Modifications

Screenshots (if appropriate)

Types of changes

Checklist

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 1, 2023

Choose a reason for hiding this comment

haydenbaker commented Oct 31, 2023 •

edited

Loading