Fix HeadersConfigurer#permissionsPolicy method with customizer

[florianberthe]

Closes gh-14803

[florianberthe]

@marcusdacoregio finally I recreate a new branch as I got some conflicts when rebasing to 5.8.x branch.

[jzheaux]

@florianberthe thank you for the PR. Since there is a simple workaround and also no way to make this binary compatible, let's please make this change on main instead of 5.8.x. I've left more details in my inline comment.

UPDATE - I see that @marcusdacoregio advised something different on an earlier PR. Please wait on my suggestions until we are both aligned.

[jzheaux]

While certainly a bug, we cannot change the return type of a public method and remain binary compatible. And given that this has been in place for a few years now and that there is a simple workaround, I hesitate to make a change this aggressive.

Let's instead introduce a new method and deprecate this one. Perhaps permissionsPolicyHeader would be an appropriate name. We can remove permissionsPolicy in Spring Security 7.

[florianberthe]

I understand your point of view but permissionsPolicyHeader name doesn't seem to be coherent with other "DSL" methods remaining in Spring Security 7. Would you keep this incoherence in Spring Security 7 ? Unless you will rename other DSL methods with same convention.

[florianberthe]

I will wait your thoughts 😄

[florianberthe]

Hi @jzheaux have you seen my comments above?

[marcusdacoregio]

Hi @florianberthe. Yes, the method will be renamed from permissionsPolicyHeader to permissionsPolicy in Spring Security 7 when we remove the deprecated method.

[florianberthe]

Hi, I will make this change later

[codingtim]

So the plan is to add a permissionsPolicyHeader method in Spring 6.3 with a correct return type?
Then in 7 the old permissionsPolicy method with incorrect return type will be removed and permissionsPolicyHeader will be renamed?
That would mean that even if you now use the undeprecated permissionsPolicyHeader method in 6.3 you will have a breaking change when you upgrade to 7?
Or will the permissionsPolicyHeader method also exist in 7 and be deprecated then?

[marcusdacoregio]

So the plan is to add a permissionsPolicyHeader method in Spring 6.3 with a correct return type?

Yes, in Spring Security 6.4. As well as deprecating the current method with the wrong return type.

Then in 7 the old permissionsPolicy method with incorrect return type will be removed and permissionsPolicyHeader will be renamed?

That's right.

That would mean that even if you now use the undeprecated permissionsPolicyHeader method in 6.3 you will have a breaking change when you upgrade to 7?

Yes, a breaking change is fine since that will be a major version bump.

[codingtim]

Just my 2 cents but that does not seem entirely user friendly.

Yes in version 7 you can remove the deprecated method with incorrect return type.
But instead of renaming the temporary method of version 6.4, deprecate that method in version 7.
Instead of removing the deprecated method it gets replaced by one with the correct return.

That way users of 6.4 have no breaking changes but can upgrade by searching deprecation.

So 6.4:

@deprecated("Deprecated invalid return type, use temporary method below")
PermissionsPolicyConfig permissionsPolicy(..)

HeadersConfigurer<H> permissionsPolicyHeader(..)

And 7.0:

HeadersConfigurer<H> permissionsPolicy(..)

@deprecated("Use corrected method above")
HeadersConfigurer<H> permissionsPolicyHeader(..)

[codingtim]

Hi, While upgrading spring-security to 6.2.4 we ran into this issue.
We try to remove deprecation warning of and() usage but because of the incorrect return type of permissionsPolicy(Customizer<PermissionsPolicyConfig> permissionsPolicyCustomizer) this is currently not possible.

[florianberthe]

Hi, While upgrading spring-security to 6.2.4 we ran into this issue. We try to remove deprecation warning of and() usage but because of the incorrect return type of permissionsPolicy(Customizer<PermissionsPolicyConfig> permissionsPolicyCustomizer) this is currently not possible.

Personally, I moved call to this method at the end of the chaining as workaround. You could consider a little ugly but it avoids a deprecated warning.