Apply review comments

lib/livebook/apps.ex

@@ -7,6 +7,7 @@ defmodule Livebook.Apps do
   require Logger
 
   alias Livebook.App
+  alias Livebook.Apps
 
   @doc """
   Returns app process pid for the given slug.
@@ -79,13 +80,11 @@ defmodule Livebook.Apps do
   @spec authorized?(App.t(), Livebook.Users.User.t()) :: boolean()
   def authorized?(app, user)
 
-  def authorized?(%{app_spec: %Livebook.Apps.TeamsAppSpec{}}, %{restricted_apps_groups: []}),
-    do: false
+  def authorized?(%{app_spec: %Apps.TeamsAppSpec{}}, %{groups: [], access_type: :apps}), do: false
+  def authorized?(_app, %{access_type: :full}), do: true
 
-  def authorized?(_app, %{restricted_apps_groups: nil}), do: true
-
-  def authorized?(%{slug: slug, app_spec: %Livebook.Apps.TeamsAppSpec{hub_id: id}}, user) do
-    Livebook.Hubs.TeamClient.user_app_access?(id, user.restricted_apps_groups, slug)
+  def authorized?(%{slug: slug, app_spec: %Apps.TeamsAppSpec{hub_id: id}}, user) do
+    Livebook.Hubs.TeamClient.user_app_access?(id, user.groups, slug)
   end
 
   @doc """

lib/livebook/hubs/team_client.ex

@@ -158,22 +158,6 @@ defmodule Livebook.Hubs.TeamClient do
     GenServer.call(registry_name(id), {:check_app_access, groups, slug})
   end
 
-  @doc """
-  Returns if the Team client uses Livebook Teams authorization groups.
-  """
-  @spec authorization_groups_enabled?(String.t()) :: boolean()
-  def authorization_groups_enabled?(id) do
-    GenServer.call(registry_name(id), :authorization_groups_enabled?)
-  end
-
-  @doc """
-  Returns if the Team client is an app server for given deployment group.
-  """
-  @spec current_app_server_deployment_group?(String.t(), String.t()) :: boolean()
-  def current_app_server_deployment_group?(id, deployment_group_id) do
-    GenServer.call(registry_name(id), {:current_deployment_group?, deployment_group_id})
-  end
-
   @doc """
   Returns if the Team client is connected.
   """
@@ -317,7 +301,8 @@ defmodule Livebook.Hubs.TeamClient do
       case fetch_deployment_group(id, state) do
         {:ok, deployment_group} ->
           {:reply,
-           not deployment_group.groups_auth or
+           not deployment_group.teams_auth or
+             not deployment_group.groups_auth or
              authorized_group?(deployment_group.authorization_groups, groups), state}
 
         _ ->
@@ -333,7 +318,8 @@ defmodule Livebook.Hubs.TeamClient do
       with {:ok, deployment_group} <- fetch_deployment_group(id, state),
            {:ok, app_deployment} <- fetch_app_deployment_from_slug(slug, state) do
         app_access? =
-          not deployment_group.groups_auth or
+          not deployment_group.teams_auth or
+            not deployment_group.groups_auth or
             (authorized_group?(deployment_group.authorization_groups, groups) or
                authorized_group?(app_deployment.authorization_groups, groups))
 
@@ -346,21 +332,6 @@ defmodule Livebook.Hubs.TeamClient do
     end
   end
 
-  def handle_call(:authorization_groups_enabled?, _caller, state) do
-    if id = state.deployment_group_id do
-      case fetch_deployment_group(id, state) do
-        {:ok, deployment_group} -> {:reply, deployment_group.groups_auth, state}
-        _ -> {:reply, false, state}
-      end
-    else
-      {:reply, false, state}
-    end
-  end
-
-  def handle_call({:current_deployment_group?, id}, _caller, state) do
-    {:reply, state.deployment_group_id == id, state}
-  end
-
   @impl true
   def handle_info(:connected, state) do
     Hubs.Broadcasts.hub_connected(state.hub.id)

lib/livebook/teams/broadcasts.ex

@@ -37,7 +37,7 @@ defmodule Livebook.Teams.Broadcasts do
 
   Topic `#{@app_server_topic}`:
 
