-
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(appsync): extend authorization configuration #6260
feat(appsync): extend authorization configuration #6260
Conversation
257e725
to
9bc636d
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
9bc636d
to
3adf65a
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Overall looks great! We have to replace the union type so this works for JSII.
Question about this vs the cloudformation resource params. Any auth mode can either be default or "additional" right? That would mean we can just add more to the AuthModes struct going forward to keep the api consistent.
return (obj as ApiKeyConfig).apiKeyDesc !== undefined; | ||
} | ||
|
||
type AuthModes = UserPoolConfig | ApiKeyConfig; |
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.
JSII doesn't support unions currently. Usually the workaround is to make an interface that multiple classes implement.
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.
I didn't get any compiler errors and I see examples of union types elsewhere.
I tried a common interface and I got errors complaining about them not being used in the exported classes, but I can try again.
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.
With
interface UserPoolConfig extends AuthMode { ... }
interface ApiKeyConfig extends AuthMode { ... }
export interface AuthorizationConfig {
readonly defaultAuthorization?: AuthMode;
readonly additionalAuthorizationModes?: [AuthMode]
}
I get:
error: [awslint:no-unused-type:@aws-cdk/aws-appsync.UserPoolDefaultAction] type or enum is not used by exported classes (declared at lib/graphqlapi.ts:17)
Because now the exported classes deal in terms of AuthMode
.
This feels like a bug in awslint
because I'm still using those types in a function (e.g., userPoolDescFrom(upConfig: UserPoolConfig)
), but I'm unsure how to proceed.
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.
It packaged fine with the union type. In Java, it mapped to
@software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
public Builder defaultAuthorization(software.amazon.awscdk.services.appsync.UserPoolConfig defaultAuthorization) {
this.defaultAuthorization = defaultAuthorization;
return this;
}
@software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
public Builder defaultAuthorization(software.amazon.awscdk.services.appsync.ApiKeyConfig defaultAuthorization) {
this.defaultAuthorization = defaultAuthorization;
return this;
}
Yes, any mode can be default and an additional one. |
3adf65a
to
988a115
Compare
Added a commit replacing the union type with a type hierarchy. |
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -17,6 +35,7 @@ const customerTable = new Table(stack, 'CustomerTable', { | |||
name: 'id', | |||
type: AttributeType.STRING, | |||
}, | |||
removalPolicy: RemovalPolicy.DESTROY, |
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.
Was the retain removal policy causing failures in the integ tests for you?
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.
No, but it was annoying me to have a ton of left over tables in my account.
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.
Agreed. Lets pull it out of this PR so we can double check with other team members on implications before doing this. Most likely fine though for a future chore.
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.
Done. I opened #6401.
By default, the AppSync L2 constructs use API key authorization, but it doesn't allow configuring the API key. Fix that by allowing a default authorization mode to be specified. Currently, the supported modes are Cognito user pools and API keys. When specifying API key authorization, allow configuring it. BREAKING CHANGE: Configuration the user pool authorization is now done through the authorizationConfig property. This allows us to specify a default authorization mode out of the supported ones, currently limited to Cognito user pools and API keys. Fixes aws#6246 Signed-off-by: Duarte Nunes <duarte@uma.ni>
Currently the AppSync L2 constructs don't provide a way to configure additional authorization modes. Add the ability to specify additional authorization modes, currently limited to Cognito user pools and API keys. Fixes aws#6247 Signed-off-by: Duarte Nunes <duarte@uma.ni>
988a115
to
f9fb09c
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Test using cognito user pools as the default authorization mode and an api key as the additional mode. Signed-off-by: Duarte Nunes <duarte@uma.ni>
f9fb09c
to
78ed6b8
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
🔥 🔥 🔥
Thank you for contributing! Your pull request is now being automatically merged. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request is now being automatically merged. |
By default, the AppSync L2 constructs use API key authorization, but don't allow configuring the API key.
Fix that by allowing a default authorization mode to be specified. Currently, the supported modes are Cognito user pools and API keys. When specifying API key authorization, allow configuring it.
Also add the ability to specify additional authorization modes, currently limited to Cognito user pools and API keys.
BREAKING CHANGE:
Configuring the user pool authorization is now done through the
authorizationConfig
property, allowing to configure the defaultand additional authorization modes.
Fixes #6246
Fixes #6247
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license