Implement the user authorization flow with Livebook Teams #2984

aleDsz · 2025-04-13T21:38:08Z

No description provided.

github-actions · 2025-04-13T21:41:00Z

Uffizzi Ephemeral Environment `deployment-62733`

☁️ https://app.uffizzi.com/github.com/livebook-dev/livebook/pull/2984

📄 View Application Logs etc.

What is Uffizzi? Learn more!

jonatanklosko · 2025-04-14T03:31:51Z

lib/livebook/hubs/team_client.ex

+      cond do
+        # If the user have full access, it should be verified first
+        authorized_group?(deployment_group.authorization_groups, user.groups) ->
+          {:reply, true, state}
+
+        authorized_group?(app_deployment.authorization_groups, user.groups) ->
+          {:reply, true, state}
+
+        true ->
+          {:reply, false, state}
+      end


Suggested change

cond do

# If the user have full access, it should be verified first

authorized_group?(deployment_group.authorization_groups, user.groups) ->

{:reply, true, state}

authorized_group?(app_deployment.authorization_groups, user.groups) ->

{:reply, true, state}

true ->

{:reply, false, state}

end

app_access? =

authorized_group?(deployment_group.authorization_groups, user.groups) or

authorized_group?(app_deployment.authorization_groups, user.groups)

{:reply, app_access?, state}

jonatanklosko · 2025-04-14T03:43:10Z

test/livebook_teams/zta/livebook_teams_test.exs

+      # Get the user with updated groups
+      erpc_call(node, :update_user_info_groups, [code, [group]])
+      assert {%{halted: true} = conn, nil} = LivebookTeams.authenticate(test, conn, [])
+      assert html_response(conn, 200) =~ "You don&#39;t have permission to access this app"


The status should be 401, same in other cases :)

jonatanklosko · 2025-04-14T03:46:23Z

test/livebook_teams/zta/livebook_teams_test.exs

+
+      erpc_call(node, :update_user_info_groups, [code, [group]])
+
+      for page <- ["/settings", "/learn", "/hub", "/apps-dashboard"] do


I think it's fine to test just one of those pages, we want to test a category of pages rather than specific pages.

jonatanklosko · 2025-04-14T03:47:59Z

test/livebook_teams/zta/livebook_teams_test.exs

+      app_deployment_id = to_string(app_deployment_id)
+      assert_receive {:app_deployment_started, %{id: ^app_deployment_id}}
+
+      Livebook.Apps.Manager.sync_permanent_apps()


Syncing should happen automatically on new deployment, we shouldn't have to do it programmatically?

jonatanklosko · 2025-04-14T03:49:40Z

lib/livebook_web/controllers/error_html.ex

@@ -29,7 +35,7 @@ defmodule LivebookWeb.ErrorHTML do
    """
  end

-  attr :status, :integer, required: true
+  attr :status, :any, required: true


Let's pick :integer or :string and pass it like that in all cases for consistency.

jonatanklosko · 2025-04-14T05:00:47Z

lib/livebook/zta/livebook_teams.ex

+  end
+
+  defp authorize_user({%{path_info: ["apps", slug | _]} = conn, metadata}, team) do
+    if Livebook.Apps.exists?(slug) do


Why do we need to check this upfront? If the app doesn't exist, a redirect should happen down the line and that would again check for full access?

jonatanklosko · 2025-04-14T05:10:02Z

lib/livebook/hubs/team_client.ex

+  defp manager_sync(%{deployment_group_id: id}, state) do
+    with {:ok, deployment_group} <- fetch_deployment_group(id, state) do
+      if deployment_group.id == state.deployment_group_id do
+        # Each node runs the teams client, but we only need to call sync once
+        if Apps.Manager.local?() do
+          Apps.Manager.sync_permanent_apps()
+        end
+      end
    end
  end


I'm not sure if there was a good reason to call fetch_deployment_group, I think we can just compare the ids:

Suggested change

defp manager_sync(%{deployment_group_id: id}, state) do

with {:ok, deployment_group} <- fetch_deployment_group(id, state) do

if deployment_group.id == state.deployment_group_id do

# Each node runs the teams client, but we only need to call sync once

if Apps.Manager.local?() do

Apps.Manager.sync_permanent_apps()

end

end

end

end

defp manager_sync(app_deployment, state) do

# We only need to sync if the app deployment belongs to the current

# deployment group

if app_deployment.deployment_group_id == state.deployment_group_id do

# Each node runs the teams client, but we only need to call sync once

if Apps.Manager.local?() do

Apps.Manager.sync_permanent_apps()

end

end

end

jonatanklosko · 2025-04-14T06:36:20Z

lib/livebook/zta/livebook_teams.ex

+  defp authorize_user({%{halted: true} = conn, metadata}, _team) do
+    {conn, metadata}
+  end
+
+  defp authorize_user({%{path_info: path_info} = conn, metadata}, _team)
+       when path_info in [[], ["apps"]] do
+    {conn, metadata}
+  end


I'm not sure if this should be a part of the ZTA implementation. The purpose behind ZTA plug is very focused.

For apps we have AppAuthHook, so we could have the extra authorization there. For the full access, we may need a separate plug/hook.

@josevalim wdyt?

I deleted my previous comments to avoid confusion.

Yes, you are correct. Here is my suggestion, rename user.groups to user.restricted_apps_groups. Then I think the logic here should be this:

If the user has full access, pass the user forward with user.restricted_apps_groups == nil

If the user does not have full access, set user.restricted_apps_groups to the groups

In AuthPlug check if user.restricted_apps_groups != nil, if it is, they can only access apps, and therefore we should fail

In AppAuthHook check if user.restricted_apps_groups != nil, if so, it only access if the user belongs to the app groups

In other words, the goal of the ZTA is to populate restrict_apps_groupsaccordingly, the other plugs redirect.

Sounds good!

josevalim · 2025-04-14T13:28:53Z

lib/livebook_web/controllers/error_html.ex

@@ -3,13 +3,13 @@ defmodule LivebookWeb.ErrorHTML do

  def render("404.html", assigns) do
    ~H"""
