Allow UserStorageProvider implementations to opt into storing service account users - #49804

Suppressed comments (1)

model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java:1071

• This external lookup bypasses the timeout and graceful-degradation path used by other federated user lookups (tryResolveFederatedUser). A slow or StorageUnavailableException-throwing high-priority provider can therefore fail/block service-account resolution, including client-credentials grants, instead of continuing to the next provider. Use mapEnabledStorageProvidersWithTimeout and handle unavailability consistently.

        return getEnabledStorageProviders(client.getRealm(), UserServiceAccountProvider.class)
                .map(provider -> provider.getServiceAccount(client))
                .filter(Objects::nonNull)
                .findFirst()

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Balanced

Suppressed comments (3)

server-spi-private/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:33

• The lookup contract described here does not match the implementation: UserStorageManager.getServiceAccount checks local storage first and only then queries capability providers. Document creation and lookup ordering separately so providers do not assume they can override an existing local service account.

 * <p>All storage providers that implement this interface will be looped through in priority order.
 * If a method returns null, the next provider is tried. If no provider handles the request,
 * local storage is used as a fallback.

server-spi-private/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:37

• UserRegistrationProvider is mandatory for an external model, not optional: UserStorageManager.removeUser resolves the model's provider as that capability and throws a ModelException when it is absent. Either make this interface include a removal contract or state the requirement as mandatory.

 * <p>Providers implementing this interface should also implement {@link UserRegistrationProvider}
 * so that service account users can be removed when the owning client is deleted or has service
 * accounts disabled.

server-spi-private/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:50

• The returned model must also support durable username changes: ClientManager.clientIdChanged unconditionally calls setUsername on the service-account user. Without this contract, a provider can satisfy the documented methods but make ordinary client-ID updates fail.

     * <p>The returned {@link UserModel} must support {@link UserModel#setEnabled(boolean)} and
     * {@link UserModel#setServiceAccountClientLink(String)}, and must durably persist the client
     * link so that it survives across sessions and is returned by
     * {@link UserModel#getServiceAccountClientLink()} on subsequent lookups.

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

Suppressed comments (3)

server-spi-private/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:51

• The returned model must also support setUsername: changing a client's ID unconditionally renames its service-account user in ClientManager.clientIdChanged. Without this contract, a conforming provider may fail ordinary client updates.

     * <p>The returned {@link UserModel} must support {@link UserModel#setEnabled(boolean)} and
     * {@link UserModel#setServiceAccountClientLink(String)}, and must durably persist the client
     * link so that it survives across sessions and is returned by
     * {@link UserModel#getServiceAccountClientLink()} on subsequent lookups.

tests/custom-providers/src/main/java/org/keycloak/tests/providers/federation/ServiceAccountUserStorage.java:62

• Renaming any client with an externally stored service account currently fails because the client update path always calls setUsername. Update this test provider to move the map entry and keep the adapter's username current, then cover client-ID changes in the integration test.

            @Override
            public void setUsername(String username) {
                throw new UnsupportedOperationException();

server-spi-private/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:40

• This capability currently permits providers that cannot complete the lifecycle it initiates: provider-qualified users are removed only through UserRegistrationProvider and resolved by ID only through UserLookupProvider, otherwise deletion throws and later lookups return null. Make these mandatory capabilities (or route both operations through this interface).

 * <p>Providers implementing this interface should also implement {@link UserRegistrationProvider}
 * so that service account users can be removed when the owning client is deleted or has service
 * accounts disabled.
 */
public interface UserServiceAccountProvider {

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

Suppressed comments (2)

server-spi-private/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:39

• Implementing only this advertised capability is insufficient when the user cache is enabled. UserCacheSession.findServiceAccount caches the returned external ID and later reloads it through getUserById, while UserStorageManager.getUserById only consults UserLookupProvider; after cache invalidation/partial eviction, a provider implementing this interface alone can no longer resolve its service account. Either require UserLookupProvider here or adapt the service-account cache reload path to use this capability.

public interface UserServiceAccountProvider extends UserRegistrationProvider {

model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java:523

• The new external-service-account preRemove branch is not exercised by the added deletion test: that test only checks the provider's map, which is removed regardless of whether federated sidecar cleanup ran. Add a federated attribute/role/group mapping before deletion and assert its row is gone afterward, so reverting this condition would fail the test.

        if (getFederatedStorage() != null
                && (user.getServiceAccountClientLink() == null || !StorageId.isLocalStorage(user.getId()))) {
            getFederatedStorage().preRemove(realm, user);

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

Suppressed comments (1)

model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java:1067

• Add integration coverage for the promised local-first compatibility path: create a local service account, then enable the capability provider and verify lookup returns the original local user rather than the external provider. The current fallback test removes the provider before creation, so it would not catch a regression in this ordering.

        UserModel user = localStorage().getServiceAccount(client);
        if (user != null) return user;
        return getEnabledStorageProviders(client.getRealm(), UserServiceAccountProvider.class)

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Balanced

Suppressed comments (3)

server-spi/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:39

• The linked issue and both discussions explicitly keep this capability in server-spi-private so it is not a stable public SPI. Defining the same FQCN here also publishes it from keycloak-server-spi and leaves duplicate class definitions across the two artifacts; remove this public copy and retain only the private-SPI declaration.

public interface UserServiceAccountProvider extends UserRegistrationProvider {

model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java:1071

• This direct provider call bypasses the storage-provider timeout wrapper used by federated lookups (tryResolveFederatedUser, lines 1298–1309). A slow external store can therefore stall service-account lookup—and client-credentials requests—without the configured timeout handling; use mapEnabledStorageProvidersWithTimeout here.

        return getEnabledStorageProviders(client.getRealm(), UserServiceAccountProvider.class)
                .map(provider -> provider.getServiceAccount(client))
                .filter(Objects::nonNull)
                .findFirst()
                .orElse(null);

tests/base/src/test/java/org/keycloak/tests/admin/userstorage/ServiceAccountUserStorageTest.java:148

• This exercises only the default transient client-credentials session, so it never verifies that the externally stored user can be reloaded by ID. Add a case enabling USE_REFRESH_TOKEN_FOR_CLIENT_CREDENTIALS_GRANT and refreshing the token; otherwise the persistent-session path required for external service accounts can regress unnoticed.

        // client_credentials grant works against the externally-stored service account.
        oauth.client(CLIENT_ID, CLIENT_SECRET);
        AccessTokenResponse response = oauth.doClientCredentialsGrantAccessTokenRequest();

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

Suppressed comments (1)

server-spi/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:39

• The linked issue explicitly lists promotion to the public server-spi as a non-goal and says this experimental contract should live in server-spi-private. Defining it here exposes a public compatibility commitment before the contract is proven; move the interface to the private SPI module and update consumers accordingly.

public interface UserServiceAccountProvider extends UserRegistrationProvider {

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

Suppressed comments (2)

model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java:1071

• This new external lookup bypasses the storage-provider timeout and graceful-degradation path used by regular external user lookups (tryResolveFederatedUser at lines 1298-1310). If the highest-priority provider is unavailable, client-credentials requests fail instead of continuing to the next provider; use the timeout helper and handle StorageUnavailableException consistently.

        return getEnabledStorageProviders(client.getRealm(), UserServiceAccountProvider.class)
                .map(provider -> provider.getServiceAccount(client))
                .filter(Objects::nonNull)

server-spi-private/src/main/java/org/keycloak/storage/user/UserServiceAccountProvider.java:42

• The capability does not require UserLookupProvider, although service-account IDs are subsequently resolved via getUserById (including cache delegate reloads in UserAdapter#getUserModel). A provider implementing only this declared contract can create and return an account initially, but later updates or ID-based resolution fail; make lookup part of the capability contract, as the test provider already assumes.

public interface UserServiceAccountProvider extends UserRegistrationProvider {

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced