(HMS-4562) Allow Blueprints update without changing User password

[andywaltlova]

HMS-4562

See the issue for detailed acceptance criteria, in short the PR:

• Blueprint can be updated without specifying the password, and password is not changed.
• <REDACTED> value is not used at all, instead users password property is omitted from response
• either password or ssh_key must be set for every user (or both)
• empty string can be used to remove password or ssh_key (while still having either ssh_key or password set) == user can be changed from 'password user' to 'ssh_key user' and back

[app-sre-bot]

Can one of the admins verify this patch?

[ezr-ondrej]

Proper TDD (tests already done before code) 🧡 it!

[andywaltlova]

Current logic:

During blueprint update user can

1. use "<REDACTED>" as a value - this will try to use latest password or throws an error
2. omit password in request (=nil)
   - this keeps password as it is in database
   - if ssh_key passed it will just add ssh_key as well
3. use ""
   - this will remove non-empty password in database but it will be an empty string NOT nil (can't use nil because that one just keeps old password)
   - is valid only if ssh_key is non-empty (in request or already in database)

I don't think I can make step 3 work differently without messing with database structure, the behavior of nil is dependent on how pxe and postgres behaves, nil is no-op

[lzap]

Current logic:

It sounds a little bit complicated and while I agree with some points, here is a suggestion for easy to understand behavior:

• Omitted field = do nothing. This is very well understood API REST behavior overall.
• Empty string = an error like 4xx Not Allowed. It is a good practice for systems to not to allow empty passwords, these are more often than not an unintentional input error.
• Anything else, including the literal <REDACTED>, should actually hash the string and store the hashed value. The special <REDACTED> string should have no special treatment.

[andywaltlova]

Current logic:

It sounds a little bit complicated and while I agree with some points, here is a suggestion for easy to understand behavior:

• Omitted field = do nothing. This is very well understood API REST behavior overall.
• Empty string = an error like 4xx Not Allowed. It is a good practice for systems to not to allow empty passwords, these are more often than not an unintentional input error.
• Anything else, including the literal <REDACTED>, should actually hash the string and store the hashed value. The special <REDACTED> string should have no special treatment.

but then how will you be able to update the user in the blueprint? E.g. what if I want keep the user names and remove the passwords in favor of ssh keys? One of the obvious solutions would be to just instruct the users to remove the whole user customizations and then recreate it or to just create new blueprint, if thats fine then I think we can simplify it as you suggested :)

[lzap]

Good point, then I am thinking using the empty password case as "remove password" behavior.

[andywaltlova]

Good point, then I am thinking using the empty password case as "remove password" behavior.

Yeah thats what is there right now, but since the customization is saved as an json raw message, in database that means it will equal to empty string and not NULL, essentially I don't think that's bad, but thats why the code is more complicated because I have to count with possibility that if someone removes password it won't be null but can be also empty string and then make sure every time that at least one of ssh_key or password is not empty (not null and not empty string)

Just to be clear, the suggestion you have about <REDACTED> value, that I can change to just hash it and it will be the new user password but I did not because that was explicitly one of the criteria in jira issue written by @ezr-ondrej :

Passing <REDACTED> or another special value as user password, should ensure the current password is kept on update.

So if just not passing password counts toward this criteria (password=nil), it will just keep password as it was and I can remove the <REDACTED> to also keep the password it will just be used as a password.

[lzap]

Just for the record, we are elaborating on the design trying to explore alternative solutions to the problem. We will report back once we agree on something.

[lzap]

Here is my initial review, I think the most important thing is to isolate the most complex logic into a separate function that takes a pointer to User type and modifies it accordingly. This way, you will be able to easily write a tabular tests for all possible test cases. This will simplify the overall HTTP handler tests where you only need to cover positive test case.

[lzap]

This should be just:

u.Password = nil

It is cleaner and faster to just overwrite a value with a new one, even without effect.

Quite frankly, since this method is now a one-liner I encourage you to get rid of it and expand it everywhere.

[andywaltlova]

it is now

[lzap]

What I really meant was this:

func (u *User) RedactPassword() {
	// no need to do this: if u.Password != nil
	u.Password = nil
}

If password is nil and you set it to nil it is faster and two lines shorter than:

• Check if it is nil
• Set it to nil

In other words: redacting is operation that simply removes a password before we return the data to the user, no matter what the original value was.

[lzap]

Ok this looks awesome, few small nitpicks and one big ask.

I would love to see a table-driven test for method called isValidForUpdate (I actually ask you to rename it). Test for various inputs we covered in the Jira epic description.

[lzap]

What I really meant was this:

func (u *User) RedactPassword() {
	// no need to do this: if u.Password != nil
	u.Password = nil
}

If password is nil and you set it to nil it is faster and two lines shorter than:

• Check if it is nil
• Set it to nil

In other words: redacting is operation that simply removes a password before we return the data to the user, no matter what the original value was.

[lzap]

Correction: it looks I missed the handler test, it covers all important cases. Feel free to ignore my comment about a new TDD test for the newly created function. Not needed.

[lzap]

Awesome work, just add a note in the description about reading passwords it is not mentioned and it can be surprising behavior. Please squash into a single commit, I am good to go.

[lzap]

An actual (relevant) test failure.

[codecov]

Codecov Report

Attention: Patch coverage is 87.01299% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/v1/handler_blueprints.go	86.48%	5 Missing and 5 partials ⚠️

Files with missing lines	Coverage Δ
internal/clients/composer/client.go	54.41% <ø> (-1.31%)	⬇️
internal/v1/api.go	47.98% <ø> (ø)
internal/v1/handler.go	68.90% <100.00%> (+0.05%)	⬆️
internal/v1/handler_blueprints.go	75.97% <86.48%> (-1.90%)	⬇️

[croissanne]

/retest

[lzap]

Thanks for the help with CI! Great work @andywaltlova.