Thank you for the PR. I pushed one more commit to fix it at the source, so all other consumers of that method can benefit from it. I also checked the return value which is null.

To ensure that this works also in the future we need an integration test. Please provide one for this PR to proceed. Maybe @cgeorgilakis can help providing one.

Proposed test class to add :

package org.keycloak.testsuite.broker;

import org.keycloak.broker.oidc.OIDCIdentityProviderConfig;
import org.keycloak.models.IdentityProviderModel;
import org.keycloak.models.IdentityProviderSyncMode;
import org.keycloak.protocol.oidc.OIDCLoginProtocol;
import org.keycloak.representations.idm.IdentityProviderRepresentation;

import java.util.Map;

import static org.keycloak.broker.oidc.OAuth2IdentityProviderConfig.TOKEN_ENDPOINT_URL;
import static org.keycloak.testsuite.broker.BrokerTestConstants.*;
import static org.keycloak.testsuite.broker.BrokerTestTools.*;

public class KcOidcBrokerColonAliasClientSecretBasicAuthTest extends AbstractBrokerTest {

    @Override
    protected BrokerConfiguration getBrokerConfiguration() {
        return new KcOidcBrokerColonAliasClientSecretBasicAuthTest.KcOidcBrokerColonAliasConfigurationWithBasicAuthAuthentication();
    }

    private class KcOidcBrokerColonAliasConfigurationWithBasicAuthAuthentication extends KcOidcBrokerConfiguration {

        public final static String CLIENT_ID_COLON = "https://kc-dev.general.gr/staging/realms/general";

        @Override
        public IdentityProviderRepresentation setUpIdentityProvider(IdentityProviderSyncMode syncMode) {
            IdentityProviderRepresentation idp = createIdentityProvider(IDP_OIDC_ALIAS, IDP_OIDC_PROVIDER_ID);
            Map<String, String> config = idp.getConfig();
            applyDefaultConfiguration(config, syncMode);
            config.put("clientAuthMethod", OIDCLoginProtocol.CLIENT_SECRET_BASIC);
            return idp;
        }

        @Override
        protected void applyDefaultConfiguration(final Map<String, String> config, IdentityProviderSyncMode syncMode) {
            config.put(IdentityProviderModel.SYNC_MODE, syncMode.toString());
            config.put("clientId", CLIENT_ID_COLON);
            config.put("clientSecret", CLIENT_SECRET);
            config.put("prompt", "login");
            config.put("loginHint", "true");
            config.put(OIDCIdentityProviderConfig.ISSUER, getProviderRoot() + "/auth/realms/" + REALM_PROV_NAME);
            config.put("authorizationUrl", getProviderRoot() + "/auth/realms/" + REALM_PROV_NAME + "/protocol/openid-connect/auth");
            config.put(TOKEN_ENDPOINT_URL, getProviderRoot() + "/auth/realms/" + REALM_PROV_NAME + "/protocol/openid-connect/token");
            config.put("logoutUrl", getProviderRoot() + "/auth/realms/" + REALM_PROV_NAME + "/protocol/openid-connect/logout");
            config.put("userInfoUrl", getProviderRoot() + "/auth/realms/" + REALM_PROV_NAME + "/protocol/openid-connect/userinfo");
            config.put("defaultScope", "email profile");
            config.put("backchannelSupported", "true");
            config.put(OIDCIdentityProviderConfig.JWKS_URL,
                    getProviderRoot() + "/auth/realms/" + REALM_PROV_NAME + "/protocol/openid-connect/certs");
            config.put(OIDCIdentityProviderConfig.USE_JWKS_URL, "true");
            config.put(OIDCIdentityProviderConfig.VALIDATE_SIGNATURE, "true");
        }

        @Override
        public String getIDPClientIdInProviderRealm() {
            return CLIENT_ID_COLON;
        }

    }
}

You could investigate any tests failure.

