New: [AEA-4497] - deploy cognito #138
Conversation
…d updates references to source code for cf functions
…d updates references to source code for cf functions
|
| import { render, screen } from '@testing-library/react'; | ||
| import App from './App'; | ||
|
|
||
| test('renders learn react link', () => { |
| setIsSignedIn(true) | ||
| const currentUser = await getCurrentUser(); | ||
| setUser(currentUser); | ||
| } |
| setError("An error has ocurred during the OAuth flow."); | ||
| break; | ||
| case "customOAuthState": | ||
| setCustomState(payload.data); // this is the customState provided on signInWithRedirect function |
There was a problem hiding this comment.
odd that we're capturing these and not showing them anywhere
| // If you want to start measuring performance in your app, pass a function | ||
| // to log results (for example: reportWebVitals(console.log)) | ||
| // or send to an analytics endpoint. Learn more: https://bit.ly/CRA-vitals | ||
| reportWebVitals(); |
There was a problem hiding this comment.
This seems like an unnecessary dependency to be dragging along
| props.mockOidcIssuer === undefined | ||
| ) { | ||
| throw new Error("Attempt to use mock oidc but variables are not defined") | ||
| } |
There was a problem hiding this comment.
This seems a bit OTT, we're not doing the same checks for the primary props.
| OAuthScope.COGNITO_ADMIN | ||
| ], | ||
| callbackUrls: [ | ||
| "http://localhost:3000/auth/", |
There was a problem hiding this comment.
this feels like it should be conditional, only present for internal-dev of something
| }) | ||
|
|
||
| const supportedIdentityProviders: Array<UserPoolClientIdentityProvider> = [ | ||
| UserPoolClientIdentityProvider.COGNITO, |
There was a problem hiding this comment.
Why are we allowing Cognito itself to be an identity provider? I thought we said that real CIS2 and mock auth would be all we need.
| userPoolWebClient.node.addDependency(primaryPoolIdentityProvider) | ||
| if (props.useMockOidc) { | ||
| userPoolWebClient.node.addDependency(mockPoolIdentityProvider) | ||
| } |
There was a problem hiding this comment.
This seems like overkill, they are directly referenced in the definition of the client, do we definitely need the explicit dependency as well?
| props.mockOidcIssuer === undefined | ||
| ) { | ||
| throw new Error("Attempt to use mock oidc but variables are not defined") | ||
| } |
| principals: [ | ||
| new AccountRootPrincipal | ||
| ], | ||
| resources: ["*"] |
There was a problem hiding this comment.
declaring resources on this is meaningless, can we just remove this property?
| "kms:Encrypt", | ||
| "kms:GenerateDataKey*" | ||
| ], | ||
| resources:["*"] |
| ] | ||
| }) | ||
| }) | ||
| jwtKmsKey.addAlias(`alias/${props.stackName}-jwtKmsKey`) |
There was a problem hiding this comment.
this is creating a duplicate alias, the "alias" property directly on the key is already making one.
| actions: [ | ||
| "secretsmanager:PutSecretValue" | ||
| ], | ||
| resources:["*"] |
| oidcjwksEndpoint: props.mockOidcjwksEndpoint, | ||
| jwtPrivateKeyArn: mockJwtPrivateKey.secretArn, | ||
| userInfoEndpoint: props.mockOidcUserInfoEndpoint, | ||
| useSignedJWT: "false", |
There was a problem hiding this comment.
If we're not using a signed jwt, why do we need a jwt private key?
| readonly table: TableV2 | ||
| } | ||
|
|
||
| export class DynamodbResources extends Construct { |
There was a problem hiding this comment.
I'm not sure why we're separating these into their own file. Why not just declare them in packages/cdk/resources/Dynamodb.ts
| const authorizer = new CognitoUserPoolsAuthorizer(this, "Authorizer", { | ||
| authorizerName: "cognitoAuth", | ||
| cognitoUserPools: [props.userPool], | ||
| identitySource: "method.request.header.authorization" |
There was a problem hiding this comment.
I think this is the default, we could be explicit but I think I would prefer specify the minimum we can
Summary
Details