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.

Sign up for GitHub

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

Jump to bottom

Add support for Okta, TOTP and PingID MFA methods #1395

Merged
merged 7 commits into from
Apr 4, 2022
Merged

Add support for Okta, TOTP and PingID MFA methods #1395

merged 7 commits into from
Apr 4, 2022

Conversation

vinay-gopalan
Copy link
Contributor

Adds three new resources to support Okta, TOTP and PingID sys/mfa methods.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestMFAOktaBasic'
=== RUN   TestMFAOktaBasic
--- PASS: TestMFAOktaBasic (1.73s)

$ make testacc TESTARGS='-run=TestMFATOTPBasic'
=== RUN   TestMFATOTPBasic
--- PASS: TestMFATOTPBasic (1.65s)

@benashz benashz added this to the 3.5.0 milestone Mar 31, 2022
benashz
benashz reviewed Apr 1, 2022
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Looking good. I've completed the first pass. Added some suggestions/comments etc.

vault/resource_mfa_okta_test.go
resource.Test(t, resource.TestCase{
PreCheck: func() { testutil.TestEntPreCheck(t) },
Providers: testProviders,
Steps: []resource.TestStep{
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should add an import step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added import step in 5d506d7

vault/resource_mfa_totp_test.go
resource.Test(t, resource.TestCase{
PreCheck: func() { testutil.TestEntPreCheck(t) },
Providers: testProviders,
Steps: []resource.TestStep{
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should add an import step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added import step in 5d506d7

vault/resource_mfa_pingid_test.go Show resolved Hide resolved
vault/resource_mfa_okta.go Outdated
func mfaOktaRequestData(d *schema.ResourceData) map[string]interface{} {
data := map[string]interface{}{}

nonBooleanAPIFields := []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it's okay just name this variable fields like you have in other functions, and to move it relative to its usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5d506d7

vault/resource_mfa_okta.go Outdated
"mount_accessor": {
Type: schema.TypeString,
Required: true,
Description: "The mount to tie this method to for use in automatic mappings. The mapping will use the Name field of Aliases associated with this mount as the username in the mapping.",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fold long lines whenever possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5d506d7

vault/resource_mfa_totp_test.go Outdated
)

func TestMFATOTPBasic(t *testing.T) {
mfaTOTPPath := acctest.RandomWithPrefix("mfa-totp")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shadow declaration of mfaTOTPPath().

It's preferable to use generic variable names in this scope.

Suggested change
mfaTOTPPath := acctest.RandomWithPrefix("mfa-totp")
path := acctest.RandomWithPrefix("mfa-totp")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5d506d7

vault/resource_mfa_totp_test.go Outdated
return fmt.Sprintf(`
resource "vault_mfa_totp" "test" {
name = %q
issuer = "hashicorp"
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
issuer = "hashicorp"
issuer = "hashicorp"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5d506d7

vault/resource_mfa_totp_test.go Outdated
Comment on lines 39 to 42
issuer = "hashicorp"
period = 60
algorithm = "SHA256"
digits = 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Mix of spaces and hard tabs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5d506d7

vault/resource_mfa_okta.go Show resolved Hide resolved
vault/resource_mfa_okta.go
return data
}

func mfaOktaWrite(d *schema.ResourceData, meta interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using this function for both create and update, would it make sense to give it a more explicit name e.g. mfaOktaCreateOrUpdate()

@vinay-gopalan vinay-gopalan requested a review from benashz April 1, 2022 19:38
benashz
benashz approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Looks great! Ship it!

vault/resource_mfa_okta.go Show resolved Hide resolved
@vinay-gopalan vinay-gopalan merged commit 13e8833 into main Apr 4, 2022
@clintmod clintmod mentioned this pull request Aug 2, 2022
Closed
marcboudreau pushed a commit to marcboudreau/terraform-provider-vault that referenced this pull request Nov 6, 2022
@vinay-gopalan
Add support for Okta, TOTP and PingID MFA methods (hashicorp#1395)
efb37bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benashz benashz benashz approved these changes

Assignees
No one assigned
Labels
documentation size/XL
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@vinay-gopalan @benashz