[keycloak-dev] Better support authenticationSessions in multiple browser tabs

[Stian Thorgersen]

Looks good to me

On 11 December 2017 at 13:27, Marek Posolda <mposolda at redhat.com> wrote:

> So I've sent PR https://github.com/keycloak/keycloak/pull/4832 for this.
> Now there is support of authenticationSessions in multiple browser tabs.
> Summary of the changes in PR:
>
> - It adds the new parameter "tab_id" to the endpoints Authentication Flow
> endpoints (especially those in LoginActionsService and few in
> IdentityBrokerService).
>
> - AuthenticationSessionModel needs to be looked up by the ID from cookie
> (AUTH_SESSION_ID) together with tab_id and client.
>
> - When application redirects to Keycloak login screen through sending
> request to the OIDC AuthorizationEndpoint or corresponding SAML or
> DockerProtocol authentication endpoint (opening authz endpoint in new
> browser tab), the new Authentication session is always created with new
> tab_id. There is no join existing AuthenticationSessionModel. The cookie
> AUTH_SESSION_ID is still the same. At infinispan level, there is
> RootAuthenticationSessionModel, which represents whole browser and it
> contains list of AuthenticationSessionModel, where every
> authenticationSessionModel represents browser tab.
>
> - All the state of authentication (executions, authNotes, clientNotes) is
> still tracked in AuthenticationSessionModel. So the state is not shared
> among browser tabs.
>
> - The whole RootAuthenticationSession is cleared after login and also
> after logout (In case of logout, there is "helper" AuthenticationSession
> used just during logout to track the logout state of clients among multiple
> redirects from frontchannel-logout clients). The cookie AUTH_SESSION_ID is
> still kept, so sticky session is still preserved and the UserSessionModel
> created after authentication should be available locally during SSO logins
> (or also during backchannel requests from javascript clients as those
> participate in sticky session too).
>
> - The PR fixes those JIRAs and adds automated tests for them:
> https://issues.jboss.org/browse/KEYCLOAK-5938 - Support
> authenticationSession for login same client in multiple browser tabs
> https://issues.jboss.org/browse/KEYCLOAK-5936 - Use of Back button on
> "Create user from social provider account" leads to Exception
> https://issues.jboss.org/browse/KEYCLOAK-5466 - X.509 Auth - cannot login
> after an error
> https://issues.jboss.org/browse/KEYCLOAK-6001 - WARNING in the log when
> logout from admin console
> https://issues.jboss.org/browse/KEYCLOAK-6012 - Refresh after setting
> VERIFY_ACTION leads to Already authenticated . Thanks Hynek for adding
> automated tests!
>
> - It should also fix:
> https://issues.jboss.org/browse/KEYCLOAK-5982 - NPE when logout from
> wordpress OneLogin SAML SSO
> https://issues.jboss.org/browse/KEYCLOAK-5981 - Impersonate function not
> working after logout
>
> but need to add the automated tests for those. I hope to send separate PRs
> for it.
>
> WDYT?
>
> Marek
>
>
> On 29/11/17 15:09, Marek Posolda wrote:
>
> On 29/11/17 14:44, Stian Thorgersen wrote:
>
> I would target this to 3.4.2. I don't want to delay the 3.4.1 release if
> we can help it.
>
> I'd also suggest some (short if possible) random key (or a counter?!)
> rather than relying on protocol specific values. 'state' is not actually
> required in OAuth right? It's just recommended.
>
> Yes, it's not required. And same for SAML. Was wondering about the same.
> Will use the random key or counter. Thinking if counter doesn't have some
> corner case issues (EG. If 2 tabs are opened concurrently after logout and
> will try to use same counter value as authSession update from tab2 won't be
> yet visible in tab1).
>
> Marek
>
>
> On 29 November 2017 at 13:00, Marek Posolda <mposolda at redhat.com> wrote:
>
>> I am looking at https://issues.jboss.org/browse/KEYCLOAK-5797 issue now.
>> It's about the fact, that when user has opened browser with multiple
>> browser tabs, then after successful login in tab1 (clientA), he may be
>> redirected to the incorrect client (clientB, which was opened in tab2).
>>
>> The thing is, that authenticationSession was tracked by the cookie and
>> didn't support multiple clients. So both browser tabs "tab1" and "tab2"
>> used same authenticationSession, which can reference just one of the
>> clients, hence there could be the conflict and one of the tabs
>> redirected to incorrect client after authentication.
>>
>> I am working on a fix, that allow better support for multiple clients.
>> What I am doing is, that there is "RootAuthenticationSessionModel",
>> which is now referenced by the ID from the cookie. That root session can
>> reference more actual authentication sessions through the map like
>> "Map<String, AuthenticationSessionModel>" . The key is the client UUID.
>> In the Authentication SPI, there are no changes. Those still use the
>> AuthenticationSessionModel as before.
>>
>> This is easily possible as we have "client-id" parameter already
>> available during authentication flows in every tab. So every browser tab
>> can reference correct client and redirect to it.
>>
>> However even with the fix, there is another corner case about support
>> for multiple browser tabs with *same* client. This will be still an
>> issue, especially for the javascript clients. I've created another JIRA
>> https://issues.jboss.org/browse/KEYCLOAK-5938 with the example issue,
>> which can be simulated with our admin console. To properly support
>> multiple tabs of same client, the key will need to be like: "client-id +
>> state" instead of just "client-id"  The "state" will need to be either
>> the OIDC/SAML state or some randomly generated string (As in both the
>> OAuth2 and SAML, the state/relayState parameter is not mandatory AFAIK),
>> which will need to be added to the URL during authentication flows as
>> well.
>>
>> Fixing this will take me another few days of work (maybe 2?) as there
>> will need to be change in many files for adding the new parameter + some
>> more authenticationSession model changes etc. So I wonder if we want to:
>> 1) Fix this in 3.4.1 . Will likely mean to delay the release?
>> 2) Fix this in 3.4.2. It will affect many files and there is some chance
>> of regression (hopefully not big as we have lots of the tests for
>> various other corner cases)
>> 3) Fix this later in 4.X .
>>
>> My vote is 1 or 2. WDYT? Any other possibility?
>>
>> Marek
>>
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>
>
>
>