Merge pull request 'Fix OAuth consumer mode' (#668) from tcmal/akkoma:develop into develop

Reviewed-on: AkkomaGang/akkoma#668

docs/docs/configuration/cheatsheet.md

@@ -958,6 +958,15 @@ config :ueberauth, Ueberauth,
   ]
 ```
 
+You may also need to set up your frontend to use oauth logins. For example, for `akkoma-fe`:
+
+```elixir
+config :pleroma, :frontend_configurations,
+  pleroma_fe: %{
+    loginMethod: "token"
+  }
+```
+
 ## Link parsing
 
 ### :uri_schemes

lib/pleroma/web/o_auth/o_auth_controller.ex

@@ -39,6 +39,7 @@ defmodule Pleroma.Web.OAuth.OAuthController do
   action_fallback(Pleroma.Web.OAuth.FallbackController)
 
   @oob_token_redirect_uri "urn:ietf:wg:oauth:2.0:oob"
+  @state_cookie_name "akkoma_oauth_state"
 
   # Note: this definition is only called from error-handling methods with `conn.params` as 2nd arg
   def authorize(%Plug.Conn{} = conn, %{"authorization" => _} = params) do
@@ -443,13 +444,10 @@ def prepare_request(%Plug.Conn{} = conn, %{
       |> Map.put("scope", scope)
       |> Jason.encode!()
 
-    params =
-      auth_attrs
-      |> Map.drop(~w(scope scopes client_id redirect_uri))
-      |> Map.put("state", state)
-
     # Handing the request to Ueberauth
-    redirect(conn, to: ~p"/oauth/#{provider}?#{params}")
+    conn
+    |> put_resp_cookie(@state_cookie_name, state)
+    |> redirect(to: ~p"/oauth/#{provider}")
   end
 
   def request(%Plug.Conn{} = conn, params) do
@@ -468,20 +466,26 @@ def request(%Plug.Conn{} = conn, params) do
   end
 
   def callback(%Plug.Conn{assigns: %{ueberauth_failure: failure}} = conn, params) do
-    params = callback_params(params)
+    params = callback_params(conn, params)
     messages = for e <- Map.get(failure, :errors, []), do: e.message
     message = Enum.join(messages, "; ")
 
-    conn
+    error_message = dgettext("errors", "Failed to authenticate: %{message}.", message: message)
-    |> put_flash(
+
-      :error,
+    if params["redirect_uri"] do
-      dgettext("errors", "Failed to authenticate: %{message}.", message: message)
+      conn
-    )
+      |> put_flash(
-    |> redirect(external: redirect_uri(conn, params["redirect_uri"]))
+        :error,
+        error_message
+      )
+      |> redirect(external: redirect_uri(conn, params["redirect_uri"]))
+    else
+      send_resp(conn, :bad_request, error_message)
+    end
   end
 
   def callback(%Plug.Conn{} = conn, params) do
-    params = callback_params(params)
+    params = callback_params(conn, params)
 
     with {:ok, registration} <- Authenticator.get_registration(conn) do
       auth_attrs = Map.take(params, ~w(client_id redirect_uri scope scopes state))
@@ -511,8 +515,9 @@ def callback(%Plug.Conn{} = conn, params) do
     end
   end
 
-  defp callback_params(%{"state" => state} = params) do
+  defp callback_params(%Plug.Conn{} = conn, params) do
-    Map.merge(params, Jason.decode!(state))
+    fetch_cookies(conn)
+    Map.merge(params, Jason.decode!(Map.get(conn.req_cookies, @state_cookie_name, "{}")))
   end
 
   def registration_details(%Plug.Conn{} = conn, %{"authorization" => auth_attrs}) do

mix.exs

@@ -156,7 +156,7 @@ defp deps do
       {:ex_syslogger, "~> 2.0.0"},
       {:floki, "~> 0.34"},
       {:timex, "~> 3.7"},
-      {:ueberauth, "~> 0.10"},
+      {:ueberauth, "== 0.10.5"},
       {:linkify, git: "https://akkoma.dev/AkkomaGang/linkify.git"},
       {:http_signatures,
        git: "https://akkoma.dev/AkkomaGang/http_signatures.git",

mix.lock

@@ -124,7 +124,7 @@
   "timex": {:hex, :timex, "3.7.11", "bb95cb4eb1d06e27346325de506bcc6c30f9c6dea40d1ebe390b262fad1862d1", [:mix], [{:combine, "~> 0.10", [hex: :combine, repo: "hexpm", optional: false]}, {:gettext, "~> 0.20", [hex: :gettext, repo: "hexpm", optional: false]}, {:tzdata, "~> 1.1", [hex: :tzdata, repo: "hexpm", optional: false]}], "hexpm", "8b9024f7efbabaf9bd7aa04f65cf8dcd7c9818ca5737677c7b76acbc6a94d1aa"},
   "trailing_format_plug": {:hex, :trailing_format_plug, "0.0.7", "64b877f912cf7273bed03379936df39894149e35137ac9509117e59866e10e45", [:mix], [{:plug, "> 0.12.0", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "bd4fde4c15f3e993a999e019d64347489b91b7a9096af68b2bdadd192afa693f"},
   "tzdata": {:hex, :tzdata, "1.1.1", "20c8043476dfda8504952d00adac41c6eda23912278add38edc140ae0c5bcc46", [:mix], [{:hackney, "~> 1.17", [hex: :hackney, repo: "hexpm", optional: false]}], "hexpm", "a69cec8352eafcd2e198dea28a34113b60fdc6cb57eb5ad65c10292a6ba89787"},
-  "ueberauth": {:hex, :ueberauth, "0.10.6", "8dbefd5aec30c5830af2b6ce6e03f62cc28ae0757f34e2986454f54b8dca3f65", [:mix], [{:plug, "~> 1.5", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "b0ad1c7508f3cfd5c2c1c668d1a32bafd77de4c56af82c7bfd7e54ed078a7928"},
+  "ueberauth": {:hex, :ueberauth, "0.10.5", "806adb703df87e55b5615cf365e809f84c20c68aa8c08ff8a416a5a6644c4b02", [:mix], [{:plug, "~> 1.5", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "3efd1f31d490a125c7ed453b926f7c31d78b97b8a854c755f5c40064bf3ac9e1"},
   "unicode_util_compat": {:hex, :unicode_util_compat, "0.7.0", "bc84380c9ab48177092f43ac89e4dfa2c6d62b40b8bd132b1059ecc7232f9a78", [:rebar3], [], "hexpm", "25eee6d67df61960cf6a794239566599b09e17e668d3700247bc498638152521"},
   "unsafe": {:hex, :unsafe, "1.0.2", "23c6be12f6c1605364801f4b47007c0c159497d0446ad378b5cf05f1855c0581", [:mix], [], "hexpm", "b485231683c3ab01a9cd44cb4a79f152c6f3bb87358439c6f68791b85c2df675"},
   "vex": {:hex, :vex, "0.9.1", "cb65348ebd1c4002861b65bef36e524c29d9a879c90119b2d0e674e323124277", [:mix], [], "hexpm", "a0f9f3959d127ad6a6a617c3f607ecfb1bc6f3c59f9c3614a901a46d1765bafe"},

test/pleroma/web/o_auth/o_auth_controller_test.exs

@@ -81,9 +81,7 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", %
 
       assert html_response(conn, 302)
 
-      redirect_query = URI.parse(redirected_to(conn)).query
+      assert {:ok, state_components} = Jason.decode(conn.resp_cookies["akkoma_oauth_state"].value)
-      assert %{"state" => state_param} = URI.decode_query(redirect_query)
-      assert {:ok, state_components} = Jason.decode(state_param)
 
       expected_client_id = app.client_id
       expected_redirect_uri = app.redirect_uris
@@ -97,7 +95,7 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", %
     end
 
     test "with user-bound registration, GET /oauth/<provider>/callback redirects to `redirect_uri` with `code`",
-         %{app: app, conn: conn} do
+         %{app: app, conn: _} do
       registration = insert(:registration)
       redirect_uri = OAuthController.default_redirect_uri(app)
 
@@ -109,15 +107,17 @@ test "with user-bound registration, GET /oauth/<provider>/callback redirects to
       }
 
       conn =
-        conn
+        build_conn()
+        |> put_req_cookie("akkoma_oauth_state", Jason.encode!(state_params))
+        |> Plug.Session.call(Plug.Session.init(@session_opts))
+        |> fetch_session()
         |> assign(:ueberauth_auth, %{provider: registration.provider, uid: registration.uid})
         |> get(
           "/oauth/twitter/callback",
           %{
             "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM",
             "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs",
-            "provider" => "twitter",
+            "provider" => "twitter"
-            "state" => Jason.encode!(state_params)
           }
         )
 
@@ -162,15 +162,42 @@ test "with user-unbound registration, GET /oauth/<provider>/callback renders reg
 
     test "on authentication error, GET /oauth/<provider>/callback redirects to `redirect_uri`", %{
       app: app,
-      conn: conn
+      conn: _
     } do
       state_params = %{
         "scope" => Enum.join(app.scopes, " "),
         "client_id" => app.client_id,
-        "redirect_uri" => OAuthController.default_redirect_uri(app),
+        "redirect_uri" => OAuthController.default_redirect_uri(app)
-        "state" => ""
       }
 
+      conn =
+        build_conn()
+        |> put_req_cookie("akkoma_oauth_state", Jason.encode!(state_params))
+        |> Plug.Session.call(Plug.Session.init(@session_opts))
+        |> fetch_session()
+        |> assign(:ueberauth_failure, %{errors: [%{message: "(error description)"}]})
+        |> get(
+          "/oauth/twitter/callback",
+          %{
+            "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM",
+            "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs",
+            "provider" => "twitter",
+            "state" => ""
+          }
+        )
+
+      assert html_response(conn, 302)
+      assert redirected_to(conn) == app.redirect_uris
+
+      assert Phoenix.Flash.get(conn.assigns.flash, :error) ==
+               "Failed to authenticate: (error description)."
+    end
+
+    test "on authentication error with no prior state, GET /oauth/<provider>/callback returns 400",
+         %{
+           app: _,
+           conn: conn
+         } do
       conn =
         conn
         |> assign(:ueberauth_failure, %{errors: [%{message: "(error description)"}]})
@@ -180,15 +207,11 @@ test "on authentication error, GET /oauth/<provider>/callback redirects to `redi
             "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM",
             "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs",
             "provider" => "twitter",
-            "state" => Jason.encode!(state_params)
+            "state" => ""
           }
         )
 
-      assert html_response(conn, 302)
+      assert response(conn, 400)
-      assert redirected_to(conn) == app.redirect_uris
-
-      assert Phoenix.Flash.get(conn.assigns.flash, :error) ==
-               "Failed to authenticate: (error description)."
     end
 
     test "GET /oauth/registration_details renders registration details form", %{