Skip to content
This repository has been archived by the owner on Jun 14, 2021. It is now read-only.

Adds support to create users with credentials #334

Merged
merged 4 commits into from
Nov 21, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions examples/okta_user/all_attributes.tf
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,5 @@ resource "okta_user" "test" {
title = "Director"
user_type = "Employee"
zip_code = "11111"
password = "Abcd1234"
Copy link
Contributor Author

@vijetm vijetm Nov 18, 2019

Choose a reason for hiding this comment

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

Had to add the password variable to all setup files in examples/okta_user directory, as update tests would fail.
This is because in the update test, the user should have a previous password to update the current password. If not, okta api returns an error. I don't know if you prefer this, but another way is to probably have a separate method/test to check for password update. I thought this was easier, not sure if cleaner though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this test case separate? Just create another password specific test case? That way we can keep all of these existing test cases as is. I want to be sure all of the old tests pass as is (no passwords changed or set) while also testing the new functionality.

}
1 change: 1 addition & 0 deletions examples/okta_user/basic.tf
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ resource "okta_user" "test" {
last_name = "Smith"
login = "test-acc-replace_with_uuid@example.com"
email = "test-acc-replace_with_uuid@example.com"
password = "Abcd1234"
}
9 changes: 9 additions & 0 deletions examples/okta_user/basic_with_credentials.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
resource "okta_user" "test" {
first_name = "TestAcc"
last_name = "Smith"
login = "test-acc-replace_with_uuid@example.com"
email = "test-acc-replace_with_uuid@example.com"
password = "SuperSecret007"
recovery_question = "What is the answer to life, the universe, and everything?"
recovery_answer = "Forty Two"
}
1 change: 1 addition & 0 deletions examples/okta_user/staged.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ resource "okta_user" "test" {
last_name = "Smith"
login = "test-acc-replace_with_uuid@example.com"
email = "test-acc-replace_with_uuid@example.com"
password = "Abcd1234"
status = "STAGED"
}
93 changes: 92 additions & 1 deletion okta/resource_okta_user.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ var profileKeys = []string{
"title",
"user_type",
"zip_code",
"password",
"recovery_question",
"recovery_answer",
}

func resourceUser() *schema.Resource {
Expand Down Expand Up @@ -265,6 +268,24 @@ func resourceUser() *schema.Resource {
Optional: true,
Description: "User zipcode or postal code",
},
"password": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Sensitive: true,
Description: "User Password",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want this to be Sensitive: true. Can we also make it clear in the docs and description that this WILL be stored in plain text in state?

Copy link
Contributor

Choose a reason for hiding this comment

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

It also looks like these properties are set on create only. Can we make that clear? "User's initial password. This value cannot be updated."

Technically these should be ForceNew: true if they can only be set on create. We should warn of that in the description.

Copy link
Contributor Author

@vijetm vijetm Nov 14, 2019

Choose a reason for hiding this comment

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

Technically, okta has an API that allows you to update a user's password. The go lang SDK also has an API to do this - https://github.com/okta/okta-sdk-golang/blob/master/tests/integration/user_test.go#L237
Would we still need the ForceNew: true flag?

Copy link
Contributor

@quantumew quantumew Nov 14, 2019

Choose a reason for hiding this comment

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

Oh coolio, no in that case we would just need to update our update function and support that. Hopefully I am reading this diff right, I am only seeing this logic in resourceUserCreate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're reading it right. 😄
I will update the resourceUserUpdate function as well.

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 believe I've addressed all your concerns. Take a look.

},
"recovery_question": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Description: "User Password Recovery Question",
},
"recovery_answer": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Sensitive: true,
ValidateFunc: validation.StringLenBetween(4, 1000),
Description: "User Password Recovery Answer",
},
Copy link
Contributor

@quantumew quantumew Nov 15, 2019

Choose a reason for hiding this comment

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

Can either of these be updated? If so should we include them in the update logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they can be updated. Let me include them in the update logic. Thanks for catching!

},
}
}
Expand All @@ -291,7 +312,31 @@ func resourceUserCreate(d *schema.ResourceData, m interface{}) error {
qp = query.NewQueryParams(query.WithActivate(false))
}

userBody := okta.User{Profile: profile}
password := d.Get("password").(string)
recoveryQuestion := d.Get("recovery_question").(string)
recoveryAnswer := d.Get("recovery_answer").(string)

if recoveryQuestion != "" {
return fmt.Errorf("[ERROR] Okta does not allow security answers with less than 4 characters")
}

uc := &okta.UserCredentials{
Password: &okta.PasswordCredential{
Value: password,
},
}

