Add identity provider modules #650

rocketnova · 2024-06-20T19:38:51Z

Ticket

N/A

Changes

What was added, updated, or removed in this PR.

Adds identity-provider module to configure an AWS Cognito user pool
Adds identity-provider-client module to configure an AWS Cognito user pool app client
Updates the service layer to optionally optionally enable an identity provider

Context for reviewers

Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers.

Many projects need an identity provider. This PR adds an option to the app config to allow projects to specify whether they want to enable one using AWS Cognito.

This is needed by the rails template, which comes with auth out-of-the-box.

Testing

Provide evidence that the code works as expected. Explain what was done for testing and the results of the test plan. Include screenshots, GIF demos, shell commands or output to help show the changes working as expected. ProTip: you can drag and drop or paste images into this textbox.

Tested on platform-test

Using navapbc/platform-test#108

Screenshot showing the Cognito user pool:

Screenshot showing the Cognito user pool app client:

Screenshot of ECS task definition showing that the new Cognito env vars and secrets are passed through to the ECS task:

Test deploy on platform-test-rails

Using navapbc/platform-test-rails#1

aligg · 2024-06-25T16:50:24Z

infra/test/infra_test.go

@@ -132,13 +132,23 @@ func EnableDestroyService(t *testing.T, terraformOptions *terraform.Options) {
 		},
 		WorkingDir: "../../",
 	})
+	shell.RunCommand(t, shell.Command{


What is this section for?

By default, deletion protection is on for cognito. This script is for CI testing the infra service layer. To be able to delete cognito, we have to explicitly disable deletion protection.

lorenyu

Looks good, I think we can remove the sed line from infra_test.go and re-run ci-infra-service.yml in the platform-test repo to make sure it still works just leveraging the is_temporary flag. And I had some comments on getting rid of what seems like unused admin email configuration for now.

lorenyu · 2024-06-25T16:37:35Z

infra/app/app-config/dev.tf

  has_database                    = local.has_database
  has_incident_management_service = local.has_incident_management_service


tangential thought: i've been thinking of renaming these at some point to:

enable_database

enable_incident_management_service_integration

infra/app/app-config/env-config/outputs.tf

infra/app/service/main.tf

infra/modules/identity-provider/main.tf

infra/modules/identity-provider/variables.tf

infra/test/infra_test.go

rocketnova · 2024-06-26T17:43:20Z

@lorenyu I think I've addressed all the comments in your previous review. Could you please take another look? Thanks!

lorenyu

Looks good! Had a few non-blocking questions that I left as comments.

I had just one more question on the topic of testing, I see the tests that created the resources. How complicated would it be to add some minimal auth logic to the example app in platform-test that leverages the identity provider? Also, it doesn't necessarily have to be with platform-test example app — if you tested the identity provider with the rails template, I wonder if there is some evidence we can provide that this infra setup works with the rails app — maybe a link to a platform-test-rails PR (if that exists), or if you have a private test repo that's using this same infra maybe just sharing some screenshots or something from there?

infra/app/app-config/dev.tf

infra/app/app-config/env-config/identity-provider.tf

lorenyu · 2024-06-26T17:29:18Z

infra/app/app-config/env-config/identity-provider.tf

+    email = {
+      # When you're ready to use SES instead of the Cognito default to send emails, set this
+      # to the SES-verified email address to be used when sending emails.
+      sender_email = null
+
+      # Configure the name that users see in the "From" section of their inbox, so that it's
+      # clearer who the email is from.
+      sender_display_name = null
+
+      # Configure the REPLY-TO email address if it should be different from the sender.
+      reply_to_email = null
+    }


idea / todo : what do you think about DRY'ing this up in the future by pulling the sender_email automatically once we have an associated notifications.tf config? e.g. something like

email = { sender_email = var.enable_notifications ? local.notifications_config.sender_email : null ... }

i'd also be interested in exploring the idea of just eliminating these fields altogether from identity_provider_config and just having a single email_config/notifications_config that lives separately and would be used to configure both AWS Pinpoint/SES and AWS Cognito. the idea being that in the common case you want all your service to behave consistently i.e. have the same sender_email, same sender_display_name, etc.

even though we don't have notifications yet, if we separate out the config now, we might be able to avoid moving the config around (which can trigger a breaking change for projects) when we eventually add notifications. or if you think that relies on too much guesswork, we could keep it here for now and refactor later.

thoughts?

Oh I think that's a very good idea. Let me take a look at the SES/Pinpoint config to get a sense of how best to construct a /infra/app/app-config/env-config/notifications.tf config file. If it's easy-ish, then I'll go ahead and lay the foundation now. If it's complicated, then I think we should plan to refactor.

Ok I cross-referenced the Cognito, SES, and Pinpoint docs and configs and did some manual testing to make sure everything would play nicely together. I have a proposed change, but I haven't had time to test it yet, so I'll re-ping once that's done.

Testing complete in the latest version.

infra/app/app-config/env-config/variables.tf

infra/app/app-config/main.tf

infra/app/service/main.tf

infra/modules/identity-provider/variables.tf

rocketnova · 2024-06-27T19:57:16Z

How complicated would it be to add some minimal auth logic to the example app in platform-test that leverages the identity provider?

I think this is too complicated to add to this PR. But does bring up some good questions about how we want to make sure our test apps are exercising all of the core functionality. For instance, I could see wanting to do something similar for notifications. Should we add a follow-up issue for that?

Also, it doesn't necessarily have to be with platform-test example app — if you tested the identity provider with the rails template, I wonder if there is some evidence we can provide that this infra setup works with the rails app — maybe a link to a platform-test-rails PR (if that exists), or if you have a private test repo that's using this same infra maybe just sharing some screenshots or something from there?

Yes, I've been testing with the platform-test-rails repo. I can include some screenshots from there.

rocketnova · 2024-06-27T19:57:21Z

I'm realizing that I totally missed writing documentation for this feature, so I'll add that in, too.

lorenyu · 2024-08-01T05:22:13Z

@rocketnova one thing that came up today is the notion that it would be nice to have PR environments use the same cognito pool as the default workspace, so that we can test using existing users and data rather than needing to go through the account creation flow etc on every PR.

It doesn't have to be in this PR, but I think we'd want to not call the identity provider module if it's a temporary environment and instead use a data source to fetch the existing user pool. Thoughts?

rocketnova · 2024-08-01T18:03:58Z

@lorenyu That's interesting. I think that can work. Let's not put it into this PR. This one basically ready to go (finishing final testing now) and I don't want to delay getting it merged in anymore.

coilysiren · 2024-08-01T20:30:19Z

Yes, I've been testing with the platform-test-rails repo. I can include some screenshots from there.

For future readers, here's some of the cognito application logic:

rocketnova · 2024-08-01T20:32:14Z

Yes, I've been testing with the platform-test-rails repo. I can include some screenshots from there.

For future readers, here's some of the cognito application logic:
* https://github.com/navapbc/platform-test-rails/blob/6cd6cf1d8a35767ab693515374f0658d13ab2e7d/app-rails/app/adapters/auth/cognito_adapter.rb

* https://github.com/navapbc/platform-test-rails/blob/6cd6cf1d8a35767ab693515374f0658d13ab2e7d/app-rails/app/models/concerns/devise/models/cognito_authenticatable.rb

👍 I also added a gif and screenshot to the "Testing" portion of the PR description if that's helpful. Sorry I forgot to call that out explicitly.

coilysiren · 2024-08-01T19:40:54Z

infra/app/app-config/env-config/identity-provider.tf

+locals {
+  # If your application should redirect users, after successful authentication, to a
+  # page other than the homepage, specify the path fragment here.
+  # Example: "profile"


Suggested change

# Example: "profile"

# Example: "profile"

# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-client-apps.html

coilysiren · 2024-08-01T19:41:03Z

infra/app/app-config/env-config/identity-provider.tf

+
+  # If your application should redirect users, after signing out, to a page other than
+  # the homepage, specify the path fragment here.
+  # Example: "logout"


Suggested change

# Example: "logout"

# Example: "logout"

# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-client-apps.html

coilysiren · 2024-08-01T19:42:26Z

infra/app/app-config/env-config/identity-provider.tf

+    }
+
+    # Optionally configure email template for resetting a password.
+    # Set any attribute to a non-null value to override AWS Cognito defaults.


Suggested change

# Set any attribute to a non-null value to override AWS Cognito defaults.

# Set any attribute to a non-null value to override AWS Cognito defaults.

# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pool-settings-message-customizations.html

coilysiren · 2024-08-01T19:46:20Z

infra/app/app-config/env-config/notifications.tf

+# Notifications configuration
+locals {
+  notifications_config = var.enable_notifications ? {
+    # Set to an SES-verified email address to be used when sending emails.


Suggested change

# Set to an SES-verified email address to be used when sending emails.

# Set to an SES-verified email address to be used when sending emails.

# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-email.html

coilysiren · 2024-08-01T20:20:20Z

infra/app/service/main.tf

+  reply_to_email      = local.notifications_config == null ? null : local.notifications_config.reply_to_email
+}
+
+module "identity_provider_client" {


(non-blocking question) what's the reasoning behind splitting this up into two terraform modules?

Good question! This work is adapted from a another project where they were already two modules. Not a great reason on its own, but it was the starting point and in combination with the following reasons, I felt it was the right choice here:

They are configuring slightly different things: the user pool vs the app client that talks to the user pool. This PR doesn't include the following functionality, but a project could have a single user pool with multiple app clients or a project could have just a user pool without an app client. I wanted to keep the foundation flexible to accommodate that additional configuration.

The two modules have largely distinct rather than overlapping variables. Combining them makes a larger variables file than necessary.

If we want to combine them, it's easier to do so than to pull them apart.

Thoughts?

coilysiren · 2024-08-01T20:31:50Z

infra/modules/identity-provider-client/main.tf

+  refresh_token_validity       = 1
+  access_token_validity        = 60
+  id_token_validity            = 60
+  token_validity_units {
+    refresh_token = "days"
+    access_token  = "minutes"
+    id_token      = "minutes"
+  }


(non-blocking question) I'm assuming you just guessed at these numbers?

I brought these values over from a previous project that established these as reasonable defaults. Do you think they should be something else?

coilysiren · 2024-08-01T20:35:17Z

infra/modules/identity-provider-client/main.tf

@@ -0,0 +1,37 @@
+resource "aws_cognito_user_pool_client" "client" {


Suggested change

resource "aws_cognito_user_pool_client" "client" {

# docs: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cognito_user_pool_client

resource "aws_cognito_user_pool_client" "client" {

This is something I like to do, especially for complex terraform resources. I still remember years ago being a terraform newbie and not knowing how to connect the configuration to the documentation

coilysiren · 2024-08-01T20:39:57Z

infra/modules/identity-provider-client/main.tf

+  generate_secret                      = true
+  allowed_oauth_flows_user_pool_client = true
+  allowed_oauth_flows                  = ["code"]
+  allowed_oauth_scopes                 = ["phone", "email", "openid", "profile"]
+  explicit_auth_flows                  = ["ALLOW_ADMIN_USER_PASSWORD_AUTH", "ALLOW_REFRESH_TOKEN_AUTH"]
+
+  # Avoid security issue where error messages indicate when a user doesn't exist
+  prevent_user_existence_errors = "ENABLED"
+
+  enable_token_revocation                       = true
+  enable_propagate_additional_user_context_data = false
+
+  read_attributes  = ["email", "email_verified", "phone_number", "phone_number_verified", "updated_at"]
+  write_attributes = ["email", "updated_at", "phone_number"]


This whole section could use some inline documentation, but I don't wanna block on that.

We should probably establish some guidelines on how much we want to inline document modules, which we are currently anticipating most projects should not modify, vs the /infra/app dirs, which is where we are currently instructing template adopters to put configuration.

coilysiren · 2024-08-01T20:43:03Z

infra/modules/identity-provider/main.tf

+resource "aws_cognito_user_pool" "main" {
+  name = var.name
+
+  # Use a separate line to support automated terraform destroy commands


Suggested change

# Use a separate line to support automated terraform destroy commands

# We use a separate line here to support automated terraform destroy commands.

# Do not edit this line, the destroy command looks for it verbatim with `sed`

All of the lines that get sed by destroy commands have the same comment. I'm going to leave this one for now so that it's easier for us to do a comprehensive find and replace in a later PR.

coilysiren · 2024-08-01T20:43:40Z

template-only-bin/destroy-app-service

@@ -18,11 +18,12 @@ backend_config_file="${environment}.s3.tfbackend"
 sed -i.bak 's/force_destroy = var.is_temporary/force_destroy = true/g' infra/modules/service/access-logs.tf
 sed -i.bak 's/force_destroy = var.is_temporary/force_destroy = true/g' infra/modules/storage/main.tf
 sed -i.bak 's/enable_deletion_protection = !var.is_temporary/enable_deletion_protection = false/g' infra/modules/service/load-balancer.tf
+sed -i.bak 's/deletion_protection = var.is_temporary ? "INACTIVE" : "ACTIVE"/deletion_protection = "INACTIVE"/g' infra/modules/identity-provider/main.tf


This feels very brittle to me (...) but it's the existing pattern, so I suppose I can forgive it ^^

Agree. Would love to adopt a more robust pattern.

lorenyu

yay

rocketnova · 2024-08-07T02:31:55Z

@rocketnova one thing that came up today is the notion that it would be nice to have PR environments use the same cognito pool as the default workspace, so that we can test using existing users and data rather than needing to go through the account creation flow etc on every PR.

It doesn't have to be in this PR, but I think we'd want to not call the identity provider module if it's a temporary environment and instead use a data source to fetch the existing user pool. Thoughts?

Created #719 for this feature.

rocketnova changed the title ~~Add IdP modules~~ Add identity provider modules Jun 20, 2024

rocketnova force-pushed the rocket/idp branch from 7e10778 to 608e5c2 Compare June 20, 2024 23:05

rocketnova added 6 commits June 24, 2024 15:31

Add identity provider modules

9016261

Add identity provider option to service layer

9bbc5de

Add support for auto deleting the identity provider

321e873

Use is_temporary

33550ee

Update ci-infra-service test

1441550

Format

dbd45a9

rocketnova force-pushed the rocket/idp branch from 608e5c2 to dbd45a9 Compare June 24, 2024 22:47

rocketnova added 2 commits June 24, 2024 15:54

Replace old all caps var with lowercase

78cb519

Escape double quotes in go

b2137e5

rocketnova marked this pull request as ready for review June 25, 2024 00:19

rocketnova requested review from a team, lorenyu and shawnvanderjagt June 25, 2024 00:19

aligg reviewed Jun 25, 2024

View reviewed changes

lorenyu reviewed Jun 25, 2024

View reviewed changes

rocketnova added 4 commits June 25, 2024 14:05

Address PR feedback

1d72080

Fix locals output structure

9c6b3a2

Move config to right level

61fc8b6

Merge branch 'main' into rocket/idp

546ca8e

rocketnova requested a review from lorenyu June 26, 2024 17:42

lorenyu reviewed Jun 26, 2024

View reviewed changes

rocketnova added 6 commits June 27, 2024 11:18

Remove extra _auth_ in var name

c36bbf1

Sort variables.tf and outputs.tf alphabetically

54b8fba

Use active voice

c8bf7d2

Use consistent naming convention

96c771b

Use better variable description

dcf394b

Use clearer comment language

c24f582

rocketnova added 3 commits June 27, 2024 11:21

Support callback/logout paths

7560036

Fix typo

bc414e5

Break out email configuration into separate config

d1ab6af

rocketnova added 3 commits July 26, 2024 17:48

Handle case if sender_display_name is null

fb4e88e

Merge branch 'main' into rocket/idp

f551243

Alphabetize env config vars

34728aa

rocketnova added 3 commits August 1, 2024 10:14

Merge branch 'main' into rocket/idp

0957f2c

Fix bad locals path

08008e4

Add missing variable

906b3c3

rocketnova mentioned this pull request Aug 1, 2024

Development branch for cognito idp feature navapbc/platform-test-rails#1

Open

coilysiren approved these changes Aug 1, 2024

View reviewed changes

lorenyu approved these changes Aug 1, 2024

View reviewed changes

Add links to aws docs

9928371

rocketnova merged commit 7fcff11 into main Aug 1, 2024
9 checks passed

rocketnova deleted the rocket/idp branch August 1, 2024 22:40

rocketnova mentioned this pull request Aug 7, 2024

Connect to existing identity provider in temporary environments #719

Closed

rocketnova mentioned this pull request Aug 8, 2024

Document how to enable identity provider support #723

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add identity provider modules #650

Add identity provider modules #650

rocketnova commented Jun 20, 2024 •

edited

Loading

aligg Jun 25, 2024

rocketnova Jun 25, 2024

lorenyu left a comment

lorenyu Jun 25, 2024

rocketnova commented Jun 26, 2024

lorenyu left a comment

lorenyu Jun 26, 2024

rocketnova Jun 27, 2024

rocketnova Jun 27, 2024

rocketnova Aug 1, 2024

rocketnova commented Jun 27, 2024

rocketnova commented Jun 27, 2024

lorenyu commented Aug 1, 2024

rocketnova commented Aug 1, 2024 •

edited

Loading

coilysiren commented Aug 1, 2024

rocketnova commented Aug 1, 2024

coilysiren Aug 1, 2024

coilysiren Aug 1, 2024

coilysiren Aug 1, 2024

coilysiren Aug 1, 2024

coilysiren Aug 1, 2024

rocketnova Aug 1, 2024

coilysiren Aug 1, 2024

rocketnova Aug 1, 2024

coilysiren Aug 1, 2024

coilysiren Aug 1, 2024

rocketnova Aug 1, 2024

coilysiren Aug 1, 2024

rocketnova Aug 1, 2024

coilysiren Aug 1, 2024

rocketnova Aug 1, 2024

lorenyu left a comment

rocketnova commented Aug 7, 2024

		has_database = local.has_database
		has_incident_management_service = local.has_incident_management_service

	# Example: "profile"
	# Example: "profile"
	# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-client-apps.html

	# Example: "logout"
	# Example: "logout"
	# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-client-apps.html

	# Set any attribute to a non-null value to override AWS Cognito defaults.
	# Set any attribute to a non-null value to override AWS Cognito defaults.
	# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pool-settings-message-customizations.html

	# Set to an SES-verified email address to be used when sending emails.
	# Set to an SES-verified email address to be used when sending emails.
	# docs: https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-email.html

		@@ -0,0 +1,37 @@
		resource "aws_cognito_user_pool_client" "client" {

	resource "aws_cognito_user_pool_client" "client" {
	# docs: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cognito_user_pool_client
	resource "aws_cognito_user_pool_client" "client" {

	# Use a separate line to support automated terraform destroy commands
	# We use a separate line here to support automated terraform destroy commands.
	# Do not edit this line, the destroy command looks for it verbatim with `sed`

Add identity provider modules #650

Add identity provider modules #650

Conversation

rocketnova commented Jun 20, 2024 • edited Loading

Ticket

Changes

Context for reviewers

Testing

Tested on platform-test

Test deploy on platform-test-rails

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketnova commented Jun 26, 2024

lorenyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketnova commented Jun 27, 2024

rocketnova commented Jun 27, 2024

lorenyu commented Aug 1, 2024

rocketnova commented Aug 1, 2024 • edited Loading

coilysiren commented Aug 1, 2024

rocketnova commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenyu left a comment

Choose a reason for hiding this comment

rocketnova commented Aug 7, 2024

rocketnova commented Jun 20, 2024 •

edited

Loading

rocketnova commented Aug 1, 2024 •

edited

Loading