I would be more comfortable updating the AbstractOAuth2IdentityProvider because, AFAIK, the HTTP Basic specification does not say anything about url encoding the username and password.

Also, I'm wondering if this note from OAuth2.1 spec is mentioning a problem with url encoding?

Note: This method of initially form-encoding the client identifier and secret, and then using the encoded values as the HTTP Basic authentication username and password, has led to many interoperability problems in the past. Some implementations have missed the encoding step, or decided to only encode certain characters, or ignored the encoding requirement when validating the credentials, leading to clients having to special-case how they present the credentials to individual authorization servers. Including the credentials in the request body content avoids the encoding issues and leads to more interoperable implementations.

If we don't use the proposed fix, we should at least change the getBytes() method to specify the encoding to use, otherwise it will be platform-dependent.

I would be more comfortable updating the AbstractOAuth2IdentityProvider

@pedroigor - thank you for pointing this out. I reverted my changes to the PR, and added the test case @cgeorgilakis suggested. Let's wait for the CI.

If we don't use the proposed fix, we should at least change the getBytes() method to specify the encoding to use, otherwise it will be platform-dependent.

@sschu - the kc.sh will run Keycloak with UTF-8 encoding in all environments, so this should not be a problem IMHO.

@pedroigor / @sschu - do you think this is a "notable" change for the migration guide, as it might break some people's implementations?

While we should now be standard compliant, would we need to provide a switch to revert to the old behavior? I hope not...

Also asking: Should this be backported?

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.account.AccountRestServiceTest#listApplicationsWithoutPermission

Keycloak CI - Java Distribution IT (windows-latest - temurin - 25)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Windows Server 2025', os.arch: 'amd64', os.version: '10.0', java.version: '25'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

I would be more comfortable updating the AbstractOAuth2IdentityProvider because, AFAIK, the HTTP Basic specification does not say anything about url encoding the username and password.

Also, I'm wondering if this note from OAuth2.1 spec is mentioning a problem with url encoding?

Note: This method of initially form-encoding the client identifier and secret, and then using the encoded values as the HTTP Basic authentication username and password, has led to many interoperability problems in the past. Some implementations have missed the encoding step, or decided to only encode certain characters, or ignored the encoding requirement when validating the credentials, leading to clients having to special-case how they present the credentials to individual authorization servers. Including the credentials in the request body content avoids the encoding issues and leads to more interoperable implementations.

That’s why we mainly use Client Secret as POST.

However, Client Secret as basic auth also exists.
The bad things is that dynamic client registration return this in response for Client with secret. However, this may be a broken change.

Another test was broken with change in CLIENT_SECRET. Will be a solution overridden this also in new test?

@cgeorgilakis - I wanted to update the test to also check if the test works for secrets that need URL encoding. While most tests did, the one that relied on values in the vault failed. I've reverted the change, and I assume the next build will be green.

If we don't use the proposed fix, we should at least change the getBytes() method to specify the encoding to use, otherwise it will be platform-dependent.

@sschu - the kc.sh will run Keycloak with UTF-8 encoding in all environments, so this should not be a problem IMHO.

I saw this is a bit inconsistent across the codebase, in some places it is specified, in some places it isn't. I would probably prefer to have it explicit but its not urgent.

@pedroigor / @sschu - do you think this is a "notable" change for the migration guide, as it might break some people's implementations?

While we should now be standard compliant, would we need to provide a switch to revert to the old behavior? I hope not...

Also asking: Should this be backported?

I think I would mention this but not provide a compatibility switch since the old behaviour was essentially a bug.

@pedroigor - I've added a "notable change". Please review when you have the time, and then I would proceed with merging and backporting to 26.2 + 26.4

@pedroigor / @cgeorgilakis / @sschu - I've updated this PR after talking to @srose as this change can break people's setup as we now start to use proper encoding.

So the old behavior is now available, although you need to switch your configuration in case you want to use it. The new default behavior is the correct behavior.

Please review and approve. Thanks!