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 all commits
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
67 changes: 45 additions & 22 deletions auth0/resource_auth0_guardian.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package auth0

import (
"fmt"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
Expand Down Expand Up @@ -92,20 +91,32 @@ func newGuardian() *schema.Resource {
},
},
},
"email": {
Type: schema.TypeBool,
Optional: true,
Default: false,
},
},
}
}

func createGuardian(d *schema.ResourceData, m interface{}) error {
d.SetId(resource.UniqueId())
return updateGuardian(d, m)
}

func deleteGuardian(d *schema.ResourceData, m interface{}) error {
api := m.(*management.Management)
api.Guardian.MultiFactor.Phone.Enable(false)
if err := api.Guardian.MultiFactor.Phone.Enable(false); err != nil {
return err
}
if err := api.Guardian.MultiFactor.Email.Enable(false); err != nil {
return err
}
d.SetId("")
return nil
}

func updateGuardian(d *schema.ResourceData, m interface{}) (err error) {
api := m.(*management.Management)

Expand All @@ -118,24 +129,38 @@ 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
}
if ok {
api.Guardian.MultiFactor.Phone.Enable(true)
if err := configurePhone(d, api); err != nil {
if err := api.Guardian.MultiFactor.Phone.Enable(true); err != nil {
return err
}
} else {
api.Guardian.MultiFactor.Phone.Enable(false)
return configurePhone(d, api)
}
return readGuardian(d, m)
return api.Guardian.MultiFactor.Phone.Enable(false)
}

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)
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 +266,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 +280,16 @@ 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.

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

Expand Down Expand Up @@ -308,19 +344,6 @@ func typeAssertToStringArray(from []interface{}) *[]string {
return &stringArray
}

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

Comment on lines -311 to -323
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for removing this unused method! 馃憤馃徎

// Determines if the factor should be updated. This depends on if it is in the state, if it is about to be added to the state.
func factorShouldBeUpdated(d *schema.ResourceData, factor string) (bool, error) {
_, ok := d.GetOk(factor)
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
}
`
10 changes: 6 additions & 4 deletions docs/resources/guardian.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
layout: "auth0"
page_title: "Auth0: auth0_guardian"
description: |-
With this reasource, you can configure some of the MFA options
With this resource, you can configure some of the MFA options
---

# auth0_guardian
Expand All @@ -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"
verification_message = "{{code}} is your verification code for {{tenant.friendly_name}}"
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 All @@ -42,7 +44,7 @@ Arguments accepted by this resource include:
* `options`- (Required) List(Resource). Options for the various providers. See [Options](#options).

### 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).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is awesome, thanks for fixing these typos!


### Auth0
* `enrollment_message` (Optional) String. This message will be sent whenever a user enrolls a new device for the first time using MFA. Supports liquid syntax, see [Auth0 docs](https://auth0.com/docs/mfa/customize-sms-or-voice-messages).
Expand Down
14 changes: 14 additions & 0 deletions example/guardian/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
provider "auth0" {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a much needed example:) Thanks!


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}}."
}
}
}