livebook-dev/livebook
1
1
Fork 0
mirror of https://github.com/livebook-dev/livebook.git synced 2025-10-30 07:16:04 +08:00
Code Issues Projects Releases Packages Wiki Activity

Apply review comments

Browse source
This commit is contained in:
Alexandre de Souza 2025-04-28 16:02:14 -03:00
parent 040836fca6
commit 6fe6bdaf3f
No known key found for this signature in database
GPG key ID: E39228FFBA346545
13 changed files with 159 additions and 143 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
lib/livebook/apps.ex
View file

@ -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.
"""

2
lib/livebook/config.ex
View file

@ -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.

54
lib/livebook/hubs/team_client.ex
View file

@ -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

4
lib/livebook/teams/authorization_group.ex
View file

@ -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

8
lib/livebook/users/user.ex
View file

@ -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

67
lib/livebook/zta/livebook_teams.ex
View file

@ -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

10
lib/livebook_web/controllers/error_html.ex
View file

@ -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"""
<!DOCTYPE html>
<html lang="en">

30
lib/livebook_web/live/app_session_live.ex
View file

@ -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"""
<div class="h-screen flex items-center justify-center">
<div class="flex flex-col space-y-4 items-center">
@ -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"""
<div class="h-full relative overflow-y-auto px-4 md:px-20" data-el-notebook>
<div class="w-full max-w-screen-lg py-4 mx-auto" data-el-notebook-content>
@ -232,6 +240,16 @@ defmodule LivebookWeb.AppSessionLive do
"""
end
def render(assigns) when not assigns.app_authorized? do
~H"""
<LivebookWeb.ErrorHTML.error_page
status={401}
title="Not authorized"
details="You don't have permission to access this app"
/>
"""
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}

48
lib/livebook_web/live/hooks/app_auth_hook.ex
View file

@ -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

26
lib/livebook_web/plugs/auth_plug.ex
View file

@ -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))

2
proto/lib/livebook_proto/authorization_group.pb.ex
View file

@ -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

2
proto/messages.proto
View file

@ -207,7 +207,7 @@ message EnvironmentVariable {
}
message AuthorizationGroup {
string oidc_provider_id = 1;
string provider_id = 1;
string group_name = 2;
}

40
test/livebook_teams/zta/livebook_teams_test.exs
View file

@ -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&#39;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&#39;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