Bug fix: show logout template after logging out

[hugobarauna]

When the user is logged in using an identity provider that supports logout (like OIDC), after clicking logout

We want them to see the logout page:

Before this PR, they were being redirected to the "auth via Teams" page:

[github-actions]

Uffizzi Preview deployment-60021 was deleted.

[josevalim]

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

[hugobarauna]

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.

[josevalim]

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?

[hugobarauna]

Agree. Done in e3ed654. ✅

[jonatanklosko]

I would keep it in the auth controller/view, I think it fits there, and there's no need add microfiles unnecessarily :)

[josevalim]

We discussed this and the goal for now is to mirror LivebookWeb.UserPlug and LivebookWeb.AuthPlug. So I am fine with this but we can revisit later if necessary.

[jonatanklosko]

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.

[josevalim]

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.