feat(connect-form): Add new form kerberos options COMPASS-5500

[mcasimir]

Adds new CANONICALIZE_HOSTNAME options and field to enter password.

[addaleax]

Maybe just say “Provide password”? “directly” sounds like Compass would otherwise infer the password somehow

[mcasimir]

That's kinda what I wanted to imply, isn't it what we mean? Maybe there is a way to say "hey you don't normally need the password, but in case you know what you are doing put it here"

[addaleax]

My (possibly wrong) understanding is that if there is an existing kerberos authentication context, the kerberos library would not need to fully authenticate again and can instead just use that (in which case there is no password coming up at any point)

But tbh, I’m also not sure on what the best wording here is

[mcasimir]

Yeah. It should be super rare that a password is needed, considering that nobody actually complained when we removed it while people complained that it was a password field. Normally people in an org should have machines where kerberos is set up at login and the ticket have the same identity as the user that is logged in. Once you try to connect you should always have a context ready. In my mind we want people to use kinit or the default kerberos subsystem way to acquire credentials over passing the password here.

Providing credentials explicitly should only be needed with a weird setup, when there is an issue with the context or if a user is trying to connect as a different user than the one currently logged in for some reason. Would be nice to know a bit better, but that is why with my understanding I was suggesting to kind of "hide" that input or even remove it. Should be something more dangerous than useful.

[addaleax]

I have very limited understanding of how these things work in react, but should there also be a reverse transition for setShowPassword(false); when there is no password? I just tried this locally, if I add a password in the connection string, the Provide password directly box becomes checked and the password filled in, but if I remove the password in the connection string the box doesn’t become unchecked (this is a pretty minor thing of course, I’m mostly asking because this UI feature seems to be a good template for the TLS system CA option)

[mcasimir]

Yeah, you're right. That was intentional, didn't try from the connection string but I guess is fine to have it that way. Basically, that is to support the case where a user already has a password set and wants to change it. If they would delete the whole content before typing the new one the password field would suddenly disappear as soon as it becomes empty which is not super convenient.

[gribnoysup]

If I'm understanding the logic right, I think this effect tries to check too much, the only thing it should care about is if password exists or not, right? And then the state change of the checkbox should be the trigger for resetting the password, something like this maybe?

Suggested change

	useEffect(() => {
	if (!showPassword && password.length) {
	setShowPassword(true);
	}
	}, [password, showPassword, updateConnectionFormField]);
	// If connection string password changed and there is one now, check the checkbox automatically
	useEffect(() => {
	if (password.length) {
	setShowPassword(true);
	}
	}, [password]);
	// When password field is hidden, clean the password from the connection string
	useEffect(() => {
	if (!showPassword) {
	updateConnectionFormField({
	type: 'update-password',
	password: '',
	});
	}
	}, [showPassword, updateConnectionFormField])

This also means that you don't need to handle updateConnectionFormField in multiple places and the checkbox input only need to change the state of the showPassword and nothing else

[mcasimir]

That's nicer yeah, won't that need to wait for the re-render in order to clean the password? or useEffect is completely detached?

[gribnoysup]

It would need to, yeah (the flow would be something like this: uncheck input -> state change [showPassword is false now] -> render -> effect [because showPassword changed] -> reducer change), but it would be absolutely instant in this case, I wouldn't be concerned about this. But also I submitted the review before I saw that it got approved and merged already and it's alright like it is now too

[gribnoysup]

This should probably wrapped in its own FormFieldContainer? The paddings of this field look a bit weird when it's visible right now