Skip to content

Updates StsCredentialsProvider to explicitly handle AwsSessionCredentials #4067

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.

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

Merged
merged 3 commits into from
Jun 8, 2023

Conversation

cenedhryn
Copy link
Contributor

@cenedhryn cenedhryn commented Jun 6, 2023

Motivation and Context

The abstract base class for STS credentials provider was recently made public. This change changes the protected getUpdatedCredentials method to return AwsSessionCredentials, a public class, instead of instead of the STS model class Credentials, so that the SDK handles and stores credentials in a type which may be modified by the SDK. The change is not strictly backwards compatible, but given the short timespan between the two PRs it's unlikely that more than the occasional user would be affected.

Modifications

  • Changes the protected getUpdatedCredentials method to return AwsSessionCredentials instead of the STS model class Credentials.
  • Updates all known subclasses of StsCredentialsProvider.
  • Adds builder and expirationTime to AwsSessionCredentials in order to convert STS credentials to session credentials. Previously, expiration has been stored outside of the session credentials
  • Removes SessionCredentialsHolder which has been rendered obsolete by this change

Testing

Unit tests added.

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

@cenedhryn cenedhryn requested a review from a team as a code owner June 6, 2023 19:01
@cenedhryn cenedhryn changed the title Updates StsCredentialsProvider to explicitly handle AwsSessionCredent… Updates StsCredentialsProvider to explicitly handle AwsSessionCredentials Jun 6, 2023
"type": "feature",
"category": "AWS SDK for Java v2, AWS STS",
"contributor": "",
"description": "Updates the core STS credential provider logic to return AwsSessionCredentials instead of STS Credentials, and adds expirationTime to AwsSessionCredentials\""
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra quote

@@ -0,0 +1,6 @@
{
"type": "feature",
"category": "AWS SDK for Java v2, AWS STS",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we just document the addition of expirationTime to AwsSessionCredentials? The other one is hopefully internal-only(ish). Otherwise I think we should have two different release notes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will update

return this;
}

public Builder expiration(Instant expiration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: expirationTime?

return hashCode;
}

public static final class Builder {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we javadoc this?

private StsAuthUtils() {
}

public static AwsSessionCredentials fromStsCredentials(Credentials credentials) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think toAwsSessionCredentials might be a better name. The "from" type is evident by the method parameter. The "to" type is more useful in the method name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, it read better

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

88.9% 88.9% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@gosar gosar left a comment

Choose a reason for hiding this comment

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

LGTM

@cenedhryn cenedhryn merged commit d1d1ee1 into master Jun 8, 2023
@cenedhryn cenedhryn deleted the salande/merge-sts-updates-to-main branch June 8, 2023 17:41
L-Applin pushed a commit that referenced this pull request Jul 24, 2023
…ials (#4067)

* Updates StsCredentialsProvider to explicitly handle AwsSessionCredentials
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants