From bfff9d9ddb63324c7b161003dd4c58897ac607b4 Mon Sep 17 00:00:00 2001 From: Antoine Augusti Date: Mon, 8 Jan 2024 10:40:05 +0100 Subject: [PATCH] Add TransportWeb.Session --- apps/transport/lib/transport_web.ex | 1 - .../controllers/page_controller.ex | 8 +-- .../controllers/session_controller.ex | 42 ++---------- .../live/backoffice/jobs_live.ex | 5 +- .../live/backoffice/proxy_config_live.ex | 23 +------ apps/transport/lib/transport_web/router.ex | 7 +- apps/transport/lib/transport_web/session.ex | 67 +++++++++++++++++++ .../dataset/_dataset_scores_chart.html.heex | 2 +- .../templates/dataset/details.html.heex | 2 +- .../templates/dataset/index.html.heex | 2 +- .../templates/layout/_header.html.heex | 4 +- .../controllers/session_controller_test.exs | 6 +- .../test/transport_web/session_text.exs | 42 ++++++++++++ 13 files changed, 130 insertions(+), 81 deletions(-) create mode 100644 apps/transport/lib/transport_web/session.ex create mode 100644 apps/transport/test/transport_web/session_text.exs diff --git a/apps/transport/lib/transport_web.ex b/apps/transport/lib/transport_web.ex index a263eb9efc..5a7a64b74a 100644 --- a/apps/transport/lib/transport_web.ex +++ b/apps/transport/lib/transport_web.ex @@ -42,7 +42,6 @@ defmodule TransportWeb do use TransportWeb.InputHelpers import TransportWeb.Router.Helpers - import TransportWeb.Router, only: [is_transport_data_gouv_member?: 1] import TransportWeb.ErrorHelpers import TransportWeb.InputHelpers import TransportWeb.Gettext diff --git a/apps/transport/lib/transport_web/controllers/page_controller.ex b/apps/transport/lib/transport_web/controllers/page_controller.ex index b538d20efa..7cf023916a 100644 --- a/apps/transport/lib/transport_web/controllers/page_controller.ex +++ b/apps/transport/lib/transport_web/controllers/page_controller.ex @@ -188,16 +188,10 @@ defmodule TransportWeb.PageController do conn |> assign(:datasets, datasets) - |> refresh_is_producer(datasets) + |> TransportWeb.Session.set_is_producer(datasets) |> render("espace_producteur.html") end - defp refresh_is_producer(%Plug.Conn{} = conn, datasets) do - is_producer = not Enum.empty?(datasets) - current_user = get_session(conn, :current_user, %{}) - conn |> put_session(:current_user, Map.put(current_user, "is_producer", is_producer)) - end - defp aoms_with_dataset do aoms_legal_owners = Dataset.base_query() diff --git a/apps/transport/lib/transport_web/controllers/session_controller.ex b/apps/transport/lib/transport_web/controllers/session_controller.ex index c04d0ef8e3..09338610da 100644 --- a/apps/transport/lib/transport_web/controllers/session_controller.ex +++ b/apps/transport/lib/transport_web/controllers/session_controller.ex @@ -4,7 +4,6 @@ defmodule TransportWeb.SessionController do """ use TransportWeb, :controller alias Datagouvfr.Authentication - import Ecto.Query require Logger def new(conn, _) do @@ -128,46 +127,17 @@ defmodule TransportWeb.SessionController do end def save_current_user(%Plug.Conn{} = conn, %{} = user_params) do - conn |> put_session(:current_user, user_params_for_session(user_params)) + conn + |> put_session(:current_user, user_params_for_session(user_params)) + |> TransportWeb.Session.set_is_producer(user_params) + |> TransportWeb.Session.set_is_admin(user_params) end - def user_params_for_session(%{} = params) do - params + defp user_params_for_session(%{} = params) do # Remove the list of `organizations` from the final map: it's already stored in the database # and maintained up-to-date by `Transport.Jobs.UpdateContactsJob` # and it can be too big to be stored in a cookie - |> Map.delete("organizations") - # - `is_admin` is needed to check permissions - # - `is_producer` is used to get access to the "Espace producteur" - # `is_producer` is also refreshed when they visit their "Espace producteur" - |> Map.merge(%{"is_producer" => is_producer?(params), "is_admin" => is_admin?(params)}) - end - - @doc """ - Are you a data producer? - You're a data producer if you're a member of an organization with an active dataset - on transport.data.gouv.fr. - This is set when you log in and refreshed when you visit your "Espace producteur". - """ - def is_producer?(%{"organizations" => orgs}) do - org_ids = Enum.map(orgs, & &1["id"]) - - DB.Dataset.base_query() |> where([dataset: d], d.organization_id in ^org_ids) |> DB.Repo.exists?() - end - - @doc """ - Are you a transport.data.gouv.fr admin? - You're an admin if you're a member of the PAN organization on data.gouv.fr. - - iex> is_admin?(%{"organizations" => [%{"slug" => "equipe-transport-data-gouv-fr"}, %{"slug" => "foo"}]}) - true - iex> is_admin?(%{"organizations" => [%{"slug" => "foo"}]}) - false - iex> is_admin?(%{"organizations" => []}) - false - """ - def is_admin?(%{"organizations" => orgs}) do - Enum.any?(orgs, &(&1["slug"] == "equipe-transport-data-gouv-fr")) + Map.delete(params, "organizations") end defp get_redirect_path(%Plug.Conn{} = conn) do diff --git a/apps/transport/lib/transport_web/live/backoffice/jobs_live.ex b/apps/transport/lib/transport_web/live/backoffice/jobs_live.ex index db0f7b4025..c8b2a46239 100644 --- a/apps/transport/lib/transport_web/live/backoffice/jobs_live.ex +++ b/apps/transport/lib/transport_web/live/backoffice/jobs_live.ex @@ -33,9 +33,10 @@ defmodule TransportWeb.Backoffice.JobsLive do # https://hexdocs.pm/phoenix_live_view/security-model.html#disconnecting-all-instances-of-a-given-live-user # def ensure_admin_auth_or_redirect(socket, current_user, func) do - if current_user && TransportWeb.Router.is_transport_data_gouv_member?(current_user) do + socket = assign(socket, current_user: current_user) + + if TransportWeb.Session.is_admin?(socket) do # We track down the current admin so that it can be used by next actions - socket = assign(socket, current_admin_user: current_user) # Then call the remaining code, which is expected to return the socket func.(socket) else diff --git a/apps/transport/lib/transport_web/live/backoffice/proxy_config_live.ex b/apps/transport/lib/transport_web/live/backoffice/proxy_config_live.ex index 23bcffdaa1..587988f655 100644 --- a/apps/transport/lib/transport_web/live/backoffice/proxy_config_live.ex +++ b/apps/transport/lib/transport_web/live/backoffice/proxy_config_live.ex @@ -4,6 +4,7 @@ defmodule TransportWeb.Backoffice.ProxyConfigLive do """ use Phoenix.LiveView alias Transport.Telemetry + import TransportWeb.Backoffice.JobsLive, only: [ensure_admin_auth_or_redirect: 3] import TransportWeb.Router.Helpers # The number of past days we want to report on (as a positive integer). @@ -20,25 +21,6 @@ defmodule TransportWeb.Backoffice.ProxyConfigLive do end)} end - # - # If one calls "redirect" and does not leave immediately, the remaining code will - # be executed, opening security issues. This method goal is to minimize this risk. - # See https://hexdocs.pm/phoenix_live_view/security-model.html for overall docs. - # - # Also, disconnect will have to be handled: - # https://hexdocs.pm/phoenix_live_view/security-model.html#disconnecting-all-instances-of-a-given-live-user - # - defp ensure_admin_auth_or_redirect(socket, current_user, func) do - if current_user && TransportWeb.Router.is_transport_data_gouv_member?(current_user) do - # We track down the current admin so that it can be used by next actions - socket = assign(socket, current_admin_user: current_user) - # Then call the remaining code, which is expected to return the socket - func.(socket) - else - redirect(socket, to: "/login") - end - end - defp schedule_next_update_data do Process.send_after(self(), :update_data, 1000) end @@ -57,8 +39,7 @@ defmodule TransportWeb.Backoffice.ProxyConfigLive do end def handle_event("refresh_proxy_config", _value, socket) do - if socket.assigns.current_admin_user, do: config_module().clear_config_cache!() - + config_module().clear_config_cache!() {:noreply, socket} end diff --git a/apps/transport/lib/transport_web/router.ex b/apps/transport/lib/transport_web/router.ex index 6268012abe..b9a0025a0f 100644 --- a/apps/transport/lib/transport_web/router.ex +++ b/apps/transport/lib/transport_web/router.ex @@ -353,11 +353,6 @@ defmodule TransportWeb.Router do end end - # NOTE: method visibility set to public because we need to call the same logic from LiveView - # `current_user` is set by TransportWeb.SessionController.user_params_for_session/1 - def is_transport_data_gouv_member?(%{"is_admin" => true} = _current_user), do: true - def is_transport_data_gouv_member?(_), do: false - # Check that a secret key is passed in the URL in the `export_key` query parameter defp check_export_secret_key(%Plug.Conn{params: params} = conn, _) do export_key_value = Map.get(params, "export_key", "") @@ -374,7 +369,7 @@ defmodule TransportWeb.Router do end defp transport_data_gouv_member(%Plug.Conn{} = conn, _) do - if is_transport_data_gouv_member?(conn.assigns[:current_user]) do + if TransportWeb.Session.is_admin?(conn) do conn else conn diff --git a/apps/transport/lib/transport_web/session.ex b/apps/transport/lib/transport_web/session.ex new file mode 100644 index 0000000000..e3e8fca929 --- /dev/null +++ b/apps/transport/lib/transport_web/session.ex @@ -0,0 +1,67 @@ +defmodule TransportWeb.Session do + @moduledoc """ + Web session getters and setters. + """ + import Ecto.Query + import Plug.Conn + + @is_admin_key_name "is_admin" + @is_producer_key_name "is_producer" + + @doc """ + Are you a data producer? + + You're a data producer if you're a member of an organization with an active dataset + on transport.data.gouv.fr. + This is set when you log in and refreshed when you visit your "Espace producteur". + """ + @spec set_is_producer(Plug.Conn.t(), map() | [DB.Dataset.t()]) :: Plug.Conn.t() + def set_is_producer(%Plug.Conn{} = conn, %{"organizations" => _} = params) do + set_session_attribute_attribute(conn, @is_producer_key_name, is_producer?(params)) + end + + def set_is_producer(%Plug.Conn{} = conn, [%DB.Dataset{}] = _datasets_for_user) do + set_session_attribute_attribute(conn, @is_producer_key_name, true) + end + + def set_is_producer(%Plug.Conn{} = conn, [] = _datasets_for_user) do + set_session_attribute_attribute(conn, @is_producer_key_name, false) + end + + @doc """ + Are you a transport.data.gouv.fr admin? + You're an admin if you're a member of the PAN organization on data.gouv.fr. + """ + def set_is_admin(%Plug.Conn{} = conn, %{"organizations" => _} = params) do + set_session_attribute_attribute(conn, @is_admin_key_name, is_admin?(params)) + end + + def is_admin?(%{"organizations" => orgs}) do + Enum.any?(orgs, &(&1["slug"] == "equipe-transport-data-gouv-fr")) + end + + def is_admin?(%Plug.Conn{} = conn) do + conn |> current_user() |> Map.get(@is_admin_key_name, false) + end + + def is_admin?(%Phoenix.LiveView.Socket{assigns: %{current_user: current_user}}) do + Map.get(current_user, @is_admin_key_name, false) + end + + def is_producer?(%Plug.Conn{} = conn) do + conn |> current_user() |> Map.get(@is_producer_key_name, false) + end + + def is_producer?(%{"organizations" => orgs}) do + org_ids = Enum.map(orgs, & &1["id"]) + DB.Dataset.base_query() |> where([dataset: d], d.organization_id in ^org_ids) |> DB.Repo.exists?() + end + + @spec set_session_attribute_attribute(Plug.Conn.t(), binary(), boolean()) :: Plug.Conn.t() + defp set_session_attribute_attribute(%Plug.Conn{} = conn, key, value) do + current_user = current_user(conn) + conn |> put_session(:current_user, Map.put(current_user, key, value)) + end + + defp current_user(%Plug.Conn{} = conn), do: get_session(conn, :current_user, %{}) +end diff --git a/apps/transport/lib/transport_web/templates/dataset/_dataset_scores_chart.html.heex b/apps/transport/lib/transport_web/templates/dataset/_dataset_scores_chart.html.heex index beae528280..1818a4a7f3 100644 --- a/apps/transport/lib/transport_web/templates/dataset/_dataset_scores_chart.html.heex +++ b/apps/transport/lib/transport_web/templates/dataset/_dataset_scores_chart.html.heex @@ -1,4 +1,4 @@ -
+

