If authType=none don't use the old signer, even if overridden #4523
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some of these tests are failing. Will fix them in next commit
millems
approved these changes
Oct 2, 2023
|
SonarCloud Quality Gate failed.
|
millems
approved these changes
Oct 3, 2023
| if (context.executionAttributes().getAttribute(SdkInternalExecutionAttribute.AUTH_SCHEMES) != null) { | ||
| SelectedAuthScheme<?> selectedAuthScheme = | ||
| context.executionAttributes().getAttribute(SdkInternalExecutionAttribute.SELECTED_AUTH_SCHEME); | ||
| log.debug(() -> String.format("Using SelectedAuthScheme: %s", selectedAuthScheme.authSchemeOption().schemeId())); |
Contributor
There was a problem hiding this comment.
No need to use String.format.
12 tasks
sugmanue
pushed a commit
that referenced
this pull request
Oct 31, 2023
* Add authType=none tests to AwsExecutionContentBuilderTest Some of these tests are failing. Will fix them in next commit * If authType=none don't use the old signer, even if overridden * Update signing stage logic for authType=none * Add asserts on identity resolution for authType=none
sugmanue
added a commit
that referenced
this pull request
Nov 6, 2023
* Set TIME_OFFSET attribtue in SRA signin methods (#4483) * Verify that execution attributes set by old interceptors can be read by new signers. (#4494) * Move S3Presigner fully to SRA with HttpSigner (#4569) * Remove derived attribute logic for PRESIGNER_EXPIRATION (#4602) * Remove derived attribute logic for PRESIGNER_EXPIRATION Because of the 2 instant() calls in the write/read mappings, a basic write this attribute and read it back, would give different values. Since this attribute should only be set/read explicitly by our presigner code, we don't need this mapping logic at all. Also, update S3PresignerTest to assert isEqualTo instead of isCloseTo and remove unnecessary override * Remove PRESIGNER_EXPIRATION mirroring test * Enable SRA Auth for S3 * Make follow up changes from PR 4396 (#4454) * Make SigingStage updates similar to AsyncSigningStage. * Simplify (Async)SigningStageTests based on updated logic. * Minor typos * If authType=none don't use the old signer, even if overridden (#4523) * Add authType=none tests to AwsExecutionContentBuilderTest Some of these tests are failing. Will fix them in next commit * If authType=none don't use the old signer, even if overridden * Update signing stage logic for authType=none * Add asserts on identity resolution for authType=none * Add a missing http-auth-aws dependecy to aws-core * Add a changelog entry --------- Co-authored-by: Zoe Wang <[email protected]> Co-authored-by: Matthew Miller <[email protected]> Co-authored-by: Jaykumar Gosar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
This addresses #4396 (comment). If authType=none, there is no need to do identity resolution. #4519 shows that that was the behavior before SRA (since they pass in the PR). But this behavior was changing because of #4396 (comment) and this PR fixes it.
Modifications
First I added a commit that adds the same pre SRA tests from #4519. Also, added some more tests for similar expected behavior for post SRA. Without making the change to AwsExecutionContextBuilder, these tests failed:
Then I added a 2nd commit to the source code that fixes all these tests. It kind reverts the change to loadSigner from #4396.
I don't intend to merge #4519 to master directly, but those tests will get added via this PR to the SRA feature branch, and then merged to master.
Testing
see notes above.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License