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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

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).

On 1 December 2017 at 14:53, Bill Burke <bburke(a)redhat.com&gt; wrote: > > On Wed, Nov 29, 2017 at 9:09 AM, Marek Posolda <mposolda(a)redhat.com&gt; > 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). > > > > the "state" parameter IS required. Its how the client can figure out > that it initiated the login or not. https://tools.ietf.org/html/rfc6749#section-4.1.1 It's RECOMMENDED

> > > I don't understand your solution...BTW, going back to auth_session_id > within the URL instead of a cookie like we used to do would fix this > too :). If you're already going to add a "client-id" query parameter, > why not just revert back to the old way of doing this? Cookie solves a lot of issues. Can't remember the details of all of them, but these have been discussed several times on the mailing list this year.

On 11/12/17 15:54, Bill Burke wrote: > > On Mon, Dec 4, 2017 at 2:56 AM, Stian Thorgersen <sthorger(a)redhat.com&gt; > wrote: >> >> >> On 1 December 2017 at 14:53, Bill Burke <bburke(a)redhat.com&gt; wrote: >>> >>> On Wed, Nov 29, 2017 at 9:09 AM, Marek Posolda <mposolda(a)redhat.com&gt; >>> 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). >>>> >>> the "state" parameter IS required. Its how the client can figure out >>> that it initiated the login or not. >> >> >> https://tools.ietf.org/html/rfc6749#section-4.1.1 >> >> It's RECOMMENDED >> > You don't remove a recommended aspect of a protocol just because its > inconvenient to you. AFAIK Keycloak needs to integrate with 3rd party OIDC adapters and SAML adapters. For our own adapters, we control everything, so we can make sure that "state" parameter is always send by the adapter. But for 3rd party adapters, we can't rely on the fact that "state" parameter is available in initial OIDC AuthorizationEndpoint request due the fact that it's just RECOMMENDED. So we can't reliably use "state" as the identificator of the authenticationSession tab_id (or anything else on the server) as it would be null in some cases. So we rather generate some other random ID to reference authSession. So this is not about removing any aspects of the protocol, but rather the opposite - make sure that things work regardless if "state" is or isn't available. Or did I just misunderstood what you meant?

On Wed, Nov 29, 2017 at 9:09 AM, Marek Posolda <mposolda(a)redhat.com&gt; 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). > the "state" parameter IS required. Its how the client can figure out that it initiated the login or not. I don't understand your solution...BTW, going back to auth_session_id within the URL instead of a cookie like we used to do would fix this too :). If you're already going to add a "client-id" query parameter, why not just revert back to the old way of doing this?

-- Bill Burke Red Hat

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(a)redhat.com&gt; 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(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev >

I feel a bit better that state is no longer shared between tabs and you're cleaning up auth sessions. Still worried though as I still don't fully understand how it works. I also worry that a lot of different areas of the codebase needed to be touched to fix these problems. This probably needs another refactoring pass to consolidate code and create a better abstraction so that if we need to change things its done in one place. I know I'm being vague here, by my intuition and experience is telling me things need to be cleaned up....Unfortunately there's a lot of areas in our codebase like this (many which originated from me), and we're overloaded with work. Hopefully one day we get back to this.

On Mon, Dec 11, 2017 at 7:27 AM, Marek Posolda <mposolda(a)redhat.com&gt; 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(a)redhat.com >>> <mailto:mposolda@redhat.com>> wrote: >>> >>> I am looking at https://issues.jboss.org/browse/KEYCLOAK-5797 >>> <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 >>> <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(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org> >>> https://lists.jboss.org/mailman/listinfo/keycloak-dev >>> <https://lists.jboss.org/mailman/listinfo/keycloak-dev> >>> >>> > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev