LocalContainer not adhering to specifications

[mariobodemann]

While testing the wrapper we found that the initial implementation needed some more love.

This PR fixes cruicial local container creation, and adds love.

[mariobodemann]

Executing the test locally (revolutionary concept):

JS bridge error. expected:<"[]"> but was:<"[Exception: {\"message\":\"Failed to set the 'innerHTML' property on 'Element': This document requires 'TrustedHTML' assignment.\"}]">
at org.junit.Assert.assertEquals(Assert.java:117)
at com.yubico.ykbrowser.StartUpTest.javascriptInjectionIsCorrect(StartUpTest.kt:91)

I.e. someone from the maintainers (@tladesignz, or @jessevanmuijden maybe?) need to see how to fix the frontend requiring TrustedHTML in combination with adding the debug icon to the dom.

Good luck.

[jessevanmuijden]

Suggested change

	if (secureStore.containsAlias(alias)) {
	secureStore.deleteEntry(alias)
	} else {
	failureCallback(IllegalStateException("Credential alias nor found."))
	}
	if (!secureStore.containsAlias(alias)) {
	failureCallback(IllegalStateException("Credential alias nor found."))
	}
	secureStore.deleteEntry(alias)

[mariobodemann]

I am not sure about this one: It looks like you'd always delete an entry, if it exists or not.

(Also just noticed a typo in the original code: nor should probably be not.)

[jessevanmuijden]

Suggested change

	if (filename !in context.fileList()) {
	// no delete necessary
	failureCallback(IllegalStateException("File $filename not found."))
	} else {
	if (context.deleteFile(filename)) {
	successCallback()
	} else {
	failureCallback(RuntimeException("Failed to delete $filename file."))
	}
	}
	if (filename !in context.fileList()) {
	// no delete necessary
	failureCallback(IllegalStateException("File $filename not found."))
	}
	if (!context.deleteFile(filename)) {
	failureCallback(RuntimeException("Failed to delete $filename file."))
	}
	successCallback()

[mariobodemann]

🤔

Wouldn't that open the possibility to call the failureCallback() and then the successCallback() when the file doesn't exist.

Also the failure callback would get invoked twice if the file doesn't exist: First on check, and then when the non existant file is tried to get deleted...

The idea for me is that there shouldn't be an option where both callbacks can get called (or one callback gets called multiple times).

[mariobodemann]

I am not sure if I get the intent of the proposed changes. See inline for detailed feedback.

[mariobodemann]

🤔

Wouldn't that open the possibility to call the failureCallback() and then the successCallback() when the file doesn't exist.

Also the failure callback would get invoked twice if the file doesn't exist: First on check, and then when the non existant file is tried to get deleted...

The idea for me is that there shouldn't be an option where both callbacks can get called (or one callback gets called multiple times).

[mariobodemann]

I am not sure about this one: It looks like you'd always delete an entry, if it exists or not.

(Also just noticed a typo in the original code: nor should probably be not.)

[jessevanmuijden]

@mariobodemann the proposed changes were just a suggestion to replace nested if statements with early returns for readability.

[mariobodemann]

Ah, I get the intent.

But that wouldn't work. In your suggestions you'll call the callbacks but you aren't exiting the function.

You'd need an explicit return, or nested ifs with the right value returned.