Repository navigation
Support for transient users for brokered accounts - #23977
Conversation
2 flaky tests on run #9554 ↗︎
Details:
|
|||||||||||||||
There was a problem hiding this comment.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the below 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.ui.account2.LinkedAccountsTest#unlinkAccountTestKeycloak CI - Account Console IT (firefox) org.keycloak.testsuite.ui.account2.WelcomeScreenTest#applicationsTestKeycloak CI - Account Console IT (firefox) org.keycloak.testsuite.adapter.example.cors.CorsExampleAdapterTest#angularCorsProductTest |
c2ea1d5 to
7c97f1d
Compare
There was a problem hiding this comment.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the below 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.ui.account2.LinkedAccountsTest#unlinkAccountTestKeycloak CI - Account Console IT (firefox) org.keycloak.testsuite.ui.account2.WelcomeScreenTest#applicationsTestKeycloak CI - Account Console IT (firefox) org.keycloak.testsuite.adapter.example.cors.CorsExampleAdapterTest#angularCorsProductTest |
There was a problem hiding this comment.
Changes to the front-end look fine to me.
7c97f1d to
d28814c
Compare
|
Rebased to include fix for #23965 |
There was a problem hiding this comment.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the below 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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststoreKeycloak CI - FIPS IT (non-strict) |
5e1352c to
7684e37
Compare
|
Is this supposed to work with the offline_access / OfflineSession feature? |
|
@hmlnarik Thanks for the PR! It looks great! Few questions:
IMO if we have flag (1) and PREVIEW feature, we don't need 2,3,4 in this PR, but can be done in follow-up PRs (if we need 3 at all, depends on the feedback...) as we have more "freedom" to change the behaviour and/or fix possible bugs and provide docs afterwards in the follow-up PRs if needed. |
@thomasdarimont This is a good question. It should (as also @mposolda replied) and I've added a test for it based on
I've added
It is. I have currently only added the offline_access test since that one is most cumbersome, and will work on the others later. Could you please check if they are sufficient?
I would not mind either way, but I need to check if this is possible. Not sure especially in case of consents (case where the user authenticates to two applications but additional consent is required for the second one). Also when the user in the user session is writable, it is possible for the admin to tune the roles by assigning the roles to the user from admin console directly instead of doing that via mappers and logout-login trial rounds which may lead to more convenient user (admin) experience. |
@hmlnarik Thanks! I would like to approve your PR once tests are OK (Updates are needed in quarkus integration tests always when new feature is added. You can check recent PR from @thomasdarimont on what needs update when new feature is added #23892 )
That works for me, Thanks!
Good points. Regarding consents, it looks that they may still work even for read-only users? The methods for CRUD consents are not part of the |
8b063e5 to
d46ea90
Compare
Unreported flaky test detectedIf the below 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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststoreKeycloak CI - FIPS IT (strict) org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFileKeycloak CI - FIPS IT (strict) |
There was a problem hiding this comment.
Unreported flaky test detected, please review
There was a problem hiding this comment.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the below 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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststoreKeycloak CI - FIPS IT (strict) org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFileKeycloak CI - FIPS IT (strict) |
|
Thanks for your clarifications @hmlnarik! Two topics come to my mind when I think this feature.
Stable user identifierWhen the user information is only transient, then it is IMHO cruicial to ensure that the generated user identifier is stable, since it is exposed to applications. Applications might then rely on the user identifier (userUuid) to store service related information. Is it feasible to generate a stable user identifier based on the information provided by the IdP or are we depedent on the IdP to generate a stable identifier for the user? Memory ConsumptionHowever, I think this will impact memory consumption in usage scenarios with many offline sessions, which might be valid for quite a long time (multiple days, weeks, months...). For reference: currently an offline user session is stored like this in the database - a representation of this is stored in infinispan. IMHO, This encoding is wasteful as the information is rarely used and could be encoded more efficiently. I think there is room for optimization here. :) From your PR I see that you use the offline session entity to store the "transient" user data as a session note here: https://github.com/keycloak/keycloak/pull/23977/files#diff-1445188baf4e2f8d90de96fc76d0423dd86425d2a0a9ede5c3e20d88c3f85077R762 I was wondering whether this will increase memory consumption even more, e.g. if this is used in large real world CIAM use cases. For completeness: an offline client session from the database: These changes might look tiny, but optimizations like this can have a noticeable impact on large systems. |
|
@thomasdarimont Good points! IMO the stable user identifier might be useful for most of the cases. If we can use ID relying on "ID from IDP" instead of using broker session ID (which is not stable as it would differ between logins), we can ensure stable ID IMO? One advantage can be application compatibility, but also when returning back to consents: Stable ID will allow to use persistent consents as even if the user himself is not saved in the Keycloak DB, the consents can be persisted and user won't need to confirm consent at every login as long as IDP ID is stable and consents are persisted in Keycloak DB. Here I am not 100% sure if consents should be persistent or transient by default. Looks rather transient as I suppose lots of people, who want "Transient users" feature, usually want this due to very rare login of their users? Also not 100% sure if there is foreign key from "consents" table to the USER_ENTITY table (or other tables), which would prevent to save consent for the user, which is not persisted anywhere in Keycloak DB, but I guess this can be solved if needed... Regarding memory: It looks that we have lots of space for improvements in general. Some notes are not relevant for offline sessions at all. Also the "keys" can be shortened to avoid repeating and/or we can maybe come with more memory friendly format than using JSON for the "data". Maybe we can come with some SPI, which will allow flexibility for what should go to offline sessions? Like "Offline session mapper SPI" or something like that? IMO both those points can be addressed as follow-ups. Especially "offline session tuning" is separate topic not directly related to this one though as it is already a problem (without this PR being merged). I would just add a note (or warning) to the documentation that using "transient users" can increase memory consumption. And especially for offline sessions, people might be encouraged to consider whether to use this or rather use "persistent" users if they need offline sessions. |
|
Thanks for your thoughts, Marek! I wonder if it would make sense to store brokered users as dedicated entities still, e.g. when the scope offline_access is used - instead of just storing the user data as a session note (SESSION_NOTE_LIGHTWEIGHT_USER). Having dedicated user entities allows the use of existing structures, store consents, and more straightforward queries. When persisted in the database, the user session notes SESSION_NOTE_LIGHTWEIGHT_USER is another form to make the user persistent - but it's much harder to use (queries, consents, etc.). Regarding the Offline-Session Optimization: I don't think that users should need an "Offline session mapper SPI," but for internal purposes, it would open up additional optimizations like using different encoding for the data. The SPI should allow to read/write the data blob of an offline session. |
|
@thomasdarimont Thank you for the detailed concerns.
|
|
Thanks for your comments @hmlnarik! Perhaps we could use the discussion here: #24263 to collect ideas for optimizing the offline session feature. I agree that the offline session limitations for transient users must be clearly documented. |
|
I believe that there are the following use cases, each covering a differrent feature:
The difference between Options 1,2 and 3,4 is whether the user owns the decision when their data is removed from Keycloak. This PR adds capability to do so, supporting cases 3 and 4. Technically it uses purely the sessions to make it straightforward to implement and assure correct behaviour. For this reason I would prefer storing consents (in a follow-up PR) in the session data only, similarly to the user record. Related to Thomas's comment: it can use custom session notes storage in the future, the rule would be to keep it local to the session.
The aim of the transient users feature is to leave any trace of the user or information correlable with a user out of Keycloak database. (This relates e.g. to storage limitation as specified in GDPR Art. 5.) Keycloak as a system cannot determine what the appropriate storage period is however an admin can. If the admin determines that the users should be kept in Keycloak as transient, they may knowingly disable or enable offline access and I believe it is correct to assume that Keycloak does not accidentally store any such information which would lead to personally-identifiable data. Keeping consents of transient users in the database with a identifiable stable identifier would leave a trace of the user there which can lead to identification of the user: consents relate to (group of) applications which may be used by a specific (group of) users. This seems to me to go against the original intention of keeping no direct or indirect information about the user stored after the user vanishes from the system. Persistent consents would only be possible for persistent users. I will copy / summarize this discussion into an follow-up issue for consents. |
|
@hmlnarik +1 for having "transient consents for transient users" as the default user and priority. We can see later based on the feedback if we ever need also "persistent consents for transient users". |
This PRototype adds capability of not importing brokered users into keycloak database, also known as transient users for brokering feature.
The core of the PR is that the user data is stored as a part of an authentication session (modifiable) and once authentication is done, it is stored in user session.
To do so, lightweight user model based on existing
InMemoryUserAdapterclass is introduced, and is de/serialized into a auth / user session attribute when needed.An option is added in identity provider configuration to not store user into database. This option is disabled by default, keeping the behaviour backward compatible. When enabled, instead of creating a user in user database, a read/write lightweight user is created and its representation stored in authentication session. Once authentication finishes and user session is created, the user is stored as EDIT: read-write END object there.
This has several implications:
Importsync mode since it always treats the user as new. Hence sync mode settings is hidden in admin console identity provider screen when transient users are enabledThis PR is split into multiple commits:
InMemoryUserAdapterso that it is visible both for services and model modulesInMemoryUserAdapterDo not store userssettings, specifically instead of creating records in database, it creates lightweight users insteadFor this to become a full-blown PR, the approach needs to be confirmed and remaining tasks finished:
cc: @keycloak/core
Closes: #11334