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

[TIC-958] Signup domain allowlist/blocklist #11

Merged
merged 8 commits into from
Oct 2, 2024

Conversation

itailevi98
Copy link
Contributor

@itailevi98 itailevi98 commented Sep 9, 2024

Background

This PR introduces the new env. config fields signup_domain_allowlist_enabled, signup_domain_allowlist, signup_domain_blocklist_enabled, and signup_domain_blocklist.

Logic was smoke-tested with the following resource, with all create, read, and update functionality working as expected:

terraform {
  required_providers {
    propelauth = {
      source = "registry.terraform.io/propelauth/propelauth"
    }
  }
}

provider "propelauth" {
...
}

resource "propelauth_basic_auth_configuration" "example" {
  signup_domain_allowlist         = ["example.com", "a.example.com"]
 # or  signup_domain_blocklist         = ["b.example.com"]
}

@itailevi98 itailevi98 self-assigned this Sep 9, 2024
@itailevi98 itailevi98 requested a review from a team as a code owner September 9, 2024 15:21
Copy link
Collaborator

@mrmauer mrmauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly lgtm. Just two questions/comments for improved DX:

  1. Can we add a validation that we don't get both a allow list and block list? See here for an example of the ValidateConfig interface.
  2. Can we just drop the ..._list_enabled attributes and infer these from a list being provided in the plan (and if none provided, then false)?

…w/blocklist, handle enabled logic internally
@itailevi98
Copy link
Contributor Author

@mrmauer great suggestions :) Updated the basic_auth_configuration_resource to handle the ..._enabled functionality internally, as well as validate that both are not set together

mrmauer
mrmauer previously approved these changes Oct 2, 2024
Copy link
Collaborator

@mrmauer mrmauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic all lgtm!
Just a couple little clean-up nits before merging

},
// "signup_domain_allowlist_enabled": schema.BoolAttribute{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Can we deleted the commented out code here and below?

"signup_domain_allowlist": schema.ListAttribute{
ElementType: types.StringType,
Optional: true,
Description: "A list of email domains that are allowed to sign up. This is only used if `signup_domain_allowlist_enabled` is true.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove any references in these field docstrings to the now deleted fields such as signup_domain_allowlist_enabled?

Copy link
Collaborator

@mrmauer mrmauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mrmauer mrmauer merged commit 6640890 into main Oct 2, 2024
3 checks passed
@mrmauer mrmauer deleted the TIC-958/Allow-and-block-list-for-domains-on-signup branch October 2, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants