Adds support to create users with credentials #334

vijetm · 2019-11-14T18:13:30Z

Create user with password and recovery question/answer
Added a new test to check for credentials update

vijetm · 2019-11-14T18:19:22Z

@quantumew - I have tested the config manually, but haven't run the acceptance tests. Is there a way to run a single acceptance test?
I used go test -run TestAccOktaUser_updateAllAttributes and it passes.
Is there a better way to run acceptance tests locally?

quantumew · 2019-11-14T18:21:09Z

@vijetm try TEST_FILTER=TestAccOktaUser_updateAllAttributes make testacc

vijetm · 2019-11-14T18:38:09Z

okta/resource_okta_user.go

+	}
+
+	if recoveryQuestion != "" {
+		uc := &okta.UserCredentials{


Just found a bug in this code where recovery_question isn't being set for a user. Will fix it.
Will also need to update the tests to check if it is set. Sorry for this half-baked PR. :|

No worries at all. Thanks for the contribution. I will wait to review it.

@quantumew - I have fixed the bug and tested it. Also ran the user resource related acceptance tests. Review when you can.

quantumew

Hey @vijetm thanks for your update! Just a few things to address, thanks!

quantumew · 2019-11-14T22:27:02Z

okta/resource_okta_user.go

+			"password": &schema.Schema{
+				Type:        schema.TypeString,
+				Optional:    true,
+				Description: "User Password",


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?

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.

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?

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

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

I believe I've addressed all your concerns. Take a look.

quantumew · 2019-11-14T22:28:42Z

okta/resource_okta_user.go

+	recoveryQuestion := d.Get("recovery_question").(string)
+	recoveryAnswer := d.Get("recovery_answer").(string)
+
+	if recoveryQuestion != "" && len(recoveryAnswer) < 4 {


This can be done via a ValidationFunc in the schema attribute.

quantumew · 2019-11-14T22:37:38Z

okta/resource_okta_user.go

+	userBody := okta.User{
+		Profile:     profile,
+		Credentials: uc,
+	}


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

quantumew · 2019-11-14T22:38:11Z

okta/resource_okta_user_test.go

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


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

vijetm · 2019-11-15T00:10:21Z

okta/resource_okta_user.go

+				Type:        schema.TypeString,
+				Optional:    true,
+				Sensitive:   true,
+				ValidateFunc: validation.StringLenBetween(8, 1000), // Hope no one uses password > 1000 chars


default okta password length is 8. Users can change their policy to 4 characters, but I think it's good to enforce the 8 character minimum limit

I think we can just remove this validation imo. I definitely strongly recommend longer passwords, but if it is possible to have shorter or longer passwords in Okta, let's let them do that.

quantumew · 2019-11-15T01:39:55Z

okta/resource_okta_user.go

+				Sensitive:    true,
+				ValidateFunc: validation.StringLenBetween(4, 1000),
+				Description:  "User Password Recovery Answer",
+			},


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

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

quantumew

Thanks for the update. Just a few small comments/questions. Can you run make fmt and commit the change?

vijetm · 2019-11-15T15:48:20Z

okta/resource_okta_user.go

+			RecoveryQuestion: rq,
+		}
+
+		_, _, err := client.User.ChangeRecoveryQuestion(d.Id(), *nuc)


@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.

vijetm · 2019-11-15T18:30:35Z

@quantumew - N00b question: Where can I see the statements printed using log.Printf? Trying to debug an issue. Wondering what's the best way to debug without breakpoints...

quantumew · 2019-11-15T18:36:57Z

@vijetm I recommend delve with breakpoints but if you are not feeling like you need that, then just fmt.Print to stdout, make sure tests are run with -v, and you will likely have to set TF_LOG=debug.

vijetm · 2019-11-18T17:35:43Z

@quantumew - After some debugging, I see that the call to change recovery credentials isn't being routed to okta. I also notice that you're still using v0.1.0 of the okta golang SDK - https://github.com/articulate/terraform-provider-okta/blob/master/go.mod#L20 (which could be the reason for this issue)
The latest version is v1.0.1. Apart from the go.mod file, is there any place I need to change to use the latest version?

quantumew · 2019-11-18T17:57:42Z

@vijetm That version might be misleading. Notice the replace directive at the bottom. Our fork should be up to date with the latest SDK, maybe one patch version behind. If you set TF_LOG=debug you can see requests going out and responses in, you are not seeing this request go out, despite it reaching your code and calling the update func?

vijetm · 2019-11-18T19:50:48Z

@quantumew - I see the call being made now. A previous password change test was failing which caused the next call to not execute.
The tests also seem to be passing now. Had to make some changes to the staged.tf file to add password. Looking at the best way to change this now. Will submit the fix when I finish. Thanks for the help.

vijetm · 2019-11-18T23:57:01Z

examples/okta_user/all_attributes.tf

@@ -31,4 +31,5 @@ resource "okta_user" "test" {
  title              = "Director"
  user_type          = "Employee"
  zip_code           = "11111"
+  password           = "Abcd1234"


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.

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.

okta/resource_okta_user_test.go

vijetm · 2019-11-20T17:11:53Z

@quantumew - Did you get a chance to run the acceptance tests? Let me know if you want me to run them locally.

quantumew

LGTM!

quantumew self-requested a review November 14, 2019 18:23

vijetm commented Nov 14, 2019

View reviewed changes

vijetm force-pushed the enhancement/user-with-credentials branch 5 times, most recently from 47162f5 to ab261cb Compare November 14, 2019 22:29

quantumew reviewed Nov 14, 2019

View reviewed changes

Adds support to create users with credentials

a95efd4

vijetm force-pushed the enhancement/user-with-credentials branch 2 times, most recently from 46fdc6e to cf8ef86 Compare November 15, 2019 00:04

Adds user password update functionality

4d300d4

vijetm force-pushed the enhancement/user-with-credentials branch from cf8ef86 to 4d300d4 Compare November 15, 2019 00:09

vijetm commented Nov 15, 2019

View reviewed changes

quantumew self-requested a review November 15, 2019 01:36

quantumew reviewed Nov 15, 2019

View reviewed changes

vijetm force-pushed the enhancement/user-with-credentials branch from 9732d9d to 6880f8e Compare November 15, 2019 15:46

vijetm commented Nov 15, 2019

View reviewed changes

Support updating recovery credentials

6270b93

vijetm force-pushed the enhancement/user-with-credentials branch from 6880f8e to 6270b93 Compare November 18, 2019 23:54

vijetm commented Nov 18, 2019

View reviewed changes

Adds a new test case for update credentials

4e0debd

vijetm commented Nov 19, 2019

View reviewed changes

okta/resource_okta_user_test.go Show resolved Hide resolved

quantumew self-requested a review November 19, 2019 02:55

quantumew approved these changes Nov 20, 2019

View reviewed changes

quantumew merged commit 003388d into articulate:master Nov 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support to create users with credentials #334

Adds support to create users with credentials #334

vijetm commented Nov 14, 2019 •

edited

vijetm commented Nov 14, 2019

quantumew commented Nov 14, 2019

vijetm Nov 14, 2019

quantumew Nov 14, 2019

vijetm Nov 14, 2019

quantumew left a comment

quantumew Nov 14, 2019

quantumew Nov 14, 2019

vijetm Nov 14, 2019 •

edited

quantumew Nov 14, 2019 •

edited

vijetm Nov 14, 2019

vijetm Nov 15, 2019

quantumew Nov 14, 2019

quantumew Nov 14, 2019

quantumew Nov 14, 2019

vijetm Nov 15, 2019

quantumew Nov 15, 2019 •

edited

quantumew Nov 15, 2019 •

edited

vijetm Nov 15, 2019

quantumew left a comment

vijetm Nov 15, 2019

vijetm commented Nov 15, 2019

quantumew commented Nov 15, 2019

vijetm commented Nov 18, 2019 •

edited

quantumew commented Nov 18, 2019

vijetm commented Nov 18, 2019

vijetm Nov 18, 2019 •

edited

quantumew Nov 19, 2019

vijetm commented Nov 20, 2019

quantumew left a comment

Adds support to create users with credentials #334

Adds support to create users with credentials #334

Conversation

vijetm commented Nov 14, 2019 • edited

vijetm commented Nov 14, 2019

quantumew commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantumew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijetm Nov 14, 2019 • edited

Choose a reason for hiding this comment

quantumew Nov 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantumew Nov 15, 2019 • edited

Choose a reason for hiding this comment

quantumew Nov 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantumew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijetm commented Nov 15, 2019

quantumew commented Nov 15, 2019

vijetm commented Nov 18, 2019 • edited

quantumew commented Nov 18, 2019

vijetm commented Nov 18, 2019

vijetm Nov 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijetm commented Nov 20, 2019

quantumew left a comment

Choose a reason for hiding this comment

vijetm commented Nov 14, 2019 •

edited

vijetm Nov 14, 2019 •

edited

quantumew Nov 14, 2019 •

edited

quantumew Nov 15, 2019 •

edited

quantumew Nov 15, 2019 •

edited

vijetm commented Nov 18, 2019 •

edited

vijetm Nov 18, 2019 •

edited