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

feat(cognito): identity pools #16190

Merged
merged 196 commits into from
Jan 14, 2022
Merged

feat(cognito): identity pools #16190

merged 196 commits into from
Jan 14, 2022

Conversation

smguggen
Copy link
Contributor

@smguggen smguggen commented Aug 24, 2021

Adds Identity Pool L2 Construct which to date has not been implemented. Since Identity Pool's can't be used without default auth and unauth roles, also incorporated the L1 CfnIdentityPoolRoleAttachment into the Construct. Contains unit and integration tests as well as fully updated ReadMe.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

smguggen and others added 30 commits August 16, 2021 12:49
Bumps [tar](https://github.com/npm/node-tar) from 4.4.13 to 4.4.16.
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.13...v4.4.16)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ben Chaimberg <chaimber@amazon.com>
* chore(deps-dev): bump jszip from 3.6.0 to 3.7.0

Bumps [jszip](https://github.com/Stuk/jszip) from 3.6.0 to 3.7.0.
- [Release notes](https://github.com/Stuk/jszip/releases)
- [Changelog](https://github.com/Stuk/jszip/blob/master/CHANGES.md)
- [Commits](Stuk/jszip@v3.6.0...v3.7.0)

---
updated-dependencies:
- dependency-name: jszip
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>

* updates package.json as well and removes unneeded types dependency

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ben Chaimberg <chaimber@amazon.com>
Error in the documentation and type checking. Fixed both the readme and the related PR (integ test used a `Field` type so it still works as intended).

Related PR: aws#10078

Fixes: aws#16071

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Co-authored-by: AWS CDK Team <aws-cdk@amazon.com>
"Properties": {
"AllowUnauthenticatedIdentities": false,
"AllowClassicFlow": true,
"CognitoIdentityProviders": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

@smguggen it looks like the integration tests are not testing the cognito identity providers.

Copy link
Contributor Author

@smguggen smguggen Jan 6, 2022

Choose a reason for hiding this comment

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

@corymhall Oh wow, thanks for catching that, the user pool in the integ test actually has no identity providers in it. So I added 2 identity providers (so there would be more than 1 provider name in the pool) and ran it both ways: looping through userPool.identityProviders to get the providerName and then using userPool.userPoolProviderName. The userPool.userPoolProviderName did just like you said and used the same provider over and over while the identityProviders included the entire list of providers. Now I'm just waiting to see if the stack succeeds with the identityProvider names to confirm that all of this works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@smguggen did the integration tests succeed? I tried running them and received a lot of errors.

How are you generating the integ.identitypool.expected.json?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should generate the expected template by running

yarn integ integ.identitypool.js

That will actually perform a deployment and make sure it is successful and then output the template in the expected.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@corymhall Well I just completely whiffed on that one, didn't I? 😂😂😂

The reason I was so sure that worked (yarn.integ had always given me credentials problems) was that I was using this as an extension library in one of my side projects, and in that project I was using this feature. Well, turns out I'd forgotten about a bug I fixed here weeks ago that was causing IdentityPool.cognitoIdentityProviders to always be an empty array, so the Identity Pool had only been using an OIDC provider directly and wasn't using the user pool at all.

Anyway lessons learned. Thanks for being so thorough about that, changes have been made, so tell me what you think.

@smguggen smguggen requested a review from corymhall January 6, 2022 03:52
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@smguggen looking good! Just a couple more comments on the new bind implementation.

smguggen and others added 2 commits January 13, 2022 17:26
…er-pool-authentication-provider.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
@mergify mergify bot dismissed corymhall’s stale review January 13, 2022 17:29

Pull request has been modified.

smguggen and others added 7 commits January 13, 2022 11:29
…er-pool-authentication-provider.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
…er-pool-authentication-provider.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
…er-pool-authentication-provider.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
…er-pool-authentication-provider.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
…er-pool-authentication-provider.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
…er-pool-authentication-provider.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
@smguggen
Copy link
Contributor Author

@corymhall One pending suggestion and then I can push changes

@smguggen smguggen requested a review from corymhall January 13, 2022 21:33
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@smguggen great job on this!

@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: c3e9e6c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 59fe395 into aws:master Jan 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
Adds Identity Pool L2 Construct which to date has not been implemented. Since Identity Pool's can't be used without default auth and unauth roles, also incorporated the L1 CfnIdentityPoolRoleAttachment into the Construct. Contains unit and integration tests as well as fully updated ReadMe.


*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito effort/large Large work item – several weeks of effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.