Support for custom Cognito User Pools #129

majdarbash · 2024-08-09T02:42:43Z

With this change, users will be able to select custom Cognito User Pool, rather than using the default Cognito User Pool created with Deployment dashboard. This will give users control on using support Cognito Identity Providers through being able to configure different user pools per use case deployed.

knihit · 2024-08-09T02:47:35Z

Thank you very much for your contribution @majdarbash . Once we review the code, we will include it in the next possible minor/ major release of the solution.

jamesnixon-aws

Thank you for your contribution to GAAB! I have left a couple of review comments on the infrastructure and lambda portions of your PR. I will leave the UI changes to be reviewed by my colleague, @OmarRad.

One additional general comment is that we require unit tests be provided for all new functionality, which I think is lacking from this PR. Feel free to reach out and we can work together to get those added.

jamesnixon-aws · 2024-08-26T13:18:19Z

source/infrastructure/lib/api/model-schema/deploy-usecase-body.ts

@@ -58,6 +58,10 @@ export const deployUseCaseBodySchema: JsonSchema = {
            description: 'Deploy the CloudFront based UI for the use case',
            default: true
        },
+        ExistingCognitoUserPoolId: {
+            type: [JsonSchemaType.STRING, JsonSchemaType.NULL],


If the intention is to use the default user pool created by the deployment dashboard when this is not provided, I would simply leave the type as string, and expect a user of the API to not pass any value for ExistingCognitoUserPoolId if they want to use the default.

The problem occurs when we want to update the ExistingCognitoUserPoolId:
(image user set the cognitouserpool id, but then wants to update it to the default one?)

Passing / not passing string doesn't indicate whether we want to explicitly reset user pool to the default one. With giving ability to explicitly set it as null, we can use this value when "Use Existing User Pool = false" in the UI.

let me know if this makes sense.

Understood. That is a workflow I had not considered, and I will need to think about it more. We block certain types of edits (e.g. if you use an existing kendra index for RAG, an edit can't then tell the use case to provision a new kendra index), and I think this would be a potentially good candidate for that approach.

As an alternative, perhaps if a user deploys with a custom user pool, perhaps they could return to the default one by editing and providing the id/client of the default pool in the wizard.

jamesnixon-aws · 2024-08-26T13:18:33Z

source/infrastructure/lib/api/model-schema/update-usecase-body.ts

@@ -43,6 +43,10 @@ export const updateUseCaseBodySchema: JsonSchema = {
            description: 'Email address of the user who will be created with permissions to use the deployed use-case',
            format: 'email'
        },
+        ExistingCognitoUserPoolId: {
+            type: [JsonSchemaType.STRING, JsonSchemaType.NULL],


same type comment as the deploy body

jamesnixon-aws · 2024-08-26T13:37:53Z

source/lambda/use-case-management/model/use-case.ts

@@ -296,7 +296,7 @@ export class ChatUseCaseDeploymentAdapter extends UseCase {
        );
        cfnParameters.set(CfnParameterKeys.UseCaseConfigTableName, process.env[USE_CASE_CONFIG_TABLE_NAME_ENV_VAR]!);

-        cfnParameters.set(CfnParameterKeys.ExistingCognitoUserPoolId, process.env[USER_POOL_ID_ENV_VAR]!);
+        cfnParameters.set(CfnParameterKeys.ExistingCognitoUserPoolId, eventBody.ExistingCognitoUserPoolId  || process.env[USER_POOL_ID_ENV_VAR]!);


At a glance, I don't think that setting this CfnParameter alone will be sufficient for the functionality you are looking for. Below, we set parameters for ExistingCognitoUserPoolClient and CognitoDomainPrefix based on environment variables which are set on this lambda at deployment time, and refer to resources corresponding to the User Pool created at deployment, which would differ from those required when providing your own user pool.

Have you tested if this modification allowed deployment to succeed with the desired functionality?

That's correct - this worked properly before CognitoDomainPrefix modification as of v2.0. We need to adjust this part as CognitoDomainPrefix seems to be set based on the default user pool id. I will coordinate directly with you on this change.

Understood. So I think that for this to be compatible, we need all 3 values to be provided in the UI (user pool id, client id, and domain prefix). Onus would be on the user to have a working client in the user pool the wish to use.

yes, otherwise we could also support obtaining domain prefix dynamically for the user pool id that's specified - something TBD

jamesnixon-aws · 2024-08-26T13:41:30Z

source/infrastructure/lib/api/model-schema/deploy-usecase-body.ts

@@ -58,6 +58,10 @@ export const deployUseCaseBodySchema: JsonSchema = {
            description: 'Deploy the CloudFront based UI for the use case',
            default: true
        },
+        ExistingCognitoUserPoolId: {


Rather than having this parameter at the top level of the payload, I would perhaps suggest we create a CognitoParams subschema here containing ExistingCognitoUserPoolId and any other potential future params related to cognito. Or perhaps even a generic AuthenticationParams which itself contains provider specific auth params, which for now would just be CognitoParams. This would follow a similar pattern to what we use for KnowledeBaseParams and LlmParams.

Open to suggestions from your end.

I agree, but I believe it would make sense to consider in the future releases, for the same of backward compatibility with the current used versions of GAAB. Introducing CognitoParams will mean that all currently deployed GAAB instances should have a way to migrate, i.e. set CognitoParams from ExistingCognitoUserPoolId (currently in use).

If you agree, we can work on it in the next iterations, as an enhancement to this, and we can possibly consider extending with more user management functionality as well.

Well since existing versions do not have the ability to set a custom user pool, adding CognitoParams as a non-required param would not break compatibility as far as I can tell. To continue using the default (created for you) pool, the front end would just not send that object in the request. On the other hand, if we add this and then change the positioning of it later, that certainly will break API compatibility.

OmarRad · 2024-08-26T18:53:55Z

source/ui-deployment/src/components/wizard/tools-content.jsx

@@ -26,6 +26,44 @@ export const TOOLS_CONTENT = {
                }
            ]
        },
+        existingUserPool: {


This works but we most recently switched to having the relevant tools content for each component be in that components file so we are slowly moving away from having it all in tools-content.jsx. See here for an example:

generative-ai-application-builder-on-aws/source/ui-deployment/src/components/wizard/Prompt/PromptExperience.tsx

Line 186 in 019a9cd

const maxInputLengthInfoPanel = {

Thanks @OmarRad , noted.

OmarRad

Thank you, Majd, for your contribution. The UI side is thoroughly complete and looks good to me!

2 very minor comments:

For organization purposes, I would move the userpool-related UI files into a dedicated UserPool directory under the UseCase directory
I've left a small comment regarding the location of the tools content.

Otherwise, all that's required is the unit tests. Please let us know if you have any questions or require support on that.

majdarbash · 2024-08-26T21:34:14Z

Thanks Omar, I will work and update the PR according to your feedback.

bogdankatishev · 2024-09-24T12:51:11Z

Any updates on this? This would be helpful for us

tabdunabi · 2024-09-24T12:58:29Z

Thank you @bogdankatishev for reaching out!.

The team is working with @majdarbash to address few code reviews. This feature is a high priority and we are working on including it in a subsequent release ASAP.

majdarbash · 2024-09-27T18:34:26Z

Hi team, the changes are done as we discussed.
Please feel free to continue reviewing the changes @jamesnixon-aws , @OmarRad.
Thanks!

jamesnixon-aws

Couple small comments, but looking good on my end. Let's connect to discuss steps on getting unit tests in here.

source/lambda/use-case-management/model/use-case-validator.ts

source/lambda/use-case-management/model/use-case.ts

jamesnixon-aws

Apologies for the late catch, but one of my colleagues mentioned this requirement to ensure all the values are validated for both the API and the UI fields.

source/infrastructure/lib/api/model-schema/deploy-usecase-body.ts

source/infrastructure/lib/api/model-schema/update-usecase-body.ts

source/ui-deployment/src/components/wizard/UseCase/UserPool/UserPool.tsx

jamesnixon-aws · 2024-11-20T19:46:47Z

This feature has been integrated as of release v2.1.0 (#157)
Thanks for your contribution!

majdarbash added 2 commits August 8, 2024 22:19

Supporting ExistingCognitoUserPoolId

4b0942d

fix

10837e5

jamesnixon-aws reviewed Aug 26, 2024

View reviewed changes

Merge branch 'aws-solutions:main' into main

255b9d1

OmarRad reviewed Aug 26, 2024

View reviewed changes

majdarbash and others added 4 commits September 23, 2024 10:45

Merge branch 'aws-solutions:main' into main

5384f3b

ui-deployment: api related changes

0ce3b04

more updates

080b52d

ui-deployment: reorganized files and added user pool client

e89ff08

maarbash-aws added 14 commits September 24, 2024 10:22

ui-deplyoment: disable AuthenticationParams on edit

9222393

lambda: api changes to support the AuthenticationParams

b903afb

lambda: api changes to support the AuthenticationParams

c2ebd8c

resolving the cognito domain prefix dynamically

82bea47

fixes

deffaaf

reset layer packages

b19eb6b

deps fixes

cfb22e9

fix

8fc4cdc

ui-deployment: fix

810f54f

lambda: fix

9a03945

lambda: allow to describe user pools

f3351c1

lambda: fix

5185286

lambda: retain authentication parameters on update

b2baef6

ui-deployment: fix

f7408b9

jamesnixon-aws requested changes Sep 30, 2024

View reviewed changes

source/lambda/use-case-management/model/use-case-validator.ts Outdated Show resolved Hide resolved

source/lambda/use-case-management/model/use-case.ts Show resolved Hide resolved

source/lambda/use-case-management/model/use-case.ts Outdated Show resolved Hide resolved

maarbash-aws added 2 commits September 30, 2024 11:34

lambda: pr fixes

ff359e7

lambda: pr fixes

04a3b78

jamesnixon-aws requested changes Sep 30, 2024

View reviewed changes

maarbash-aws and others added 8 commits September 30, 2024 14:40

ui-deployment: warning for locked user pool settings

e2efdba

api: schema update

a106314

api: schema tests

9d569a2

api: schema tests

2dc12dc

lambda: add tests for AuthenticationParams

53bf9fc

ui-deployment: updated test

1399718

Merge branch 'aws-solutions:main' into main

15fd462

ui-deployment: fix

72f2489

majdarbash requested a review from jamesnixon-aws October 10, 2024 14:56

jamesnixon-aws mentioned this pull request Nov 20, 2024

Updates for release v2.1.0 #156

Merged

ihmaws mentioned this pull request Nov 20, 2024

Updates for release v2.1.0 #157

Merged

jamesnixon-aws closed this Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for custom Cognito User Pools #129

Support for custom Cognito User Pools #129

majdarbash commented Aug 9, 2024 •

edited

Loading

knihit commented Aug 9, 2024

jamesnixon-aws left a comment

jamesnixon-aws Aug 26, 2024

majdarbash Aug 26, 2024

jamesnixon-aws Aug 26, 2024

jamesnixon-aws Aug 26, 2024

jamesnixon-aws Aug 26, 2024

majdarbash Aug 26, 2024

jamesnixon-aws Aug 26, 2024

majdarbash Aug 26, 2024

jamesnixon-aws Aug 26, 2024

majdarbash Aug 26, 2024

jamesnixon-aws Aug 26, 2024

OmarRad Aug 26, 2024

majdarbash Aug 26, 2024

OmarRad left a comment

majdarbash commented Aug 26, 2024

bogdankatishev commented Sep 24, 2024

tabdunabi commented Sep 24, 2024

majdarbash commented Sep 27, 2024

jamesnixon-aws left a comment

jamesnixon-aws left a comment

jamesnixon-aws commented Nov 20, 2024

Support for custom Cognito User Pools #129

Support for custom Cognito User Pools #129

Conversation

majdarbash commented Aug 9, 2024 • edited Loading

knihit commented Aug 9, 2024

jamesnixon-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarRad left a comment

Choose a reason for hiding this comment

majdarbash commented Aug 26, 2024

bogdankatishev commented Sep 24, 2024

tabdunabi commented Sep 24, 2024

majdarbash commented Sep 27, 2024

jamesnixon-aws left a comment

Choose a reason for hiding this comment

jamesnixon-aws left a comment

Choose a reason for hiding this comment

jamesnixon-aws commented Nov 20, 2024

majdarbash commented Aug 9, 2024 •

edited

Loading