From 478e0c69901ce062bdeb50869ae23014c5134040 Mon Sep 17 00:00:00 2001 From: Vini Brasil Date: Wed, 7 Dec 2022 23:32:14 -0300 Subject: [PATCH] Handle GA authentication errors (#2505) --- lib/plausible/google/http.ex | 13 ++--- .../controllers/auth_controller.ex | 33 +++++++++++ .../controllers/site_controller.ex | 42 +++++++++----- .../import_from_google_view_id_form.html.eex | 25 +++------ test/plausible/google/api_test.exs | 56 +++++++++++++++++++ .../controllers/auth_controller_test.exs | 11 ++++ .../controllers/site_controller_test.exs | 23 ++------ 7 files changed, 150 insertions(+), 53 deletions(-) diff --git a/lib/plausible/google/http.ex b/lib/plausible/google/http.ex index fa2a7144d..c8216ace7 100644 --- a/lib/plausible/google/http.ex +++ b/lib/plausible/google/http.ex @@ -135,17 +135,16 @@ defmodule Plausible.Google.HTTP do headers = [{"Authorization", "Bearer #{access_token}"}] - case HTTPClient.get(url, headers) do + case HTTPClient.impl().get(url, headers) do {:ok, %Finch.Response{body: body, status: 200}} -> {:ok, body} - {:error, %{reason: %Finch.Response{body: body}}} -> - Sentry.capture_message("Error fetching Google view ID", extra: %{body: inspect(body)}) - {:error, body} + {:error, %HTTPClient.Non200Error{} = error} when error.reason.status in [401, 403] -> + {:error, :authentication_failed} - {:error, %{reason: reason} = e} -> - Sentry.capture_message("Error fetching Google view ID", extra: %{error: inspect(e)}) - {:error, reason} + {:error, %HTTPClient.Non200Error{} = error} -> + Sentry.capture_message("Error listing GA views for user", extra: %{error: error}) + {:error, :unknown} end end diff --git a/lib/plausible_web/controllers/auth_controller.ex b/lib/plausible_web/controllers/auth_controller.ex index e67a858e2..51721bbe1 100644 --- a/lib/plausible_web/controllers/auth_controller.ex +++ b/lib/plausible_web/controllers/auth_controller.ex @@ -561,6 +561,39 @@ defmodule PlausibleWeb.AuthController do |> redirect(to: redirect_to) end + def google_auth_callback(conn, %{"error" => error, "state" => state} = params) do + [site_id, _redirect_to] = Jason.decode!(state) + site = Repo.get(Plausible.Site, site_id) + + case error do + "access_denied" -> + conn + |> put_flash( + :error, + "We were unable to authenticate your Google Analytics account. Please check that you have granted us permission to 'See and download your Google Analytics data' and try again." + ) + |> redirect(to: Routes.site_path(conn, :settings_general, site.domain)) + + message when message in ["server_error", "temporarily_unavailable"] -> + conn + |> put_flash( + :error, + "We are unable to authenticate your Google Analytics account because Google's authentication service is temporarily unavailable. Please try again in a few moments." + ) + |> redirect(to: Routes.site_path(conn, :settings_general, site.domain)) + + _any -> + Sentry.capture_message("Google OAuth callback failed. Reason: #{inspect(params)}") + + conn + |> put_flash( + :error, + "We were unable to authenticate your Google Analytics account. If the problem persists, please contact support for assistance." + ) + |> redirect(to: Routes.site_path(conn, :settings_general, site.domain)) + end + end + def google_auth_callback(conn, %{"code" => code, "state" => state}) do res = Plausible.Google.HTTP.fetch_access_token(code) [site_id, redirect_to] = Jason.decode!(state) diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index 2a7eb1e44..04d527528 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -686,19 +686,35 @@ defmodule PlausibleWeb.SiteController do "refresh_token" => refresh_token, "expires_at" => expires_at }) do - site = conn.assigns[:site] - view_ids = Plausible.Google.Api.list_views(access_token) + case Plausible.Google.Api.list_views(access_token) do + {:ok, view_ids} -> + conn + |> assign(:skip_plausible_tracking, true) + |> render("import_from_google_view_id_form.html", + access_token: access_token, + refresh_token: refresh_token, + expires_at: expires_at, + site: conn.assigns.site, + view_ids: view_ids, + layout: {PlausibleWeb.LayoutView, "focus.html"} + ) - conn - |> assign(:skip_plausible_tracking, true) - |> render("import_from_google_view_id_form.html", - access_token: access_token, - refresh_token: refresh_token, - expires_at: expires_at, - site: site, - view_ids: view_ids, - layout: {PlausibleWeb.LayoutView, "focus.html"} - ) + {:error, :authentication_failed} -> + conn + |> put_flash( + :error, + "We were unable to authenticate your Google Analytics account. Please check that you have granted us permission to 'See and download your Google Analytics data' and try again." + ) + |> redirect(to: Routes.site_path(conn, :settings_general, conn.assigns.site.domain)) + + {:error, _any} -> + conn + |> put_flash( + :error, + "We were unable to list your Google Analytics properties. If the problem persists, please contact support for assistance." + ) + |> redirect(to: Routes.site_path(conn, :settings_general, conn.assigns.site.domain)) + end end # see https://stackoverflow.com/a/57416769 @@ -715,7 +731,7 @@ defmodule PlausibleWeb.SiteController do case start_date do {:ok, nil} -> site = conn.assigns[:site] - view_ids = Plausible.Google.Api.list_views(access_token) + {:ok, view_ids} = Plausible.Google.Api.list_views(access_token) conn |> assign(:skip_plausible_tracking, true) diff --git a/lib/plausible_web/templates/site/import_from_google_view_id_form.html.eex b/lib/plausible_web/templates/site/import_from_google_view_id_form.html.eex index bd8bbee54..42099ed0a 100644 --- a/lib/plausible_web/templates/site/import_from_google_view_id_form.html.eex +++ b/lib/plausible_web/templates/site/import_from_google_view_id_form.html.eex @@ -5,22 +5,15 @@ <%= hidden_input(f, :refresh_token, value: @refresh_token) %> <%= hidden_input(f, :expires_at, value: @expires_at) %> - <%= case @view_ids do %> - <% {:ok, view_ids} -> %> +
+ Choose the view in your Google Analytics account that will be imported to the <%= @site.domain %> dashboard. +
-
- Choose the view in your Google Analytics account that will be imported to the <%= @site.domain %> dashboard. -
+
+ <%= styled_label(f, :view_id, "Google Analytics view") %> + <%= styled_select f, :view_id, @view_ids, prompt: "(Choose view)", required: "true" %> + <%= styled_error(@conn.assigns[:selected_view_id_error]) %> +
-
- <%= styled_label(f, :view_id, "Google Analytics view") %> - <%= styled_select f, :view_id, view_ids, prompt: "(Choose view)", required: "true" %> - <%= styled_error(@conn.assigns[:selected_view_id_error]) %> -
- <% {:error, error} -> %> -

The following error occurred when fetching your Google Analytics view ids.

-

<%= error %>

- <% end %> - - <%= submit "Continue ->", class: "button mt-6" %> + <%= submit "Continue ->", class: "button mt-6" %> <% end %> diff --git a/test/plausible/google/api_test.exs b/test/plausible/google/api_test.exs index 67ed6ecac..14dfcbeee 100644 --- a/test/plausible/google/api_test.exs +++ b/test/plausible/google/api_test.exs @@ -344,4 +344,60 @@ defmodule Plausible.Google.ApiTest do end end end + + test "list_views/1 returns view IDs grouped by hostname" do + expect( + Plausible.HTTPClient.Mock, + :get, + fn url, _headers -> + assert url == + "https://www.googleapis.com/analytics/v3/management/accounts/~all/webproperties/~all/profiles" + + response = "fixture/ga_list_views.json" |> File.read!() |> Jason.decode!() + {:ok, %Finch.Response{status: 200, body: response}} + end + ) + + assert {:ok, + %{ + "one.test" => [{"57238190 - one.test", "57238190"}], + "two.test" => [{"54460083 - two.test", "54460083"}] + }} == Plausible.Google.Api.list_views("access_token") + end + + test "list_views/1 returns authentication_failed when request fails with HTTP 403" do + expect( + Plausible.HTTPClient.Mock, + :get, + fn _url, _headers -> + {:error, %Plausible.HTTPClient.Non200Error{reason: %{status: 403, body: %{}}}} + end + ) + + assert {:error, :authentication_failed} == Plausible.Google.Api.list_views("access_token") + end + + test "list_views/1 returns authentication_failed when request fails with HTTP 401" do + expect( + Plausible.HTTPClient.Mock, + :get, + fn _url, _headers -> + {:error, %Plausible.HTTPClient.Non200Error{reason: %{status: 401, body: %{}}}} + end + ) + + assert {:error, :authentication_failed} == Plausible.Google.Api.list_views("access_token") + end + + test "list_views/1 returns error when request fails with HTTP 500" do + expect( + Plausible.HTTPClient.Mock, + :get, + fn _url, _headers -> + {:error, %Plausible.HTTPClient.Non200Error{reason: %{status: 500, body: "server error"}}} + end + ) + + assert {:error, :unknown} == Plausible.Google.Api.list_views("access_token") + end end diff --git a/test/plausible_web/controllers/auth_controller_test.exs b/test/plausible_web/controllers/auth_controller_test.exs index ad5cea058..c563c7bf6 100644 --- a/test/plausible_web/controllers/auth_controller_test.exs +++ b/test/plausible_web/controllers/auth_controller_test.exs @@ -759,6 +759,17 @@ defmodule PlausibleWeb.AuthControllerTest do end end + describe "GET /auth/google/callback" do + test "shows error and redirects back to settings when authentication fails", %{conn: conn} do + site = insert(:site) + callback_params = %{"error" => "access_denied", "state" => "[#{site.id},\"import\"]"} + conn = get(conn, Routes.auth_path(conn, :google_auth_callback), callback_params) + + assert redirected_to(conn, 302) == Routes.site_path(conn, :settings_general, site.domain) + assert get_flash(conn, :error) =~ "unable to authenticate your Google Analytics" + end + end + defp mock_captcha_success() do mock_captcha(true) end diff --git a/test/plausible_web/controllers/site_controller_test.exs b/test/plausible_web/controllers/site_controller_test.exs index 872d1edea..39e59c998 100644 --- a/test/plausible_web/controllers/site_controller_test.exs +++ b/test/plausible_web/controllers/site_controller_test.exs @@ -1013,26 +1013,15 @@ defmodule PlausibleWeb.SiteControllerTest do setup [:create_user, :log_in, :create_new_site] test "lists Google Analytics views", %{conn: conn, site: site} do - bypass = Bypass.open() - - Bypass.expect_once( - bypass, - "GET", - "/analytics/v3/management/accounts/~all/webproperties/~all/profiles", - fn conn -> - response_body = File.read!("fixture/ga_list_views.json") - - conn - |> Plug.Conn.put_resp_header("content-type", "application/json") - |> Plug.Conn.resp(200, response_body) + expect( + Plausible.HTTPClient.Mock, + :get, + fn _url, _body -> + body = "fixture/ga_list_views.json" |> File.read!() |> Jason.decode!() + {:ok, %Finch.Response{body: body, status: 200}} end ) - :plausible - |> Application.get_env(:google) - |> Keyword.put(:api_url, "http://0.0.0.0:#{bypass.port}") - |> then(&Application.put_env(:plausible, :google, &1)) - response = conn |> get("/#{site.domain}/import/google-analytics/view-id", %{