-    * `{:server_authorization_updated, map()}`
+    * `{:server_authorization_updated, DeploymentGroup.t()}`
 
   """
   @spec subscribe(atom() | list(atom())) :: :ok | {:error, term()}

lib/livebook/users/user.ex

@@ -12,12 +12,15 @@ defmodule Livebook.Users.User do
 
   alias Livebook.Utils
 
+  @type access_type :: :full | :apps
+
   @type t :: %__MODULE__{
           id: id(),
           name: String.t() | nil,
           email: String.t() | nil,
           avatar_url: String.t() | nil,
-          restricted_apps_groups: list(map()) | nil,
+          access_type: access_type(),
+          groups: list(map()) | nil,
           payload: map() | nil,
           hex_color: hex_color()
         }
@@ -29,7 +32,8 @@ defmodule Livebook.Users.User do
     field :name, :string
     field :email, :string
     field :avatar_url, :string
-    field :restricted_apps_groups, {:array, :map}
+    field :access_type, Ecto.Enum, values: ~w[full apps]a, default: :full
+    field :groups, {:array, :map}, default: []
     field :payload, :map
     field :hex_color, Livebook.EctoTypes.HexColor
   end
@@ -44,15 +48,17 @@ defmodule Livebook.Users.User do
       name: nil,
       email: nil,
       avatar_url: nil,
-      restricted_apps_groups: nil,
+      access_type: :full,
+      groups: [],
       payload: nil,
       hex_color: Livebook.EctoTypes.HexColor.random()
     }
   end
 
+  @doc false
   def changeset(user, attrs \\ %{}) do
     user
-    |> cast(attrs, [:name, :email, :avatar_url, :restricted_apps_groups, :hex_color, :payload])
+    |> cast(attrs, [:name, :email, :avatar_url, :access_type, :groups, :hex_color, :payload])
     |> validate_required([:hex_color])
   end
 end

lib/livebook/zta.ex

@@ -59,7 +59,8 @@ defmodule Livebook.ZTA do
     * `:name` - the user name
     * `:email` - the user email
     * `:avatar_url` - the user avatar
-    * `:restricted_apps_groups` - the user restricted groups to access apps
+    * `:access_type` - the user access type
+    * `:groups` - the user groups
     * `:payload` - the provider payload
 
   Note that none of the keys are required. The metadata returned depends
@@ -70,7 +71,8 @@ defmodule Livebook.ZTA do
           optional(:name) => String.t(),
           optional(:email) => String.t(),
           optional(:avatar_url) => String.t() | nil,
-          optional(:restricted_apps_groups) => list(map()) | nil,
+          optional(:access_type) => Livebook.Users.User.access_type(),
+          optional(:groups) => list(map()),
           optional(:payload) => map()
         }
 

lib/livebook/zta/livebook_teams.ex

@@ -176,19 +176,17 @@ defmodule Livebook.ZTA.LivebookTeams do
       "avatar_url" => avatar_url
     } = payload
 
-    restricted_apps_groups =
-      if not Livebook.Hubs.TeamClient.authorization_groups_enabled?(hub_id) or
-           Livebook.Hubs.TeamClient.user_full_access?(hub_id, groups) do
-        nil
-      else
-        groups
-      end
+    access_type =
+      if Livebook.Hubs.TeamClient.user_full_access?(hub_id, groups),
+        do: :full,
+        else: :apps
 
     %{
       id: id,
       name: name,
       avatar_url: avatar_url,
-      restricted_apps_groups: restricted_apps_groups,
+      access_type: access_type,
+      groups: groups,
       email: email,
       payload: payload
     }

lib/livebook_web/live/hooks/auth_hook.ex

@@ -14,13 +14,16 @@ defmodule LivebookWeb.AuthHook do
     end
   end
 
-  defp handle_info({:server_authorization_updated, _deployment_group}, socket) do
+  defp handle_info({:server_authorization_updated, %{hub_id: hub_id}}, socket) do
     # We already updated the current user, so we just need to force the redirection.
     # But, for apps, we redirect directly from the app session
-    if socket.assigns.current_user.restricted_apps_groups do
-      {:halt, redirect(socket, to: ~p"/")}
-    else
+    current_user = socket.assigns.current_user
+
+    if current_user.access_type == :full and
+         Livebook.Hubs.TeamClient.user_full_access?(hub_id, current_user.groups) do
       {:halt, socket}
+    else
+      {:halt, redirect(socket, to: ~p"/")}
     end
   end
 

lib/livebook_web/plugs/auth_plug.ex

@@ -64,7 +64,7 @@ defmodule LivebookWeb.AuthPlug do
         get_session(conn),
         conn.assigns.identity_data,
         conn.assigns.user_data
-      ).restricted_apps_groups == nil
+      ).access_type == :full
   end
 
   @doc """
@@ -77,7 +77,7 @@ defmodule LivebookWeb.AuthPlug do
         session,
         session["identity_data"],
         session["user_data"]
-      ).restricted_apps_groups == nil
+      ).access_type == :full
   end
 
   defp authenticate(conn) do

test/livebook_teams/zta/livebook_teams_test.exs

@@ -73,7 +73,7 @@ defmodule Livebook.ZTA.LivebookTeamsTest do
           }
         ])
 
-      {conn, code, %{restricted_apps_groups: []}} = authenticate_user(conn, node, test)
+      {conn, code, %{groups: [], access_type: :apps}} = authenticate_user(conn, node, test)
       session = get_session(conn)
 
       conn =
@@ -88,7 +88,7 @@ defmodule Livebook.ZTA.LivebookTeamsTest do
       # Get the user with updated groups
       erpc_call(node, :update_user_info_groups, [code, [group]])
 
-      assert {%{halted: false}, %{restricted_apps_groups: nil}} =
+      assert {%{halted: false}, %{groups: [^group], access_type: :full}} =
                LivebookTeams.authenticate(test, conn, [])
     end
 
@@ -163,7 +163,7 @@ defmodule Livebook.ZTA.LivebookTeamsTest do
                       }}
 
       # Now we need to check if the current user has access to this app
-      {conn, code, %{restricted_apps_groups: []}} = authenticate_user(conn, node, test)
+      {conn, code, %{groups: [], access_type: :apps}} = authenticate_user(conn, node, test)
       session = get_session(conn)
 
       group = %{
@@ -199,7 +199,7 @@ defmodule Livebook.ZTA.LivebookTeamsTest do
           }
         ])
 
-      {conn, code, %{restricted_apps_groups: []}} = authenticate_user(conn, node, test)
+      {conn, code, %{groups: [], access_type: :apps}} = authenticate_user(conn, node, test)
 
       group = %{
         "provider_id" => to_string(oidc_provider.id),
@@ -212,7 +212,7 @@ defmodule Livebook.ZTA.LivebookTeamsTest do
         build_conn(:get, ~p"/settings")
         |> init_test_session(get_session(conn))
 
-      assert {_conn, %{restricted_apps_groups: [^group]}} =
+      assert {_conn, %{groups: [^group], access_type: :apps}} =
                LivebookTeams.authenticate(test, conn, [])
     end