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

Add session_cookie to tenant #220

Merged
merged 6 commits into from
Jul 12, 2022
Merged

Add session_cookie to tenant #220

merged 6 commits into from
Jul 12, 2022

Conversation

willvedd
Copy link
Contributor

@willvedd willvedd commented Jul 6, 2022

Description

As pointed out in #55, the tenant resource is lacking support for modifying session cookie behavior. This PR adds this functionality through the session_cookie property. It mimics the Management API data shape with a single mode sub-property that accepts one of two strings: "persistent" or "non-persistent".

This configuration can be found in the dashboard by following the https://manage.auth0.com/dashboard/us/<YOUR_TENANT>/tenant/advanced route:

Screen Shot 2022-07-06 at 5 33 18 PM

This depends on a similar PR to add functionality to the Go SDK.

Checklist

Note: Checklist required to be completed before a PR is considered to be reviewable.

Auth0 Code of Conduct

Auth0 General Contribution Guidelines

Changes include test coverage?

  • Yes
  • Not needed

Does the description provide the correct amount of context?

  • Yes, the description provides enough context for the reviewer to understand what these changes accomplish

Have you updated the documentation?

  • Yes, I've updated the appropriate docs
  • Not needed

Is this code ready for production?

  • Yes, all code changes are intentional and no debugging calls are left over

@willvedd willvedd requested a review from a team as a code owner July 6, 2022 21:34
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2022

Codecov Report

Merging #220 (63c54ee) into main (7b53f6d) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
+ Coverage   83.63%   83.71%   +0.08%     
==========================================
  Files          36       36              
  Lines        6587     6620      +33     
==========================================
+ Hits         5509     5542      +33     
  Misses        859      859              
  Partials      219      219              
Impacted Files Coverage Δ
auth0/resource_auth0_tenant.go 97.32% <100.00%> (+0.15%) ⬆️
auth0/structure_auth0_tenant.go 90.19% <100.00%> (+0.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b53f6d...63c54ee. Read the comment docs.

auth0/structure_auth0_tenant.go Outdated Show resolved Hide resolved
go.mod Outdated
@@ -3,7 +3,7 @@ module github.com/auth0/terraform-provider-auth0
go 1.18

require (
github.com/auth0/go-auth0 v0.8.0
github.com/auth0/go-auth0 v0.0.0-20220706202431-cce36896b06c
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take @Widcket's suggestion to keep these kind of PRs in Draft mode till we make a release in go-auth0 SDK, wdyt? So we can easily differentiate between the ones that can be immediately merged and these that shouldn't yet.

"session_cookie": {
Type: schema.TypeList,
Optional: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need computed both here and on the mode? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, do we? Frankly, it's a confusing property that possesses unintuitive behavior; I'm seeing that we set many things to Computed: true so I'm just following a precedent.

Copy link
Contributor

Choose a reason for hiding this comment

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

What we can do is try first the parameters without the computed property. The computed is usually needed if there are weird diffs constantly showing whenever terraform performs a refresh, that have no explanation (my personal heuristic on when to use computed or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out the computed property only needs to be enabled on the root object, not necessarily the children, or at least for this example. However, enabling for the child too did not alter behavior.

I'd prefer to not require manual verification testing for these types of additions, so I added a third stage for the TestAccTenant test. It attempts to apply an empty tenant resource block to see if it triggers any odd behavior from Terraform or the Management API. Specifically for this case, it caught a 400 error that occurred when Terrafrom attempted to set the session_cookie to an empty object, something that the Management API prohibits.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're sending an empty object because the expand func is flawed. We can remove the Computed property and refactor the expand as follows:

func expandTenantSessionCookie(d ResourceData) *management.TenantSessionCookie {
	var sessionCookie *management.TenantSessionCookie

	List(d, "session_cookie").Elem(func(d ResourceData) {
		sessionCookie = &management.TenantSessionCookie{Mode: String(d, "mode")}
	})

	return sessionCookie
}

This will make it so that sessionCookie is nil in case it's not present in the config so it will get omitted from the payload because of the omitempty tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Computed: true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggested change to refactor expandTenantSessionCookie works, but it does not enable us to remove the Computed property because it fails the "reset" test stage. The plan is not empty afterwards. I'm going to opt to keep it as-is because there is no discernible change in code. Further, the code for that expand function matches the existing precedent set by expandTenantChangePassword, expandTenantGuardianMFAPage, expandTenantErrorPage and expandTenantUniversalLogin.

@willvedd willvedd marked this pull request as draft July 6, 2022 22:48
Comment on lines +187 to +195
func expandTenantSessionCookie(d ResourceData) *management.TenantSessionCookie {
var sessionCookie management.TenantSessionCookie

List(d, "session_cookie").Elem(func(d ResourceData) {
sessionCookie.Mode = String(d, "mode")
})

return &sessionCookie
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func expandTenantSessionCookie(d ResourceData) *management.TenantSessionCookie {
var sessionCookie management.TenantSessionCookie
List(d, "session_cookie").Elem(func(d ResourceData) {
sessionCookie.Mode = String(d, "mode")
})
return &sessionCookie
}
func expandTenantSessionCookie(d ResourceData) *management.TenantSessionCookie {
var sessionCookie *management.TenantSessionCookie
List(d, "session_cookie").Elem(func(d ResourceData) {
sessionCookie = &management.TenantSessionCookie{Mode: String(d, "mode")}
})
return sessionCookie
}

@willvedd willvedd marked this pull request as ready for review July 12, 2022 14:39
@willvedd willvedd merged commit ad50529 into main Jul 12, 2022
@willvedd willvedd deleted the add-tenant-session-cookie branch July 12, 2022 14:59
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.

3 participants