-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Add 'aws_cognito_user_pool_client' resource #1803
Conversation
@radeksimko do you have any pointers on how I should implement the custom user pool client attributes that belong to the user pool client? See the AWS API reference below: These are different to the typical "attributes" that belong to resources (i.e. attributes are an entity in themselves to some extent). I could add a further resource "aws_cognito_user_pool_client_attribute" or have them as a set on the client_pool resource I've already added, similar to ingress/egress rules on a security group... |
We will take a look at this PR 🔜 after merging #1419 which should be either today or tomorrow, but given the tight schedule for 1.3.0 we intend to ship tomorrow I reckon we'll leave this PR for the next release. |
Hi @psyvision User Pools have just been merged and it's all available in AWS 1.3.0! Thanks! |
Hi @Ninir, I saw that, I'll merge in this evening for you :) Cheers |
@Ninir I got there one way or another, all merged now! |
So excited to see this coming in soon! |
@psyvision that would be nice to have long term but in the short term what we need is the ability to create them and specify the refresh token validity. We need React frontends to be able to interact with Cognito as a priority. We've resorted to adding the client creation through a python script in the short term. |
@psyvision @Ninir Great to see cognito user pools! Just waiting on app clients and custom attributes before we can use it |
@CalebMacdonaldBlack app clients are in, I can't remember if I covered off all functionality (I think maybe so...) I don't know how to go about implementing custom attributes, otherwise I would have done so. Once @Ninir gets on to this or gives me some pointers I'll do it :) |
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.
Hi @psyvision
thanks for the PR. I left you some comments.
Regarding custom attributes I think we should model that as a separate resource, e.g. aws_cognito_user_attribute
- which is outside of scope of this PR. We'll also need to find out whether/how it conflicts with existing resources.
|
||
if v, ok := d.GetOk("refresh_token_validity"); ok { | ||
params.RefreshTokenValidity = aws.Int64(v.(int64)) | ||
} |
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 see there are some fields missing in the schema:
AllowedOAuthFlows []*string `type:"list"`
// Set to True if the client is allowed to follow the OAuth protocol when interacting
// with Cognito user pools.
AllowedOAuthFlowsUserPoolClient *bool `type:"boolean"`
// A list of allowed OAuth scopes. Currently supported values are "phone", "email",
// "openid", and "Cognito".
AllowedOAuthScopes []*string `type:"list"`
// A list of allowed callback URLs for the identity providers.
CallbackURLs []*string `type:"list"`
// The default redirect URI. Must be in the CallbackURLs list.
DefaultRedirectURI *string `min:"1" type:"string"`
// A list of allowed logout URLs for the identity providers.
LogoutURLs []*string `type:"list"`
// A list of provider names for the identity providers that are supported on
// this client.
SupportedIdentityProviders []*string `type:"list"`
Do you plan on adding those?
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 haven't used these so I'm not sure exactly what's best / what's expected or to be able to test them for real. I can have a go though :D
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.
Yeah - I think the best we can do is to read the relevant docs and try to build a few simple examples with those fields in a test based on that. If you want to test it for real (i.e. login in the browser) then I take that as a bonus, but I'd just assume it works as long as the API accepts our request. 🤷♂️
|
||
"refresh_token_validity": { | ||
Type: schema.TypeInt, | ||
Optional: true, |
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.
The default validity seems to be set to 30
by the API which is why the acceptance test is failing:
=== RUN TestAccAWSCognitoUserPoolClient_basic
--- FAIL: TestAccAWSCognitoUserPoolClient_basic (34.36s)
testing.go:503: Step 0 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: aws_cognito_user_pool_client.client
refresh_token_validity: "30" => "0"
Do you mind setting Default: 30
here too?
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 issues with that
} | ||
|
||
if v, ok := d.GetOk("refresh_token_validity"); ok { | ||
params.RefreshTokenValidity = aws.Int64(v.(int64)) |
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.
We get int
, not int64
from the ResourceData, so this would cause a crash due to invalid cast. I think we'll need to change it to something like this:
params.RefreshTokenValidity = aws.Int64(int64(v.(int)))
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 hadn't actually tested this in AWS and was just going on what VS Code was telling me so I'll take your call.
resp, err := conn.DescribeUserPoolClient(params) | ||
|
||
if err != nil { | ||
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "ResourceNotFoundException" { |
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.
Nitpick: this error handling here can be simplified to
if isAWSErr(err, "ResourceNotFoundException", "") {
* `name` - (Required) The name of the application client. | ||
* `generate_secret` - (Optional) Should an application secret be generated. AWS JavaScript SDK requires this to be false. | ||
* `user_pool_id` - (Required) The user pool the client belongs to. | ||
* `explicit_auth_flows` - (Optional) List of authentication flows (ADMIN_NO_SRP_AUTH, CUSTOM_AUTH_FLOW_ONLY) |
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 think the list of fields here is incomplete - do you mind updating it?
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.
Will do
"name": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, |
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 looks like this field is updatable (at least it's present in UpdateUserPoolClientInput
) - is there any reason for making it ForceNew
?
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.
Nope, just me not knowing what's what :)
"explicit_auth_flows": { | ||
Type: schema.TypeList, | ||
Optional: true, | ||
ForceNew: false, |
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 looks like this field is updatable (at least it's present in UpdateUserPoolClientInput
) - is there any reason for making it ForceNew?
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.
Not sure which field this refers to? If explicit_auth_flows
then it's false
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.
Ah, sorry - I missed that - so in that case it's practically correct, but it's false
by default so we can just omit it.
"read_attributes": { | ||
Type: schema.TypeList, | ||
Optional: true, | ||
ForceNew: false, |
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.
Likewise - ^
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.
read_attributes
is ForceNew: false
but I guess the redundant code can be removed
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.
Yes, it can be removed - there's no point of having ForceNew: false
anywhere as that's default. 😉
"write_attributes": { | ||
Type: schema.TypeList, | ||
Optional: true, | ||
ForceNew: false, |
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.
Likewise - ^
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.
write_attributes
is ForceNew: false
but I guess the redundant code can be removed
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.
Correct.
"refresh_token_validity": { | ||
Type: schema.TypeInt, | ||
Optional: true, | ||
ForceNew: false, |
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.
Likewise - ^
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's just redundant - that's all 😉
@radeksimko Thank you for reviewing these. I'll go through and make some changes where required. I don't think the attributes are needed. I looked into it and they actually fall under the |
|
||
if resp.UserPoolClient.ClientSecret != nil { | ||
d.Set("client_secret", resp.UserPoolClient.ClientSecret) | ||
} |
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.
d.Set()
will perform safe dereferencing (and deal with nil
without crashing), so all of the above 5 checks are redundant. 😉
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.
Ah thanks. I've been copying code from elsewhere rather than looking into the intracacies. I'll add that to my changes. Just had to rebuild my dev VM
ok @radeksimko I've covered off the items in your review I think. Just need to add in those final attributes which I'll do shortly. |
|
||
d.SetId(*resp.UserPoolClient.ClientId) | ||
d.Set("user_pool_id", *resp.UserPoolClient.UserPoolId) | ||
d.Set("name", *resp.UserPoolClient.ClientName) |
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.
The two d.Set()
s are IMO unnecessary unless we believe the API would return something that differs from the original query - in which case we'd probably have much bigger problem.
Type: schema.TypeString, | ||
ValidateFunc: validation.StringInSlice([]string{ | ||
cognitoidentityprovider.AuthFlowTypeAdminNoSrpAuth, | ||
cognitoidentityprovider.AuthFlowTypeCustomAuth, |
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 think this should be cognitoidentityprovider.ExplicitAuthFlowsTypeCustomAuthFlowOnly
per API docs.
}, | ||
}) | ||
} | ||
|
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 think we'll need at least one more test to exercise as many fields as we can. I'd usually call such test TestAccAWSCognitoUserPoolClient_allFields
.
}, | ||
|
||
"allowed_oauth_scopes": { | ||
Type: schema.TypeList, |
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.
AFAIK (correct me if I'm wrong) ordering is not significant in this case, so we should make this field TypeSet
.
}, | ||
|
||
"read_attributes": { | ||
Type: schema.TypeList, |
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.
AFAIK (correct me if I'm wrong) ordering is not significant in this case, so we should make this field TypeSet
.
}, | ||
|
||
"write_attributes": { | ||
Type: schema.TypeList, |
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.
AFAIK (correct me if I'm wrong) ordering is not significant in this case, so we should make this field TypeSet
.
}, | ||
|
||
"explicit_auth_flows": { | ||
Type: schema.TypeList, |
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.
AFAIK (correct me if I'm wrong) ordering is not significant in this case, so we should make this field TypeSet
.
}, | ||
|
||
"allowed_oauth_flows": { | ||
Type: schema.TypeList, |
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.
AFAIK (correct me if I'm wrong) ordering is not significant in this case, so we should make this field TypeSet
.
@radeksimko changes made. I'll make up some tests tonight :) |
CheckDestroy: testAccCheckAWSCognitoUserPoolClientDestroy, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccAWSCognitoUserPoolClientConfig_basic(name), |
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 think you're referencing wrong config here 😉
Let me know when it's ready for another round of review.
@radeksimko I've changed the test, I was going to run the acceptance test but wanted to run just the user pool client ones if possible. I can't remember how to do this.... ? Edit: Got it ! |
@radeksimko Tests corrected and working: make testacc TESTARGS="-run=TestAccAWSCognitoUserPoolClient_*"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSCognitoUserPoolClient_* -timeout 120m
? github.com/terraform-providers/terraform-provider-aws [no test files]
=== RUN TestAccAWSCognitoUserPoolClient_basic
--- PASS: TestAccAWSCognitoUserPoolClient_basic (21.63s)
=== RUN TestAccAWSCognitoUserPoolClient_allFields
--- PASS: TestAccAWSCognitoUserPoolClient_allFields (19.32s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 40.966s |
Hi @psyvision I'd recommend using interactive rebase and remove all unrelated commits from the branch, see https://git-scm.com/book/id/v2/Git-Tools-Rewriting-History or let me know if you need help with that. Eventually I can try and resolve it for you. |
Well that's royally buggered it! |
b978a13
to
6176813
Compare
@radeksimko was getting late last night and I kept messing up the rebase. I need to just drop commit 52143f4, the ses changes and then I think it's done properly. |
@radeksimko done! |
Anything else holding this up folks? We are immediately dealing with an issue with creating the following:
Without the user pool client support we're going to have to hack something together with:
|
if that helps, in the meantime, i use this:
with a cloudformation yml
and for on top of that, for the stuff not in cloudformation
I've built that as a module, until the resource is ready. |
@serialseb this is amazing. Thanks so much. I didn't know you could write custom resources backed by shell commands. You rock. |
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.
LGTM now, thanks for the patience and all changes.
aws/provider.go
Outdated
@@ -293,7 +293,7 @@ func Provider() terraform.ResourceProvider { | |||
"aws_cognito_identity_pool_roles_attachment": resourceAwsCognitoIdentityPoolRolesAttachment(), | |||
"aws_cognito_user_pool": resourceAwsCognitoUserPool(), | |||
"aws_cognito_user_pool_domain": resourceAwsCognitoUserPoolDomain(), | |||
"aws_autoscaling_lifecycle_hook": resourceAwsAutoscalingLifecycleHook(), | |||
"aws_cognito_user_pool_client": resourceAwsCognitoUserPoolClient(), |
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 believe that the removal of aws_autoscaling_lifecycle_hook
was not intended here - I'll address that in a separate commit.
}, | ||
}, | ||
|
||
// analytics_configuration |
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'll add TODO here as a reminder and to make it clearer what the comment means.
2cfa50d
to
5486927
Compare
Thank you @radeksimko! |
I didn't mean to close that... wanted to cleanup your git history and somehow managed to close it. Just a friendly note for future PRs: |
I'm a little confused now. @radeksimko is user pool client now in master? Are we just waiting for a new terraform version? |
it's in master, will be part of the next release. |
This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Description
The following pull request builds upon the work of @Ninir implementing Cognito User Pools (not yet merged) by adding support for Cognito User Pool Clients.
I've added some documentation on using the new resource.
Note: This doesn't provide full API support at current but does allow a client to be created and the
ClientId
returned.API Documentation
Test
Resources