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

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@psyvision
Contributor

psyvision commented Oct 3, 2017

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

make testacc TEST=./aws TESTARGS='-run=TestAccAWSCognitoUserPoolClient_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoUserPoolClient_ -timeout 120m
=== RUN   TestAccAWSCognitoUserPoolClient_basic
--- PASS: TestAccAWSCognitoUserPoolClient_basic (19.11s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       19.126s

Resources

resource "aws_cognito_user_pool_client" "client" {
}

@psyvision psyvision changed the title from Cognito User Pool Client to Add 'aws_cognito_user_pool_client' resource Oct 4, 2017

@radeksimko radeksimko modified the milestones: v1.2.0, v1.3.0 Oct 26, 2017

@radeksimko radeksimko added the size/XXL label Nov 15, 2017

@radeksimko radeksimko modified the milestones: v1.3.0, v1.4.0 Nov 15, 2017

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Nov 15, 2017

Contributor

@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:
http://docs.aws.amazon.com/cognito-user-identity-pools/latest/APIReference/API_AddCustomAttributes.html

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...

Contributor

psyvision commented Nov 15, 2017

@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:
http://docs.aws.amazon.com/cognito-user-identity-pools/latest/APIReference/API_AddCustomAttributes.html

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...

@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Nov 15, 2017

Contributor

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.

Contributor

radeksimko commented Nov 15, 2017

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.

@Ninir

This comment has been minimized.

Show comment
Hide comment
@Ninir

Ninir Nov 16, 2017

Collaborator

Hi @psyvision

User Pools have just been merged and it's all available in AWS 1.3.0!
Could you rebase your PR against master so that we can go on on this one?

Thanks!

Collaborator

Ninir commented Nov 16, 2017

Hi @psyvision

User Pools have just been merged and it's all available in AWS 1.3.0!
Could you rebase your PR against master so that we can go on on this one?

Thanks!

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Nov 17, 2017

Contributor

Hi @Ninir,

I saw that, I'll merge in this evening for you :)

Cheers

Contributor

psyvision commented Nov 17, 2017

Hi @Ninir,

I saw that, I'll merge in this evening for you :)

Cheers

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Nov 17, 2017

Contributor

@Ninir I got there one way or another, all merged now!

Contributor

psyvision commented Nov 17, 2017

@Ninir I got there one way or another, all merged now!

@Ninir Ninir referenced this pull request Nov 20, 2017

Closed

Missing AWS Cognito Support #232

@et304383

This comment has been minimized.

Show comment
Hide comment
@et304383

et304383 Nov 20, 2017

So excited to see this coming in soon!

et304383 commented Nov 20, 2017

So excited to see this coming in soon!

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Nov 20, 2017

Contributor

@et304383 am I correct in thinking you want custom attribute support too? We need that but I'm not sure how to code it. Once @Ninir gets started on this PR I'll see what I can sort out.

Contributor

psyvision commented Nov 20, 2017

@et304383 am I correct in thinking you want custom attribute support too? We need that but I'm not sure how to code it. Once @Ninir gets started on this PR I'll see what I can sort out.

@et304383

This comment has been minimized.

Show comment
Hide comment
@et304383

et304383 Nov 20, 2017

@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.

et304383 commented Nov 20, 2017

@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.

@CalebMacdonaldBlack

This comment has been minimized.

Show comment
Hide comment
@CalebMacdonaldBlack

CalebMacdonaldBlack Nov 27, 2017

@psyvision @Ninir Great to see cognito user pools! Just waiting on app clients and custom attributes before we can use it

CalebMacdonaldBlack commented Nov 27, 2017

@psyvision @Ninir Great to see cognito user pools! Just waiting on app clients and custom attributes before we can use it

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Nov 27, 2017

Contributor

@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 :)

Contributor

psyvision commented Nov 27, 2017

@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 :)

@radeksimko

radeksimko requested changes Dec 4, 2017 edited

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.

Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated website/docs/r/cognito_user_pool_client.markdown Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Dec 4, 2017

Contributor

@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 aws_cognito_user_pool implemented in 1.3.0 (I think) and not in the client.

Contributor

psyvision commented Dec 4, 2017

@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 aws_cognito_user_pool implemented in 1.3.0 (I think) and not in the client.

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Dec 5, 2017

Contributor

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.

Contributor

psyvision commented Dec 5, 2017

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.

Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client_test.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Dec 11, 2017

Contributor

@radeksimko changes made. I'll make up some tests tonight :)

Contributor

psyvision commented Dec 11, 2017

@radeksimko changes made. I'll make up some tests tonight :)

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Dec 12, 2017

