* Hidden field in a form is not a good approach. Its very brittle
and
will not work in every situation. So huge -1 there.
* browser back button is not required to resubmit the HTTP request as
the page can be rendered from cache. Therefore you couldn't have a
"Page Expired" page displayed when the back button is pressed without
setting the header "Cache-Control: no-store, must-revalidate, max-age=0"
But that would mean that we will need to inject some common javascript
stuff into every HTML form displayed by authentication SPI. Could we
rely on that?
* Furthermore, without some type of code/information within the URL,
you also wouldn't know if somebody clicked the back button or not or
whether this was a page refresh or some other GET request.
Once we have the cookie
with loginSessionID, we can lookup the
loginSession. And loginSession will contain last code (same like
clientSession now) and last authenticator. Then we just need to compare
the code from the loginSession with the code from request. If it
matches, we are good. If it doesn't match, it's likely the refresh of
some previous page and in that case, we can just redirect to last
authenticator.
Not sure if we also need to track all codes, so we are able to distinct
between the "expired" code, and between the "false" code, which was
never valid and was possibly used by some attacker for CSRF. Maybe we
can sign codes with HMAC, so we can verify if it is "expired" or
"false"
code without need to track the list of last codes.
Marek
* Hitting Refresh button also has similar issues when removing the
code from the URL. Hitting refresh on a form POST resends the POSTed
data. Without some kind of code in the URL, the runtime has no idea
if this is a Refresh Button re-POST, or if the browser is POSTing to
the current authenticator.
The way it works now is you have two types of requests which can be
GET or POST.
/authenticate?code={code}
and
/authenticate?code={code}&execution={execution}
The first request corresponds to the Authenticator.authenticate()
method. It basically means you want to display the currrent
authenticator in the flow. The 2nd request means that you processing
a challenge sent by the current Authenticator and corresponds to the
Authenticator.action() method. Without this type of distinction, it
is really hard (impossible) to figure out how to route and process the
request.
On 3/16/17 9:43 AM, Stian Thorgersen wrote:
> We should get rid of the code for sure. Maybe we can add the ID of
> the current step in the flow instead though.
>
> Before we discuss implementation though, let's figure out what the
> ideal user experience would be then figure out how to implement it.
> What about:
>
> * Refresh just works
> * Back button will display a nicer page, something like "Page has
> expired. To restart the login process click here. To continue the
> login process click here.". Or Back button could just go to the start
> of the flow always.
> * Resubmitting forms will just display the page above
> * No need to do redirects. Redirects is bad for performance, but also
> has twice the response time which is not good from a usability
> perspective
>
> On 16 March 2017 at 12:56, Marek Posolda <mposolda(a)redhat.com
> <mailto:mposolda@redhat.com>> wrote:
>
> We should be able to detect that with the code in the URL too. So
> the question is, if hidden field has any advantage over keeping
> the code in URL?
>
>
> Thing is, that browsers always treat every POST request as unique
> even if it goes to the same URL. So for example, even if code is
> not in the URL and then the user submits username/password form
> with incorrect password 5 times, then he needs to press browser
> back-button 5 times to return back to initial
> AuthorizationEndpoint request. Even if every POST request had
> same URL without code like
> "http://host/auth/realms/master/authenticate"
> <
http://host/auth/realms/master/authenticate> .
>
> Only real advantage I see is, that code in hidden field is maybe
> a bit safer. Not sure if worth the effort, I will try to
> investigate how much effort it is to put the code to the hidden
> field instead of URL.
>
> Marek
>
>
>
> On 16/03/17 11:44, Stian Thorgersen wrote:
>> I like option #3, but what about adding a hidden field on the
>> form that contains the step in the flow. That way we can easily
>> find out if the form is a post for the current step or not. If
>> it's not then we simply ignore the post and return the current
>> step again? That would work for back/forward and refresh.
>>
>> On 14 March 2017 at 23:47, Bill Burke <bburke(a)redhat.com
>> <mailto:bburke@redhat.com>> wrote:
>>
>> Ya, similar to #3, my thought is if you combine a cookie with
>> code-in-url, you have a solution for backbutton and refresh
>> and there's
>> no special headers you have to specify. We used to do #2,
>> but lot of
>> people, specifically
jboss.org <
http://jboss.org> guys,
>> complained about it.
>>
>>
>> On 3/14/17 4:49 PM, Marek Posolda wrote:
>> > Thanks, that looks similar to my (3) though.
>> >
>> > Besides that I wonder if we should save just the ID of
>> loginSession in
>> > the cookie and the "current-code" keep inside the
loginSession
>> > (infinispan) similarly like it is now?
>> >
>> > I am thinking about the case when potential attacker
>> tricks Keycloak
>> > by manually sending the request, which will just use same
>> code in the
>> > cookie and in the URL. Keycloak will then always treat
>> this request as
>> > valid due the code in the URL and in cookie will always match.
>> > Couldn't that be an issue?
>> >
>> > Marek
>> >
>> > On 14/03/17 13:50, Bill Burke wrote:
>> >> I've got an idea. What about
>> >>
>> >> * keep the code in the URL
>> >>
>> >> * Additionally add a "current-code" cookie
>> >>
>> >> If code in the URL doesn't match the cookie, then
>> redirect to the URL of
>> >> the current-code.
>> >>
>> >>
>> >> On 3/14/17 6:53 AM, Marek Posolda wrote:
>> >>> When working on login sessions, I wonder if we want to
>> improve browser
>> >>> back-button and browser refreshes.
>> >>>
>> >>> In shortcut, I can see 3 basic options:
>> >>>
>> >>> 1) Keep same like now and rely on header
"Cache-Control:
>> no-store,
>> >>> must-revalidate, max-age=0" . This works fine and
users
>> never saw
>> >>> outdated form and never submit outdated form 2 times.
>> However the
>> >>> usability sucks a bit IMO. When you press back-button
>> after POST
>> >>> request, you can see the ugly browser page "Web page
has
>> expired" . And
>> >>> if you press F5 on this, you will see the unfriendly
>> Keycloak error
>> >>> page
>> >>> "Error was occured. Please login again through your
>> application"
>> >>> because
>> >>> of invalid code.
>> >>>
>> >>> 2) Use the pattern with POST followed by the redirect to
>> GET. Since we
>> >>> will have loginSession with the ID in the cookie, the
>> GET request
>> >>> can be
>> >>> sent to the URL without any special query parameter.
>> Something like
>> >>>
>>
"http://localhost:8180/auth/realms/master/login-actions/authenticate
>>
<
http://localhost:8180/auth/realms/master/login-actions/authenticate>&q...
>> .
>> >>> This will allow us that in every stage of
>> authentication, user can
>> >>> press
>> >>> back-button and will be always redirected to the first
>> step of the
>> >>> flow.
>> >>> When he refreshes the page, it will re-send just the GET
>> request and
>> >>> always brings him to the current execution.
>> >>>
>> >>> This looks most user-friendly. But there is the issue
>> with performance
>> >>> though. As we will need to followup every POST request
>> with one
>> >>> additional GET request.
>> >>>
>> >>> 3) Don't do anything special regarding back-button or
>> refresh. But in
>> >>> case that page is refreshed AND the post with invalid
>> (already used)
>> >>> code will be re-submitted, we won't display the ugly
>> page "Error was
>> >>> occured.", but we will just redirect to current step
of
>> the flow.
>> >>>
>> >>> Example:
>> >>> a) User was redirected from the application to OIDC
>> >>> AuthorizationEndpoint request. Login page is shown
>> >>> b) User confirmed invalid username and password with
>> POST request.
>> >>> Login
>> >>> form with error page "Invalid password" is shown
>> >>> c) User confirmed valid username and password with POST
>> request. TOTP
>> >>> page is shown.
>> >>> d) User press back-button. Now he will see again the
>> page with
>> >>> username/password form.
>> >>> e) User press F5. The POST request will be re-sent, but
>> it will use
>> >>> previous "code", which is outdated now. So in this
case,
>> we will
>> >>> redirect to the current execution and TOTP form will be
>> shown. No
>> >>> re-submission of username/password form will happen.
>> >>>
>> >>> In case 3, the username/password form will be shown
>> again, but user
>> >>> won't be able to resubmit it.
>> >>>
>> >>> In shortcut: With 2 and 3, users will never see the
>> browser page "Web
>> >>> page is expired" or Keycloak "Error occured. Go
back to the
>> >>> application". With 2, there is additional GET request
>> needed. With 3,
>> >>> the back-button may show the authentication forms, which
>> user already
>> >>> successfully confirmed, but he won't be able to
>> re-submit them. Is it
>> >>> bad regarding usability? To me, it looks better than
>> showing "Web page
>> >>> is expired".
>> >>>
>> >>> So my preference is 3,2,1. WDYT? Any other options?
>> >>>
>> >>> 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
>> <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
>> <mailto:keycloak-dev@lists.jboss.org>
>>
https://lists.jboss.org/mailman/listinfo/keycloak-dev
>> <
https://lists.jboss.org/mailman/listinfo/keycloak-dev>
>>
>>
>
>