-    <.error_page status={@status} title="No Numbats here!" />
+    <.error_page status="404" title="No Numbats here!" />


Suggested change

<.error_page status="404" title="No Numbats here!" />

<.error_page status={404} title="No Numbats here!" />

So we always pass integer :)

josevalim · 2025-04-14T13:30:13Z

lib/livebook_web/live/app_session_live.ex

+    # With this strategy, we guarantee that unauthorized users
+    # won't be able to keep reading the app which they
+    # should't have access.
+    {:noreply, redirect(socket, to: ~p"/apps/#{slug}/sessions/#{socket.assigns.session.id}")}


This can cause the user to lose some state, like something they passed in the form. Could we redirect only if they lose access?

josevalim · 2025-04-14T13:33:50Z

lib/livebook/hubs/team_client.ex

+  defp build_authorization_groups(%{authorization_groups: authorization_groups}) do
+    for authorization_group <- authorization_groups do
+      %Teams.AuthorizationGroup{
+        oidc_provider_id: authorization_group.oidc_provider_id,


Let's call it provider_id both on server and here, this will make it easier if somehow we need to support authorization groups from other providers in the future. :)

Yes, I can see Livebook Teams being a provider of authorization groups in the future, for example.

aleDsz added 7 commits April 13, 2025 15:16

Add :groups field to user struct

53b079c

Handle app_deployment_updated event

d2f726d

Add 401 error page

30407ee

Add function to validate if user is authorized

2d36982

Add function to list apps that user is authorized

48afcf8

Listen to app_deployment_updated events

c9698eb

Check user authorization before accessing the page

040836f

aleDsz requested review from hugobarauna, josevalim and jonatanklosko April 13, 2025 21:38

aleDsz self-assigned this Apr 13, 2025

jonatanklosko reviewed Apr 14, 2025

View reviewed changes

josevalim reviewed Apr 14, 2025

View reviewed changes

livebook-dev deleted a comment from jonatanklosko Apr 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the user authorization flow with Livebook Teams #2984

Implement the user authorization flow with Livebook Teams #2984

aleDsz commented Apr 13, 2025

github-actions bot commented Apr 13, 2025 •

edited

Loading

jonatanklosko Apr 14, 2025

jonatanklosko Apr 14, 2025

jonatanklosko Apr 14, 2025

jonatanklosko Apr 14, 2025

jonatanklosko Apr 14, 2025

jonatanklosko Apr 14, 2025

jonatanklosko Apr 14, 2025

jonatanklosko Apr 14, 2025

josevalim Apr 14, 2025

jonatanklosko Apr 15, 2025

josevalim Apr 14, 2025

josevalim Apr 14, 2025

josevalim Apr 14, 2025

hugobarauna Apr 15, 2025


		erpc_call(node, :update_user_info_groups, [code, [group]])

		for page <- ["/settings", "/learn", "/hub", "/apps-dashboard"] do

	<.error_page status="404" title="No Numbats here!" />
	<.error_page status={404} title="No Numbats here!" />

Implement the user authorization flow with Livebook Teams #2984

Are you sure you want to change the base?

Implement the user authorization flow with Livebook Teams #2984

Conversation

aleDsz commented Apr 13, 2025

github-actions bot commented Apr 13, 2025 • edited Loading

Uffizzi Ephemeral Environment deployment-62733

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 13, 2025 •

edited

Loading

Uffizzi Ephemeral Environment `deployment-62733`