diff --git a/changelog.d/oauth-registration-redirect_uris.fix b/changelog.d/oauth-registration-redirect_uris.fix index e69de29bb..76ace55df 100644 --- a/changelog.d/oauth-registration-redirect_uris.fix +++ b/changelog.d/oauth-registration-redirect_uris.fix @@ -0,0 +1 @@ +Fix OAuth app registration to accept `redirect_uris` as an array of strings (RFC 7591), while keeping backwards compatibility with string input. diff --git a/lib/pleroma/web/api_spec/operations/admin/o_auth_app_operation.ex b/lib/pleroma/web/api_spec/operations/admin/o_auth_app_operation.ex index 5d4306754..7d83066ca 100644 --- a/lib/pleroma/web/api_spec/operations/admin/o_auth_app_operation.ex +++ b/lib/pleroma/web/api_spec/operations/admin/o_auth_app_operation.ex @@ -123,8 +123,10 @@ defmodule Pleroma.Web.ApiSpec.Admin.OAuthAppOperation do name: %Schema{type: :string, description: "Application Name"}, scopes: %Schema{type: :array, items: %Schema{type: :string}, description: "oAuth scopes"}, redirect_uris: %Schema{ - type: :array, - items: %Schema{type: :string}, + oneOf: [ + %Schema{type: :string}, + %Schema{type: :array, items: %Schema{type: :string}} + ], description: "Where the user should be redirected after authorization. To display the authorization code to the user instead of redirecting to a web page, use `urn:ietf:wg:oauth:2.0:oob` in this parameter." }, @@ -158,8 +160,10 @@ defmodule Pleroma.Web.ApiSpec.Admin.OAuthAppOperation do name: %Schema{type: :string, description: "Application Name"}, scopes: %Schema{type: :array, items: %Schema{type: :string}, description: "oAuth scopes"}, redirect_uris: %Schema{ - type: :array, - items: %Schema{type: :string}, + oneOf: [ + %Schema{type: :string}, + %Schema{type: :array, items: %Schema{type: :string}} + ], description: "Where the user should be redirected after authorization. To display the authorization code to the user instead of redirecting to a web page, use `urn:ietf:wg:oauth:2.0:oob` in this parameter." }, diff --git a/lib/pleroma/web/api_spec/operations/app_operation.ex b/lib/pleroma/web/api_spec/operations/app_operation.ex index dfa2237c0..71c15a665 100644 --- a/lib/pleroma/web/api_spec/operations/app_operation.ex +++ b/lib/pleroma/web/api_spec/operations/app_operation.ex @@ -97,7 +97,10 @@ defmodule Pleroma.Web.ApiSpec.AppOperation do properties: %{ client_name: %Schema{type: :string, description: "A name for your application."}, redirect_uris: %Schema{ - type: :string, + oneOf: [ + %Schema{type: :string}, + %Schema{type: :array, items: %Schema{type: :string}} + ], description: "Where the user should be redirected after authorization. To display the authorization code to the user instead of redirecting to a web page, use `urn:ietf:wg:oauth:2.0:oob` in this parameter." }, diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index f1145d500..a2841b2bb 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -14,7 +14,7 @@ defmodule Pleroma.Web.OAuth.App do schema "apps" do field(:client_name, :string) - field(:redirect_uris, {:array, :string}) + field(:redirect_uris, :string) field(:scopes, {:array, :string}, default: []) field(:website, :string) field(:client_id, :string) @@ -31,9 +31,32 @@ defmodule Pleroma.Web.OAuth.App do @spec changeset(t(), map()) :: Ecto.Changeset.t() def changeset(struct, params) do + params = normalize_redirect_uris_param(params) + cast(struct, params, [:client_name, :redirect_uris, :scopes, :website, :trusted, :user_id]) end + defp normalize_redirect_uris_param(%{} = params) do + case params do + %{redirect_uris: redirect_uris} when is_list(redirect_uris) -> + Map.put(params, :redirect_uris, normalize_redirect_uris(redirect_uris)) + + %{"redirect_uris" => redirect_uris} when is_list(redirect_uris) -> + Map.put(params, "redirect_uris", normalize_redirect_uris(redirect_uris)) + + _ -> + params + end + end + + defp normalize_redirect_uris(redirect_uris) when is_list(redirect_uris) do + redirect_uris + |> Enum.filter(&is_binary/1) + |> Enum.map(&String.trim/1) + |> Enum.reject(&(&1 == "")) + |> Enum.join("\n") + end + @spec register_changeset(t(), map()) :: Ecto.Changeset.t() def register_changeset(struct, params \\ %{}) do changeset = diff --git a/test/pleroma/web/admin_api/controllers/o_auth_app_controller_test.exs b/test/pleroma/web/admin_api/controllers/o_auth_app_controller_test.exs index 10eefbeca..2c2d13bb7 100644 --- a/test/pleroma/web/admin_api/controllers/o_auth_app_controller_test.exs +++ b/test/pleroma/web/admin_api/controllers/o_auth_app_controller_test.exs @@ -57,6 +57,28 @@ defmodule Pleroma.Web.AdminAPI.OAuthAppControllerTest do } = response end + test "success with redirect_uris array", %{conn: conn} do + base_url = Endpoint.url() + app_name = "Trusted app" + + response = + conn + |> put_req_header("content-type", "application/json") + |> post("/api/pleroma/admin/oauth_app", %{ + name: app_name, + redirect_uris: [base_url] + }) + |> json_response_and_validate_schema(200) + + assert %{ + "client_id" => _, + "client_secret" => _, + "name" => ^app_name, + "redirect_uri" => ^base_url, + "trusted" => false + } = response + end + test "with trusted", %{conn: conn} do base_url = Endpoint.url() app_name = "Trusted app" diff --git a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs index bc9d4048c..45902d7d9 100644 --- a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs @@ -61,6 +61,33 @@ defmodule Pleroma.Web.MastodonAPI.AppControllerTest do assert app.user_id == nil end + test "creates an oauth app with redirect_uris array", %{conn: conn} do + app_attrs = build(:oauth_app) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/apps", %{ + client_name: app_attrs.client_name, + redirect_uris: [app_attrs.redirect_uris] + }) + + [app] = Repo.all(App) + + expected = %{ + "name" => app.client_name, + "website" => app.website, + "client_id" => app.client_id, + "client_secret" => app.client_secret, + "id" => app.id |> to_string(), + "redirect_uri" => app.redirect_uris, + "vapid_key" => Push.vapid_config() |> Keyword.get(:public_key) + } + + assert expected == json_response_and_validate_schema(conn, 200) + assert app.user_id == nil + end + test "creates an oauth app with a user", %{conn: conn} do user = insert(:user) app_attrs = build(:oauth_app) diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index f90bee358..a69ba371e 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -10,20 +10,20 @@ defmodule Pleroma.Web.OAuth.AppTest do describe "get_or_make/2" do test "gets exist app" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: ["."]} + attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]})) {:ok, %App{} = exist_app} = App.get_or_make(attrs, []) assert exist_app == app end test "make app" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: ["."]} + attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) assert app.scopes == ["write"] end test "gets exist app and updates scopes" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: ["."]} + attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]})) {:ok, %App{} = exist_app} = App.get_or_make(attrs, ["read", "write", "follow", "push"]) assert exist_app.id == app.id @@ -31,10 +31,10 @@ defmodule Pleroma.Web.OAuth.AppTest do end test "has unique client_id" do - insert(:oauth_app, client_name: "", redirect_uris: [""], client_id: "boop") + insert(:oauth_app, client_name: "", redirect_uris: "", client_id: "boop") error = - catch_error(insert(:oauth_app, client_name: "", redirect_uris: [""], client_id: "boop")) + catch_error(insert(:oauth_app, client_name: "", redirect_uris: "", client_id: "boop")) assert %Ecto.ConstraintError{} = error assert error.constraint == "apps_client_id_index" @@ -55,7 +55,7 @@ defmodule Pleroma.Web.OAuth.AppTest do end test "removes orphaned apps" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: ["."]} + attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} {:ok, %App{} = old_app} = App.get_or_make(attrs, ["write"]) # backdate the old app so it's within the threshold for being cleaned up @@ -66,7 +66,7 @@ defmodule Pleroma.Web.OAuth.AppTest do |> Pleroma.Repo.query([one_hour_ago, old_app.id]) # Create the new app after backdating the old one - attrs = %{client_name: "PleromaFE", redirect_uris: ["."]} + attrs = %{client_name: "PleromaFE", redirect_uris: "."} {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) # Ensure the new app has a recent timestamp diff --git a/test/pleroma/web/o_auth/o_auth_controller_test.exs b/test/pleroma/web/o_auth/o_auth_controller_test.exs index 3788b9c65..260442771 100644 --- a/test/pleroma/web/o_auth/o_auth_controller_test.exs +++ b/test/pleroma/web/o_auth/o_auth_controller_test.exs @@ -406,7 +406,7 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest do describe "GET /oauth/authorize" do setup do [ - app: insert(:oauth_app, redirect_uris: ["https://redirect.url"]), + app: insert(:oauth_app, redirect_uris: "https://redirect.url"), conn: build_conn() |> Plug.Session.call(Plug.Session.init(@session_opts))