Create an initial password for new users

[Floby]

Fixes #76

I got to the point where I can login with a newly created user.

However, I don't know how to deal with the diffing from an existing user which is why my test continues to fail. Reading through the terraform docs for the Resource didn't help me much. I would like some help with your terraform-fu @mrparkers :]

Anyway, I'd like a review on my code as it is my first time writing actual go.

[mrparkers]

This is a great start. The code comments are mostly nitpicky, and you did exactly what I would have done when testing this.

I do have a few other requests before this gets merged:

1. Could you add this attribute to one of the existing users defined within the example config? I like to try and include every feature within this file you so can quickly run tf apply to see it all at once.
2. I pulled down your branch and ran through a few scenarios locally, and it looks like terraform will attempt an update if this attribute is changed. Obviously, this update does nothing, but this may seem confusing to the end user. I think it would be best to suppress all diffs when this field is updated. You can use DiffSuppressFunc on the attribute schema to solve this. If you can add a test case for this as well, that would be great. I can help you with this if you have any questions.

Thanks!

[Floby]

I tried quickly with DiffSuppressFunc but couldn't figure out how to use it. I have a hard time debugging and figuring out when the function is called and with what arguments :/

[mrparkers]

The function signature looks like this:

type SchemaDiffSuppressFunc func(resourceName, old, new string, data *ResourceData) bool

The goal here would be to suppress all diffs for this attribute after the resource has been created. You can use data to see if the resource has an ID - if it does, you know the resource has been created, because Keycloak is responsible for generating those IDs.

[Floby]

Aaah great, I missed that fact. I was only trying to check with IsNew. Thanks Le jeu. 3 janv. 2019 à 18:34, Michael Parker <notifications@github.com> a écrit :

…

The function signature looks like this: type SchemaDiffSuppressFunc func(resourceName, old, new string, data *ResourceData) bool The goal here would be to suppress all diffs for this attribute after the resource has been created. You can use data to see if the resource has an ID - if it does, you know the resource has been created, because Keycloak is responsible for generating those IDs. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOYMuYUfSDOykSxthhTtSniRWPa9X4iks5u_j8sgaJpZM4ZoCXX> .

[Floby]

Hello @mrparkers,
I was able to supress the diff when updating a existing user. I also added (and documented) a flag initial_password_temporary which can set up the password to be renewed on first login. Could not figure how to add a test simply though.

I'm happy with how the feature works now but I'm still happy to get feedback.

[mrparkers]

Thanks for addressing the comments. The code looks good, although I am not sure how I feel about the new initial_password_temporary field. It seems weird to have a companion attribute that is only relevant if another attribute is also set.

If you want to keep that functionality, what would you think of this instead?

resource "keycloak_user" "user_with_password" {
	realm_id   = "${keycloak_realm.test.id}"
	username   = "user-with-password"

	email      = "user-with-password@fakedomain.com"
	first_name = "Testy"
	last_name  = "Tester"

	initial_password {
            value     = "my-password"
            temporary = true # defaults to false
        }
}

I have to admit, I am not that familiar with implementing this type of schema, but I do think it looks cleaner. Would you be okay with taking a stab at this?

If not, I'd prefer that we just make a decision to hardcode the initial password to either always be temporary or not (whatever is better for your use case), and I can open an issue and try to do this at a later time.

Let me know what you think.

[Floby]

I agree it would be cleaner. As a matter of fact I tried several things but I could not find many examples around.

You can either :

• Declare initial_password to be a schema.TypeMap but the keys are then arbitrary which would make for less usefull (absent in fact) error messages when using it wrongly
• Declare initial_password to be a schema.TypeSet which allows to have a schema.Resource as the Elem property. But this allows to have an arbitrary number of declarations for initial_password which seems a bit silly. I guess I could go this way and throw a custom error if this is declared more than once.

[Floby]

It seems this is the way to go https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_mq_broker.go#L42-L61

[mrparkers]

Yes, it seems everyone uses TypeList for stuff like this, which I agree is strange. I often reference the Google provider and it does this as well.

[Floby]

Got it to work with a block declaration !

[mrparkers]

Looks good, nice work! I just had a few style and doc nitpicks. I'll merge this as soon as those are addressed.

[Floby]

Hello, I think we're close to good here :)
Do you need these commits squashed ?

[mrparkers]

Looks great! Thanks for your contribution!

[Floby]

Thank you for bearing with me and accepting my work :) Your repo is neat and very tidy. Very easy to get into.

Do you plan on making a new release soon ?

[mrparkers]

I appreciate the kind words! I'm about to cut a new release for you right now. It'll be ready in 10 minutes or so.

[Floby]

Great ! thanks a lot :)