add "auth0_global_client" resource #172

yinzara · 2020-02-18T02:31:41Z

Adds ability to enable and manage html of Universal Custom Login page

$ make testacc TESTS=TestAccTenant
==> Checking that code complies with gofmt requirements...
?   	github.com/terraform-providers/terraform-provider-auth0	[no test files]
=== RUN   TestAccTenant
--- PASS: TestAccTenant (1.76s)
PASS
coverage: 15.2% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0	1.989s	coverage: 15.2% of statements
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0/internal/random	0.186s	coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0/internal/validation	0.227s	coverage: 0.0% of statements [no tests to run]

alexkappa · 2020-02-18T09:43:38Z

Hi Matthew, I've been thinking that since these properties are already there for the client resource, we should try to fit the workflow through that instead of mixing it up with the tenant.

Currently, auth0_client has the necessary fields to define the custom login pages (i.e. custom_login_page_on, custom_login_page, custom_login_page_preview).

What if we try to use that instead, and document it using a guide or something (e.g. under website/docs/g/)?

resource "auth0_client" "global" {
  name = "All Applications"
}

This can be imported using the command line:

terraform import auth0_client.global <client_id>

From here on, we can make changes to the custom_login_* properties of the global client.

Another option, might be to supply a data source for clients, which can read the global client. In which case we won't have to ask users to import the global client from the cli, but instead it exists in the terraform configuration.

data "auth0_client" "global" {
    global = true
    # first_party = true
    # app_type = [ "native", "non_interactive" ]
}

resource "auth0_client" "global" {
  id = data.auth0_client.id
  custom_login_page_on = true
  custom_login_page = "<html>...</html>"
}

We could perhaps use some of the logic from createTenant in a way that transparently reads the tenant without trying to create it first.

But as a first step, I think the import option is more straightforward and gives us more time to figure out how we want to make this easier in the future.

yinzara · 2020-02-18T18:38:11Z

Regarding the "import" plan, couple of issues:

Requiring a terraform import for a piece of functionality that has no other method of implementation won't work in an auto build scenario. This requires a specialized process flow to actually apply a change, something I can't support without modifying my entire auto build process.
Unfortunately there is no way to get the client ID of the global client without making an API call (it isn't visible anywhere in Auth0's UI) so the complexity goes up and documenting this process also becomes error prone over time. To achieve this in terraform, I'd have to use external data sources that depend on external tools (like curl and jq).

Additionally, The whole "global" client itself is something I have a feeling auth0 is going to change some day at least when it comes to the custom login page. They describe it as "legacy" in places and there is almost 0 documentation about its existence all together. For me to even discover that I had to update the custom_login_page and custom_login_page_on (the custom_login_page_preview is only an output variable), I had to read through the source of the source control plugin. I can't find a single use for the global client outside of this use case. Maybe you could use your internal Auth0 contacts to ask their plans?

Regarding the "data" source plan and making auth0_client allow updating existing clients if the client_id is specified, if we were to go that route, there isn't really a reason to have a separate data source other than not wanting a chance of modifying it. You might as well just put the "global" (or "is_global" [the query string parameter used in the API to retrieve it]) property on the auth0_client resource and if it's true, modify the global resource instead of creating a new one. This would make it so that there's no need to make the client_id an input parameter on the resource. Technically you could also add the functionality to support specifying the client_id as well, but isn't required for this solution. This would at least keep your abstraction pure to the APIs.