<%= dgettext("page-dataset-details", "Dataset scores") %>

diff --git a/apps/transport/lib/transport_web/templates/dataset/details.html.heex b/apps/transport/lib/transport_web/templates/dataset/details.html.heex index 2aa60c82f8..60304c0b54 100644 --- a/apps/transport/lib/transport_web/templates/dataset/details.html.heex +++ b/apps/transport/lib/transport_web/templates/dataset/details.html.heex @@ -16,7 +16,7 @@

<%= @dataset.custom_title %>

- <%= if is_transport_data_gouv_member?(assigns[:current_user]) do %> + <%= if TransportWeb.Session.is_admin?(@conn) do %> <%= link("backoffice", to: backoffice_page_path(@conn, :edit, @dataset.id)) %> <%= render("_dataset_scores.html", dataset_scores: @dataset_scores, locale: locale) %> diff --git a/apps/transport/lib/transport_web/templates/dataset/index.html.heex b/apps/transport/lib/transport_web/templates/dataset/index.html.heex index 110a4544bc..ac240d3be1 100644 --- a/apps/transport/lib/transport_web/templates/dataset/index.html.heex +++ b/apps/transport/lib/transport_web/templates/dataset/index.html.heex @@ -183,7 +183,7 @@ <%= dataset.custom_title %> - <%= if is_transport_data_gouv_member?(assigns[:current_user]) do %> + <%= if TransportWeb.Session.is_admin?(@conn) do %> <%= link("backoffice", to: backoffice_page_path(@conn, :edit, dataset.id)) %> diff --git a/apps/transport/lib/transport_web/templates/layout/_header.html.heex b/apps/transport/lib/transport_web/templates/layout/_header.html.heex index a3a641e65b..7b1f6e1071 100644 --- a/apps/transport/lib/transport_web/templates/layout/_header.html.heex +++ b/apps/transport/lib/transport_web/templates/layout/_header.html.heex @@ -81,10 +81,10 @@ <% end %>
- <%= if is_transport_data_gouv_member?(assigns[:current_user]) do %> + <%= if TransportWeb.Session.is_admin?(@conn) do %> <%= link("Administration", to: "/backoffice") %> <% end %> - <%= if Map.get(assigns[:current_user], "is_producer", false) do %> + <%= if TransportWeb.Session.is_producer?(@conn) do %> <%= link(gettext("Producer space"), to: page_path(@conn, :espace_producteur, utm_source: "menu_dropdown") ) %> diff --git a/apps/transport/test/transport_web/controllers/session_controller_test.exs b/apps/transport/test/transport_web/controllers/session_controller_test.exs index e17320c7da..4df85a9ff3 100644 --- a/apps/transport/test/transport_web/controllers/session_controller_test.exs +++ b/apps/transport/test/transport_web/controllers/session_controller_test.exs @@ -90,11 +90,11 @@ defmodule TransportWeb.SessionControllerTest do test "save_current_user", %{conn: conn} do pan_org = %{"slug" => "equipe-transport-data-gouv-fr", "name" => "PAN", "id" => org_id = Ecto.UUID.generate()} - assert is_admin?(%{"organizations" => [pan_org]}) - refute is_producer?(%{"organizations" => [pan_org]}) + assert TransportWeb.Session.is_admin?(%{"organizations" => [pan_org]}) + refute TransportWeb.Session.is_producer?(%{"organizations" => [pan_org]}) insert(:dataset, organization_id: org_id) # You're a producer if you're a member of an org with an active dataset - assert is_producer?(%{"organizations" => [pan_org]}) + assert TransportWeb.Session.is_producer?(%{"organizations" => [pan_org]}) user_params = %{"foo" => "bar", "organizations" => [pan_org]} diff --git a/apps/transport/test/transport_web/session_text.exs b/apps/transport/test/transport_web/session_text.exs new file mode 100644 index 0000000000..ebaeb26085 --- /dev/null +++ b/apps/transport/test/transport_web/session_text.exs @@ -0,0 +1,42 @@ +defmodule TransportWeb.SessionTest do + use ExUnit.Case, async: true + import DB.Factory + import TransportWeb.Session + doctest TransportWeb.Session, import: true + + @pan_org_id Ecto.UUID.generate() + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(DB.Repo) + end + + test "is_producer?" do + refute is_producer?(%{"organizations" => [pan_org()]}) + insert(:dataset, organization_id: @pan_org_id) + # You're a producer if you're a member of an org with an active dataset + assert is_producer?(%{"organizations" => [pan_org()]}) + end + + test "is_admin?" do + refute is_admin?(%{"organizations" => []}) + refute is_admin?(%{"organizations" => [%{"slug" => "foo"}]}) + assert is_admin?(%{"organizations" => [pan_org()]}) + end + + describe "reader" do + test "is_admin?" do + refute is_admin?(Plug.Test.init_test_session(%Plug.Conn{}, %{})) + assert is_admin?(Plug.Test.init_test_session(%Plug.Conn{}, %{current_user: %{"is_admin" => true}})) + assert is_admin?(%{"is_admin" => true}) + end + + test "is_producer?" do + assert is_producer?(Plug.Test.init_test_session(%Plug.Conn{}, %{current_user: %{"is_producer" => true}})) + refute is_producer?(Plug.Test.init_test_session(%Plug.Conn{}, %{})) + end + end + + def pan_org do + %{"slug" => "equipe-transport-data-gouv-fr", "name" => "PAN", "id" => @pan_org_id} + end +end