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

New parameter to allow query attributes to always overwrite Keycloak, even when user is cached #18

Closed dla-c-box closed 2 years ago

dla-c-box commented 2 years ago

Adds the following parameter (which is OFF by default, to keep the previous behaviour). image

By default, once a user is loaded in Keycloak, its attributes (e.g. 'email') stay as they are in Keycloak even if an attribute of the same name now returns a different value through the query. [ Technically public UserAdapter aggregates both the existing Keycloak version and the DB version of an attribute in a Set, but since e.g. email is not a list of values on the Keycloak User, the new email (2nd entry in the Set) is never assigned to the user. (not sure this is a good default behaviour, I guess it should either be the value from the DB or the value from Keycloak, not a set of both, but I didn't touch the default behaviour: I let it as is to make sure I don't break someone else's special case. ]

This new option allows to have all the attributes that are set in the SQL query to always overwrite the existing user attributes in Keycloak (e.g. if Keycloak user has email 'test@test.com' but the query fetches a field named 'email' that has a value 'example@exemple.com', the Keycloak user will now have email attribute = 'example@exemple.com' instead of keeping 'test@test.com').

This new PR also works when the Cache Settings is not "NO_CACHE", which allows e.g. to have a working "Max Login Failures": image

dla-c-box commented 2 years ago

For the record, the previous try at this (which only worked with NO_CACHE) was this PR: https://github.com/opensingular/singular-keycloak-database-federation/pull/15

viniciusuriel commented 2 years ago

Hello there, I reviewed your pull request and I feel that the rationale in the "isValid" method is not clearly stated in the configuration help, and as you also wrote in the comments, it probably should be further parametrized in future. That said, I think it is of great help to make this provider more configurable, thank you once more.

dla-c-box commented 2 years ago

Thanks. Indeed, the fields that can trigger the cache-busting (currently only 'email' and 'username') could be further parametrized in the future. Currently, all the other selected query fields (this could also be configurable) will still overwrite the Keycloak-record version of those same fields when the new parameter is ON, but these other new values will be "effective/visible" only once the cache of the given user expires.