Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make PublicKeyCredentialRequestOptions Serializable #16438

Closed

Conversation

franticticktick
Copy link
Contributor

Closes gh-16432

Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the Pull Request. Historically the serialVersionUID no longer used for new classes implementing Serializable. Going forward we should generate a serialVersionUID.

@franticticktick
Copy link
Contributor Author

Hi @rwinch, thanks for feedback. If we use the generated serialVersionUID SpringSecurityCoreVersionSerializableTests will not work. It only works with SpringSecurityCoreVersion. Maybe we should add such support to SpringSecurityCoreVersionSerializableTests?

@rwinch
Copy link
Member

rwinch commented Jan 18, 2025

Please take a look at 6f379aa which was progress towards gh-16276 for an example for how to implement it. It uses generated serialVersionUID and adds tests. The ticket gh-16276 also has instructions on how to address it.

NOTE: This PR has some overlap with gh-16285 (which I provided the same advice for)

@franticticktick
Copy link
Contributor Author

Thanks for help @rwinch. Could you review changes please?

Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've provided feedback inline

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this failed to write

CredProtectAuthenticationExtensionsClientInput.CredProtect credProtect = new CredProtectAuthenticationExtensionsClientInput.CredProtect(
CredProtectAuthenticationExtensionsClientInput.CredProtect.ProtectionPolicy.USER_VERIFICATION_OPTIONAL,
true);
Bytes id = new Bytes(("test").getBytes());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you used a constant value here vs generating a new value. Please use a more realistic value. To make this easy I recently added TestBytes.get() gh-16461

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImmutableAuthenticationExtensionClientInput has generic parameter, so SpringSecurityCoreVersionSerializableTests doesn't work. To serialize the class I used this trick:

Object instance;
if(clazz.equals(ImmutableAuthenticationExtensionsClientInput.class)) {
	instance = instancioWithParameter((Class<ImmutableAuthenticationExtensionsClientInput>) clazz)
			.create();
} else {
	instance = instancioWithDefaults(clazz).create();
}

private static InstancioApi<?> instancioWithParameter(Class<ImmutableAuthenticationExtensionsClientInput> clazz) {
	InstancioApi<?> instancio = Instancio.of(clazz).withTypeParameters(Boolean.class);
	if (generatorByClassName.containsKey(clazz)) {
		instancio.supply(Select.all(clazz), generatorByClassName.get(clazz));
	}
	return instancio;
}

Perhaps there is an easier way to do this. We should think about this.

generatorByClassName.put(Bytes.class, (b) -> id);
generatorByClassName.put(PublicKeyCredentialDescriptor.class, (d) -> descriptor);
// @formatter:off
generatorByClassName.put(PublicKeyCredentialRequestOptions.class, (o) -> PublicKeyCredentialRequestOptions.builder()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use (or at least start with) TestPublicKeyCredentialRequestOptions

@franticticktick franticticktick force-pushed the gh-16432 branch 2 times, most recently from 1b421da to cd8d6e0 Compare January 23, 2025 10:26
@franticticktick
Copy link
Contributor Author

SpringSecurityCoreVersionSerializableTests#serializeAndDeserializeAreEqual does not work due to ImmutableAuthenticationExtensionsClientInput. As I already said, it can be serialized, but due to specifying a parameter with a generic, test serializeAndDeserializeAreEqual will fail. I don't see an easy way to solve this problem. @rwinch could you help please?

@rwinch rwinch closed this in 5bc443a Jan 24, 2025
@rwinch rwinch added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 24, 2025
@rwinch rwinch added this to the 6.5.0-M2 milestone Jan 24, 2025
@rwinch
Copy link
Member

rwinch commented Jan 24, 2025

@franticticktick Thanks for the pull request. This is now merged into master along with a841737 to fix the generic types.

NOTE: This can also be fixed using ResolvableType.forClassWithGenerics(AuthenticationExtensionsClientInput.class, Boolean.class).resolve()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make PublicKeyCredentialRequestOptions Serializable
3 participants