Feature/sync 2

[fsch]

No description provided.

[gemini-code-assist]

Hello @fsch, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Gemini here, providing a summary for this pull request. This PR introduces user synchronization capabilities to the database user storage provider. The core changes involve implementing Keycloak's ImportSynchronization interface to allow pulling user data from the external database into Keycloak. Additionally, a new REST endpoint is added to manually trigger this synchronization process. Supporting changes include updating the project's build environment to Java 17 and Keycloak 25, adding necessary dependencies like Mockito for testing, and setting up a VS Code dev container for easier development.

Highlights

• User Synchronization: Implemented the ImportSynchronization interface in DBUserStorageProvider to enable pulling users from the configured database into Keycloak. This includes logic for adding new users and updating existing ones based on the allowDatabaseToOverwriteKeycloak setting.
• Manual Sync Trigger: Added a new RealmResourceProvider (DBUserStorageResource) with a /sync endpoint to allow manual triggering of the user synchronization process via a REST call.
• Build Environment and Dependencies Update: Updated the project's Java compiler source/target to 17 and the Keycloak version dependency to 25.0.0. Added necessary dependencies for testing (Mockito) and Keycloak internal APIs (JAX-RS, Resteasy, keycloak-services). A VS Code dev container configuration was added to standardize the development environment.
• Configuration Options: Added new configuration properties to the user storage provider factory (DBUserStorageProviderFactory) to control synchronization: syncEnabled (to enable/disable sync) and listAllForSync (a specific query for fetching users during sync).
• Testing: Added new JUnit tests for the DBUserStorageProvider and the new DBUserStorageResource to cover the synchronization logic and the REST endpoint functionality.

Changelog

Click here to see the changelog

• .devcontainer/Dockerfile
  • Added instructions to install Maven 3.9.6.
• .devcontainer/devcontainer.json
  • Configured VS Code dev container to use the new Dockerfile.
  • Set Java runtime to JavaSE-17 (via base image) and added Java extensions.
  • Forwarded Keycloak ports 8080 and 8443.
  • Added post-create command to verify Java and Maven versions.
• pom.xml
  • Added blank line after junit dependency (line 145).
  • Added Mockito Core (v5.11.0), JAX-RS API (v2.1.1), Resteasy JAXRS (v3.15.1.Final), and Keycloak Services (v${keycloak.version}) dependencies (lines 161-187).
  • Added configuration for maven-surefire-plugin to run tests (lines 228-240).
  • Updated maven.compiler.source and maven.compiler.target to 17 (lines 246-247).
  • Updated keycloak.version property to 25.0.0 (line 253).
• src/main/java/org/opensingular/dbuserprovider/DBUserStorageProvider.java
  • Implemented ImportSynchronization interface (line 33).
  • Changed constructor to accept UserStorageProviderModel (line 41).
  • Added syncEnabled field (line 39).
  • Modified updateCredential to check user's federation link before attempting DB update (lines 106-115).
  • Added unlinkUser method (lines 272-285).
  • Implemented sync method for full user synchronization (lines 288-338).
  • Implemented syncSince method (currently calls full sync) (lines 340-346).
  • Added mapUserAttributes helper method (lines 348-353).
• src/main/java/org/opensingular/dbuserprovider/DBUserStorageProviderFactory.java
  • Changed create method signature to accept UserStorageProviderModel and added overloaded method for ComponentModel (lines 50-54).
  • Changed configure method signature to accept UserStorageProviderModel (line 59).
  • Added syncEnabled and listAllForSync parameters to QueryConfigurations constructor call (lines 80-81).
  • Changed validateConfiguration method signature to accept UserStorageProviderModel and added overloaded method for ComponentModel (lines 88-91).
  • Updated getConfigProperties to add syncEnabled and listAllForSync properties and refined help texts/default queries (lines 109-229).
• src/main/java/org/opensingular/dbuserprovider/DBUserStorageResource.java
  • Added new file implementing RealmResourceProvider (lines 1-57).
  • Added /sync POST endpoint to trigger user synchronization (lines 40-56).
• src/main/java/org/opensingular/dbuserprovider/DBUserStorageResourceProviderFactory.java
  • Added new file implementing RealmResourceProviderFactory and registering the new resource factory (lines 1-40).
• src/main/java/org/opensingular/dbuserprovider/model/QueryConfigurations.java
  • Added syncEnabled and listAllForSync fields (lines 19-20).
  • Updated constructor to include new fields (line 22).
  • Added getter methods for syncEnabled and listAllForSync (lines 95-100).
• src/main/java/org/opensingular/dbuserprovider/persistence/UserRepository.java
  • Added getAllUsersForSync method to execute the sync query (lines 210-213).
• src/main/resources/META-INF/services/org.keycloak.services.resource.RealmResourceProviderFactory
  • Added new file to register DBUserStorageResourceProviderFactory.
• src/test/java/org/opensingular/dbuserprovider/DBUserStorageProviderTest.java
  • Added new file with JUnit tests for DBUserStorageProvider sync functionality (lines 1-125).
• src/test/java/org/opensingular/dbuserprovider/DBUserStorageResourceTest.java
  • Added new file with JUnit tests for DBUserStorageResource sync endpoint (lines 1-104).
• src/test/java/org/opensingular/dbuserprovider/mocks/MockDataSourceProvider.java
  • Added new file providing a mock DataSource implementation for testing (lines 1-71).
• src/test/java/org/opensingular/dbuserprovider/util/TestUtils.java
  • Added new file with basic Mockito utility methods (lines 1-15).
• src/test/resources/logging.properties
  • Added new file to configure logging for tests (lines 1-5).
• src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
  • Added new file to configure Mockito for inline mocking (lines 1-1).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

Database users, synced with care,
From SQL tables, brought to share.
Keycloak knows them, one and all,
Ready to answer freedom's call.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a user synchronization feature, allowing users from an external database to be synced into Keycloak. It also includes a JAX-RS endpoint to trigger this synchronization, updates to Java 17 and Keycloak 25, and a dev container setup. These are significant and valuable enhancements.

The overall structure is good, and the addition of tests is appreciated. However, there are a few critical and high-severity issues, particularly in the new synchronization logic and the JAX-RS resource setup, that need to be addressed before this can be merged. I've detailed these in the comments below.

It would also be beneficial to update the pull request description with more details about the changes and their purpose.

Summary of Findings

• Critical: User Creation in Sync Logic: The DBUserStorageProvider.sync() method uses this.addUser() to create new users in Keycloak. Since this.addUser() always returns null, new users from the database will not be created, and a NullPointerException will occur when trying to set the federation link.
• Critical: NullPointerException in JAX-RS Resource: The DBUserStorageResource is initialized with a null model by its factory, which will cause a NullPointerException when the /sync endpoint is called and tries to access model.getProviderId().
• High: Potential SQL Error in Sync Query: If the listAllForSync query is not configured (defaults to an empty string), UserRepository.getAllUsersForSync() will attempt to execute an empty SQL query, likely leading to an SQLException.
• Medium: Redundant pom.xml File: A new file named pom.xml (update with Mockito dependencies) has been added. This file appears to be redundant as Mockito dependencies are already managed in the main pom.xml.
• Medium: Incorrect Mock in Test: DBUserStorageProviderTest.testSync() mocks userRepository.getAllUsers() instead of the actually called userRepository.getAllUsersForSync().
• Java and Keycloak Version Upgrades: The project has been upgraded to Java 17 and Keycloak 25.0.0, which are positive updates.
• Dev Container Setup: The addition of a dev container configuration (.devcontainer/Dockerfile and .devcontainer/devcontainer.json) is a good step for improving the development environment consistency.

Merge Readiness

This pull request introduces valuable synchronization functionality and important version upgrades. However, due to the identified critical and high severity issues, particularly concerning the core sync logic and the JAX-RS endpoint, I recommend that these issues be addressed before merging. Once these are resolved, the PR will be in much better shape. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from team members.

[gemini-code-assist]

The sync method attempts to add new users to Keycloak using keycloakUser = this.addUser(realm, username);. However, the addUser method in this class (lines 255-258) is implemented to always return null:

// src/main/java/org/opensingular/dbuserprovider/DBUserStorageProvider.java:255-258
@Override
public UserModel addUser(RealmModel realm, String username) {
    // from documentation: "If your provider has a configuration switch to turn off adding a user, returning null from this method will skip the provider and call the next one."
    return null;
}

This means that if keycloakUser is null at line 313, the subsequent keycloakUser.setFederationLink(model.getId()); on line 318 will result in a NullPointerException, and new users from the database will not be correctly created and linked in Keycloak.

To fix this, you should use Keycloak's UserProvider to create the user, for example:

keycloakUser = session.users().addUser(realm, username);
// After this, you can set the federation link and map attributes.

Could you review this logic to ensure new users are correctly provisioned in Keycloak during synchronization?

[gemini-code-assist]

The getAllUsersForSync method uses queryConfigurations.getListAllForSync() to fetch users. The default value for the listAllForSync configuration property is an empty string (see DBUserStorageProviderFactory.java:184).

If this property is not explicitly configured by the administrator, an empty string will be passed as the SQL query to doQuery. This will likely result in an SQLException when the database attempts to execute an empty query.

Consider adding a check for an empty or null listAllForSync query. If it's not configured, should it perhaps fall back to queryConfigurations.getListAll() or log a clear error message and return an empty list if synchronization is enabled but the required query is missing?

[Copilot]

Pull Request Overview

Adds full user synchronization support to the DB user storage provider, including a new REST endpoint, configuration flags, and test coverage.

• Introduces syncEnabled and listAllForSync in QueryConfigurations and UserRepository.getAllUsersForSync().
• Implements sync and syncSince in DBUserStorageProvider and exposes a /sync endpoint in DBUserStorageResource.
• Updates factories to pass new sync settings and adds unit tests for the sync flows.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/main/java/org/opensingular/dbuserprovider/model/QueryConfigurations.java	Added syncEnabled and listAllForSync fields and accessors
src/main/java/org/opensingular/dbuserprovider/persistence/UserRepository.java	Added getAllUsersForSync() for full user list
src/main/java/org/opensingular/dbuserprovider/DBUserStorageProvider.java	Implemented sync, syncSince, and user unlinking logic
src/main/java/org/opensingular/dbuserprovider/DBUserStorageResource.java	Added POST /sync endpoint for triggering import sync
src/main/java/org/opensingular/dbuserprovider/DBUserStorageProviderFactory.java	Updated factory to configure new sync flags
src/main/java/org/opensingular/dbuserprovider/DBUserStorageResourceProviderFactory.java	Registered the new resource provider
src/test/java/org/opensingular/dbuserprovider/DBUserStorageProviderTest.java	Added tests for full sync and disabled-sync scenarios
src/test/java/org/opensingular/dbuserprovider/DBUserStorageResourceTest.java	Added tests for the REST sync endpoint
src/test/java/org/opensingular/dbuserprovider/util/TestUtils.java	Introduced Mockito helper methods
pom.xml	Added Mockito dependencies and Surefire plugin for logging config
src/test/resources/logging.properties	Configured console logging for tests

Comments suppressed due to low confidence (3)

src/main/java/org/opensingular/dbuserprovider/DBUserStorageProvider.java:341

• The new syncSince method isn’t covered by any unit tests. Consider adding tests to verify its behavior (especially that it delegates correctly to sync).

