Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fix: show logout template after logging out #2913

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Bug fix: show logout template after logging out #2913

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa49344
Show logout template after logging out
hugobarauna Jan 17, 2025
6a907a3
apply code review
hugobarauna Jan 17, 2025
e3ed654
Apply code review suggestions
hugobarauna Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/livebook/config.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ defmodule Livebook.Config do
identity_logout? =
Code.ensure_loaded?(module) and function_exported?(module, :logout, 2)

authentication().mode != :disabled or identity_logout?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR authentication().mode != :disabled was intentional, the reasoning is that currently we have no way to sign out from Livebook after you authenticate with LIVEBOOK_PASSWORD. I'm fine either way though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they have to be different things because when using an identity provider, you still need to log in as admin. We may also want to add a "Logout" button to the menu of deployed apps, which will also trigger this. So having the button doing different things at different stages could be confusing, so I think tying it exclusively to identity for now is the right call.

identity_logout?
end

@doc """
Expand Down
32 changes: 4 additions & 28 deletions lib/livebook_web/controllers/auth_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
defmodule LivebookWeb.AuthController do
use LivebookWeb, :controller
alias LivebookWeb.AuthHelpers

plug(:require_unauthenticated)

alias LivebookWeb.AuthPlug

defp require_unauthenticated(conn, _opts) do
if AuthPlug.authenticated?(conn) do
redirect_to(conn)
AuthHelpers.redirect_to(conn)
else
conn
end
Expand All @@ -30,7 +31,7 @@ defmodule LivebookWeb.AuthController do
conn = AuthPlug.store(conn, :password, password)

if AuthPlug.authenticated?(conn) do
redirect_to(conn)
AuthHelpers.redirect_to(conn)
else
render_form_error(conn, :password)
end
Expand All @@ -40,23 +41,12 @@ defmodule LivebookWeb.AuthController do
conn = AuthPlug.store(conn, :token, token)

if AuthPlug.authenticated?(conn) do
redirect_to(conn)
AuthHelpers.redirect_to(conn)
else
render_form_error(conn, :token)
end
end

def logout(conn, _params) do
if get_session(conn, :user_id) do
conn
|> configure_session(renew: true)
|> clear_session()
|> render("logout.html")
else
redirect_to(conn)
end
end

defp render_form_error(conn, authentication_mode) do
errors = [{"%{authentication_mode} is invalid", [authentication_mode: authentication_mode]}]

Expand All @@ -65,18 +55,4 @@ defmodule LivebookWeb.AuthController do
authentication_mode: authentication_mode
)
end

defp redirect_to(conn) do
conn
|> then(fn conn ->
if redirect_to = get_session(conn, :redirect_to) do
conn
|> delete_session(:redirect_to)
|> redirect(to: redirect_to)
else
redirect(conn, to: ~p"/")
end
end)
|> halt()
end
end
17 changes: 17 additions & 0 deletions lib/livebook_web/controllers/auth_helpers.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
defmodule LivebookWeb.AuthHelpers do
use LivebookWeb, :controller

def redirect_to(conn) do
conn
|> then(fn conn ->
if redirect_to = get_session(conn, :redirect_to) do
conn
|> delete_session(:redirect_to)
|> redirect(to: redirect_to)
else
redirect(conn, to: ~p"/")
end
end)
|> halt()
end
end
15 changes: 15 additions & 0 deletions lib/livebook_web/controllers/user_auth_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
defmodule LivebookWeb.UserAuthController do
use LivebookWeb, :controller
alias LivebookWeb.AuthHelpers

def logout(conn, _params) do
if get_session(conn, :user_id) do
conn
|> configure_session(renew: true)
|> clear_session()
|> render("logout.html")
else
AuthHelpers.redirect_to(conn)
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of moving this to another controller? We now need to create a separate module to share functionality, causing more indirection. I'd prefer to keep them together without a AuthHelpers. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that in the first place. But they have different requirements.

AuthController's actions requires require_unauthenticated:

livebook/lib/livebook_web/controllers/auth_controller.ex

Lines 9 to 15 in fa49344

defp require_unauthenticated(conn, _opts) do
if AuthPlug.authenticated?(conn) do
AuthHelpers.redirect_to(conn)
else
conn
end
end

UserAuthContoller.logout does not.

After trying to battle that, I ended up with that solution: two different controllers.

I actually liked it because I have different concerns. AuthContoller is about "admin authentication". UserAuthController is about "instance/user-identity" based authentication.

We have a similar approach with two related plugs. We have AuthPlug that's more related to "admin authentication" and there's UserPlug that's more related to "instance/user-identity" based authentication.

Copy link
Contributor

@josevalim josevalim Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do action based plugs in Phoenix controllers, I believe it would be:

 plug :require_unauthenticated when action_name != :logout

Or something like that. :)

But I see your points about keeping them separate because they deal with different concerns, and I agree. Here are my suggestions:

  1. Rename it to UserController and UserHTML, in order to mirror the plugs (AuthPlug vs UserPlug)

  2. Don't use the redirect_to helper in logout, leave it only on AuthController. It is used for login because you want to redirect back to the page you were trying to enter, but that doesn't make sense for logout. The page you were at cannot be accessible anymore

I think the suggestions above keep your proposals for code organization and remove my concerns, wdyt?

Copy link
Member Author

@hugobarauna hugobarauna Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Done in e3ed654. ✅

5 changes: 5 additions & 0 deletions lib/livebook_web/controllers/user_auth_html.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
defmodule LivebookWeb.UserAuthHTML do
use LivebookWeb, :html

embed_templates "user_auth_html/*"
end
0 ...eb/controllers/auth_html/logout.html.heex → ...ntrollers/user_auth_html/logout.html.heex
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion lib/livebook_web/router.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ defmodule LivebookWeb.Router do

scope "/", LivebookWeb do
pipe_through [:browser]
get "/logout", AuthController, :logout
get "/logout", UserAuthController, :logout
end

defp within_iframe_secure_headers(conn, _opts) do
Expand Down
29 changes: 29 additions & 0 deletions test/livebook_web/controllers/user_auth_controller_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
defmodule LivebookWeb.UserAuthControllerTest do
use LivebookWeb.ConnCase, async: true

describe "GET /logout" do
test "renders logout template when logged in", %{conn: conn} do
conn = login_user(conn)

conn = get(conn, ~p"/logout")

assert html_response(conn, 200) =~ "You have been logged out"
end

test "redirects when already logged out", %{conn: conn} do
conn = logout_user(conn)

conn = get(conn, ~p"/logout")

assert redirected_to(conn) == ~p"/"
end

defp login_user(conn) do
Phoenix.ConnTest.init_test_session(conn, %{user_id: 1})
end

defp logout_user(conn) do
Phoenix.ConnTest.init_test_session(conn, %{})
end
end
end
Loading