-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
…nto identity-pool
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": [], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
...ages/@aws-cdk/aws-cognito-identitypool/lib/identitypool-user-pool-authentication-provider.ts
Outdated
Show resolved
Hide resolved
…er-pool-authentication-provider.ts Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
Pull request has been modified.
…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>
…nto identity-pool
…er-pool-authentication-provider.ts Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
@corymhall One pending suggestion and then I can push changes |
There was a problem hiding this 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!
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
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*
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