Apply review comments

lib/livebook/hubs/team_client.ex

@@ -153,7 +153,7 @@ defmodule Livebook.Hubs.TeamClient do
   @doc """
   Returns if the given user groups has access to given app.
   """
-  @spec user_app_access?(String.t(), Livebook.Users.User.t(), String.t()) :: boolean()
+  @spec user_app_access?(String.t(), list(map()), String.t()) :: boolean()
   def user_app_access?(id, groups, slug) do
     GenServer.call(registry_name(id), {:check_app_access, groups, slug})
   end

lib/livebook_web/live/app_session_live.ex

@@ -8,11 +8,28 @@ defmodule LivebookWeb.AppSessionLive do
   alias Livebook.Notebook.Cell
 
   @impl true
+  def mount(%{"slug" => slug} = params, _session, socket)
+      when not socket.assigns.app_authenticated? do
+    if connected?(socket) do
+      to =
+        if id = params["id"] do
+          ~p"/apps/#{slug}/authenticate?id=#{id}"
+        else
+          ~p"/apps/#{slug}/authenticate"
+        end
 
-  def mount(%{"slug" => slug, "id" => session_id}, _session, socket)
-      when socket.assigns.app_authenticated? and socket.assigns.app_authorized? do
+      {:ok, push_navigate(socket, to: to)}
+    else
+      {:ok, socket}
+    end
+  end
+
+  def mount(_params, _session, socket) when not socket.assigns.app_authorized? do
+    {:ok, socket, layout: false}
+  end
+
+  def mount(%{"slug" => slug, "id" => session_id}, _session, socket) do
     {:ok, app} = Livebook.Apps.fetch_app(slug)
-
     app_session = Enum.find(app.sessions, &(&1.id == session_id))
 
     if app_session && app_session.app_status.lifecycle == :active do
@@ -54,26 +71,6 @@ defmodule LivebookWeb.AppSessionLive do
     end
   end
 
-  def mount(_params, _session, socket)
-      when socket.assigns.app_authenticated? and not socket.assigns.app_authorized? do
-    {:ok, socket, layout: false}
-  end
-
-  def mount(%{"slug" => slug} = params, _session, socket) do
-    if connected?(socket) do
-      to =
-        if id = params["id"] do
-          ~p"/apps/#{slug}/authenticate?id=#{id}"
-        else
-          ~p"/apps/#{slug}/authenticate"
-        end
-
-      {:ok, push_navigate(socket, to: to)}
-    else
-      {:ok, socket}
-    end
-  end
-
   # Puts the given assigns in `socket.private`,
   # to ensure they are not used for rendering.
   defp assign_private(socket, assigns) do
@@ -116,7 +113,7 @@ defmodule LivebookWeb.AppSessionLive do
                 <.remix_icon icon="arrow-down-s-line" />
               </button>
             </:toggle>
-            <.menu_item :if={@livebook_authenticated?}>
+            <.menu_item :if={@livebook_authorized?}>
               <.link navigate={~p"/"} role="menuitem">
                 <.remix_icon icon="home-6-line" />
                 <span>Home</span>
@@ -143,7 +140,7 @@ defmodule LivebookWeb.AppSessionLive do
                 <span>View source</span>
               </.link>
             </.menu_item>
-            <.menu_item :if={@livebook_authenticated?}>
+            <.menu_item :if={@livebook_authorized?}>
               <.link patch={~p"/sessions/#{@session.id}"} role="menuitem">
                 <.remix_icon icon="terminal-line" />
                 <span>Debug</span>
@@ -183,7 +180,7 @@ defmodule LivebookWeb.AppSessionLive do
               <div class="flex items-center gap-6">
                 <span class="tooltip top" data-tooltip="Debug">
                   <.link
-                    :if={@livebook_authenticated?}
+                    :if={@livebook_authorized?}
                     navigate={~p"/sessions/#{@session.id}" <> "#cell-#{@data_view.errored_cell_id}"}
                   >
                     <.remix_icon icon="terminal-line" />
@@ -383,19 +380,16 @@ defmodule LivebookWeb.AppSessionLive do
     {:noreply, redirect_on_closed(socket)}
   end
 
-  def handle_info(
-        {:app_deployment_updated, %{slug: slug, hub_id: hub_id}},
-        %{assigns: %{slug: slug}} = socket
-      ) do
+  def handle_info({:app_deployment_updated, %{slug: slug}}, %{assigns: %{slug: slug}} = socket) do
     # We force the redirection in case of
     # the current user loses access to this app.
 
     # With this strategy, we guarantee that unauthorized users
     # won't be able to keep reading the app which they
     # should't have access.
-    groups = socket.assigns.current_user.restricted_apps_groups
+    {:ok, app} = Livebook.Apps.fetch_app(slug)
 