public SynchronizationResult syncSince(Date lastSync, KeycloakSessionFactory sessionFactory, String realmId, UserStorageProviderModel model) {

src/main/java/org/opensingular/dbuserprovider/DBUserStorageProviderFactory.java:38

• The DBUserStorageProviderFactory class invokes log.info(...) without defining or generating a log field. Add a logger annotation (e.g., @JBossLog) or declare a logger instance to avoid a compilation error.

log.info("Initializing DBUserStorageProviderFactory");

src/main/java/org/opensingular/dbuserprovider/persistence/UserRepository.java:211

• The call to log.infov(...) assumes a log field exists. Ensure the class defines a logger (e.g., via @JBossLog or a manual logger declaration) before using log.

log.infov("Getting all users for sync using query: {0}", queryConfigurations.getListAllForSync());

[Copilot]

Pull Request Overview

This PR introduces user synchronization capabilities into the DB user storage provider and enhances the associated configuration and testing.

• Added sync methods to DBUserStorageProvider implementing ImportSynchronization.
• Updated provider factory and configuration to support new sync parameters (syncEnabled, listAllForSync).
• Extended tests and documentation to cover synchronization scenarios.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker	Added Mockito inline mock configuration.
src/test/resources/logging.properties	Configured logging levels for tests.
src/test/java/org/opensingular/dbuserprovider/util/TestUtils.java	Introduced simple Mockito test utilities.
src/test/java/org/opensingular/dbuserprovider/mocks/MockDataSourceProvider.java	Added a mock data source provider for testing.
src/test/java/org/opensingular/dbuserprovider/DBUserStorageResourceTest.java	Added tests for sync endpoints and error scenarios.
src/test/java/org/opensingular/dbuserprovider/DBUserStorageProviderTest.java	Expanded tests to cover sync, credential validation, and unlinking.
src/main/resources/META-INF/services/org.keycloak.services.resource.RealmResourceProviderFactory	Registered resource provider factory.
src/main/java/org/opensingular/dbuserprovider/DBUserStorageResource.java	Implemented REST endpoints for synchronization.
src/main/java/org/opensingular/dbuserprovider/DBUserStorageProviderFactory.java	Extended factory to support sync configurations.
src/main/java/org/opensingular/dbuserprovider/DBUserStorageProvider.java	Extended provider to implement ImportSynchronization and added sync logic.
pom.xml, CLAUDE.md, Devcontainer files	Updated dependencies and documentation to support new features.

Comments suppressed due to low confidence (1)

src/main/java/org/opensingular/dbuserprovider/DBUserStorageProviderFactory.java:179

• The 'listAllForSync' SQL query property currently defaults to an empty string. Consider providing a meaningful default query or adding documentation indicating that it must be explicitly set when synchronization is enabled.

.property().name("listAllForSync")

[Copilot]

[nitpick] If the external database can contain a large number of users, consider processing synchronization in batches or implementing parallel processing to improve performance.

Suggested change

	for (Map<String, String> dbUserMap : usersFromDb) {
	try {
	String username = dbUserMap.get("username");
	if (username == null || username.trim().isEmpty()) {
	log.warnv("User from DB is missing or empty username: {0}", dbUserMap);
	result.increaseFailed();
	continue;
	}
	UserModel keycloakUser = syncSession.users().getUserByUsername(realm, username);
	if (keycloakUser == null) {
	List<List<Map<String, String>>> batches = PagingUtil.createBatches(usersFromDb, BATCH_SIZE);
	for (List<Map<String, String>> batch : batches) {
	batch.parallelStream().forEach(dbUserMap -> {
	try {
	String username = dbUserMap.get("username");
	if (username == null || username.trim().isEmpty()) {
	log.warnv("User from DB is missing or empty username: {0}", dbUserMap);
	result.increaseFailed();
	return;
	}
	UserModel keycloakUser = syncSession.users().getUserByUsername(realm, username);
	if (keycloakUser == null) {