lgallard / terraform-aws-cognito-user-pool

Terraform module to create Amazon Cognito User Pools, configure its attributes and resources such as app clients, domain, resource servers. Amazon Cognito User Pools provide a secure user directory that scales to hundreds of millions of users.
Apache License 2.0
93 stars 95 forks source link

Removing device configuration config where challenge_required_on_new_… #123

Closed trahim closed 1 month ago

trahim commented 1 year ago

…device and device_only_remembered_on_user_prompt cannot both be false.

PR to help resolve issue https://github.com/lgallard/terraform-aws-cognito-user-pool/issues/122

lgallard commented 10 months ago

@trahim thanks for your PR. I understand the issue but I wonder if instead of removing the code it would be better approach to consider the edge cases as in #124 or something like this:

device_configuration {
  challenge_required_on_new_device      = var.device_configuration["challenge_required_on_new_device"] != null ? var.device_configuration["challenge_required_on_new_device"] : null
  device_only_remembered_on_user_prompt = var.device_configuration["device_only_remembered_on_user_prompt"] != null ? var.device_configuration["device_only_remembered_on_user_prompt"] : null
}
trahim commented 8 months ago

Hey @lgallard sorry for the late reply. I think my latest change by setting both var.device_configuration_challenge_required_on_new_device and var.device_configuration_device_only_remembered_on_user_prompt to null should account for all use cases mentioned in https://github.com/lgallard/terraform-aws-cognito-user-pool/issues/124

Let me know if I have missed any:

1) "Don’t remember" Option

device_configuration = { challenge_required_on_new_device = null device_only_remembered_on_user_prompt = null }

Not setting either var.device_configuration_challenge_required_on_new_device or var.device_configuration_device_only_remembered_on_user_prompt and not having challenge_required_on_new_device and device_only_remembered_on_user_prompt in var.device_configuration will result in this use case.

2) "User opt-in", Allows users to bypass MFA for trusted devices "YES" Option

device_configuration = { challenge_required_on_new_device = true device_only_remembered_on_user_prompt = true }

Setting both var.device_configuration_challenge_required_on_new_device and var.device_configuration_device_only_remembered_on_user_prompt as true or challenge_required_on_new_device and device_only_remembered_on_user_prompt in var.device_configuration being set to true will result in this use case.

3) "User opt-in", Allows users to bypass MFA for trusted devices "NO" Option

device_configuration = { challenge_required_on_new_device = false device_only_remembered_on_user_prompt = true }

Setting var.device_configuration_challenge_required_on_new_device as false and var.device_configuration_device_only_remembered_on_user_prompt as true or challenge_required_on_new_device as false and device_only_remembered_on_user_prompt as true in var.device_configuration will result in this use case.

4) "Always remember", Allow users to bypass MFA for trusted devices "NO" Option

device_configuration = { challenge_required_on_new_device = false device_only_remembered_on_user_prompt = false }

Setting both var.device_configuration_challenge_required_on_new_device and var.device_configuration_device_only_remembered_on_user_prompt as false or challenge_required_on_new_device and device_only_remembered_on_user_prompt in var.device_configuration being set to false will result in this use case.

5) "Always remember", Allow users to bypass MFA for trusted devices "YES" Option

device_configuration = { challenge_required_on_new_device = true device_only_remembered_on_user_prompt = false }

Setting var.device_configuration_challenge_required_on_new_device as true and var.device_configuration_device_only_remembered_on_user_prompt as false or challenge_required_on_new_device as true and device_only_remembered_on_user_prompt as false in var.device_configuration will result in this use case.

This will be a breaking change, my last change would also work but you could not get option 1 without explicitly setting null on var.device_configuration_challenge_required_on_new_device and var.device_configuration_device_only_remembered_on_user_prompt.

trahim commented 4 months ago

@lgallard any thoughts on the above?

lgallard commented 1 month ago

@trahim so sorry for the delay. Your changes make sense for the edge cases.