I also would entertain the idea of moving back to the originally discussed design of using a separate "pages" resource like all of the source control plugins, but that means the mfa, change password, and error pages would be modifiable using both the auth0_tenant or auth0_pages resources. This is actually true of the current Auth0 source control implementation as well (you can set the values in the tenant.json file in a repo or by specifying html/js files in the "pages" directory). That would keep a consistent structure with the source control plugins and limit the changes to a new resource type (i.e. so people already using auth0_tenant won't be affected or have additional API calls made to get the global client info). I also find this solution attractive as it abstracts away any notion of "global client" from the terraform provider (the source control integrations also abstract it away) so that if in the future, Auth0 does deprecate global clients, only the Terraform plugin has to change, not people using it.

I thought about an "auth0_page" resource with a "page_type" enumerated property, but the "error_page" has two additional properties that the others pages don't have and lacks an "enabled" property so we'd have to support optional properties that only get read for some page types and I don't like that idea.

alexkappa · 2020-02-19T14:21:40Z

You might as well just put the "global" (or "is_global" [the query string parameter used in the API to retrieve it]) property on the auth0_client resource and if it's true, modify the global resource instead of creating a new one.

I like this idea (or a version of it) most. I like how it's still just a client, with a well documented behavior. I think we should explore this possibility rather than considering an auth0_page resource. You understand my reasoning for this, you captured it well with your comment on keeping our abstractions pure to the API.

Using the auth0_client as is, might cause confusion in the long run, and maybe end up being buggy due to the two paths the code can take depending on a value from is_global. For example, what if the value of is_global changes?

What about a special case of client (e.g. auth0_global_client), which can re-use the schema, update and read methods, while overriding the create and delete methods.

Seems like the aws team did this with default vpc's 1. Following the pattern would be more straightforward for users, likely being exposed to it to begin with.

Example:

resource auth0_global_client global {
  custom_login_page_on = true
  custom_login_page = "<html>...</html>"
}

yinzara · 2020-02-21T19:33:09Z

"auth0_global_client" resource is the solution then. I'll just extend from the auth0_client and override create and delete as stated.

Regarding you comment on hashicorp/terraform-provider-auth0#4

I feel like we might need an "auth0_default_connection" or maybe "auth0_builtin_connection" that functions a lot like the difference between auth0_global_client and auth0_client (i.e. it would extend from auth0_connection and override create and delete). Maybe it's just a field on the auth0_connection that says "import if it doesn't exist and don't delete when its removed"? Maybe "Import_on_create" that defaults to false and "delete_on_destroy" that defaults to true? maybe that just becomes the default behavior when we get the error about the connection already existing?

I'll work on the auth0_global_client resource and we can chat about the other one in the other issue.

alexkappa · 2020-02-21T20:47:17Z

Awesome, thanks Matt.

Maybe "Import_on_create" that defaults to false and "delete_on_destroy" that defaults to true? maybe that just becomes the default behavior when we get the error about the connection already existing?

This sounds most elegant to me honestly. The rationale is that each connection type can be created only once. If this statement holds true, we could apply this more broadly. I can see email provider being a similar cause of trouble.

To play devils advocate here, there is also a catch. And its a big one. What if we confuse users into thinking they are creating a new resource, when they are actually replacing an existing one? A 409 error at least lets users know that such resource already exists.

yinzara · 2020-02-23T07:41:18Z

Alrighty. Here's the new "auth0_global_client" resource. Test cases added and pass.

$ make testacc TESTS=TestAccGlobalClient
==> Checking that code complies with gofmt requirements...
?   	github.com/terraform-providers/terraform-provider-auth0	[no test files]
=== RUN   TestAccGlobalClient
--- PASS: TestAccGlobalClient (3.27s)
PASS
coverage: 13.4% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0	3.506s	coverage: 13.4% of statements
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0/internal/random	0.188s	coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0/internal/validation	0.228s	coverage: 0.0% of statements [no tests to run]

alexkappa

Nice work @yinzara, I've added some comments, but this looks very good. Thanks for going the extra mile and adding an example as well.

auth0/resource_auth0_tenant.go

auth0/resource_auth0_tenant_test.go

auth0/resource_auth0_global_client.go

yinzara force-pushed the feature-pages branch 2 times, most recently from 4629fa9 to ab94ff2 Compare February 18, 2020 03:36

alexkappa mentioned this pull request Feb 19, 2020

Unable to edit default database hashicorp/terraform-provider-auth0#4

Closed

yinzara changed the title ~~add "custom_login" configuration to auth0_tenant resource~~ add "auth0_global_client" resource Feb 23, 2020

yinzara force-pushed the feature-pages branch from 40cb2e0 to 140fd4b Compare February 23, 2020 07:49

alexkappa reviewed Feb 23, 2020

View reviewed changes

add "auth0_global_client" resource

e45b0b9

yinzara force-pushed the feature-pages branch from 5e6f439 to e45b0b9 Compare February 23, 2020 17:39

alexkappa merged commit 29950cd into alexkappa:master Feb 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add "auth0_global_client" resource #172

add "auth0_global_client" resource #172

yinzara commented Feb 18, 2020

alexkappa commented Feb 18, 2020

yinzara commented Feb 18, 2020 •

edited

alexkappa commented Feb 19, 2020

yinzara commented Feb 21, 2020 •

edited

alexkappa commented Feb 21, 2020

yinzara commented Feb 23, 2020

alexkappa left a comment

add "auth0_global_client" resource #172

add "auth0_global_client" resource #172

Conversation

yinzara commented Feb 18, 2020

alexkappa commented Feb 18, 2020

yinzara commented Feb 18, 2020 • edited

alexkappa commented Feb 19, 2020

yinzara commented Feb 21, 2020 • edited

alexkappa commented Feb 21, 2020

yinzara commented Feb 23, 2020

alexkappa left a comment

Choose a reason for hiding this comment

yinzara commented Feb 18, 2020 •

edited

yinzara commented Feb 21, 2020 •

edited