Skip to content
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

[Bug]: adding ["COGNITO"] as supported_identity_providers triggers deletion of token_validity_units #34285

Open
garysassano opened this issue Nov 7, 2023 · 9 comments · May be fixed by #34362
Labels
bug Addresses a defect in current functionality. service/cognitoidp Issues and PRs that pertain to the cognitoidp service.

Comments

@garysassano
Copy link
Contributor

Terraform Core Version

1.5.0

AWS Provider Version

5.23.0

Affected Resource(s)

aws_cognito_user_pool_client

Expected Behavior

Before:

resource "aws_cognito_user_pool_client" "grafana" {
  name            = "grafana-client"
  generate_secret = false
  user_pool_id    = aws_cognito_user_pool.grafana.id
  # supported_identity_providers         = ["COGNITO"]
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
}

After:

resource "aws_cognito_user_pool_client" "grafana" {
  name                                 = "grafana-client"
  generate_secret                      = false
  user_pool_id                         = aws_cognito_user_pool.grafana.id
  supported_identity_providers         = ["COGNITO"]
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
}

I expected nothing to happen when executing terraform apply.

Actual Behavior

Terraform asks me to delete token_validity_units, and throws error if I approve.

tf-apply

Workaround: add token_validity_units to the resource configuration even if you don't need to change default values.

resource "aws_cognito_user_pool_client" "grafana" {
  name                                 = "grafana-client"
  generate_secret                      = false
  supported_identity_providers         = ["COGNITO"]
  user_pool_id                         = aws_cognito_user_pool.grafana.id
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
  token_validity_units {
    access_token  = "minutes"
    id_token      = "minutes"
    refresh_token = "days"
  }
}

Relevant Error/Panic Output Snippet

No response

Terraform Configuration Files

see above

Steps to Reproduce

see above

Debug Output

No response

Panic Output

No response

Important Factoids

No response

References

No response

Would you like to implement a fix?

None

@garysassano garysassano added the bug Addresses a defect in current functionality. label Nov 7, 2023
@github-actions github-actions bot added the service/cognitoidp Issues and PRs that pertain to the cognitoidp service. label Nov 7, 2023
Copy link

github-actions bot commented Nov 7, 2023

Community Note

Voting for Prioritization

  • Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

Volunteering to Work on This Issue

  • If you are interested in working on this issue, please leave a comment.
  • If this would be your first contribution, please review the contribution guide.

@terraform-aws-provider terraform-aws-provider bot added the needs-triage Waiting for first response or review from a maintainer. label Nov 7, 2023
@justinretzolk justinretzolk removed the needs-triage Waiting for first response or review from a maintainer. label Nov 7, 2023
@jtyrus
Copy link
Contributor

jtyrus commented Nov 10, 2023

I can take a look at this

@jtyrus
Copy link
Contributor

jtyrus commented Nov 10, 2023

Not able to reproduce and my state file (with or without supported_identity_providers) doesn't show token_validity_units.

Looks like there was a change in 5.24.0 for this, but I wasn't able to reproduce using 5.23.0.
#30662

Are you updating existing user pool clients made a while ago? Can probably make this backwards compatible.

Terraform will perform the following actions:

  # aws_cognito_user_pool_client.test will be updated in-place
  ~ resource "aws_cognito_user_pool_client" "test" {
        id                                            = "5gk7j025k0304g2pm8hr7he7qk"
        name                                          = "mypool-client"
      ~ supported_identity_providers                  = [
          + "COGNITO",
        ]
        # (16 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

@garysassano
Copy link
Contributor Author

This is my current configuration:

resource "aws_cognito_user_pool_client" "grafana" {
  name                = "grafana-client"
  user_pool_id        = aws_cognito_user_pool.grafana.id
  generate_secret     = false
  explicit_auth_flows = ["ALLOW_REFRESH_TOKEN_AUTH", "ALLOW_ADMIN_USER_PASSWORD_AUTH", "ALLOW_USER_PASSWORD_AUTH"]
  token_validity_units {
    refresh_token = "days"
    access_token  = "minutes"
    id_token      = "minutes"
  }
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
  supported_identity_providers         = ["COGNITO"]
}

And if I remove token_validity_units, Terraform asks me to delete it even if it's the default value.
I'm using AWS provider v5.24.0, so I don't think there was any recent change for this resouce.

@jtyrus
Copy link
Contributor

jtyrus commented Nov 11, 2023

Ahh got it I see the issue. I was able to reproduce the error using.

token_validity_units {
    refresh_token = "days"
    access_token  = "minutes"
    id_token      = "minutes"
  }

The issue is the default units for access_token and id_token are hours with a value of 1, but the minimum time 5 minutes. So when you set it to minutes without setting the length, you get the error because you're trying to set it to 1 minute. Try this

access_token_validity = 5
  id_token_validity = 5
  token_validity_units {
    refresh_token = "days"
    access_token  = "minutes"
    id_token      = "minutes"
  }

Or use days, hours, hours which is the default..

Not sure this warrants a change since you're editing 1 half of default behavior and the docs do specify the minimum is 5 minutes.

@garysassano
Copy link
Contributor Author

@jtyrus But I didn't even want to set it in the first place... Since I'm using the defaults, I shouldn't be forced to put that in my Terraform code.

@jtyrus
Copy link
Contributor

jtyrus commented Nov 11, 2023

I wasn't getting an error when I didn't include it.

But I'm testing more and it looks like if I remove everything after first defining the units and validity errors still pop up. Which leads me to believe it's a state issue around the validity.

@jtyrus jtyrus linked a pull request Nov 11, 2023 that will close this issue
@jtyrus
Copy link
Contributor

jtyrus commented Nov 11, 2023

@garysassano Have a fix I think will work, adding/updating tests now. If you're pressed for time manually adding those fields should work for now, default is.

  refresh_token_validity = 1
  access_token_validity = 24
  id_token_validity = 24
  token_validity_units {
    refresh_token = "days"
    access_token  = "hours"
    id_token      = "hours"
  }

Still wasn't able to repro your original scenario, but if you're creating new user pool clients you don't have to include those.

@ckaenzig
Copy link

Hi. I'm also hitting this issue with terraform 1.7.5 and the aws provider version 5.42.0.

I also created the user pool client initially without COGNITO in supported_identity_providers (I use a third party OIDC provider) and later added it. My terraform code doesn't have (and never had) the token_validity_units block.

Subsequent plans or applys show that it wants to remove the token_valididty_units block:

`Terraform will perform the following actions:

  # aws_cognito_user_pool_client.dev_test will be updated in-place
  ~ resource "aws_cognito_user_pool_client" "dev_test" {
        id                                            = "4ifaj1l7okv7s38aelq8tu31u6"
        name                                          = "test"
        # (18 unchanged attributes hidden)

      - token_validity_units {
          - access_token  = "minutes" -> null
          - id_token      = "minutes" -> null
          - refresh_token = "days" -> null
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

If I apply this, I get an error from AWS:

aws_cognito_user_pool_client.dev_test: Modifying... [id=4ifaj1l7okv7s38aelq8tu31u6]
╷
│ Error: updating Cognito User Pool Client (4ifaj1l7okv7s38aelq8tu31u6)
│ 
│   with aws_cognito_user_pool_client.dev_test,
│   on dev.tf line 29, in resource "aws_cognito_user_pool_client" "dev_test":
│   29: resource "aws_cognito_user_pool_client" "dev_test" {
│ 
│ InvalidParameterException: Invalid range for token validity.

I hope this can help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses a defect in current functionality. service/cognitoidp Issues and PRs that pertain to the cognitoidp service.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants