Skip to content
This repository has been archived by the owner on Mar 8, 2022. It is now read-only.

add email mfa factor #499

Merged
merged 5 commits into from
Jan 25, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 41 additions & 3 deletions auth0/resource_auth0_guardian.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ func newGuardian() *schema.Resource {
},
},
},
"email": {
Type: schema.TypeBool,
Optional: true,
Default: false,
},
},
}
}
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if err := api.Guardian.MultiFactor.Email.Enable(false); err != nil {
return err
}
d.SetId("")
return nil
}
Expand All @@ -118,7 +126,16 @@ func updateGuardian(d *schema.ResourceData, m interface{}) (err error) {
err = api.Guardian.MultiFactor.UpdatePolicy(&management.MultiFactorPolicies{p})
}
}
//TODO: Extend for other MFA types
if err := updatePhoneFactor(d, api); err != nil {
return err
}
if err := updateEmailFactor(d, api); err != nil {
return err
}
return readGuardian(d, m)
}

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

Choose a reason for hiding this comment

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

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)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

ok, err := factorShouldBeUpdated(d, "phone")
if err != nil {
return err
Expand All @@ -131,11 +148,20 @@ func updateGuardian(d *schema.ResourceData, m interface{}) (err error) {
} else {
api.Guardian.MultiFactor.Phone.Enable(false)
}
return readGuardian(d, m)
return nil
}

func configurePhone(d *schema.ResourceData, api *management.Management) (err error) {
func updateEmailFactor(d *schema.ResourceData, api *management.Management) error {
if changed := d.HasChange("email"); changed {
enabled := d.Get("email").(bool)
if err := api.Guardian.MultiFactor.Email.Enable(enabled); err != nil {
return err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

}
return nil
}

func configurePhone(d *schema.ResourceData, api *management.Management) (err error) {
md := make(MapData)
List(d, "phone").Elem(func(d ResourceData) {
md.Set("provider", String(d, "provider", HasChange()))
Expand Down Expand Up @@ -241,6 +267,7 @@ func readGuardian(d *schema.ResourceData, m interface{}) error {
if err != nil {
return err
}

ok, err := factorShouldBeUpdated(d, "phone")
if err != nil {
return err
Expand All @@ -254,6 +281,17 @@ func readGuardian(d *schema.ResourceData, m interface{}) error {
if err != nil {
return err
}

factors, err := api.Guardian.MultiFactor.List()
if err != nil {
return err
}
for _, v := range factors {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}

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 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 :).

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

switch *v.Name {
case "email":
d.Set("email", v.Enabled)
}
}
return nil
}

Expand Down
32 changes: 32 additions & 0 deletions auth0/resource_auth0_guardian_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,22 @@ func TestAccGuardian(t *testing.T) {
resource.TestCheckNoResourceAttr("auth0_guardian.foo", "phone"),
),
},

{
Config: testAccConfigureEmail,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("auth0_guardian.foo", "policy", "all-applications"),
resource.TestCheckResourceAttr("auth0_guardian.foo", "email", "true"),
),
},

{
Config: testAccConfigureEmailUpdate,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("auth0_guardian.foo", "policy", "all-applications"),
resource.TestCheckResourceAttr("auth0_guardian.foo", "email", "false"),
),
},
},
})
}
Expand Down Expand Up @@ -161,3 +177,19 @@ resource "auth0_guardian" "foo" {
policy = "all-applications"
}
`

const testAccConfigureEmail = `

resource "auth0_guardian" "foo" {
policy = "all-applications"
email = true
}
`

const testAccConfigureEmailUpdate = `

resource "auth0_guardian" "foo" {
policy = "all-applications"
email = false
}
`
4 changes: 3 additions & 1 deletion docs/resources/guardian.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ resource "auth0_guardian" "default" {
provider = "auth0"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

message_types = ["sms"]
options {
enrollment_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}}. Please enter this code to verify your enrollment"
verification_message = "{{code}} is your verification code for {{tenant.friendly_name}}"
}
}
email = true
}
```

Expand All @@ -32,6 +33,7 @@ Arguments accepted by this resource include:

* `policy` - (Required) String. Policy to use. Available options are `never`, `all-applications` and `confidence-score`. The option `confidence-score` means the trigger of MFA will be adaptive. See [Auth0 docs](https://auth0.com/docs/mfa/adaptive-mfa)
* `phone` - (Optional) List(Resource). Configuration settings for the phone MFA. For details, see [Phone](#phone).
* `email` - (Optional) Boolean. Indicates whether or not email MFA is enabled.

### Phone

Expand Down