if recoveryQuestion != "" {
uc.RecoveryQuestion = &okta.RecoveryQuestionCredential{
Question: recoveryQuestion,
Answer: recoveryAnswer,
}
}

userBody := okta.User{
Profile: profile,
Credentials: uc,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be cleaned up a bit, no need to instantiate the struct twice

	uc := &okta.UserCredentials{
		Password: &okta.PasswordCredential{
		        Value: password,
	        },
	}

	if recoveryQuestion != "" && len(recoveryAnswer) >= 4 {
		uc.RecoveryQuestion := &okta.RecoveryQuestionCredential{
			Question: recoveryQuestion,
			Answer:   recoveryAnswer,
		}
	}

user, _, err := client.User.CreateUser(userBody, qp)

if err != nil {
Expand Down Expand Up @@ -383,6 +428,9 @@ func resourceUserUpdate(d *schema.ResourceData, m interface{}) error {
roleChange := d.HasChange("admin_roles")
groupChange := d.HasChange("group_memberships")
userChange := hasProfileChange(d)
passwordChange := d.HasChange("password")
recoveryQuestionChange := d.HasChange("recovery_question")
recoveryAnswerChange := d.HasChange("recovery_answer")

// run the update status func first so a user that was previously deprovisioned
// can be updated further if it's status changed in it's terraform configs
Expand Down Expand Up @@ -423,6 +471,49 @@ func resourceUserUpdate(d *schema.ResourceData, m interface{}) error {
}
d.SetPartial("group_memberships")
}

if passwordChange {
oldPassword, newPassword := d.GetChange("password")

op := &okta.PasswordCredential{
Value: oldPassword.(string),
}
np := &okta.PasswordCredential{
Value: newPassword.(string),
}
npr := &okta.ChangePasswordRequest{
OldPassword: op,
NewPassword: np,
}

_, _, err := client.User.ChangePassword(d.Id(), *npr, nil)
if err != nil {
return fmt.Errorf("[ERROR] Error Updating User password in Okta: %v", err)
}
}

if recoveryQuestionChange || recoveryAnswerChange {
p := &okta.PasswordCredential{
Value: d.Get("password").(string),
}

rq := &okta.RecoveryQuestionCredential{
Question: d.Get("recovery_question").(string),
Answer: d.Get("recovery_answer").(string),
}

nuc := &okta.UserCredentials{
Password: p,
RecoveryQuestion: rq,
}

_, _, err := client.User.ChangeRecoveryQuestion(d.Id(), *nuc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quantumew - The recovery credentials update doesn't seem to work in my test environment. Okta APIs don't throw an error but the credentials are not updated in the user account. (Password is updated, but recovery question/answer is not). Trying to debug the issue. Don't merge this yet.


if err != nil {
return fmt.Errorf("[ERROR] Error Updating User password recovery credentials in Okta: %v", err)
}
}

d.Partial(false)

return resourceUserRead(d, m)
Expand Down
12 changes: 12 additions & 0 deletions okta/resource_okta_user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ func TestAccOktaUser_updateAllAttributes(t *testing.T) {
config := mgr.GetFixtures("staged.tf", ri, t)
updatedConfig := mgr.GetFixtures("all_attributes.tf", ri, t)
minimalConfig := mgr.GetFixtures("basic.tf", ri, t)
minimalConfigWithCredentials := mgr.GetFixtures("basic_with_credentials.tf", ri, t)
resourceName := fmt.Sprintf("%s.test", oktaUser)
email := fmt.Sprintf("test-acc-%d@example.com", ri)

Expand Down Expand Up @@ -233,6 +234,17 @@ func TestAccOktaUser_updateAllAttributes(t *testing.T) {
resource.TestCheckResourceAttr(resourceName, "email", email),
),
},
{
Config: minimalConfigWithCredentials,
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we add the ForceNew this will actually recreate this resource and test the create logic.

Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "first_name", "TestAcc"),
resource.TestCheckResourceAttr(resourceName, "last_name", "Smith"),
resource.TestCheckResourceAttr(resourceName, "login", email),
resource.TestCheckResourceAttr(resourceName, "email", email),
resource.TestCheckResourceAttr(resourceName, "password", "SuperSecret007"),
resource.TestCheckResourceAttr(resourceName, "recovery_answer", "Forty Two"),
),
},
},
})
}
Expand Down
6 changes: 6 additions & 0 deletions website/docs/r/user.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ The following arguments are supported:

* `zip_code` - (Optional) User profile property.

* `password` - (Optional) User password.

* `recovery_question` - (Optional) User password recovery question.

* `recovery_answer` - (Optional) User password recovery answer.

## Attributes Reference

* `index` - (Optional) ID of the User schema property.
Expand Down