add email mfa factor by phil-f · Pull Request #499 · alexkappa/terraform-provider-auth0

phil-f · 2022-01-24T13:05:32Z

Proposed Changes

Adds the email mfa factor

Acceptance Test Output

Click to expand

$ make testacc TESTS=TestAccGuardian
==> Checking that code complies with gofmt requirements...
?       github.com/alexkappa/terraform-provider-auth0   [no test files]
=== RUN   TestAccGuardian
--- PASS: TestAccGuardian (93.45s)
PASS
coverage: 11.6% of statements
ok      github.com/alexkappa/terraform-provider-auth0/auth0     93.861s coverage: 11.6% of statements
?       github.com/alexkappa/terraform-provider-auth0/auth0/internal/debug      [no test files]
?       github.com/alexkappa/terraform-provider-auth0/auth0/internal/digitalocean       [no test files]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok      github.com/alexkappa/terraform-provider-auth0/auth0/internal/hash       1.796s  coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok      github.com/alexkappa/terraform-provider-auth0/auth0/internal/random     2.075s  coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok      github.com/alexkappa/terraform-provider-auth0/auth0/internal/validation 0.215s  coverage: 0.0% of statements [no tests to run]
?       github.com/alexkappa/terraform-provider-auth0/version   [no test files]
...

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

phil-f · 2022-01-24T13:17:01Z

First time contributing to a Terraform provider, comments/feedback would be very much welcome!

@apamildner
Would be great to get your feedback on this too if possible, seeing as this builds on your PR. I wasn't able to implement it in the same way as phone, due to the fact that email is just a simple bool rather than having additional config like phone does.

I'm conscious it potentially makes the code a bit less coherent, if you believe that to be the case then more than happy to work on getting this PR into a better state.

I also wanted to ask about the ignored errors on the management client when updating phone, is that intentional? I left it alone as I wasn't sure.

apamildner

Nice job! I understand what you mean by not following my "idea" of how this resource should be extended, but as you say it is just a simple boolean so I think this looks good. Thanks for pointing out the error that i had missed to handle, it should of course be dealt with. I added some comments on the code and also found some small typos I had missed, would be great if you could solve them while you're at it 👍

apamildner · 2022-01-24T13:36:55Z

auth0/resource_auth0_guardian.go

@@ -103,6 +108,9 @@ func createGuardian(d *schema.ResourceData, m interface{}) error {
 func deleteGuardian(d *schema.ResourceData, m interface{}) error {
 	api := m.(*management.Management)
 	api.Guardian.MultiFactor.Phone.Enable(false)


As you mentioned - I probably forgot to handle the error here so you can definitely add it in this PR 👍

Also, it seems like isFactorEnabled function is unused (also my bad), might just remove it in this PR 👍

apamildner · 2022-01-24T14:07:45Z

auth0/resource_auth0_guardian.go

+	if err != nil {
+		return err
+	}
+	for _, v := range factors {


Isn't it more readable to avoid the switch here? I think it could be just

if v.Name != nil && *v.Name == "email"{ d.Set("email", v.Enabled) }

I went for switch here thinking that it'll be a bit more concise in terms of syntax once one-time-password & recovery-code get added. Would you say you feel strongly about this point re: the readability aspect? If so then happy to change :).

I see what you mean, I think we can leave the switch there 👍 But isn't there still a possible null pointer error here that is not adressed by your code? If v.Name == nil that is. Or is it handled by the switch logic of Golang maybe?

I'd agree that @apamildner 's if statement is slightly clearer. If and when one-time-password and recovery-code get added, a switch will likely make more sense.

apamildner · 2022-01-24T14:17:59Z

docs/resources/guardian.md

@@ -19,10 +19,11 @@ resource "auth0_guardian" "default" {
    provider      = "auth0"


Not part of your code but might just clean it up: I'm unable to comment on the specific line, but this file has a typo at line 5 which could be adressed: With this reasource, you can configure some of the MFA options -> With this resource, you can configure some of the MFA options

phil-f · 2022-01-24T16:51:43Z

@apamildner
Thanks for such swift feedback, much appreciated! I've addressed those points & added one comment re: the switch statement.

apamildner · 2022-01-24T19:19:08Z

I left a response, but otherwise I think this one should be ready to merge! I cannot do that though since I'm not a maintainer, just a contributor.

willvedd

Solid addition, thank you! Would also be great for you to create an example file: example/guardian/main.tf with something like:

provider "auth0" {}

resource "auth0_guardian" "guardian"{
  email = false
  policy = "all-applications"
  phone{
    provider  = "auth0"
    message_types = ["sms","voice"]
    options{
      verification_message = "{{code}} is your verification code for {{tenant.friendly_name}}. Please enter this code to verify your enrollment."
      enrollment_message = "{{code}} is your verification code for {{tenant.friendly_name}}."
    }
  }
}

This is what allowed me to verify this branch locally and serves as a reference to folks. But otherwise, looks good to me. Happy to merge once this and that one stylistic change is made.

phil-f · 2022-01-25T09:31:06Z

@willvedd
Thanks for the feedback! Good point about the example, have added it now & changed the switch to an if statement.

sergiught

Thanks a lot for the contribution! Solid work here! 💪🏻

sergiught · 2022-01-25T11:32:45Z

auth0/resource_auth0_guardian.go

+	return readGuardian(d, m)
+}
+
+func updatePhoneFactor(d *schema.ResourceData, api *management.Management) error {


We could probably simplify the logic inside this func as follows:

Suggested change

func updatePhoneFactor(d *schema.ResourceData, api *management.Management) error {

func updatePhoneFactor(d *schema.ResourceData, api *management.Management) error {

ok, err := factorShouldBeUpdated(d, "phone")

if err != nil {

return err

}

if ok {

if err := configurePhone(d, api); err != nil {

return err

}

}

return api.Guardian.MultiFactor.Phone.Enable(ok)

}

Definitely easier on the eyes with reducing the nesting, thanks for raising it about this & the other func! Had to change this one slightly, as phone needs to be enabled before it's configured. Also added api.Guardian.MultiFactor.Phone.Enable(false) for the negative case, as this means it's not present/being removed from state and we need to toggle it off.

That makes sense, thanks @phil-f ! Awesome work here

sergiught · 2022-01-25T11:34:48Z

auth0/resource_auth0_guardian.go

+		if err := api.Guardian.MultiFactor.Email.Enable(enabled); err != nil {
+			return err
+		}


We can return here directly and remove 1 extra indentation level.

Suggested change

if err := api.Guardian.MultiFactor.Email.Enable(enabled); err != nil {

return err

}

return api.Guardian.MultiFactor.Email.Enable(enabled)

sergiught · 2022-01-25T11:35:43Z

auth0/resource_auth0_guardian.go

-func isFactorEnabled(factor string, api *management.Management) (*bool, error) {
-	mfs, err := api.Guardian.MultiFactor.List()
-	if err != nil {
-		return nil, err
-	}
-	for _, mf := range mfs {
-		if *mf.Name == factor {
-			return mf.Enabled, nil
-		}
-	}
-	return nil, fmt.Errorf("factor %s is not among the possible factors", factor)
-}
-


Thanks for removing this unused method! 👍🏻

sergiught · 2022-01-25T11:36:40Z

docs/resources/guardian.md


 ### Options
-`options` supports different arguments depending on the provider specificed in [Phone](#phone).
+`options` supports different arguments depending on the provider specified in [Phone](#phone).


This is awesome, thanks for fixing these typos!

sergiught · 2022-01-25T11:36:56Z

example/guardian/main.tf

@@ -0,0 +1,14 @@
+provider "auth0" {}


This was a much needed example:) Thanks!

phil-f · 2022-01-25T14:32:58Z

@sergiughf
Thanks for the review :). I've applied the changes now & commented on one of them that I had to tweak slightly.

willvedd

Great work!

add email mfa factor

e749632

github-actions bot added the resource/auth0_guardian label Jan 24, 2022

apamildner reviewed Jan 24, 2022

View reviewed changes

fix typo, remove unused func, capture phone mgmt api errors

f01555e

willvedd force-pushed the feature/add-email-mfa-factor branch from 5ebd83f to f01555e Compare January 24, 2022 20:51

willvedd reviewed Jan 24, 2022

View reviewed changes

use if instead of switch, add example, fix typo

883b731

sergiught reviewed Jan 25, 2022

View reviewed changes

Phil added 2 commits January 25, 2022 14:23

reduce nesting

4c678da

space between funcs

6e0a76f

sergiught approved these changes Jan 25, 2022

View reviewed changes

sergiught requested a review from willvedd January 25, 2022 14:48

willvedd approved these changes Jan 25, 2022

View reviewed changes

willvedd merged commit c509b5c into alexkappa:master Jan 25, 2022

		@@ -19,10 +19,11 @@ resource "auth0_guardian" "default" {
		provider = "auth0"

Uh oh!

Conversation

phil-f commented Jan 24, 2022

Proposed Changes

Acceptance Test Output

Community Note

Uh oh!

phil-f commented Jan 24, 2022

Uh oh!

apamildner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apamildner Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-f commented Jan 24, 2022

Uh oh!

apamildner commented Jan 24, 2022

Uh oh!

willvedd left a comment

Choose a reason for hiding this comment

Uh oh!

phil-f commented Jan 25, 2022

Uh oh!

sergiught left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-f commented Jan 25, 2022

Uh oh!

willvedd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

apamildner left a comment •

edited

Loading

apamildner Jan 24, 2022 •

edited

Loading