Authentication session improvements (You are already logged-in) - #16
Authentication session improvements (You are already logged-in)#16mposolda wants to merge 3 commits into
Conversation
|
|
||
| * User will be authenticated even if he provides invalid username/password or even he clicks "Submit" without providing any username/password. As username/password verification didn't really happen in tab2, even if it looks to user as it happened. IMO this is not major issue and same behaviour can be seen by Twitter for example. | ||
|
|
||
| * After confirm username/password screen, user won't see TOTP (or any other 2 factor authenticators) even if he potentially has 2 factor enabled. He will be directly redirected to the application. I am not sure about usability of this and if it's confusing to users or if it's ok. |
There was a problem hiding this comment.
IMO, this is ok, since the user had to enter 2FA already and they know about it. Hence interrupting the flow is IMO the correct behaviour.
There was a problem hiding this comment.
Yes, it is the correct behaviour. My little fear is just about usability. In theory, user may forget that he already filled TOTP an hour ago and now after fill username/password form, he may think like "Hey, where is my TOTP screen" .
But hopefully it is ok. We may see if users will report more JIRAs around usability. If not, we may not need to implement "advanced" approach at all :)
| * Send the browser "refresh" request | ||
|
|
||
| * Authentication session will be shared per whole browser (with some exceptions described below in section [different flows in same browser](#different-flows-in-same-browser). | ||
| So on KC side, we will check the latest state from AuthenticationSession and update the page accordingly including URLs. This will allow all browser tabs will be updated to TOTP after confirm username/password in tab1 |
There was a problem hiding this comment.
Sadly, this suffers similar issue as described below with to custom authenticators when the user gets to a brokered IdP in several tabs concurrently and spends some time there, then returns back to keycloak in one tab but remains logging-in in the other tabs. Then the flow would only be updated in the first tab and not the other ones.
Further, would this become an issue when using different flows for individual clients?
Can we keep the refreshes manual? I think the design would remain just the same. Javascript would be convenient but I think it is an optimization that can be done after we actually solve the primary problem of displaying the message of being logged in.
(Furthermore, this would mean peaks in number of requests when e.g. work shift starts and a users start logging in into their applications, and I'm not convinced this is really necessary.)
There was a problem hiding this comment.
I agree that first priority is to get rid of "You are already logged-in" . Also agree with the point of more uneccessary browser requests. However I think that manual refreshes is not very good. That would actually mean that user will in most cases still see the username/password screen and fill the username/password as most of users don't do browser refresh.
So maybe the JS can refresh the page in case that particular browser tab is clicked and gains focus? We can always refresh the page when gaining focus, which means that we won't need the timestamp cookie at all then.
The Idp flow or client-specific flow shouldn't be an issue. As mentioned below, there will be still more authentication sessions in the rootAuthSession to handle those cases. Example:
- There are 3 browser tabs. Tabs 1 and 2 are using flow1 (default realm flow). Tab 3 is using some client specific flow - flow2.
- User fills username/password on tab 1. He is moved to step 2 (TOTP screen)
- User clicks on tab 2. JS will refresh the page and user will be moved to TOTP screen
- User clicks on tab 3. JS will refresh the page, but in the flow 2 (client specific flow), user didn't anything yet. So refresh will just keep user at step 1 of the flow 2.
There was a problem hiding this comment.
So maybe the JS can refresh the page in case that particular browser tab is clicked and gains focus? We can always refresh the page when gaining focus, which means that we won't need the timestamp cookie at all then.
+1 to this approach. When the user clicks the tab, we only need to put some notification to the user that the page is being refreshed so that they don't fill and submit a form in the meantime (could be just a shading full screen div or whatever)
There was a problem hiding this comment.
I've changed the design per this. Changes are in the last commit: 931c234
| some side-effects. For example unecessarily creating the HTTP session for the application where user was redirected in other browser tabs. | ||
| So in other browser tabs, we may display some "info" stating like "Authentication was completed. Redirecting to the | ||
| application in few seconds". There will be also some additional JS code, which will be triggered when user enters the particular browser tab. This JS | ||
| will refresh the page and will automatically redirect user to the application. |
There was a problem hiding this comment.
+1 to this particular last-step javascript. This would be limited to a single server page, thus easy to implement. It in fact would replace what currently is the "You are already logged in" page, correct?
| (For example _js-console_ application from the deprecated keycloak-examples), the adapter won't automatically redirect to Keycloak. So some changes in the adapter | ||
| may be needed to achieve this. | ||
|
|
||
| 4. For SAML, will be good to doublecheck the behaviour. I suppose that SAML adapters are server-side and hence is not an |
There was a problem hiding this comment.
No issue to redirect back for SAML secured area - the request would be resent unless anonymous access is also allowed into the secured area.
There was a problem hiding this comment.
So these apps, which allow anonymous access, is exactly my fear. If I understand correctly, after fill username/password in the browser tab2, user might be redirected back to the application, but in the anonymous mode. This can be quite confusing IMO as mentioned in the comment above. If we add some SAML specific errors, adapter can react on them and do the SSO dance instead of showing the anonymous page?
There was a problem hiding this comment.
I'd rather add such a functionality only if really demanded by the community so that we keep the SAML adapter without Keycloak extensions and stick to the standard codes as much as possible. AFAIK, there is no standard SAML code that we could use.
There was a problem hiding this comment.
Ok, I am not so concerned about SAML TBH as I don't know how many people use the anonymous mode etc. For OIDC, I would add the error/error_description with some custom values, so that our adapters can react on it instead of showing anonymous page. I think that at least keycloak.js will need some support for this to be added. Simlarly I think that java servlet adapters don't need any change as redirect_uri is always "secured" URL and adapter always redirects to KC when redirect_uri is displayed. Not sure about gatekeeper and node.js
There was a problem hiding this comment.
That works, only we need to add ensure that the server-client communication would not introduce an infinite loop. I.e. default behaviour would be no redirect; with keycloak special return code, we would only redirect once to server and accept only a single response back, no further redirect.
| `client_data_full` will contain this encoded token. No need for `client_data` parameter, which has just few of client data. | ||
|
|
||
| * There is a URL limit of 2000 characters (excluding host, but including the path, query params and fragment). So to be | ||
| on safe side, we can't include `client_data_full` as request parameter in case that is too long. I suggest to use 1200 |
There was a problem hiding this comment.
There is also limit on cookie size, number of cookies per domain, and total size of all cookies in a domain which varies across the browsers. This limit can be only 4096 bytes for the total size per http://browsercookielimits.squawky.net/. With all the info stored in the auth session notes, and some 10+ number of tabs, this limit could be reached quickly.
Since the limit is based on cookie domain and not cookie path, I don't think the boilerplate suffix would help here.
We thus need to find another way. Could we use POST instead of GET to pass the state? Perhaps part of the state could be stored in the URL (likely the code, execution, tab_id, client_data from minimal approach above)?
There was a problem hiding this comment.
Thanks for checking cookie sizes. That's not good news. My bad I did not check the cookie sizes myself :/ On the other hand, the http://browsercookielimits.squawky.net/ has latest update from 2013 and the most important browsers mentioned there has either no limit for domain or limit of 10234 (IE 8-15). Also the cookie is just a fallback and hopefully won't be needed in most of the cases.
It seems the options are:
-
Don't do anything and not implement advanced design. Which means keep client data in the authSessions on the server. This can be done in case that minimal approach will address most of the use-cases and people won't create anymore usability JIRAs :) I fear this won't be the case and better usability may be needed.
-
Count with the cookie limit based on the user-agent header. We will know the browser due the ua-parser recently added. In most of the cases, we won't either have limit or have bigger limit like 10234. So we will be able to keep just for example last 3 CLIENT_DATA_* cookies for last 3 shown browser tabs. For the other browser tabs, the redirect to the application will be needed as described in the "minimal" flow.
-
Do something else like the POST approach. But I think that POST will be problematic. For example considering all the cases like browser buttons (back/forward/refresh). Currently we remove the POST request from browser history to ensure browser buttons work correctly. I can imagine POST can work just in case that browser is still sending GET requests, but POST requests are sent in the background through ajax. This will require more refactoring of authentication SPI itself to be trully JS based...
-
Do something else and still keep the client data on the server rather than in the browser. I can think of something like divide current AuthenticationSession into 2 entities:
-- AuthenticationSession, which will keep just the authentication data
-- ClientAuthenticationSession, which will keep just the client data and reference to the AuthenticationSession. There will be ClientAuthenticationSession for each browser tab, but AuthenticationSession won't be necessarily specific to single browser tab.
However having things on the server doesn't properly solve the problem, that sessions on server can always expire. In case that session will be expired, redirect to the application will be still needed...
| I think it may be similar for other server adapters (node.js, gatekeeper etc) but will be good to doublecheck. | ||
| For the keycloak.js, I think "login_required" will work fine and automatically redirect to KC, but for the applications with "check_sso" or without any "onLoad" value used, | ||
| (For example _js-console_ application from the deprecated keycloak-examples), the adapter won't automatically redirect to Keycloak. So some changes in the adapter | ||
| may be needed to achieve this. |
There was a problem hiding this comment.
Is checking adapters really needed? For the minimal approach, the worst what can happen is that user would have to click onto "login" button in the application which IMO is acceptable for the minimal solution.
There was a problem hiding this comment.
I was thinking about that before and IMHO the usability of that approach would be even worse than the usability of the current approach with showing the "You are already logged-in" page.
Currently when user is authenticated in tab1 and he fills the username/password in tab2, he is redirected to the "You are already logged-in" page. This is not ideal, but at least, user knows that something strange is going on. So he clicks on the "Back to application" and he would notice that he is not authenticated as he is aware of the strange/non-standard behaviour due the "You are already logged-in" page previously shown.
With the approach you mention, after fill username/password screen, he will be redirected to the application like "js-console", but still in the anonymous mode. This can be quite confusing and user may think like "Why I am still not authenticated even if I just filled username/password" ? It can even have some security applications in theory (EG. user may not notice that he is not yet authenticated in the application and he may start to fill some forms in the application when still being in the anonymous mode etc).
That's the reason I propose to add some protocol-specific error parameters to the redirectUri to give adapters a chance to react on them and automatically redirect user back to the KC, which will do SSO dance and redirect user back as properly authenticated.
mposolda
left a comment
There was a problem hiding this comment.
Thanks! I've replaced occurence of "JWS" with "signed JWT" to explicitly mention that it is signed JWT token. Adding comments for the other points.
|
|
||
| * User will be authenticated even if he provides invalid username/password or even he clicks "Submit" without providing any username/password. As username/password verification didn't really happen in tab2, even if it looks to user as it happened. IMO this is not major issue and same behaviour can be seen by Twitter for example. | ||
|
|
||
| * After confirm username/password screen, user won't see TOTP (or any other 2 factor authenticators) even if he potentially has 2 factor enabled. He will be directly redirected to the application. I am not sure about usability of this and if it's confusing to users or if it's ok. |
There was a problem hiding this comment.
Yes, it is the correct behaviour. My little fear is just about usability. In theory, user may forget that he already filled TOTP an hour ago and now after fill username/password form, he may think like "Hey, where is my TOTP screen" .
But hopefully it is ok. We may see if users will report more JIRAs around usability. If not, we may not need to implement "advanced" approach at all :)
| I think it may be similar for other server adapters (node.js, gatekeeper etc) but will be good to doublecheck. | ||
| For the keycloak.js, I think "login_required" will work fine and automatically redirect to KC, but for the applications with "check_sso" or without any "onLoad" value used, | ||
| (For example _js-console_ application from the deprecated keycloak-examples), the adapter won't automatically redirect to Keycloak. So some changes in the adapter | ||
| may be needed to achieve this. |
There was a problem hiding this comment.
I was thinking about that before and IMHO the usability of that approach would be even worse than the usability of the current approach with showing the "You are already logged-in" page.
Currently when user is authenticated in tab1 and he fills the username/password in tab2, he is redirected to the "You are already logged-in" page. This is not ideal, but at least, user knows that something strange is going on. So he clicks on the "Back to application" and he would notice that he is not authenticated as he is aware of the strange/non-standard behaviour due the "You are already logged-in" page previously shown.
With the approach you mention, after fill username/password screen, he will be redirected to the application like "js-console", but still in the anonymous mode. This can be quite confusing and user may think like "Why I am still not authenticated even if I just filled username/password" ? It can even have some security applications in theory (EG. user may not notice that he is not yet authenticated in the application and he may start to fill some forms in the application when still being in the anonymous mode etc).
That's the reason I propose to add some protocol-specific error parameters to the redirectUri to give adapters a chance to react on them and automatically redirect user back to the KC, which will do SSO dance and redirect user back as properly authenticated.
| (For example _js-console_ application from the deprecated keycloak-examples), the adapter won't automatically redirect to Keycloak. So some changes in the adapter | ||
| may be needed to achieve this. | ||
|
|
||
| 4. For SAML, will be good to doublecheck the behaviour. I suppose that SAML adapters are server-side and hence is not an |
There was a problem hiding this comment.
So these apps, which allow anonymous access, is exactly my fear. If I understand correctly, after fill username/password in the browser tab2, user might be redirected back to the application, but in the anonymous mode. This can be quite confusing IMO as mentioned in the comment above. If we add some SAML specific errors, adapter can react on them and do the SSO dance instead of showing the anonymous page?
| * Send the browser "refresh" request | ||
|
|
||
| * Authentication session will be shared per whole browser (with some exceptions described below in section [different flows in same browser](#different-flows-in-same-browser). | ||
| So on KC side, we will check the latest state from AuthenticationSession and update the page accordingly including URLs. This will allow all browser tabs will be updated to TOTP after confirm username/password in tab1 |
There was a problem hiding this comment.
I agree that first priority is to get rid of "You are already logged-in" . Also agree with the point of more uneccessary browser requests. However I think that manual refreshes is not very good. That would actually mean that user will in most cases still see the username/password screen and fill the username/password as most of users don't do browser refresh.
So maybe the JS can refresh the page in case that particular browser tab is clicked and gains focus? We can always refresh the page when gaining focus, which means that we won't need the timestamp cookie at all then.
The Idp flow or client-specific flow shouldn't be an issue. As mentioned below, there will be still more authentication sessions in the rootAuthSession to handle those cases. Example:
- There are 3 browser tabs. Tabs 1 and 2 are using flow1 (default realm flow). Tab 3 is using some client specific flow - flow2.
- User fills username/password on tab 1. He is moved to step 2 (TOTP screen)
- User clicks on tab 2. JS will refresh the page and user will be moved to TOTP screen
- User clicks on tab 3. JS will refresh the page, but in the flow 2 (client specific flow), user didn't anything yet. So refresh will just keep user at step 1 of the flow 2.
| some side-effects. For example unecessarily creating the HTTP session for the application where user was redirected in other browser tabs. | ||
| So in other browser tabs, we may display some "info" stating like "Authentication was completed. Redirecting to the | ||
| application in few seconds". There will be also some additional JS code, which will be triggered when user enters the particular browser tab. This JS | ||
| will refresh the page and will automatically redirect user to the application. |
| `client_data_full` will contain this encoded token. No need for `client_data` parameter, which has just few of client data. | ||
|
|
||
| * There is a URL limit of 2000 characters (excluding host, but including the path, query params and fragment). So to be | ||
| on safe side, we can't include `client_data_full` as request parameter in case that is too long. I suggest to use 1200 |
There was a problem hiding this comment.
Thanks for checking cookie sizes. That's not good news. My bad I did not check the cookie sizes myself :/ On the other hand, the http://browsercookielimits.squawky.net/ has latest update from 2013 and the most important browsers mentioned there has either no limit for domain or limit of 10234 (IE 8-15). Also the cookie is just a fallback and hopefully won't be needed in most of the cases.
It seems the options are:
-
Don't do anything and not implement advanced design. Which means keep client data in the authSessions on the server. This can be done in case that minimal approach will address most of the use-cases and people won't create anymore usability JIRAs :) I fear this won't be the case and better usability may be needed.
-
Count with the cookie limit based on the user-agent header. We will know the browser due the ua-parser recently added. In most of the cases, we won't either have limit or have bigger limit like 10234. So we will be able to keep just for example last 3 CLIENT_DATA_* cookies for last 3 shown browser tabs. For the other browser tabs, the redirect to the application will be needed as described in the "minimal" flow.
-
Do something else like the POST approach. But I think that POST will be problematic. For example considering all the cases like browser buttons (back/forward/refresh). Currently we remove the POST request from browser history to ensure browser buttons work correctly. I can imagine POST can work just in case that browser is still sending GET requests, but POST requests are sent in the background through ajax. This will require more refactoring of authentication SPI itself to be trully JS based...
-
Do something else and still keep the client data on the server rather than in the browser. I can think of something like divide current AuthenticationSession into 2 entities:
-- AuthenticationSession, which will keep just the authentication data
-- ClientAuthenticationSession, which will keep just the client data and reference to the AuthenticationSession. There will be ClientAuthenticationSession for each browser tab, but AuthenticationSession won't be necessarily specific to single browser tab.
However having things on the server doesn't properly solve the problem, that sessions on server can always expire. In case that session will be expired, redirect to the application will be still needed...
…esign when they gain focus
There was a problem hiding this comment.
Well written and detailed. However, I'm far from convinced about either approaches outlined here. "Minimal approach" is just another layer of band-aids on what we have today, and the "Advanced approach" is just that, it's rather advanced/complex.
While reading this it made me think. Why even bother sharing authentication sessions between multiple tabs? It's causing a lot more headache than benefits.
Another approach could be something along the following where there is more isolation to simplify and reduce corner-cases:
Login cookie (http-only, isolated per-flow, either by array within cookie, or separate cookies per-flow)
- user-id
- session-id
- authentication-state (level, list of authenticators, etc.)
Session cookie (not http-only)
- session-id
- revision
Authentication Session
- One per-tab
Authentication Request URL
- state (JWS with auth_session_id and values needed to redirect error back to client if auth_session is removed)
So scenario with two tabs would look something like:
- Tab1 -> login page, auth-ses#1 created
- Tab2 -> login page, auth-ses#2 created
- User completes login in tab1. Session Cookie is updated with session-id and revision=1|
- Tab2 constantly monitors session cookie and detects session is created, which causes the page to be reloaded. Authentication processing on server side detects login cookie and determines if authentication details in the login cookie is sufficient for what is request. If it is then redirects to app. If it is not then does whatever is needed to step-up the authentication
With regards to action tokens. The action token itself should contain the values from the state so it would share the authentication session with the initial tab (the initial tab obviously contains a message to click link and doesn't have a form with submit buttons).
|
I was also wondering if server-side sessions could only be created after the user has provided at least his username (after the identification phase). I understand the reasons for using notes in session to store state during the login, but maybe we could avoid that if choosing @stianst suggestions. |
|
Few points to this:
IMO it will be better if instead of the constant monitoring, the JS event is triggered "onFocus". So just when particular browser tab is opened. In other words, when you have 5 browser tabs opened and you login in tab1, you won't automatically redirect also in 4 browser tabs directly, but just when the user explicitly clicks on the particular tab. That's what we discussed above with @hmlnarik.
In your proposal, I am missing the details how you would detect that authentication details in the login cookie are sufficient? There are few things to consider like the flow type, flow ID (EG. Client-specific flow), step-up authentication, other OIDC/SAML parameters like "prompt", "max_age" etc. Also I am not sure I understand the purpose of the "revision" field on the session cookie and even the reason why the session cookie exists? I am not sure if it is anyhow easier than the "Advanced approach" and what benefits it will have over it?
In shortcut, I would suggest to start with "Minimal design" for now and add more improvements (including JS support in authenticators, cookies etc) in some later stage... |
No description provided.