opensingular / singular-keycloak-database-federation

Keycloak User Storage SPI for Relational Databases (Keycloak User Federation, supports postgresql, mysql, oracle and mysql)
Apache License 2.0
120 stars 57 forks source link

Database configs in multiple realms assume the last one saved. #7

Closed ktivdv closed 2 years ago

ktivdv commented 2 years ago

Hello. I'm using MSSQL configurations in multiple realms. Whenever I save one configuration (i.e. change the destination table), it changes it in all realms so that the users are the same in each realm. Expected behaviour was that unique connections using this federation from different realms would show unique sets of users from different tables.

ktivdv commented 2 years ago

I've confirmed that it's the case with different database types. For example, I have two realms. In one realm, I have a PostgreSQL connection and it shows those users after saving the connection. In another realm, I have an MSSQL connection. It shows the PostgreSQL users. If I open and save that (MSSQL) configuration, it shows the MSSQL users. I go back to the other realm with the PG config, and it's showing the MSSQL users.

viniciusuriel commented 2 years ago

Hi there, I can't dive into this right now, this fix could take some time to delivered. If you have some insight about how to fix this issue feel free to share it here or to submit a pull request it will certainly speed up things to me.

ktivdv commented 2 years ago

Unfortunately, I don't have any insight. I'm not even sure where the user federation configurations are stored. We don't have the expertise / resources to contribute to this, I'm afraid. But I'd be happy to contribute somehow, if possible.

mrfaco commented 2 years ago

https://github.com/opensingular/singular-keycloak-database-federation/blob/main/src/main/java/org/opensingular/dbuserprovider/DBUserStorageProviderFactory.java#L47

This line is causing the issue. If you remove the if statement, the problem is solved.

dla-c-box commented 2 years ago

If I understand correctly, removing that if statement would have the side-effect of forcing a re-connection to the DB every time a user logs in, which may be costly for performance (but if that's the only way to have it working multi-realms, that may be enough for some users for now). Otherwise we probably need a way to create the DB connection, if it doesn't exist, in method "onCreate" (which receives the realm as a parameter) and store it in a Set keyed by realm (rather than in the method "create", where it currently doesn't receive the realm, for some reason).

dla-c-box commented 2 years ago

Now that I have a second look at it, it seems that ComponentModel model in method public DBUserStorageProvider create has access to a unique ID for the instance of the plugin ( model.getId() ). If we store the parameters (including DB connection) per id, we may be able to not only configure it by realm, but to even have multiple db-user-provider for each Realm if we want to (i.e. connect to different DBs for the same realm).

viniciusuriel commented 2 years ago

https://github.com/opensingular/singular-keycloak-database-federation/blob/main/src/main/java/org/opensingular/dbuserprovider/DBUserStorageProviderFactory.java#L47

This line is causing the issue. If you remove the if statement, the problem is solved.

This solution should not be used, it can led to uncontrolled connection leak,

viniciusuriel commented 2 years ago

After comments from @dla-c-box, now I , finally, understand the issue reported by @ktivdv. I have made a solution so that the DBUserStorageProviderFactory can keep DataSourceProvider and QueryConfigurations instances per realm mapping it by ComponentModel id as highlighted by @dla-c-box .