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.

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

AWS Cognito Integration #1256

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

AWS Cognito Integration #1256

wants to merge 10 commits into from

Conversation

Forfend
Copy link
Contributor

@Forfend Forfend commented Oct 21, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This PR provides the PoC of AWS Cognito Integration, as discussed with @maciejwalkowiak in #1246. The integration provides a higher level of abstraction over AWS Cognito user pool basic auth operations such as creating a user, logging in, and resetting a password.

💡 Motivation and Context

This feature provides basic (as of this version) integration with AWS Cognito and can be enhanced with future PRs.

💚 How did you test it?

Tested it with the help of an extra pet project. More unit & integration tests are coming in the next commits in the scope of this PR.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

This integration can be extended by adding functionality for creating and managing user pools, to be discussed...

@github-actions github-actions bot added the type: dependency-upgrade Dependency version bump label Oct 21, 2024
@Forfend Forfend changed the title Gh 1246 AWS Cognito Integration Oct 21, 2024
@maciejwalkowiak maciejwalkowiak added this to the 3.3.0 M2 milestone Nov 11, 2024
@maciejwalkowiak maciejwalkowiak added type: feature Integration with a new AWS service or bigger change in existing integration and removed type: dependency-upgrade Dependency version bump labels Nov 11, 2024
@maciejwalkowiak
Copy link
Contributor

@MatejNedic anything you would like to add?

@maciejwalkowiak
Copy link
Contributor

@Forfend could you also add a sample application in spring-cloud-aws-samples directory?

@Forfend
Copy link
Contributor Author

Forfend commented Nov 13, 2024

@maciejwalkowiak yes, in progress

@MatejNedic
Copy link
Member

MatejNedic commented Nov 13, 2024

Will take a deep look tomorrow on first glance I like it.

Use case itself is valid, I like it good job @Forfend 👍

@github-actions github-actions bot added the type: dependency-upgrade Dependency version bump label Nov 14, 2024
@maciejwalkowiak
Copy link
Contributor

@Forfend apologies I wasn't specific enough. I meant more realistic example - how to use it with Spring Security for example. I will try to figure it out today myself.

…er CognitoTemplate with tests. Support logout functionality
@Forfend
Copy link
Contributor Author

Forfend commented Nov 22, 2024

@maciejwalkowiak I added some real-world examples and their usage with Spring Security

Copy link
Member

@MatejNedic MatejNedic left a comment

Choose a reason for hiding this comment

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

Hey Thanks @Forfend on PR,
Some small changes

pom.xml Outdated Show resolved Hide resolved
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>cognitoidentityprovider</artifactId>
<exclusions>
Copy link
Member

Choose a reason for hiding this comment

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

What is reason for Exclusions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MatejNedic tbh there is no specific reason, just not to include additional dependencies. can be removed

@Forfend
Copy link
Contributor Author

Forfend commented Dec 9, 2024

Hi @MatejNedic, thanks for the review. I fixed all your comments

.clientId(clientId).challengeName(ChallengeNameType.NEW_PASSWORD_REQUIRED).session(session)
.challengeResponses(Map.of(CognitoParameters.USERNAME_PARAM_NAME, username,
CognitoParameters.NEW_PASSWORD_PARAM_NAME, password, CognitoParameters.SECRET_HASH_PARAM_NAME,
CognitoUtils.calculateSecretHash(clientId, clientSecret, username)))
Copy link
Contributor

Choose a reason for hiding this comment

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

if clientSecret can be null, this may cause null pointer exception. Is clientSecret mean to be nullable?

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, indeed. It might be null for public clients (by design, they don't have a clientSecret), and those clients mainly use SPA apps. However public clients might be configured to allow auth operations with server-side auth credentials flow. Will make these flows null-safe

@maciejwalkowiak maciejwalkowiak modified the milestones: 3.3.0 M2, 3.x Dec 12, 2024
@maciejwalkowiak
Copy link
Contributor

Thanks @Forfend! We are not going to include it in next release as I need to spend a little more time with it and play.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: dependency-upgrade Dependency version bump type: feature Integration with a new AWS service or bigger change in existing integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants