login enhancements

[brianmay]

Hello,

I notice some places where logins can get enhanced:

• Before authenticating we save the current URL. But we save it without any GET parameters (conn.request_path). So requested filters, etc, get discarded. It would be good if parameters could be preserved.

  current_path(conn) might be better. Except we probably want to discard any redirected and oidc_error tags and be a bit more intelligent about adding parameters in the redirect callback.

  I see that there is a preserve_initial_request option, but as far as I can tell, this looks like it is more for POST/PUT parameters, and I am more interested in GET only parameters.

• Would be really good if there was a function call where I could do something like:

  conn = login(conn, current_path(conn))

  or for live views:

  socket = login(socket, current_path(socket))

  Which will redirect to the login page, and then redirect back to current_path(conn).

  As sometimes I want to be be able to force a login when a user does a particularly action (e.g. click a login button), and also preserve the current URL including parameters.

I can work on both of these myself (unless you want to do it?), more a heads up what I plan to do.

Regards

[tanguilp]

Regarding the first issue, I think the requests parameters are not stored when redirecting because we store information in the state cookie, which is stored in the browser, and therefore its size must be as small as possible. So the preserve_initial_request option is the way to go, and this works both for GET and PUT/POST requests. Maybe it should be on by default, at least for query params, but that's another question. (And we store the data in the browser session storage, which is less secure than cookies, this is why I wanted it to be opt-in.)

About your second question: can't you just redirect to a Plugoid protected route when clicking on the button? What you want is to have the Plugoid.authenticate/2 function public, and I already thought about that but I couldn't find a use-case for that.

[brianmay]

First issue: preserve_initial_request is a bit of an overkill when you only want to preserve GET parameters, looks like it renders more HTML stuff to the browser. It is probably better it didn't preserve POST parameters actually, those are potentially dangerous and could unexpectedly change things.

Second issue: I could have a URL like /login?next=/existing/path, which requires logins and redirects to that URL. But this won't work because the GET parameters get stripped, and also I want to try to minimise the number of redirects that the user's browser needs to go through. As every redirect will add to the total latency to login.

[tanguilp]

First issue: preserve_initial_request is a bit of an overkill when you only want to preserve GET parameters, looks like it renders more HTML stuff to the browser. It is probably better it didn't preserve POST parameters actually, those are potentially dangerous and could unexpectedly change things.

It does indeed involve showing an HTML page + javascript redirect twice. However it shouldn't break anything.

That said I'll think about it, I agree stripping request parameters is a surprising behaviour.

Second issue: I could have a URL like /login?next=/existing/path, which requires logins and redirects to that URL. But this won't work because the GET parameters get stripped, and also I want to try to minimise the number of redirects that the user's browser needs to go through. As every redirect will add to the total latency to login.

What is your flow exactly? And your constraints? Nowadays with a normal browser and good connectivity redirects to the OP can't even be seen in the address bar.

[brianmay]

Even if the browser hides redirects from the user, each request with a redirect will require an extra round trip to the server. Which will add to the latency, especially if the server is located in a different country.

Having said that, if the GET query parameters were preserved at least I could get a working solution.

[tanguilp]

Firefox's max cookie size is 4kb, versus 32kb for max URL length. How would you solve that?

I agree that not saving request parameters by default is not expected. Still understand what the problem is with redirects though. Also note that this 2 redirects done for preserving parameters are JS redirects, which should be very fast.

[brianmay]

Do you have a reference for Firefox's max cookie size? Not seen that myself. But maybe I have been lucky so far?

[tanguilp]

http://browsercookielimits.iain.guru/

[brianmay]

Do you think we are likely to exceed the 4kb limit? At the moment I am reaching 1115 bytes.

If you do exceed it, maybe time to think about some other mechanism for storing sessions. e.g. ets.

Wonder, can the data in the cookie be compressed?

[tanguilp]

There is another limit which is the max header size (https://hexdocs.pm/plugoid/Plugoid.html#module-on-state-cookies). It means all cookies are sent in one header, and this header must be <4kb with Cowboy's default. This is why I'm a bit reluctant to put more data in state cookies.

And still not understanding you use-case :) Have you measured the redirect time? Is your connectivity bad in your scenario?

[brianmay]

I need the user to be able to click the login button, login, and come back to the exact same page, with parameters.

You suggested I redirect the user to a login url. I can redirect the user to a page like /login?next=/existing/path but this doesn't work, because after the user authenticates, they will go to /login and not /login?next=/existing/path. As the arguments get removed. Then the login URL has no way of knowing what to do.

I could also use a login function such as conn = login(conn, current_path(conn)), but this hasn't been implemented.

I also want somebody who tries to access /secure/list?query=abc, to get a login page, and then go back to the same page that they requested. Without the GET query parameter it isn't the same page.

But I don't want POST parameters, POST parameters implies a change is to be made to the system. It is a bit confusing if the system changes state immediately after a login.

Does this make sense now?

[tanguilp]

The button could simply be a linked to an plugoid-protected page. For example, the user is on /home?a=1, clicks the button to /login_with_some_op/ which triggers OIDC redirect with Plugoid (and optionally saves query parameters into the session), then back from the OP to this page which redirects to /home (optionally restoring parameters).

I could also use a login function such as conn = login(conn, current_path(conn)), but this hasn't been implemented. Still considering, but the above flow would work without needing it.

I also want somebody who tries to access /secure/list?query=abc, to get a login page, and then go back to the same page that they requested. Without the GET query parameter it isn't the same page. plugoid protected /secure path + preserve_request_parameters option.

But I don't want POST parameters, POST parameters implies a change is to be made to the system. It is a bit confusing if the system changes state immediately after a login.

Still unclear. Preserving POST parameters are important when the user is filling a form, the plugoid session expires and he's redirected (seamlessly or with login at the OP) to the initial page. Not having this parameter enabled would result in losing what he was doing.

[brianmay]

The button could simply be a linked to an plugoid-protected page. For example, the user is on /home?a=1, clicks the button to /login_with_some_op/ which triggers OIDC redirect with Plugoid (and optionally saves query parameters into the session), then back from the OP to this page which redirects to /home (optionally restoring parameters).

In your proposal, I am not sure I understand how the redirect to /home would happen. The sequence of redirects would be:

1. https://.../home?a=1 - original page.
2. https://.../login_with_some_op - user clicked login button. Plugoid saves this URL in the session without any parameters.
3. https://auth.../... - redirect to auth provider.
4. https://.../openid_connect_redirect_uri - auth provider logs in. Plugoid retrieves the saved URL from the session.
5. https://.../login_with_some_op

But at step 2, we have already lost the original URL. So steps 2 to 4 cannot save the original URL because it was already lost. Which in turn means step 5 cannot redirect back to /home unless it hard codes this URL and always returns to that URL. But I want step 5 to return to the URL in step 1.

[tanguilp]

In step 1 you would have to add the current parameters to the link-button, so that in step 2 this would be a redirect to https://.../login_with_some_op?params_from_step1=URLENCODED_PARAMS_FROM_STEP_1, and save them in the session, before the plugoid plug (for example with another custom plug).

I agree that it's not very convenient though. Do you intend to use on_unauthenticated: :pass on the landing page?

Making the private authenticate/2 public would make your life easier, I'm considering it. This way you could have this button trigger the same page with an additional parameter (you would still need to add initial params to this button), save params in the session and trigger authentication.

I'm also considering adding the whole url to the state session, this make sense as in practice the URLs are shorter than the 4096 limit.

[tanguilp]

If you're still at it, you can test master, the two proposed improvements have been implemented and will be merged soon™.

[brianmay]

Sure, will have a look ASAP. Thanks!

[brianmay]

I am a bit confused, "implemented and merged soon" - does that mean you still have pending changes?

My thought process is getting a bit circular, still trying to think this through.

Anyway, playing with the current master. If I make the following change:

diff --git a/lib/plugoid.ex b/lib/plugoid.ex
index 6613ee0..cbf57bd 100644
--- a/lib/plugoid.ex
+++ b/lib/plugoid.ex
@@ -587,13 +587,23 @@ defmodule Plugoid do

     op_request_uri = OIDC.Auth.request_uri(challenge, opts) |> URI.to_string()

+    request_path = case opts[:request_path] do
+        nil -> conn.request_path
+        request_path -> request_path
+    end
+
+    request_params = case opts[:request_params] do
+        nil -> initial_request_params(conn, opts)
+        request_params ->  request_params
+    end
+
     conn =
       StateSession.store_oidc_request(
         conn,
         %OIDCRequest{
           challenge: challenge,
-          initial_request_path: conn.request_path,
-          initial_request_params: initial_request_params(conn, opts)
+          initial_request_path: request_path,
+          initial_request_params: request_params
         },
         opts[:max_concurrent_state_session]
         )

Then I can write the following view:

@spec login(Plug.Conn.t(), map()) :: Plug.Conn.t()
  def login(conn, params) do
    next = case params["next"] do
      nil -> Routes.page_path(conn, :index)
      "" -> Routes.page_path(conn, :index)
      next -> next
    end

    parsed = URI.parse(next)
    query = URI.query_decoder(parsed.query) |> Enum.into(%{})
    opts = Plugoid.init(ScroogeWeb.Router.PlugoidConfig.common)
    opts = [{:request_path, parsed.path}, {:request_params, query} | opts]
    Plugoid.authenticate(conn, opts)
  end

(Possible the next URL parsing could be simplified, not sure)

And I generate the link the the login button like:

url = Routes.page_path(conn, :login, next: current_path(conn))

Not sure I like this. It works. But the line to set the opts by calling Plugoid.init() in order to set the defaults correctly feels a bit bit hackish. Not sure I see a good way around this though.

The other option would be to do:

@spec login(Plug.Conn.t(), map()) :: Plug.Conn.t()
   next = case params["next"] do
     nil -> Routes.page_path(conn, :index)
     "" -> Routes.page_path(conn, :index)
    next -> next
  end
  redirect(conn, to: next)
end

And have protected by plugoid which would require the login. The downside is this would take a bit more cookie space, e.g. if next is /path?a=b the first one needs to store:

initial_request_path = "/path" initial_request_params = %{"a" => "b"}

Where as the next one would need to store:

initial_request_path = "/login" initial_request_params = %{"next" => "/path?a=b"}

Hope I am making sense :-)

[tanguilp]

I am a bit confused, "implemented and merged soon" - does that mean you still have pending changes? I meant implemented and released soon, everything is on master ;)

As you've noticed we rely on conn to determine the request path and params.

You manually build these but I'm wondering why you couldn't just either:

• add a query parameter do_authenticate=true in addition to the other already present query parameters to your button connect URI, and remove it from conn just before calling Plugoid.authenticate/2
• POST to the same page with the same URI with a post parameter like do_authenticate=true and trigger authentication on this condition

What do you think? Seems simpler than your hack to be honest.

[brianmay]

That is a solution I hadn't thought of. Will try and get back to you. Thanks!

[tanguilp]

Brian, may I close this issue?

[brianmay]

I haven't actually looked at this again :-(

But yes, OK to close.