From 3ed964b45bd3bc8a2455819a77288117caf7aae8 Mon Sep 17 00:00:00 2001 From: Uku Taht Date: Fri, 9 Apr 2021 10:30:51 +0300 Subject: [PATCH 1/6] Add API endpoints for site and shared link creation --- lib/plausible/site/schema.ex | 3 +- lib/plausible/sites.ex | 38 ++++++++++- .../api/external_sites_controller.ex | 50 ++++++++++++++ .../controllers/site_controller.ex | 30 +-------- .../plugs/authorize_sites_api.ex | 56 ++++++++++++++++ ...ze_api_stats.ex => authorize_stats_api.ex} | 2 +- lib/plausible_web/router.ex | 12 +++- lib/plausible_web/views/site_view.ex | 3 +- .../api/external_sites_controller_test.exs | 67 +++++++++++++++++++ 9 files changed, 225 insertions(+), 36 deletions(-) create mode 100644 lib/plausible_web/controllers/api/external_sites_controller.ex create mode 100644 lib/plausible_web/plugs/authorize_sites_api.ex rename lib/plausible_web/plugs/{authorize_api_stats.ex => authorize_stats_api.ex} (97%) create mode 100644 test/plausible_web/controllers/api/external_sites_controller_test.exs diff --git a/lib/plausible/site/schema.ex b/lib/plausible/site/schema.ex index edb99938c..deaaef63e 100644 --- a/lib/plausible/site/schema.ex +++ b/lib/plausible/site/schema.ex @@ -4,9 +4,10 @@ defmodule Plausible.Site do alias Plausible.Auth.User alias Plausible.Site.GoogleAuth + @derive {Jason.Encoder, only: [:domain, :timezone]} schema "sites" do field :domain, :string - field :timezone, :string + field :timezone, :string, default: "Etc/Greenwich" field :public, :boolean many_to_many :members, User, join_through: Plausible.Site.Membership diff --git a/lib/plausible/sites.ex b/lib/plausible/sites.ex index 66ed33da2..9bc4ca47a 100644 --- a/lib/plausible/sites.ex +++ b/lib/plausible/sites.ex @@ -1,6 +1,42 @@ defmodule Plausible.Sites do use Plausible.Repo - alias Plausible.Site.CustomDomain + alias Plausible.Site.{CustomDomain, SharedLink} + + def create(user_id, params) do + site_changeset = Plausible.Site.changeset(%Plausible.Site{}, params) + + Ecto.Multi.new() + |> Ecto.Multi.insert(:site, site_changeset) + |> Ecto.Multi.run(:site_membership, fn repo, %{site: site} -> + membership_changeset = + Plausible.Site.Membership.changeset(%Plausible.Site.Membership{}, %{ + site_id: site.id, + user_id: user_id + }) + + repo.insert(membership_changeset) + end) + |> Repo.transaction() + end + + def create_shared_link(site, name) do + changes = + SharedLink.changeset( + %SharedLink{ + site_id: site.id, + slug: Nanoid.generate() + }, + %{name: name} + ) + + Repo.insert(changes) + end + + def shared_link_url(site, link) do + base = PlausibleWeb.Endpoint.url() + domain = "/share/#{URI.encode_www_form(site.domain)}" + base <> domain <> "?auth=" <> link.slug + end def get_for_user!(user_id, domain) do Repo.one!( diff --git a/lib/plausible_web/controllers/api/external_sites_controller.ex b/lib/plausible_web/controllers/api/external_sites_controller.ex new file mode 100644 index 000000000..8111bfa43 --- /dev/null +++ b/lib/plausible_web/controllers/api/external_sites_controller.ex @@ -0,0 +1,50 @@ +defmodule PlausibleWeb.Api.ExternalSitesController do + use PlausibleWeb, :controller + use Plausible.Repo + use Plug.ErrorHandler + alias Plausible.Sites + + def create_site(conn, params) do + user_id = conn.assigns[:current_user_id] + site_params = Map.get(params, "site", %{}) + + case Sites.create(user_id, site_params) do + {:ok, %{site: site}} -> + json(conn, site) + + {:error, :site, changeset, _} -> + conn + |> put_status(400) + |> json(serialize_errors(changeset)) + end + end + + def find_or_create_shared_link(conn, %{"domain" => domain, "link_name" => link_name}) do + site = Sites.get_for_user!(conn.assigns[:current_user_id], domain) + shared_link = Repo.get_by(Plausible.Site.SharedLink, site_id: site.id, name: link_name) + + shared_link = + case shared_link do + nil -> Sites.create_shared_link(site, link_name) + link -> {:ok, link} + end + + case shared_link do + {:ok, link} -> + json(conn, %{ + name: link.name, + url: Sites.shared_link_url(site, link) + }) + end + end + + defp serialize_errors(changeset) do + {field, {msg, _opts}} = List.first(changeset.errors) + error_msg = Atom.to_string(field) <> " " <> msg + %{"error" => error_msg} + end + + def handle_errors(conn, %{kind: kind, reason: reason}) do + json(conn, %{error: Exception.format_banner(kind, reason)}) + end +end diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index 8470d53d8..20bcd236f 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -41,7 +41,7 @@ defmodule PlausibleWeb.SiteController do def create_site(conn, %{"site" => site_params}) do user = conn.assigns[:current_user] - case insert_site(user.id, site_params) do + case Sites.create(user.id, site_params) do {:ok, %{site: site}} -> Plausible.Slack.notify("#{user.name} created #{site.domain} [email=#{user.email}]") @@ -517,16 +517,7 @@ defmodule PlausibleWeb.SiteController do def create_shared_link(conn, %{"website" => website, "shared_link" => link}) do site = Sites.get_for_user!(conn.assigns[:current_user].id, website) - changes = - Plausible.Site.SharedLink.changeset( - %Plausible.Site.SharedLink{ - site_id: site.id, - slug: Nanoid.generate() - }, - link - ) - - case Repo.insert(changes) do + case Sites.create_shared_link(site, link["name"]) do {:ok, _created} -> redirect(conn, to: "/#{URI.encode_www_form(site.domain)}/settings/visibility") @@ -652,21 +643,4 @@ defmodule PlausibleWeb.SiteController do |> put_flash(:success, "Custom domain deleted successfully") |> redirect(to: "/#{URI.encode_www_form(site.domain)}/settings/custom-domain") end - - defp insert_site(user_id, params) do - site_changeset = Plausible.Site.changeset(%Plausible.Site{}, params) - - Ecto.Multi.new() - |> Ecto.Multi.insert(:site, site_changeset) - |> Ecto.Multi.run(:site_membership, fn repo, %{site: site} -> - membership_changeset = - Plausible.Site.Membership.changeset(%Plausible.Site.Membership{}, %{ - site_id: site.id, - user_id: user_id - }) - - repo.insert(membership_changeset) - end) - |> Repo.transaction() - end end diff --git a/lib/plausible_web/plugs/authorize_sites_api.ex b/lib/plausible_web/plugs/authorize_sites_api.ex new file mode 100644 index 000000000..da2c10888 --- /dev/null +++ b/lib/plausible_web/plugs/authorize_sites_api.ex @@ -0,0 +1,56 @@ +defmodule PlausibleWeb.AuthorizeSitesApiPlug do + import Plug.Conn + use Plausible.Repo + alias Plausible.Auth.ApiKey + + def init(options) do + options + end + + def call(conn, _opts) do + with {:ok, raw_api_key} <- get_bearer_token(conn), + {:ok, api_key} <- verify_access(raw_api_key) do + assign(conn, :current_user_id, api_key.user_id) + else + {:error, :missing_api_key} -> + unauthorized( + conn, + "Missing API key. Please use a valid Plausible API key as a Bearer Token." + ) + + {:error, :invalid_api_key} -> + unauthorized( + conn, + "Invalid API key. Please make sure you're using a valid API key with access to the site you've requested." + ) + end + end + + defp verify_access(api_key) do + hashed_key = ApiKey.do_hash(api_key) + found_key = Repo.get_by(ApiKey, key_hash: hashed_key) + + cond do + found_key -> {:ok, found_key} + true -> {:error, :invalid_api_key} + end + end + + defp get_bearer_token(conn) do + authorization_header = + Plug.Conn.get_req_header(conn, "authorization") + |> List.first() + + case authorization_header do + "Bearer " <> token -> {:ok, String.trim(token)} + _ -> {:error, :missing_api_key} + end + end + + defp unauthorized(conn, msg) do + conn + |> put_status(401) + |> Phoenix.Controller.json(%{error: msg}) + |> halt() + end +end diff --git a/lib/plausible_web/plugs/authorize_api_stats.ex b/lib/plausible_web/plugs/authorize_stats_api.ex similarity index 97% rename from lib/plausible_web/plugs/authorize_api_stats.ex rename to lib/plausible_web/plugs/authorize_stats_api.ex index 4d1e6cde9..961b738f0 100644 --- a/lib/plausible_web/plugs/authorize_api_stats.ex +++ b/lib/plausible_web/plugs/authorize_stats_api.ex @@ -1,4 +1,4 @@ -defmodule PlausibleWeb.AuthorizeApiStatsPlug do +defmodule PlausibleWeb.AuthorizeStatsApiPlug do import Plug.Conn use Plausible.Repo alias Plausible.Auth.ApiKey diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index c49cd01a8..3ba4ea486 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -36,10 +36,9 @@ defmodule PlausibleWeb.Router do plug PlausibleWeb.AuthorizeStatsPlug end - pipeline :external_stats_api do + pipeline :public_api do plug :accepts, ["json"] plug PlausibleWeb.Firewall - plug PlausibleWeb.AuthorizeApiStatsPlug end if Application.get_env(:plausible, :environment) == "dev" do @@ -73,7 +72,7 @@ defmodule PlausibleWeb.Router do end scope "/api/v1/stats", PlausibleWeb.Api do - pipe_through :external_stats_api + pipe_through [:public_api, PlausibleWeb.AuthorizeStatsApiPlug] get "/realtime/visitors", ExternalStatsController, :realtime_visitors get "/aggregate", ExternalStatsController, :aggregate @@ -81,6 +80,13 @@ defmodule PlausibleWeb.Router do get "/timeseries", ExternalStatsController, :timeseries end + scope "/api/v1/sites", PlausibleWeb.Api do + pipe_through [:public_api, PlausibleWeb.AuthorizeSitesApiPlug] + + post "/", ExternalSitesController, :create_site + put "/:domain/shared-links/:link_name", ExternalSitesController, :find_or_create_shared_link + end + scope "/api", PlausibleWeb do pipe_through :api diff --git a/lib/plausible_web/views/site_view.ex b/lib/plausible_web/views/site_view.ex index f93235d5c..5fd8dfcae 100644 --- a/lib/plausible_web/views/site_view.ex +++ b/lib/plausible_web/views/site_view.ex @@ -22,8 +22,7 @@ defmodule PlausibleWeb.SiteView do end def shared_link_dest(site, link) do - domain = "/share/#{URI.encode_www_form(site.domain)}" - plausible_url() <> domain <> "?auth=" <> link.slug + Plausible.Sites.shared_link_url(site, link) end def snippet(site) do diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs new file mode 100644 index 000000000..9a33cb3d3 --- /dev/null +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -0,0 +1,67 @@ +defmodule PlausibleWeb.Api.ExternalSitesControllerTest do + use PlausibleWeb.ConnCase + import Plausible.TestUtils + + setup [:create_user, :create_api_key, :use_api_key] + + describe "POST /api/v1/sites" do + test "can create a site", %{conn: conn} do + conn = + post(conn, "/api/v1/sites", %{ + "site" => %{ + "domain" => "some-site.domain", + "timezone" => "Europe/Tallinn" + } + }) + + assert json_response(conn, 200) == %{ + "domain" => "some-site.domain", + "timezone" => "Europe/Tallinn" + } + end + + test "timezone defaults to Etc/Greenwich", %{conn: conn} do + conn = + post(conn, "/api/v1/sites", %{ + "site" => %{ + "domain" => "some-site.domain" + } + }) + + assert json_response(conn, 200) == %{ + "domain" => "some-site.domain", + "timezone" => "Etc/Greenwich" + } + end + + test "domain is required", %{conn: conn} do + conn = post(conn, "/api/v1/sites", %{}) + + assert json_response(conn, 400) == %{ + "error" => "domain can't be blank" + } + end + end + + describe "PUT /api/v1/sites/shared-links/:link_name" do + setup :create_site + + test "can add a shared link to a site", %{conn: conn, site: site} do + conn = put(conn, "/api/v1/sites/#{site.domain}/shared-links/Wordpress") + + res = json_response(conn, 200) + assert res["name"] == "Wordpress" + assert String.starts_with?(res["url"], "http://") + end + + test "is idempotent find or create op", %{conn: conn, site: site} do + conn = put(conn, "/api/v1/sites/#{site.domain}/shared-links/Wordpress") + + %{"url" => url} = json_response(conn, 200) + + conn = put(conn, "/api/v1/sites/#{site.domain}/shared-links/Wordpress") + + assert %{"url" => ^url} = json_response(conn, 200) + end + end +end From 2fc136777c1f9a2a993670a7071f1d0ac87fa29f Mon Sep 17 00:00:00 2001 From: Uku Taht Date: Fri, 9 Apr 2021 11:24:25 +0300 Subject: [PATCH 2/6] Add unique index to shared link name --- lib/plausible/auth/api_key.ex | 1 + lib/plausible/site/shared_link.ex | 1 + lib/plausible_web/templates/site/edit_shared_link.html.eex | 2 +- ...20210409074413_add_unique_index_to_shared_link_name.exs | 7 +++++++ 4 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 priv/repo/migrations/20210409074413_add_unique_index_to_shared_link_name.exs diff --git a/lib/plausible/auth/api_key.ex b/lib/plausible/auth/api_key.ex index 71fcd111b..13c700d3e 100644 --- a/lib/plausible/auth/api_key.ex +++ b/lib/plausible/auth/api_key.ex @@ -5,6 +5,7 @@ defmodule Plausible.Auth.ApiKey do @required [:user_id, :key, :name] schema "api_keys" do field :name, :string + field :key, :string, virtual: true field :key_hash, :string field :key_prefix, :string diff --git a/lib/plausible/site/shared_link.ex b/lib/plausible/site/shared_link.ex index a63c3f59b..e01c582f8 100644 --- a/lib/plausible/site/shared_link.ex +++ b/lib/plausible/site/shared_link.ex @@ -17,6 +17,7 @@ defmodule Plausible.Site.SharedLink do |> cast(attrs, [:slug, :password, :name]) |> validate_required([:slug, :name]) |> unique_constraint(:slug) + |> unique_constraint(:name, name: :shared_links_site_id_name_index) |> hash_password() end diff --git a/lib/plausible_web/templates/site/edit_shared_link.html.eex b/lib/plausible_web/templates/site/edit_shared_link.html.eex index 00b98d0b3..e36d12fb9 100644 --- a/lib/plausible_web/templates/site/edit_shared_link.html.eex +++ b/lib/plausible_web/templates/site/edit_shared_link.html.eex @@ -8,5 +8,5 @@ - <%= submit "Edit shared link", class: "button mt-4 w-full" %> + <%= submit "Update", class: "button mt-4 w-full" %> <% end %> diff --git a/priv/repo/migrations/20210409074413_add_unique_index_to_shared_link_name.exs b/priv/repo/migrations/20210409074413_add_unique_index_to_shared_link_name.exs new file mode 100644 index 000000000..585e79de2 --- /dev/null +++ b/priv/repo/migrations/20210409074413_add_unique_index_to_shared_link_name.exs @@ -0,0 +1,7 @@ +defmodule Plausible.Repo.Migrations.AddUniqueIndexToSharedLinkName do + use Ecto.Migration + + def change do + create unique_index(:shared_links, [:site_id, :name], name: :shared_links_site_id_name_index) + end +end From d473accf408a16f7a4a2a3d5c54e618654e2ccd7 Mon Sep 17 00:00:00 2001 From: Uku Taht Date: Fri, 9 Apr 2021 11:53:41 +0300 Subject: [PATCH 3/6] Add API key scopes --- lib/plausible/auth/api_key.ex | 1 + .../plugs/authorize_sites_api.ex | 10 ++++++-- .../20210409082603_add_api_key_scopes.exs | 24 +++++++++++++++++++ .../api/external_sites_controller_test.exs | 21 +++++++++++++++- 4 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 priv/repo/migrations/20210409082603_add_api_key_scopes.exs diff --git a/lib/plausible/auth/api_key.ex b/lib/plausible/auth/api_key.ex index 13c700d3e..c07f1ee24 100644 --- a/lib/plausible/auth/api_key.ex +++ b/lib/plausible/auth/api_key.ex @@ -5,6 +5,7 @@ defmodule Plausible.Auth.ApiKey do @required [:user_id, :key, :name] schema "api_keys" do field :name, :string + field :scopes, {:array, :string}, default: ["stats:read:*"] field :key, :string, virtual: true field :key_hash, :string diff --git a/lib/plausible_web/plugs/authorize_sites_api.ex b/lib/plausible_web/plugs/authorize_sites_api.ex index da2c10888..46484499d 100644 --- a/lib/plausible_web/plugs/authorize_sites_api.ex +++ b/lib/plausible_web/plugs/authorize_sites_api.ex @@ -21,14 +21,20 @@ defmodule PlausibleWeb.AuthorizeSitesApiPlug do {:error, :invalid_api_key} -> unauthorized( conn, - "Invalid API key. Please make sure you're using a valid API key with access to the site you've requested." + "Invalid API key. Please make sure you're using a valid API key with access to the resource you've requested." ) end end defp verify_access(api_key) do hashed_key = ApiKey.do_hash(api_key) - found_key = Repo.get_by(ApiKey, key_hash: hashed_key) + + found_key = + Repo.one( + from a in ApiKey, + where: a.key_hash == ^hashed_key, + where: fragment("? @> ?", a.scopes, ["sites:provision:*"]) + ) cond do found_key -> {:ok, found_key} diff --git a/priv/repo/migrations/20210409082603_add_api_key_scopes.exs b/priv/repo/migrations/20210409082603_add_api_key_scopes.exs new file mode 100644 index 000000000..ab3ea600e --- /dev/null +++ b/priv/repo/migrations/20210409082603_add_api_key_scopes.exs @@ -0,0 +1,24 @@ +defmodule Plausible.Repo.Migrations.AddApiKeyScopes do + use Ecto.Migration + + def up do + alter table(:api_keys) do + add :scopes, {:array, :text} + end + + execute "UPDATE api_keys SET scopes='{stats:read:*}'" + + alter table(:api_keys) do + modify :scopes, {:array, :text}, null: false + end + + # https://stackoverflow.com/a/4059785 + create index(:api_keys, [:scopes], using: "GIN") + end + + def down do + alter table(:api_keys) do + remove :scopes + end + end +end diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index 9a33cb3d3..f5cdabf97 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -2,7 +2,12 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do use PlausibleWeb.ConnCase import Plausible.TestUtils - setup [:create_user, :create_api_key, :use_api_key] + setup %{conn: conn} do + user = insert(:user) + api_key = insert(:api_key, user: user, scopes: ["sites:provision:*"]) + conn = Plug.Conn.put_req_header(conn, "authorization", "Bearer #{api_key.key}") + {:ok, user: user, api_key: api_key, conn: conn} + end describe "POST /api/v1/sites" do test "can create a site", %{conn: conn} do @@ -41,6 +46,20 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do "error" => "domain can't be blank" } end + + test "cannot access with a bad API key scope", %{conn: conn, user: user} do + api_key = insert(:api_key, user: user, scopes: ["stats:read:*"]) + + conn = + conn + |> Plug.Conn.put_req_header("authorization", "Bearer #{api_key.key}") + |> post("/api/v1/sites", %{"site" => %{"domain" => "domain.com"}}) + + assert json_response(conn, 401) == %{ + "error" => + "Invalid API key. Please make sure you're using a valid API key with access to the resource you've requested." + } + end end describe "PUT /api/v1/sites/shared-links/:link_name" do From 80abfa6ef6238a7d3a06ac92903a6055d25e5d5f Mon Sep 17 00:00:00 2001 From: Uku Taht Date: Wed, 14 Apr 2021 15:04:25 +0300 Subject: [PATCH 4/6] Add validations --- lib/plausible/auth/api_key.ex | 3 +- lib/plausible/sites.ex | 18 ++++---- .../api/external_sites_controller.ex | 44 +++++++++++++------ lib/plausible_web/router.ex | 2 +- .../api/external_sites_controller_test.exs | 30 ++++++++----- 5 files changed, 63 insertions(+), 34 deletions(-) diff --git a/lib/plausible/auth/api_key.ex b/lib/plausible/auth/api_key.ex index c07f1ee24..61cbf66f2 100644 --- a/lib/plausible/auth/api_key.ex +++ b/lib/plausible/auth/api_key.ex @@ -3,6 +3,7 @@ defmodule Plausible.Auth.ApiKey do import Ecto.Changeset @required [:user_id, :key, :name] + @optional [:scopes] schema "api_keys" do field :name, :string field :scopes, {:array, :string}, default: ["stats:read:*"] @@ -18,7 +19,7 @@ defmodule Plausible.Auth.ApiKey do def changeset(schema, attrs \\ %{}) do schema - |> cast(attrs, @required) + |> cast(attrs, @required ++ @optional) |> validate_required(@required) |> process_key end diff --git a/lib/plausible/sites.ex b/lib/plausible/sites.ex index 9bc4ca47a..4031388bc 100644 --- a/lib/plausible/sites.ex +++ b/lib/plausible/sites.ex @@ -38,14 +38,16 @@ defmodule Plausible.Sites do base <> domain <> "?auth=" <> link.slug end - def get_for_user!(user_id, domain) do - Repo.one!( - from s in Plausible.Site, - join: sm in Plausible.Site.Membership, - on: sm.site_id == s.id, - where: sm.user_id == ^user_id, - where: s.domain == ^domain, - select: s + def get_for_user!(user_id, domain), do: Repo.one!(get_for_user_q(user_id, domain)) + def get_for_user(user_id, domain), do: Repo.one(get_for_user_q(user_id, domain)) + + def get_for_user_q(user_id, domain) do + from(s in Plausible.Site, + join: sm in Plausible.Site.Membership, + on: sm.site_id == s.id, + where: sm.user_id == ^user_id, + where: s.domain == ^domain, + select: s ) end diff --git a/lib/plausible_web/controllers/api/external_sites_controller.ex b/lib/plausible_web/controllers/api/external_sites_controller.ex index 8111bfa43..30e3e3829 100644 --- a/lib/plausible_web/controllers/api/external_sites_controller.ex +++ b/lib/plausible_web/controllers/api/external_sites_controller.ex @@ -6,9 +6,8 @@ defmodule PlausibleWeb.Api.ExternalSitesController do def create_site(conn, params) do user_id = conn.assigns[:current_user_id] - site_params = Map.get(params, "site", %{}) - case Sites.create(user_id, site_params) do + case Sites.create(user_id, params) do {:ok, %{site: site}} -> json(conn, site) @@ -19,22 +18,39 @@ defmodule PlausibleWeb.Api.ExternalSitesController do end end - def find_or_create_shared_link(conn, %{"domain" => domain, "link_name" => link_name}) do - site = Sites.get_for_user!(conn.assigns[:current_user_id], domain) - shared_link = Repo.get_by(Plausible.Site.SharedLink, site_id: site.id, name: link_name) + def find_or_create_shared_link(conn, %{"link_name" => link_name} = params) do + with {:ok, site_id} <- Map.fetch(params, "site_id"), + site when not is_nil(site) <- Sites.get_for_user(conn.assigns[:current_user_id], site_id) do + shared_link = Repo.get_by(Plausible.Site.SharedLink, site_id: site.id, name: link_name) + + shared_link = + case shared_link do + nil -> Sites.create_shared_link(site, link_name) + link -> {:ok, link} + end - shared_link = case shared_link do - nil -> Sites.create_shared_link(site, link_name) - link -> {:ok, link} + {:ok, link} -> + json(conn, %{ + name: link.name, + url: Sites.shared_link_url(site, link) + }) end + else + nil -> + conn + |> put_status(404) + |> json(%{error: "Site could not be found"}) - case shared_link do - {:ok, link} -> - json(conn, %{ - name: link.name, - url: Sites.shared_link_url(site, link) - }) + :error -> + conn + |> put_status(400) + |> json(%{error: "Query parameter `site_id` is required to create a shared link"}) + + e -> + conn + |> put_status(400) + |> json(%{error: "Something went wrong: #{inspect(e)}"}) end end diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 3ba4ea486..1a325cb7b 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -84,7 +84,7 @@ defmodule PlausibleWeb.Router do pipe_through [:public_api, PlausibleWeb.AuthorizeSitesApiPlug] post "/", ExternalSitesController, :create_site - put "/:domain/shared-links/:link_name", ExternalSitesController, :find_or_create_shared_link + put "/shared-links/:link_name", ExternalSitesController, :find_or_create_shared_link end scope "/api", PlausibleWeb do diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index f5cdabf97..fb02170b5 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -13,10 +13,8 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do test "can create a site", %{conn: conn} do conn = post(conn, "/api/v1/sites", %{ - "site" => %{ - "domain" => "some-site.domain", - "timezone" => "Europe/Tallinn" - } + "domain" => "some-site.domain", + "timezone" => "Europe/Tallinn" }) assert json_response(conn, 200) == %{ @@ -28,9 +26,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do test "timezone defaults to Etc/Greenwich", %{conn: conn} do conn = post(conn, "/api/v1/sites", %{ - "site" => %{ - "domain" => "some-site.domain" - } + "domain" => "some-site.domain" }) assert json_response(conn, 200) == %{ @@ -66,7 +62,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do setup :create_site test "can add a shared link to a site", %{conn: conn, site: site} do - conn = put(conn, "/api/v1/sites/#{site.domain}/shared-links/Wordpress") + conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=#{site.domain}") res = json_response(conn, 200) assert res["name"] == "Wordpress" @@ -74,13 +70,27 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do end test "is idempotent find or create op", %{conn: conn, site: site} do - conn = put(conn, "/api/v1/sites/#{site.domain}/shared-links/Wordpress") + conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=#{site.domain}") %{"url" => url} = json_response(conn, 200) - conn = put(conn, "/api/v1/sites/#{site.domain}/shared-links/Wordpress") + conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=#{site.domain}") assert %{"url" => ^url} = json_response(conn, 200) end + + test "returns 400 when site id missing", %{conn: conn} do + conn = put(conn, "/api/v1/sites/shared-links/Wordpress") + + res = json_response(conn, 400) + assert res["error"] == "Query parameter `site_id` is required to create a shared link" + end + + test "returns 404 when site id is non existent", %{conn: conn} do + conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=bad") + + res = json_response(conn, 404) + assert res["error"] == "Site could not be found" + end end end From 956e92f161f7b0fbe9cb0dbe24ec55c05c62b4cf Mon Sep 17 00:00:00 2001 From: Uku Taht Date: Thu, 15 Apr 2021 11:03:06 +0300 Subject: [PATCH 5/6] Change default timezone to Etc/UTC --- lib/plausible/site/schema.ex | 2 +- .../controllers/api/external_sites_controller_test.exs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/plausible/site/schema.ex b/lib/plausible/site/schema.ex index deaaef63e..b0f907160 100644 --- a/lib/plausible/site/schema.ex +++ b/lib/plausible/site/schema.ex @@ -7,7 +7,7 @@ defmodule Plausible.Site do @derive {Jason.Encoder, only: [:domain, :timezone]} schema "sites" do field :domain, :string - field :timezone, :string, default: "Etc/Greenwich" + field :timezone, :string, default: "Etc/UTC" field :public, :boolean many_to_many :members, User, join_through: Plausible.Site.Membership diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index fb02170b5..0a7bb50cb 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -23,7 +23,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do } end - test "timezone defaults to Etc/Greenwich", %{conn: conn} do + test "timezone defaults to Etc/UTC", %{conn: conn} do conn = post(conn, "/api/v1/sites", %{ "domain" => "some-site.domain" @@ -31,7 +31,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert json_response(conn, 200) == %{ "domain" => "some-site.domain", - "timezone" => "Etc/Greenwich" + "timezone" => "Etc/UTC" } end From 70f0657327ff05c73bbceac0840de8fcc8830f3d Mon Sep 17 00:00:00 2001 From: Uku Taht Date: Thu, 15 Apr 2021 11:38:44 +0300 Subject: [PATCH 6/6] Update shared link API --- .../api/external_sites_controller.ex | 30 +++++++++++-------- lib/plausible_web/controllers/api/helpers.ex | 24 +++++++++++++++ .../plugs/authorize_sites_api.ex | 12 ++------ .../plugs/authorize_stats_api.ex | 21 +++---------- lib/plausible_web/router.ex | 2 +- .../api/external_sites_controller_test.exs | 28 ++++++++++++----- 6 files changed, 71 insertions(+), 46 deletions(-) create mode 100644 lib/plausible_web/controllers/api/helpers.ex diff --git a/lib/plausible_web/controllers/api/external_sites_controller.ex b/lib/plausible_web/controllers/api/external_sites_controller.ex index 30e3e3829..c9cca48b8 100644 --- a/lib/plausible_web/controllers/api/external_sites_controller.ex +++ b/lib/plausible_web/controllers/api/external_sites_controller.ex @@ -3,6 +3,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do use Plausible.Repo use Plug.ErrorHandler alias Plausible.Sites + alias PlausibleWeb.Api.Helpers, as: H def create_site(conn, params) do user_id = conn.assigns[:current_user_id] @@ -18,8 +19,16 @@ defmodule PlausibleWeb.Api.ExternalSitesController do end end - def find_or_create_shared_link(conn, %{"link_name" => link_name} = params) do - with {:ok, site_id} <- Map.fetch(params, "site_id"), + defp expect_param_key(params, key) do + case Map.fetch(params, key) do + :error -> {:missing, key} + res -> res + end + end + + def find_or_create_shared_link(conn, params) do + with {:ok, site_id} <- expect_param_key(params, "site_id"), + {:ok, link_name} <- expect_param_key(params, "name"), site when not is_nil(site) <- Sites.get_for_user(conn.assigns[:current_user_id], site_id) do shared_link = Repo.get_by(Plausible.Site.SharedLink, site_id: site.id, name: link_name) @@ -38,19 +47,16 @@ defmodule PlausibleWeb.Api.ExternalSitesController do end else nil -> - conn - |> put_status(404) - |> json(%{error: "Site could not be found"}) + H.not_found(conn, "Site could not be found") - :error -> - conn - |> put_status(400) - |> json(%{error: "Query parameter `site_id` is required to create a shared link"}) + {:missing, "site_id"} -> + H.bad_request(conn, "Parameter `site_id` is required to create a shared link") + + {:missing, "name"} -> + H.bad_request(conn, "Parameter `name` is required to create a shared link") e -> - conn - |> put_status(400) - |> json(%{error: "Something went wrong: #{inspect(e)}"}) + H.bad_request(400, "Something went wrong: #{inspect(e)}") end end diff --git a/lib/plausible_web/controllers/api/helpers.ex b/lib/plausible_web/controllers/api/helpers.ex new file mode 100644 index 000000000..068c24cf2 --- /dev/null +++ b/lib/plausible_web/controllers/api/helpers.ex @@ -0,0 +1,24 @@ +defmodule PlausibleWeb.Api.Helpers do + import Plug.Conn + + def unauthorized(conn, msg) do + conn + |> put_status(401) + |> Phoenix.Controller.json(%{error: msg}) + |> halt() + end + + def bad_request(conn, msg) do + conn + |> put_status(400) + |> Phoenix.Controller.json(%{error: msg}) + |> halt() + end + + def not_found(conn, msg) do + conn + |> put_status(404) + |> Phoenix.Controller.json(%{error: msg}) + |> halt() + end +end diff --git a/lib/plausible_web/plugs/authorize_sites_api.ex b/lib/plausible_web/plugs/authorize_sites_api.ex index 46484499d..125f7b153 100644 --- a/lib/plausible_web/plugs/authorize_sites_api.ex +++ b/lib/plausible_web/plugs/authorize_sites_api.ex @@ -2,6 +2,7 @@ defmodule PlausibleWeb.AuthorizeSitesApiPlug do import Plug.Conn use Plausible.Repo alias Plausible.Auth.ApiKey + alias PlausibleWeb.Api.Helpers, as: H def init(options) do options @@ -13,13 +14,13 @@ defmodule PlausibleWeb.AuthorizeSitesApiPlug do assign(conn, :current_user_id, api_key.user_id) else {:error, :missing_api_key} -> - unauthorized( + H.unauthorized( conn, "Missing API key. Please use a valid Plausible API key as a Bearer Token." ) {:error, :invalid_api_key} -> - unauthorized( + H.unauthorized( conn, "Invalid API key. Please make sure you're using a valid API key with access to the resource you've requested." ) @@ -52,11 +53,4 @@ defmodule PlausibleWeb.AuthorizeSitesApiPlug do _ -> {:error, :missing_api_key} end end - - defp unauthorized(conn, msg) do - conn - |> put_status(401) - |> Phoenix.Controller.json(%{error: msg}) - |> halt() - end end diff --git a/lib/plausible_web/plugs/authorize_stats_api.ex b/lib/plausible_web/plugs/authorize_stats_api.ex index 961b738f0..b41daadff 100644 --- a/lib/plausible_web/plugs/authorize_stats_api.ex +++ b/lib/plausible_web/plugs/authorize_stats_api.ex @@ -2,6 +2,7 @@ defmodule PlausibleWeb.AuthorizeStatsApiPlug do import Plug.Conn use Plausible.Repo alias Plausible.Auth.ApiKey + alias PlausibleWeb.Api.Helpers, as: H def init(options) do options @@ -13,19 +14,19 @@ defmodule PlausibleWeb.AuthorizeStatsApiPlug do assign(conn, :site, site) else {:error, :missing_api_key} -> - unauthorized( + H.unauthorized( conn, "Missing API key. Please use a valid Plausible API key as a Bearer Token." ) {:error, :missing_site_id} -> - bad_request( + H.bad_request( conn, "Missing site ID. Please provide the required site_id parameter with your request." ) {:error, :invalid_api_key} -> - unauthorized( + H.unauthorized( conn, "Invalid API key or site ID. Please make sure you're using a valid API key with access to the site you've requested." ) @@ -56,18 +57,4 @@ defmodule PlausibleWeb.AuthorizeStatsApiPlug do _ -> {:error, :missing_api_key} end end - - defp bad_request(conn, msg) do - conn - |> put_status(400) - |> Phoenix.Controller.json(%{error: msg}) - |> halt() - end - - defp unauthorized(conn, msg) do - conn - |> put_status(401) - |> Phoenix.Controller.json(%{error: msg}) - |> halt() - end end diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 1a325cb7b..c7cbf68f4 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -84,7 +84,7 @@ defmodule PlausibleWeb.Router do pipe_through [:public_api, PlausibleWeb.AuthorizeSitesApiPlug] post "/", ExternalSitesController, :create_site - put "/shared-links/:link_name", ExternalSitesController, :find_or_create_shared_link + put "/shared-links", ExternalSitesController, :find_or_create_shared_link end scope "/api", PlausibleWeb do diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index 0a7bb50cb..65fb54e46 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -58,11 +58,14 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do end end - describe "PUT /api/v1/sites/shared-links/:link_name" do + describe "PUT /api/v1/sites/shared-links" do setup :create_site test "can add a shared link to a site", %{conn: conn, site: site} do - conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=#{site.domain}") + conn = put(conn, "/api/v1/sites/shared-links", %{ + site_id: site.domain, + name: "Wordpress" + }) res = json_response(conn, 200) assert res["name"] == "Wordpress" @@ -70,24 +73,35 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do end test "is idempotent find or create op", %{conn: conn, site: site} do - conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=#{site.domain}") + conn = put(conn, "/api/v1/sites/shared-links", %{ + site_id: site.domain, + name: "Wordpress" + }) %{"url" => url} = json_response(conn, 200) - conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=#{site.domain}") + conn = put(conn, "/api/v1/sites/shared-links", %{ + site_id: site.domain, + name: "Wordpress" + }) assert %{"url" => ^url} = json_response(conn, 200) end test "returns 400 when site id missing", %{conn: conn} do - conn = put(conn, "/api/v1/sites/shared-links/Wordpress") + conn = put(conn, "/api/v1/sites/shared-links", %{ + name: "Wordpress" + }) res = json_response(conn, 400) - assert res["error"] == "Query parameter `site_id` is required to create a shared link" + assert res["error"] == "Parameter `site_id` is required to create a shared link" end test "returns 404 when site id is non existent", %{conn: conn} do - conn = put(conn, "/api/v1/sites/shared-links/Wordpress?site_id=bad") + conn = put(conn, "/api/v1/sites/shared-links", %{ + name: "Wordpress", + site_id: "bad" + }) res = json_response(conn, 404) assert res["error"] == "Site could not be found"