diff --git a/lib/livebook/apps.ex b/lib/livebook/apps.ex index d3af1da2d..912dbe315 100644 --- a/lib/livebook/apps.ex +++ b/lib/livebook/apps.ex @@ -79,14 +79,15 @@ defmodule Livebook.Apps do @spec authorized?(App.t(), Livebook.Users.User.t()) :: boolean() def authorized?(app, user) - def authorized?(%{app_spec: %Livebook.Apps.TeamsAppSpec{}}, %{groups: []}), do: false + def authorized?(%{app_spec: %Livebook.Apps.TeamsAppSpec{}}, %{restricted_apps_groups: []}), + do: false + + 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, slug) + Livebook.Hubs.TeamClient.user_app_access?(id, user.restricted_apps_groups, slug) end - def authorized?(_app, _user), do: true - @doc """ Updates the given app info across the cluster. """ diff --git a/lib/livebook/config.ex b/lib/livebook/config.ex index 435ad7bd1..c79fc25ee 100644 --- a/lib/livebook/config.ex +++ b/lib/livebook/config.ex @@ -6,6 +6,8 @@ defmodule Livebook.Config do | %{mode: :token, secret: String.t()} | %{mode: :disabled} + @type authentication_mode :: :password | :token | :disabled + @doc """ Returns path to Livebook priv directory. diff --git a/lib/livebook/hubs/team_client.ex b/lib/livebook/hubs/team_client.ex index 5fa74737e..60630b7d8 100644 --- a/lib/livebook/hubs/team_client.ex +++ b/lib/livebook/hubs/team_client.ex @@ -143,19 +143,19 @@ defmodule Livebook.Hubs.TeamClient do end @doc """ - Returns if the given user has full access to app server. + Returns if the given user groups has full access to app server. """ - @spec user_full_access?(String.t(), Livebook.Users.User.t()) :: boolean() - def user_full_access?(id, user) do - GenServer.call(registry_name(id), {:check_full_access, user}) + @spec user_full_access?(String.t(), list(map())) :: boolean() + def user_full_access?(id, groups) do + GenServer.call(registry_name(id), {:check_full_access, groups}) end @doc """ - Returns if the given user has access to given app. + Returns if the given user groups has access to given app. """ @spec user_app_access?(String.t(), Livebook.Users.User.t(), String.t()) :: boolean() - def user_app_access?(id, user, slug) do - GenServer.call(registry_name(id), {:check_app_access, user, slug}) + def user_app_access?(id, groups, slug) do + GenServer.call(registry_name(id), {:check_app_access, groups, slug}) end @doc """ @@ -296,30 +296,24 @@ defmodule Livebook.Hubs.TeamClient do end end - def handle_call({:check_full_access, user}, _caller, %{deployment_group_id: id} = state) do + def handle_call({:check_full_access, groups}, _caller, %{deployment_group_id: id} = state) do case fetch_deployment_group(id, state) do {:ok, deployment_group} -> - {:reply, authorized_group?(deployment_group.authorization_groups, user.groups), state} + {:reply, authorized_group?(deployment_group.authorization_groups, groups), state} _ -> {:reply, false, state} end end - def handle_call({:check_app_access, user, slug}, _caller, %{deployment_group_id: id} = state) do + def handle_call({:check_app_access, groups, slug}, _caller, %{deployment_group_id: id} = state) do with {:ok, deployment_group} <- fetch_deployment_group(id, state), {:ok, app_deployment} <- fetch_app_deployment_from_slug(slug, state) do - cond do - # If the user have full access, it should be verified first - authorized_group?(deployment_group.authorization_groups, user.groups) -> - {:reply, true, state} + app_access? = + authorized_group?(deployment_group.authorization_groups, groups) or + authorized_group?(app_deployment.authorization_groups, groups) - authorized_group?(app_deployment.authorization_groups, user.groups) -> - {:reply, true, state} - - true -> - {:reply, false, state} - end + {:reply, app_access?, state} else _ -> {:reply, false, state} end @@ -575,7 +569,7 @@ defmodule Livebook.Hubs.TeamClient do 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, + provider_id: authorization_group.provider_id, group_name: authorization_group.group_name } end @@ -1003,20 +997,20 @@ defmodule Livebook.Hubs.TeamClient do defp nullify(""), do: nil defp nullify(value), do: value - 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 + 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 defp authorized_group?(authorization_groups, groups) do - Enum.any?(authorization_groups, fn %{oidc_provider_id: id, group_name: name} -> - %{"oidc_provider_id" => id, "group_name" => name} in groups + Enum.any?(authorization_groups, fn %{provider_id: id, group_name: name} -> + %{"provider_id" => id, "group_name" => name} in groups end) end end diff --git a/lib/livebook/teams/authorization_group.ex b/lib/livebook/teams/authorization_group.ex index 487c9a912..4b22c4d22 100644 --- a/lib/livebook/teams/authorization_group.ex +++ b/lib/livebook/teams/authorization_group.ex @@ -2,13 +2,13 @@ defmodule Livebook.Teams.AuthorizationGroup do use Ecto.Schema @type t :: %__MODULE__{ - oidc_provider_id: String.t() | nil, + provider_id: String.t() | nil, group_name: String.t() | nil } @primary_key false embedded_schema do - field :oidc_provider_id, :string + field :provider_id, :string field :group_name, :string end end diff --git a/lib/livebook/users/user.ex b/lib/livebook/users/user.ex index d29f137b1..52d33b422 100644 --- a/lib/livebook/users/user.ex +++ b/lib/livebook/users/user.ex @@ -17,7 +17,7 @@ defmodule Livebook.Users.User do name: String.t() | nil, email: String.t() | nil, avatar_url: String.t() | nil, - groups: list(map()) | nil, + restricted_apps_groups: list(map()) | nil, payload: map() | nil, hex_color: hex_color() } @@ -29,7 +29,7 @@ defmodule Livebook.Users.User do field :name, :string field :email, :string field :avatar_url, :string - field :groups, {:array, :map} + field :restricted_apps_groups, {:array, :map} field :payload, :map field :hex_color, Livebook.EctoTypes.HexColor end @@ -44,7 +44,7 @@ defmodule Livebook.Users.User do name: nil, email: nil, avatar_url: nil, - groups: nil, + restricted_apps_groups: nil, payload: nil, hex_color: Livebook.EctoTypes.HexColor.random() } @@ -52,7 +52,7 @@ defmodule Livebook.Users.User do def changeset(user, attrs \\ %{}) do user - |> cast(attrs, [:name, :email, :avatar_url, :groups, :hex_color, :payload]) + |> cast(attrs, [:name, :email, :avatar_url, :restricted_apps_groups, :hex_color, :payload]) |> validate_required([:hex_color]) end end diff --git a/lib/livebook/zta/livebook_teams.ex b/lib/livebook/zta/livebook_teams.ex index 034b125b6..ebc578c9a 100644 --- a/lib/livebook/zta/livebook_teams.ex +++ b/lib/livebook/zta/livebook_teams.ex @@ -79,13 +79,12 @@ defmodule Livebook.ZTA.LivebookTeams do defp handle_request(conn, team, _params) do case get_session(conn) do %{"livebook_teams_access_token" => access_token} -> - conn - |> validate_access_token(team, access_token) - |> authorize_user(team) + validate_access_token(conn, team, access_token) # it means, we couldn't reach to Teams server %{"teams_error" => true} -> {conn + |> put_status(:bad_request) |> delete_session(:teams_error) |> put_view(LivebookWeb.ErrorHTML) |> render("400.html", %{status: 400}) @@ -93,6 +92,7 @@ defmodule Livebook.ZTA.LivebookTeams do %{"teams_failed_reason" => reason} -> {conn + |> put_status(:forbidden) |> delete_session(:teams_failed_reason) |> put_view(LivebookWeb.ErrorHTML) |> render("error.html", %{ @@ -113,27 +113,6 @@ defmodule Livebook.ZTA.LivebookTeams do end end - 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 - - defp authorize_user({%{path_info: ["apps", slug | _]} = conn, metadata}, team) do - if Livebook.Apps.exists?(slug) do - check_app_access(conn, metadata, slug, team) - else - check_full_access(conn, metadata, team) - end - end - - defp authorize_user({conn, metadata}, team) do - check_full_access(conn, metadata, team) - end - defp retrieve_access_token(team, code) do with {:ok, %{"access_token" => access_token}} <- Teams.Requests.retrieve_access_token(team, code) do @@ -188,11 +167,18 @@ defmodule Livebook.ZTA.LivebookTeams do "avatar_url" => avatar_url } = payload + restricted_apps_groups = + if Livebook.Hubs.TeamClient.user_full_access?(team.id, groups) do + nil + else + groups + end + metadata = %{ id: id, name: name, avatar_url: avatar_url, - groups: groups, + restricted_apps_groups: restricted_apps_groups, email: email, payload: payload } @@ -200,35 +186,4 @@ defmodule Livebook.ZTA.LivebookTeams do {:ok, metadata} end end - - defp check_full_access(conn, metadata, team) do - if Livebook.Hubs.TeamClient.user_full_access?(team.id, get_user!(metadata)) do - {conn, metadata} - else - {conn - |> put_view(LivebookWeb.ErrorHTML) - |> render("401.html", %{details: "You don't have permission to access this server"}) - |> halt(), nil} - end - end - - defp check_app_access(conn, metadata, slug, team) do - if Livebook.Hubs.TeamClient.user_app_access?(team.id, get_user!(metadata), slug) do - {conn, metadata} - else - {conn - |> put_view(LivebookWeb.ErrorHTML) - |> render("401.html", %{details: "You don't have permission to access this app"}) - |> halt(), nil} - end - end - - defp get_user!(metadata) do - {:ok, user} = - metadata.id - |> Livebook.Users.User.new() - |> Livebook.Users.update_user(metadata) - - user - end end diff --git a/lib/livebook_web/controllers/error_html.ex b/lib/livebook_web/controllers/error_html.ex index 3b7c55dc4..6d44a2759 100644 --- a/lib/livebook_web/controllers/error_html.ex +++ b/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="404" title="No Numbats here!" /> + <.error_page status={404} title="No Numbats here!" /> """ end def render("403.html", assigns) do ~H""" - <.error_page status="403" title="No Numbats allowed here!" /> + <.error_page status={403} title="No Numbats allowed here!" /> """ end @@ -21,7 +21,7 @@ defmodule LivebookWeb.ErrorHTML do def render("401.html", assigns) do ~H""" - <.error_page status="401" title="Not authorized" details={@details} /> + <.error_page status={401} title="Not authorized" details={@details} /> """ end @@ -35,11 +35,11 @@ defmodule LivebookWeb.ErrorHTML do """ end - attr :status, :any, required: true + attr :status, :integer, required: true attr :title, :string, required: true attr :details, :string, default: nil - defp error_page(assigns) do + def error_page(assigns) do ~H""" diff --git a/lib/livebook_web/live/app_session_live.ex b/lib/livebook_web/live/app_session_live.ex index 4027a2d0e..48329f3a9 100644 --- a/lib/livebook_web/live/app_session_live.ex +++ b/lib/livebook_web/live/app_session_live.ex @@ -8,8 +8,9 @@ defmodule LivebookWeb.AppSessionLive do alias Livebook.Notebook.Cell @impl true + def mount(%{"slug" => slug, "id" => session_id}, _session, socket) - when socket.assigns.app_authenticated? do + when socket.assigns.app_authenticated? and socket.assigns.app_authorized? do {:ok, app} = Livebook.Apps.fetch_app(slug) app_session = Enum.find(app.sessions, &(&1.id == session_id)) @@ -53,6 +54,11 @@ 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 = @@ -77,7 +83,9 @@ defmodule LivebookWeb.AppSessionLive do end @impl true - def render(%{nonexistent?: true} = assigns) when assigns.app_authenticated? do + + def render(%{nonexistent?: true} = assigns) + when assigns.app_authenticated? and assigns.app_authorized? do ~H"""
@@ -96,7 +104,7 @@ defmodule LivebookWeb.AppSessionLive do """ end - def render(assigns) when assigns.app_authenticated? do + def render(assigns) when assigns.app_authenticated? and assigns.app_authorized? do ~H"""
@@ -232,6 +240,16 @@ defmodule LivebookWeb.AppSessionLive do """ end + def render(assigns) when not assigns.app_authorized? do + ~H""" + + """ + end + def render(assigns), do: auth_placeholder(assigns) attr :status, :map, required: true @@ -372,7 +390,11 @@ defmodule LivebookWeb.AppSessionLive do # 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}")} + if socket.assigns.app_authorized? do + {:noreply, socket} + else + {:noreply, redirect(socket, to: ~p"/apps/#{slug}/sessions/#{socket.assigns.session.id}")} + end end def handle_info(_message, socket), do: {:noreply, socket} diff --git a/lib/livebook_web/live/hooks/app_auth_hook.ex b/lib/livebook_web/live/hooks/app_auth_hook.ex index 4cc863162..5a53d28bc 100644 --- a/lib/livebook_web/live/hooks/app_auth_hook.ex +++ b/lib/livebook_web/live/hooks/app_auth_hook.ex @@ -34,6 +34,10 @@ 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 # + # * `:app_authorized?` - reflects the app access authorization. + # 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 # access # @@ -45,7 +49,14 @@ defmodule LivebookWeb.AppAuthHook do LivebookWeb.SessionHelpers.subscribe_to_logout() end - livebook_authenticated? = livebook_authenticated?(session, socket) + user = + LivebookWeb.UserPlug.build_current_user( + session, + session["identity_data"], + session["user_data"] + ) + + livebook_authenticated? = livebook_authenticated?(session, user, socket) socket = socket @@ -53,18 +64,29 @@ defmodule LivebookWeb.AppAuthHook do |> attach_hook(:logout, :handle_info, &handle_info/2) |> attach_hook(:logout, :handle_event, &handle_event/3) - case Livebook.Apps.fetch_settings(slug) do - {:ok, %{access_type: :public} = app_settings} -> - {:cont, assign(socket, app_authenticated?: true, app_settings: app_settings)} + 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) - {:ok, %{access_type: :protected} = app_settings} -> - app_authenticated? = livebook_authenticated? or has_valid_token?(socket, app_settings) + _ -> + true + end - {:cont, - assign(socket, app_authenticated?: app_authenticated?, app_settings: app_settings)} + app_authenticated? = + app_settings.access_type == :public or + (livebook_authenticated? or has_valid_token?(socket, app_settings)) - :error -> - {:halt, redirect(socket, to: ~p"/")} + {:cont, + assign(socket, + app_authenticated?: app_authenticated?, + app_authorized?: app_authorized?, + app_settings: app_settings + )} + else + _otherwise -> {:halt, redirect(socket, to: ~p"/")} end end @@ -73,9 +95,11 @@ defmodule LivebookWeb.AppAuthHook do {:cont, socket} end - defp livebook_authenticated?(session, socket) do + defp livebook_authenticated?(session, user, socket) do uri = get_connect_info(socket, :uri) - LivebookWeb.AuthPlug.authenticated?(session, uri.port) + + LivebookWeb.AuthPlug.authenticated?(session, uri.port) and + user.restricted_apps_groups == nil end defp handle_info(:logout, socket) do diff --git a/lib/livebook_web/plugs/auth_plug.ex b/lib/livebook_web/plugs/auth_plug.ex index ff5ad294d..a77036715 100644 --- a/lib/livebook_web/plugs/auth_plug.ex +++ b/lib/livebook_web/plugs/auth_plug.ex @@ -12,7 +12,18 @@ defmodule LivebookWeb.AuthPlug do @impl true def call(conn, _opts) do if authenticated?(conn) do - conn + 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 + render_unauthorized(conn) + end else authenticate(conn) end @@ -99,6 +110,18 @@ defmodule LivebookWeb.AuthPlug do |> halt() end + defp render_unauthorized(%{path_info: []} = conn) do + conn |> redirect(to: ~p"/apps") |> halt() + end + + defp render_unauthorized(conn) do + conn + |> put_status(:unauthorized) + |> put_view(LivebookWeb.ErrorHTML) + |> render("401.html", %{details: "You don't have permission to access this server"}) + |> halt() + end + defp path_with_query(path, params) when params == %{}, do: path defp path_with_query(path, params), do: path <> "?" <> URI.encode_query(params) @@ -112,6 +135,7 @@ defmodule LivebookWeb.AuthPlug do This mirrors `Livebook.Config.authentication/0`, except the it can be overridden in tests, for each connection. """ + @spec authentication(Plug.Conn.t() | map()) :: Livebook.Config.authentication() if Mix.env() == :test do def authentication(%Plug.Conn{} = conn), do: authentication(get_session(conn)) diff --git a/proto/lib/livebook_proto/authorization_group.pb.ex b/proto/lib/livebook_proto/authorization_group.pb.ex index 5f685aeac..71da0a191 100644 --- a/proto/lib/livebook_proto/authorization_group.pb.ex +++ b/proto/lib/livebook_proto/authorization_group.pb.ex @@ -1,6 +1,6 @@ defmodule LivebookProto.AuthorizationGroup do use Protobuf, protoc_gen_elixir_version: "0.14.1", syntax: :proto3 - field :oidc_provider_id, 1, type: :string, json_name: "oidcProviderId" + field :provider_id, 1, type: :string, json_name: "providerId" field :group_name, 2, type: :string, json_name: "groupName" end diff --git a/proto/messages.proto b/proto/messages.proto index e2d42f839..9a6ffc1c7 100644 --- a/proto/messages.proto +++ b/proto/messages.proto @@ -207,7 +207,7 @@ message EnvironmentVariable { } message AuthorizationGroup { - string oidc_provider_id = 1; + string provider_id = 1; string group_name = 2; } diff --git a/test/livebook_teams/zta/livebook_teams_test.exs b/test/livebook_teams/zta/livebook_teams_test.exs index 8f3928b48..3111b273e 100644 --- a/test/livebook_teams/zta/livebook_teams_test.exs +++ b/test/livebook_teams/zta/livebook_teams_test.exs @@ -73,7 +73,7 @@ defmodule Livebook.ZTA.LivebookTeamsTest do } ]) - {conn, code, %{groups: []}} = authenticate_user(conn, node, test) + {conn, code, %{restricted_apps_groups: []}} = authenticate_user(conn, node, test) session = get_session(conn) conn = @@ -81,13 +81,15 @@ defmodule Livebook.ZTA.LivebookTeamsTest do |> init_test_session(session) group = %{ - "oidc_provider_id" => to_string(oidc_provider.id), + "provider_id" => to_string(oidc_provider.id), "group_name" => authorization_group.group_name } # Get the user with updated groups erpc_call(node, :update_user_info_groups, [code, [group]]) - assert {%{halted: false}, %{groups: [^group]}} = LivebookTeams.authenticate(test, conn, []) + + assert {%{halted: false}, %{restricted_apps_groups: nil}} = + LivebookTeams.authenticate(test, conn, []) end @tag :tmp_dir @@ -162,26 +164,20 @@ defmodule Livebook.ZTA.LivebookTeamsTest do }} # Now we need to check if the current user has access to this app - {conn, code, %{groups: []}} = authenticate_user(conn, node, test) + {conn, code, %{restricted_apps_groups: []}} = authenticate_user(conn, node, test) session = get_session(conn) - conn = - build_conn(:get, ~p"/apps/#{slug}") - |> init_test_session(session) - group = %{ - "oidc_provider_id" => to_string(oidc_provider.id), + "provider_id" => to_string(oidc_provider.id), "group_name" => authorization_group.group_name } - # Get the user with updated groups + # Update user 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't have permission to access this app" # Guarantee we don't list the app for this user conn = build_conn(:get, ~p"/") |> init_test_session(session) - {%{halted: false}, metadata} = LivebookTeams.authenticate(test, conn, []) + {_conn, metadata} = LivebookTeams.authenticate(test, conn, []) {:ok, user} = Livebook.Users.update_user(Livebook.Users.User.new(metadata.id), metadata) assert Livebook.Apps.list_authorized_apps(user) == [] @@ -204,23 +200,21 @@ defmodule Livebook.ZTA.LivebookTeamsTest do } ]) - {conn, code, %{groups: []}} = authenticate_user(conn, node, test) + {conn, code, %{restricted_apps_groups: []}} = authenticate_user(conn, node, test) group = %{ - "oidc_provider_id" => to_string(oidc_provider.id), + "provider_id" => to_string(oidc_provider.id), "group_name" => authorization_group.group_name } erpc_call(node, :update_user_info_groups, [code, [group]]) - for page <- ["/settings", "/learn", "/hub", "/apps-dashboard"] do - conn = - build_conn(:get, page) - |> init_test_session(get_session(conn)) + conn = + build_conn(:get, ~p"/settings") + |> init_test_session(get_session(conn)) - assert {%{halted: true} = conn, nil} = LivebookTeams.authenticate(test, conn, []) - assert html_response(conn, 200) =~ "You don't have permission to access this server" - end + assert {_conn, %{restricted_apps_groups: [^group]}} = + LivebookTeams.authenticate(test, conn, []) end test "redirects to Livebook Teams with invalid access token", @@ -252,7 +246,7 @@ defmodule Livebook.ZTA.LivebookTeamsTest do {conn, nil} = LivebookTeams.authenticate(test, conn, []) - assert html_response(conn, 200) =~ + assert html_response(conn, 403) =~ "Failed to authenticate with Livebook Teams: you do not belong to this org" end end