-    if Livebook.Hubs.TeamClient.user_app_access?(hub_id, groups, slug) do
+    if Livebook.Apps.authorized?(app, socket.assigns.current_user) do
       {:noreply, socket}
     else
       {:noreply, redirect(socket, to: ~p"/apps/#{slug}/sessions/#{socket.assigns.session.id}")}

lib/livebook_web/live/hooks/app_auth_hook.ex

@@ -38,7 +38,7 @@ defmodule LivebookWeb.AppAuthHook do
   #     For public apps (or in case the user has full access) it is
   #     set to `true` on both dead and live render
   #
-  #   * `:livebook_authenticated?` - if the user has full Livebook
+  #   * `:livebook_authorized?` - if the user has full Livebook
   #     access
   #
   #   * `:app_settings` - the current app settings
@@ -49,40 +49,26 @@ defmodule LivebookWeb.AppAuthHook do
       LivebookWeb.SessionHelpers.subscribe_to_logout()
     end
 
-    user =
-      LivebookWeb.UserPlug.build_current_user(
-        session,
-        session["identity_data"],
-        session["user_data"]
-      )
-
-    livebook_authenticated? = livebook_authenticated?(session, user, socket)
+    livebook_authorized? = livebook_authorized?(session, socket)
 
     socket =
       socket
-      |> assign(livebook_authenticated?: livebook_authenticated?)
+      |> assign(livebook_authorized?: livebook_authorized?)
       |> attach_hook(:logout, :handle_info, &handle_info/2)
       |> attach_hook(:logout, :handle_event, &handle_event/3)
 
     with {:ok, app} <- Livebook.Apps.fetch_app(slug),
          {:ok, app_settings} <- Livebook.Apps.fetch_settings(slug) do
-      app_authorized? =
-        case app.app_spec do
-          %Livebook.Apps.TeamsAppSpec{hub_id: hub_id} ->
-            Livebook.Hubs.TeamClient.user_app_access?(hub_id, user.restricted_apps_groups, slug)
-
-          _ ->
-            true
-        end
-
       app_authenticated? =
-        app_settings.access_type == :public or
-          (livebook_authenticated? or has_valid_token?(socket, app_settings))
+        case app_settings.access_type do
+          :public -> true
+          :protected -> livebook_authorized? or has_valid_token?(socket, app_settings)
+        end
 
       {:cont,
        assign(socket,
          app_authenticated?: app_authenticated?,
-         app_authorized?: app_authorized?,
+         app_authorized?: app_authorized?(session, app),
          app_settings: app_settings
        )}
     else
@@ -95,11 +81,22 @@ defmodule LivebookWeb.AppAuthHook do
     {:cont, socket}
   end
 
-  defp livebook_authenticated?(session, user, socket) do
+  defp livebook_authorized?(session, socket) do
     uri = get_connect_info(socket, :uri)
 
     LivebookWeb.AuthPlug.authenticated?(session, uri.port) and
-      user.restricted_apps_groups == nil
+      LivebookWeb.AuthPlug.authorized?(session)
+  end
+
+  defp app_authorized?(session, app) do
+    user =
+      LivebookWeb.UserPlug.build_current_user(
+        session,
+        session["identity_data"],
+        session["user_data"]
+      )
+
+    Livebook.Apps.authorized?(app, user)
   end
 
   defp handle_info(:logout, socket) do

lib/livebook_web/plugs/auth_plug.ex

@@ -12,17 +12,12 @@ defmodule LivebookWeb.AuthPlug do
   @impl true
   def call(conn, _opts) do
     if authenticated?(conn) do
-      user =
-        LivebookWeb.UserPlug.build_current_user(
-          get_session(conn),
-          conn.assigns.identity_data,
-          conn.assigns.user_data
-        )
-
-      if user.restricted_apps_groups == nil do
-        conn
-      else
+      if not authorized?(conn) do
+        # User has access only to specific app pages, so they do not have
+        # access to any pages guarded by this plug.
         render_unauthorized(conn)
+      else
+        conn
       end
     else
       authenticate(conn)
@@ -62,6 +57,26 @@ defmodule LivebookWeb.AuthPlug do
     end
   end
 
+  @doc """
+  Checks if given connection or session is authorized.
+  """
+  @spec authorized?(Plug.Conn.t() | map()) :: boolean()
+  def authorized?(%Plug.Conn{} = conn) do
+    LivebookWeb.UserPlug.build_current_user(
+      get_session(conn),
+      conn.assigns.identity_data,
+      conn.assigns.user_data
+    ).restricted_apps_groups == nil
+  end
+
+  def authorized?(%{} = session) do
+    LivebookWeb.UserPlug.build_current_user(
+      session,
+      session["identity_data"],
+      session["user_data"]
+    ).restricted_apps_groups == nil
+  end
+
   defp authenticate(conn) do
     case authentication(conn) do
       %{mode: :password} ->