OAuth2ClientPropertiesRegistrationAdapter improve issuerUri value check

[bitxon]

Expected Behavior
OAuth2ClientPropertiesRegistrationAdapter.getBuilderFromIssuerIfPossible should validate that issuer is not blank
Because few lines after, code throws exception if this issuer is null or emptyString or blankString

Current Behavior
OAuth2ClientPropertiesRegistrationAdapter.getBuilderFromIssuerIfPossible validates that issuer is not null

Context

class OAuth2ClientPropertiesRegistrationAdapter {
  ...
  private static Builder getBuilderFromIssuerIfPossible(...) {
      ...
      if (issuer != null) {  // this code performs only null check
        Builder builder = ClientRegistrations.fromIssuerLocation(issuer).registrationId(registrationId);
        return getBuilder(builder, provider);
      }
      ...
  }
  ...
}

public static ClientRegistration.Builder fromIssuerLocation(String issuer) {
  Assert.hasText(issuer, "issuer cannot be empty"); // this code perform null check + String content check
  ...
}

[wilkinsona]

Thanks for the suggestion, @bitxon. Can you provide a bit of background on this please? I'd like to understand how you came to have the property configured but with an empty value.

If we treat an empty string as we currently treat null, we'll silently ignore the issuer URI configuration and use the provider's issuer URI instead. It sounds like that's what you'd prefer to happen, but someone else may prefer the current failure. If they have been trying to set the issuer URI and have made a mistake that led to it having an empty value, silently ignoring it may make that mistake harder to spot.

[bitxon]

If in application.yml we have following configuration

spring.security.oauth2.client:
  provider:
    keycloak:
      issuerUri: ${oauthUrl}
      authorization-uri: ${oauthUrl}/protocol/openid-connect/auth
      token-uri: ${oauthUrl}/protocol/openid-connect/token
      jwk-set-uri: ${oauthUrl}/protocol/openid-connect/certs
      user-info-uri: ${oauthUrl}/protocol/openid-connect/userinfo
      user-name-attribute: preferred_username

So we will not be able to unset issuerUri in application-test.yml without tricks. Because following configuration will be considered as value with empty string:

spring.security.oauth2.client:
  provider:
    keycloak:
      issuerUri:

[wilkinsona]

Thanks for the background. Have you considered switching things around so that your configuration that's in application.yml is profile-specific? You would then avoid activating this profile in situations where you're currently activating the test profile and would no longer need to unset the issuerUri property.

We'd like to make it easier to unset a configuration property. #24133 is covering (some of) that. This feels like another case where it would be useful. I am reluctant to implement a point-fix that changes the handling of an empty string in this specific case for the reasons explained above. I'll flag this one so that we can see what the rest of the team thinks.

[mbhave]

Unless switching things around where the configuration which contains issuer-uri is only active in a certain profile doesn't work , I don't think we should implement a point-fix change. As Andy pointed out we might silently ignore the failure that someone might expect.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[bitxon]

Thanks for the background. Have you considered switching things around so that your configuration that's in application.yml is profile-specific? You would then avoid activating this profile in situations where you're currently activating the test profile and would no longer need to unset the issuerUri property.

We'd like to make it easier to unset a configuration property. #24133 is covering (some of) that. This feels like another case where it would be useful. I am reluctant to implement a point-fix that changes the handling of an empty string in this specific case for the reasons explained above. I'll flag this one so that we can see what the rest of the team thinks.

This approach is working fine:
application.yml

spring.security.oauth2.client:
  provider:
    keycloak:
      #issuerUri is not specified here
      authorization-uri: ${oauthUrl}/protocol/openid-connect/auth
      token-uri: ${oauthUrl}/protocol/openid-connect/token
      jwk-set-uri: ${oauthUrl}/protocol/openid-connect/certs
      user-info-uri: ${oauthUrl}/protocol/openid-connect/userinfo
      user-name-attribute: preferred_username

application-test.yml

oauthUrl: my-test-url
spring.security.oauth2.client:
  provider:
    keycloak:
      issuerUri: ${oauthUrl}

application-dev.yml

oauthUrl: my-dev-url
spring.security.oauth2.client:
  provider:
    keycloak:
      issuerUri: ${oauthUrl}

I just wanted to emphasize that for now we have switch-on(set uri) mechanism and miss switch-off mechanism(unset uri).

[wilkinsona]

Thanks for following up, @bitxon. As mentioned above, #24133 is for making it possible to unset a property. We'll leave the handling of the issuer URI as it is for now for the reasons already discussed.

[falk-stefan]

I am facing the same issues atm as I am trying to come up with a proper config-setup for different environments.

It's great that we can combine different configurations but it would be great if it was possible to null certain values sometimes.

For example, locally I am working with a PostgreSQL database, but the deployment uses a Cloud SQL. For this I'd need to different files application-pglocal.yaml and application-pgcloud.yaml.

It would be nice if we could write a application-pg.yaml which worked for both. For that it would be necessary to be able to set environment-variables to null if they are empty.

I don't know how this could be done though. Maybe if the syntax for environment variables would be extended:

${VARIABLE:}                    # Will be empty-string if not set
${VARIABLE:default-value}       # Will never be null
${VARIABLE:default-value:true}  # Will be null if set to empty-string
${VARIABLE::true}               # Will be null if not set
${VARIABLE::false}              # Will be empty-string if not set

Then we could do something like this:

spring:
  cloud:
    gcp:
      sql:
        database-name: ${DB_NAME::true}
        instance-connection-name:  ${DB_CONNECTION_NAME::true}

  datasource:
    driver-class-name: org.postgresql.Driver
    password: ${JDBC_DATABASE_PASSWORD:postgres}
    username: ${JDBC_DATABASE_USERNAME:root}

A more pragmative solution would probably be to set any empty-string to null per default but I'm not sure how many fires this would turn on.

[wilkinsona]

@falk-stefan Support for setting a property back to null is being tracked by the previously mentioned #24133.