Spring Security ClientRegistration.Builder.tokenUri() should take a URI parameter, not a String

Expected Behavior

ClientRegistration.Builder.tokenUri() accepts a URI as its parameter.

        public Builder tokenUri(URI tokenUri) {  // <- Note the data type of the parameter
            this.tokenUri = tokenUri;
            return this;
        }

Current Behavior

ClientRegistration.Builder.tokenUri() accepts 🚨 a String as its tokenUri parameter.

https://github.com/spring-projects/spring-security/blob/0137d0254c1040c05a2b18d64826f50393d3a440/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java#L527-L530

Context

We accidentally sent the wrong parameter to ClientRegistration.Builder.tokenUri().

This cost us troubleshooting time.

If tokenUri() had required a URI rather than a String, the compiler would have caught this, saving us troubleshooting time.

Note that this would be an API breaking change, so I'm guessing this fix would have to wait for v7.

Note also that some related fields / methods should also get their types fixed accordingly. At least in this file, but potentially elsewhere in the project as well.

Comment From: walles

Unsure whether this should go under "Bug" or "Enhancement". I sort of feel this is a bug, but in the interest of not crying wolf I settled for Enhancement.

Comment From: jgrandja

@walles The "*Uri" attributes in ClientRegistration.ProviderDetails are intended to be String and this has been around since 5.0.

ClientRegistration is Serializable and therefore serializing a URI would not be ideal. Furthermore, constructing a URI could throw a parsing exception which would force a try/catch around the building of the ClientRegistration, which IMO would not be very clean.

We're comfortable with the design and types of ClientRegistration as it has gone through many releases and is stable.

I'm going to close this as there are no plans to change it.