Contributor

@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 !

Contributor

psyvision commented Dec 12, 2017

@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 !

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Dec 12, 2017

Contributor

@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
Contributor

psyvision commented Dec 12, 2017

@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
@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Jan 3, 2018

Contributor

Hi @psyvision
it looks like this is almost ready for the final review & merge, there are just some conflicts as it seems there are some unrelated commits in your branch, possibly as a result of earlier conflict resolution?

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.

Contributor

radeksimko commented Jan 3, 2018

Hi @psyvision
it looks like this is almost ready for the final review & merge, there are just some conflicts as it seems there are some unrelated commits in your branch, possibly as a result of earlier conflict resolution?

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.

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Jan 3, 2018

Contributor

Well that's royally buggered it!

Contributor

psyvision commented Jan 3, 2018

Well that's royally buggered it!

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Jan 4, 2018

Contributor

@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.

Contributor

psyvision commented Jan 4, 2018

@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.

@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Jan 4, 2018

Contributor

@radeksimko done!

Contributor

psyvision commented Jan 4, 2018

@radeksimko done!

@et304383

This comment has been minimized.

Show comment
Hide comment
@et304383

et304383 Jan 4, 2018

Anything else holding this up folks? We are immediately dealing with an issue with creating the following:

  • cognito user pool
  • cognito user pool client
  • cognito identity using the user pool client

Without the user pool client support we're going to have to hack something together with:

  • terraform dir 1
  • python script
  • terraform dir 2 with numerous data source lookups

et304383 commented Jan 4, 2018

Anything else holding this up folks? We are immediately dealing with an issue with creating the following:

  • cognito user pool
  • cognito user pool client
  • cognito identity using the user pool client

Without the user pool client support we're going to have to hack something together with:

  • terraform dir 1
  • python script
  • terraform dir 2 with numerous data source lookups
@serialseb

This comment has been minimized.

Show comment
Hide comment
@serialseb

serialseb Jan 4, 2018

if that helps, in the meantime, i use this:

resource "aws_cloudformation_stack" "user_pool_client" {
  name          = "${var.name}-user-pool-client"
  template_body = "${file("${path.module}/templates/stack.yml")}"
  on_failure    = "DELETE"

  parameters {
    UserPoolId = "${var.user_pool_id}"
    ClientName = "${var.name}"
  }
}

with a cloudformation yml

AWSTemplateFormatVersion: 2010-09-09
Parameters:
  UserPoolId:
    Type: String
  ClientName:
    Type: String
Resources:
  UserPoolClient:
    Type: 'AWS::Cognito::UserPoolClient'
    Properties:
      UserPoolId: !Ref UserPoolId
      ClientName: !Ref ClientName
      GenerateSecret: False
      RefreshTokenValidity: xxx
      ExplicitAuthFlows:
        - xxx
Outputs:
  Id:
    Value:
      Ref: UserPoolClient

and for on top of that, for the stuff not in cloudformation



resource "null_resource" "user_pool_client_identities" {
  depends_on = ["aws_cloudformation_stack.user_pool_client"]
  triggers {
    client_id = "${aws_cloudformation_stack.user_pool_client.outputs["Id"]}"
    stack        = "${aws_cloudformation_stack.user_pool_client.template_body}"
    user_pool_id = "${var.user_pool_id}"
    oauth_flows = "${join("", var.allowed_oauth_flows)}"
    oauth_scopes = "${join("", var.allowed_oauth_scopes)}"
  }

  provisioner "local-exec" {
    when ="create"
    command = <<SCRIPT
aws cognito-idp update-user-pool-client \
      --profile wf \
      --user-pool-id ${var.user_pool_id} \
      --client-id ${aws_cloudformation_stack.user_pool_client.outputs["Id"]} \
      --supported-identity-providers ${join(" ", formatlist("\"%s\"", var.supported_identity_providers))} \
      --callback-urls '[${join(",", formatlist("\"%s\"", var.callback_urls))}]' \
      --logout-urls '[${join(",", formatlist("\"%s\"", var.logout_urls))}]' \
      --allowed-o-auth-flows ${join(" ", formatlist("\"%s\"", var.allowed_oauth_flows))} \
      --allowed-o-auth-scopes ${join(" ", formatlist("\"%s\"", var.allowed_oauth_scopes))}
SCRIPT
  }
}

I've built that as a module, until the resource is ready.

serialseb commented Jan 4, 2018

if that helps, in the meantime, i use this:

resource "aws_cloudformation_stack" "user_pool_client" {
  name          = "${var.name}-user-pool-client"
  template_body = "${file("${path.module}/templates/stack.yml")}"
  on_failure    = "DELETE"

  parameters {
    UserPoolId = "${var.user_pool_id}"
    ClientName = "${var.name}"
  }
}

with a cloudformation yml

AWSTemplateFormatVersion: 2010-09-09
Parameters:
  UserPoolId:
    Type: String
  ClientName:
    Type: String
Resources:
  UserPoolClient:
    Type: 'AWS::Cognito::UserPoolClient'
    Properties:
      UserPoolId: !Ref UserPoolId
      ClientName: !Ref ClientName
      GenerateSecret: False
      RefreshTokenValidity: xxx
      ExplicitAuthFlows:
        - xxx
Outputs:
  Id:
    Value:
      Ref: UserPoolClient

and for on top of that, for the stuff not in cloudformation



resource "null_resource" "user_pool_client_identities" {
  depends_on = ["aws_cloudformation_stack.user_pool_client"]
  triggers {
    client_id = "${aws_cloudformation_stack.user_pool_client.outputs["Id"]}"
    stack        = "${aws_cloudformation_stack.user_pool_client.template_body}"
    user_pool_id = "${var.user_pool_id}"
    oauth_flows = "${join("", var.allowed_oauth_flows)}"
    oauth_scopes = "${join("", var.allowed_oauth_scopes)}"
  }

  provisioner "local-exec" {
    when ="create"
    command = <<SCRIPT
aws cognito-idp update-user-pool-client \
      --profile wf \
      --user-pool-id ${var.user_pool_id} \
      --client-id ${aws_cloudformation_stack.user_pool_client.outputs["Id"]} \
      --supported-identity-providers ${join(" ", formatlist("\"%s\"", var.supported_identity_providers))} \
      --callback-urls '[${join(",", formatlist("\"%s\"", var.callback_urls))}]' \
      --logout-urls '[${join(",", formatlist("\"%s\"", var.logout_urls))}]' \
      --allowed-o-auth-flows ${join(" ", formatlist("\"%s\"", var.allowed_oauth_flows))} \
      --allowed-o-auth-scopes ${join(" ", formatlist("\"%s\"", var.allowed_oauth_scopes))}
SCRIPT
  }
}

I've built that as a module, until the resource is ready.

@et304383

This comment has been minimized.

Show comment
Hide comment
@et304383

et304383 Jan 4, 2018

@serialseb this is amazing. Thanks so much. I didn't know you could write custom resources backed by shell commands. You rock.

et304383 commented Jan 4, 2018

@serialseb this is amazing. Thanks so much. I didn't know you could write custom resources backed by shell commands. You rock.

@radeksimko

LGTM now, thanks for the patience and all changes.

Show outdated Hide outdated aws/provider.go Outdated
Show outdated Hide outdated aws/resource_aws_cognito_user_pool_client.go Outdated
@psyvision

This comment has been minimized.

Show comment
Hide comment
@psyvision

psyvision Jan 11, 2018

Contributor

Thank you @radeksimko!

Contributor

psyvision commented Jan 11, 2018

Thank you @radeksimko!

@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Jan 11, 2018

Contributor

I didn't mean to close that... wanted to cleanup your git history and somehow managed to close it.
Anyways it's now in master under c583147b8c8b872c6c06dddee62ceb0a7b410646.

Just a friendly note for future PRs:
It can make everyone's life much easier if you open PR from a branch, not from master. It's easier for rebasing, resolving conflicts etc. 😉

Contributor

radeksimko commented Jan 11, 2018

I didn't mean to close that... wanted to cleanup your git history and somehow managed to close it.
Anyways it's now in master under c583147b8c8b872c6c06dddee62ceb0a7b410646.

Just a friendly note for future PRs:
It can make everyone's life much easier if you open PR from a branch, not from master. It's easier for rebasing, resolving conflicts etc. 😉

@et304383

This comment has been minimized.

Show comment
Hide comment
@et304383

et304383 Jan 11, 2018

I'm a little confused now. @radeksimko is user pool client now in master? Are we just waiting for a new terraform version?

et304383 commented Jan 11, 2018

I'm a little confused now. @radeksimko is user pool client now in master? Are we just waiting for a new terraform version?

@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Jan 11, 2018

Contributor

it's in master, will be part of the next release.

Contributor

radeksimko commented Jan 11, 2018

it's in master, will be part of the next release.

@bflad

This comment has been minimized.

Show comment
Hide comment
@bflad

bflad Jan 12, 2018

Contributor

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.

Contributor

bflad commented Jan 12, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment