From c3f12cf3c3597385481290b53a6bce31730a6a29 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Wed, 10 Apr 2019 21:40:38 +0300 Subject: [PATCH] [#923] OAuth consumer params handling refactoring. Registration and authorization-related params are wrapped in "authorization" in order to reduce edge cases number and simplify handling logic. --- lib/pleroma/web/auth/authenticator.ex | 18 +-- lib/pleroma/web/auth/ldap_authenticator.ex | 24 ++-- lib/pleroma/web/auth/pleroma_authenticator.ex | 29 +++-- lib/pleroma/web/oauth/fallback_controller.ex | 2 +- lib/pleroma/web/oauth/oauth_controller.ex | 115 +++++++++--------- .../templates/o_auth/o_auth/_scopes.html.eex | 2 +- .../templates/o_auth/o_auth/consumer.html.eex | 2 +- .../templates/o_auth/o_auth/register.html.eex | 7 +- .../web/templates/o_auth/o_auth/show.html.eex | 2 +- test/web/oauth/oauth_controller_test.exs | 90 ++++++++------ 10 files changed, 153 insertions(+), 138 deletions(-) diff --git a/lib/pleroma/web/auth/authenticator.ex b/lib/pleroma/web/auth/authenticator.ex index 89d88af32..b02f595dc 100644 --- a/lib/pleroma/web/auth/authenticator.ex +++ b/lib/pleroma/web/auth/authenticator.ex @@ -13,21 +13,21 @@ def implementation do ) end - @callback get_user(Plug.Conn.t(), Map.t()) :: {:ok, User.t()} | {:error, any()} - def get_user(plug, params), do: implementation().get_user(plug, params) + @callback get_user(Plug.Conn.t()) :: {:ok, User.t()} | {:error, any()} + def get_user(plug), do: implementation().get_user(plug) - @callback create_from_registration(Plug.Conn.t(), Map.t(), Registration.t()) :: + @callback create_from_registration(Plug.Conn.t(), Registration.t()) :: {:ok, User.t()} | {:error, any()} - def create_from_registration(plug, params, registration), - do: implementation().create_from_registration(plug, params, registration) + def create_from_registration(plug, registration), + do: implementation().create_from_registration(plug, registration) - @callback get_registration(Plug.Conn.t(), Map.t()) :: + @callback get_registration(Plug.Conn.t()) :: {:ok, Registration.t()} | {:error, any()} - def get_registration(plug, params), - do: implementation().get_registration(plug, params) + def get_registration(plug), do: implementation().get_registration(plug) @callback handle_error(Plug.Conn.t(), any()) :: any() - def handle_error(plug, error), do: implementation().handle_error(plug, error) + def handle_error(plug, error), + do: implementation().handle_error(plug, error) @callback auth_template() :: String.t() | nil def auth_template do diff --git a/lib/pleroma/web/auth/ldap_authenticator.ex b/lib/pleroma/web/auth/ldap_authenticator.ex index 8b6d5a77f..363c99597 100644 --- a/lib/pleroma/web/auth/ldap_authenticator.ex +++ b/lib/pleroma/web/auth/ldap_authenticator.ex @@ -13,14 +13,16 @@ defmodule Pleroma.Web.Auth.LDAPAuthenticator do @connection_timeout 10_000 @search_timeout 10_000 - defdelegate get_registration(conn, params), to: @base + defdelegate get_registration(conn), to: @base + defdelegate create_from_registration(conn, registration), to: @base + defdelegate handle_error(conn, error), to: @base + defdelegate auth_template, to: @base + defdelegate oauth_consumer_template, to: @base - defdelegate create_from_registration(conn, params, registration), to: @base - - def get_user(%Plug.Conn{} = conn, params) do + def get_user(%Plug.Conn{} = conn) do if Pleroma.Config.get([:ldap, :enabled]) do {name, password} = - case params do + case conn.params do %{"authorization" => %{"name" => name, "password" => password}} -> {name, password} @@ -34,25 +36,17 @@ def get_user(%Plug.Conn{} = conn, params) do {:error, {:ldap_connection_error, _}} -> # When LDAP is unavailable, try default authenticator - @base.get_user(conn, params) + @base.get_user(conn) error -> error end else # Fall back to default authenticator - @base.get_user(conn, params) + @base.get_user(conn) end end - def handle_error(%Plug.Conn{} = _conn, error) do - error - end - - def auth_template, do: nil - - def oauth_consumer_template, do: nil - defp ldap_user(name, password) do ldap = Pleroma.Config.get(:ldap, []) host = Keyword.get(ldap, :host, "localhost") diff --git a/lib/pleroma/web/auth/pleroma_authenticator.ex b/lib/pleroma/web/auth/pleroma_authenticator.ex index c826adb4c..d647f1e05 100644 --- a/lib/pleroma/web/auth/pleroma_authenticator.ex +++ b/lib/pleroma/web/auth/pleroma_authenticator.ex @@ -10,9 +10,9 @@ defmodule Pleroma.Web.Auth.PleromaAuthenticator do @behaviour Pleroma.Web.Auth.Authenticator - def get_user(%Plug.Conn{} = _conn, params) do + def get_user(%Plug.Conn{} = conn) do {name, password} = - case params do + case conn.params do %{"authorization" => %{"name" => name, "password" => password}} -> {name, password} @@ -29,10 +29,9 @@ def get_user(%Plug.Conn{} = _conn, params) do end end - def get_registration( - %Plug.Conn{assigns: %{ueberauth_auth: %{provider: provider, uid: uid} = auth}}, - _params - ) do + def get_registration(%Plug.Conn{ + assigns: %{ueberauth_auth: %{provider: provider, uid: uid} = auth} + }) do registration = Registration.get_by_provider_uid(provider, uid) if registration do @@ -40,7 +39,8 @@ def get_registration( else info = auth.info - Registration.changeset(%Registration{}, %{ + %Registration{} + |> Registration.changeset(%{ provider: to_string(provider), uid: to_string(uid), info: %{ @@ -54,13 +54,16 @@ def get_registration( end end - def get_registration(%Plug.Conn{} = _conn, _params), do: {:error, :missing_credentials} + def get_registration(%Plug.Conn{} = _conn), do: {:error, :missing_credentials} - def create_from_registration(_conn, params, registration) do - nickname = value([params["nickname"], Registration.nickname(registration)]) - email = value([params["email"], Registration.email(registration)]) - name = value([params["name"], Registration.name(registration)]) || nickname - bio = value([params["bio"], Registration.description(registration)]) + def create_from_registration( + %Plug.Conn{params: %{"authorization" => registration_attrs}}, + registration + ) do + nickname = value([registration_attrs["nickname"], Registration.nickname(registration)]) + email = value([registration_attrs["email"], Registration.email(registration)]) + name = value([registration_attrs["name"], Registration.name(registration)]) || nickname + bio = value([registration_attrs["bio"], Registration.description(registration)]) random_password = :crypto.strong_rand_bytes(64) |> Base.encode64() diff --git a/lib/pleroma/web/oauth/fallback_controller.ex b/lib/pleroma/web/oauth/fallback_controller.ex index afaa00242..e3984f009 100644 --- a/lib/pleroma/web/oauth/fallback_controller.ex +++ b/lib/pleroma/web/oauth/fallback_controller.ex @@ -24,6 +24,6 @@ def call(conn, _error) do conn |> put_status(:unauthorized) |> put_flash(:error, "Invalid Username/Password") - |> OAuthController.authorize(conn.params["authorization"]) + |> OAuthController.authorize(conn.params) end end diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index bee7084ad..8e5a83466 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -44,36 +44,40 @@ def authorize(%{assigns: %{token: %Token{} = token}} = conn, params) do def authorize(conn, params), do: do_authorize(conn, params) - defp do_authorize(conn, params) do - app = Repo.get_by(App, client_id: params["client_id"]) + defp do_authorize(conn, %{"authorization" => auth_attrs}) do + app = Repo.get_by(App, client_id: auth_attrs["client_id"]) available_scopes = (app && app.scopes) || [] - scopes = oauth_scopes(params, nil) || available_scopes + scopes = oauth_scopes(auth_attrs, nil) || available_scopes render(conn, Authenticator.auth_template(), %{ - response_type: params["response_type"], - client_id: params["client_id"], + response_type: auth_attrs["response_type"], + client_id: auth_attrs["client_id"], available_scopes: available_scopes, scopes: scopes, - redirect_uri: params["redirect_uri"], - state: params["state"], - params: params + redirect_uri: auth_attrs["redirect_uri"], + state: auth_attrs["state"], + params: auth_attrs }) end + defp do_authorize(conn, auth_attrs), do: do_authorize(conn, %{"authorization" => auth_attrs}) + def create_authorization( conn, - %{"authorization" => auth_params} = params, + %{"authorization" => _} = params, opts \\ [] ) do with {:ok, auth} <- do_create_authorization(conn, params, opts[:user]) do - after_create_authorization(conn, auth, auth_params) + after_create_authorization(conn, auth, params) else error -> - handle_create_authorization_error(conn, error, auth_params) + handle_create_authorization_error(conn, error, params) end end - def after_create_authorization(conn, auth, %{"redirect_uri" => redirect_uri} = auth_params) do + def after_create_authorization(conn, auth, %{ + "authorization" => %{"redirect_uri" => redirect_uri} = auth_attrs + }) do redirect_uri = redirect_uri(conn, redirect_uri) if redirect_uri == "urn:ietf:wg:oauth:2.0:oob" do @@ -86,8 +90,8 @@ def after_create_authorization(conn, auth, %{"redirect_uri" => redirect_uri} = a url_params = %{:code => auth.token} url_params = - if auth_params["state"] do - Map.put(url_params, :state, auth_params["state"]) + if auth_attrs["state"] do + Map.put(url_params, :state, auth_attrs["state"]) else url_params end @@ -98,26 +102,34 @@ def after_create_authorization(conn, auth, %{"redirect_uri" => redirect_uri} = a end end - defp handle_create_authorization_error(conn, {scopes_issue, _}, auth_params) + defp handle_create_authorization_error( + conn, + {scopes_issue, _}, + %{"authorization" => _} = params + ) when scopes_issue in [:unsupported_scopes, :missing_scopes] do # Per https://github.com/tootsuite/mastodon/blob/ # 51e154f5e87968d6bb115e053689767ab33e80cd/app/controllers/api/base_controller.rb#L39 conn |> put_flash(:error, "This action is outside the authorized scopes") |> put_status(:unauthorized) - |> authorize(auth_params) + |> authorize(params) end - defp handle_create_authorization_error(conn, {:auth_active, false}, auth_params) do + defp handle_create_authorization_error( + conn, + {:auth_active, false}, + %{"authorization" => _} = params + ) do # Per https://github.com/tootsuite/mastodon/blob/ # 51e154f5e87968d6bb115e053689767ab33e80cd/app/controllers/api/base_controller.rb#L76 conn |> put_flash(:error, "Your login is missing a confirmed e-mail address") |> put_status(:forbidden) - |> authorize(auth_params) + |> authorize(params) end - defp handle_create_authorization_error(conn, error, _auth_params) do + defp handle_create_authorization_error(conn, error, %{"authorization" => _}) do Authenticator.handle_error(conn, error) end @@ -151,7 +163,7 @@ def token_exchange( conn, %{"grant_type" => "password"} = params ) do - with {_, {:ok, %User{} = user}} <- {:get_user, Authenticator.get_user(conn, params)}, + with {_, {:ok, %User{} = user}} <- {:get_user, Authenticator.get_user(conn)}, %App{} = app <- get_app_from_request(conn, params), {:auth_active, true} <- {:auth_active, User.auth_active?(user)}, {:user_active, true} <- {:user_active, !user.info.deactivated}, @@ -214,19 +226,19 @@ def token_revoke(conn, %{"token" => token} = params) do end @doc "Prepares OAuth request to provider for Ueberauth" - def prepare_request(conn, %{"provider" => provider} = params) do + def prepare_request(conn, %{"provider" => provider, "authorization" => auth_attrs}) do scope = - oauth_scopes(params, []) + oauth_scopes(auth_attrs, []) |> Enum.join(" ") state = - params + auth_attrs |> Map.delete("scopes") |> Map.put("scope", scope) |> Poison.encode!() params = - params + auth_attrs |> Map.drop(~w(scope scopes client_id redirect_uri)) |> Map.put("state", state) @@ -260,26 +272,26 @@ def callback(%{assigns: %{ueberauth_failure: failure}} = conn, params) do def callback(conn, params) do params = callback_params(params) - with {:ok, registration} <- Authenticator.get_registration(conn, params) do + with {:ok, registration} <- Authenticator.get_registration(conn) do user = Repo.preload(registration, :user).user - auth_params = Map.take(params, ~w(client_id redirect_uri scope scopes state)) + auth_attrs = Map.take(params, ~w(client_id redirect_uri scope scopes state)) if user do create_authorization( conn, - %{"authorization" => auth_params}, + %{"authorization" => auth_attrs}, user: user ) else registration_params = - Map.merge(auth_params, %{ + Map.merge(auth_attrs, %{ "nickname" => Registration.nickname(registration), "email" => Registration.email(registration) }) conn |> put_session(:registration_id, registration.id) - |> registration_details(registration_params) + |> registration_details(%{"authorization" => registration_params}) end else _ -> @@ -293,53 +305,44 @@ defp callback_params(%{"state" => state} = params) do Map.merge(params, Poison.decode!(state)) end - def registration_details(conn, params) do + def registration_details(conn, %{"authorization" => auth_attrs}) do render(conn, "register.html", %{ - client_id: params["client_id"], - redirect_uri: params["redirect_uri"], - state: params["state"], - scopes: oauth_scopes(params, []), - nickname: params["nickname"], - email: params["email"] + client_id: auth_attrs["client_id"], + redirect_uri: auth_attrs["redirect_uri"], + state: auth_attrs["state"], + scopes: oauth_scopes(auth_attrs, []), + nickname: auth_attrs["nickname"], + email: auth_attrs["email"] }) end - def register(conn, %{"op" => "connect"} = params) do - authorization_params = Map.put(params, "name", params["auth_name"]) - create_authorization_params = %{"authorization" => authorization_params} - + def register(conn, %{"authorization" => _, "op" => "connect"} = params) do with registration_id when not is_nil(registration_id) <- get_session_registration_id(conn), %Registration{} = registration <- Repo.get(Registration, registration_id), {_, {:ok, auth}} <- - {:create_authorization, do_create_authorization(conn, create_authorization_params)}, + {:create_authorization, do_create_authorization(conn, params)}, %User{} = user <- Repo.preload(auth, :user).user, {:ok, _updated_registration} <- Registration.bind_to_user(registration, user) do conn |> put_session_registration_id(nil) - |> after_create_authorization(auth, authorization_params) + |> after_create_authorization(auth, params) else {:create_authorization, error} -> - {:register, handle_create_authorization_error(conn, error, create_authorization_params)} + {:register, handle_create_authorization_error(conn, error, params)} _ -> {:register, :generic_error} end end - def register(conn, %{"op" => "register"} = params) do + def register(conn, %{"authorization" => _, "op" => "register"} = params) do with registration_id when not is_nil(registration_id) <- get_session_registration_id(conn), %Registration{} = registration <- Repo.get(Registration, registration_id), - {:ok, user} <- Authenticator.create_from_registration(conn, params, registration) do + {:ok, user} <- Authenticator.create_from_registration(conn, registration) do conn |> put_session_registration_id(nil) |> create_authorization( - %{ - "authorization" => %{ - "client_id" => params["client_id"], - "redirect_uri" => params["redirect_uri"], - "scopes" => oauth_scopes(params, nil) - } - }, + params, user: user ) else @@ -374,15 +377,15 @@ defp do_create_authorization( %{ "client_id" => client_id, "redirect_uri" => redirect_uri - } = auth_params - } = params, + } = auth_attrs + }, user \\ nil ) do with {_, {:ok, %User{} = user}} <- - {:get_user, (user && {:ok, user}) || Authenticator.get_user(conn, params)}, + {:get_user, (user && {:ok, user}) || Authenticator.get_user(conn)}, %App{} = app <- Repo.get_by(App, client_id: client_id), true <- redirect_uri in String.split(app.redirect_uris), - scopes <- oauth_scopes(auth_params, []), + scopes <- oauth_scopes(auth_attrs, []), {:unsupported_scopes, []} <- {:unsupported_scopes, scopes -- app.scopes}, # Note: `scope` param is intentionally not optional in this context {:missing_scopes, false} <- {:missing_scopes, scopes == []}, diff --git a/lib/pleroma/web/templates/o_auth/o_auth/_scopes.html.eex b/lib/pleroma/web/templates/o_auth/o_auth/_scopes.html.eex index 4b8fb5dae..e6cfe108b 100644 --- a/lib/pleroma/web/templates/o_auth/o_auth/_scopes.html.eex +++ b/lib/pleroma/web/templates/o_auth/o_auth/_scopes.html.eex @@ -5,7 +5,7 @@ <%= for scope <- @available_scopes do %> <%# Note: using hidden input with `unchecked_value` in order to distinguish user's empty selection from `scope` param being omitted %>
- <%= checkbox @form, :"scope_#{scope}", value: scope in @scopes && scope, checked_value: scope, unchecked_value: "", name: assigns[:scope_param] || "scope[]" %> + <%= checkbox @form, :"scope_#{scope}", value: scope in @scopes && scope, checked_value: scope, unchecked_value: "", name: "authorization[scope][]" %> <%= label @form, :"scope_#{scope}", String.capitalize(scope) %>
<% end %> diff --git a/lib/pleroma/web/templates/o_auth/o_auth/consumer.html.eex b/lib/pleroma/web/templates/o_auth/o_auth/consumer.html.eex index 85f62ca64..4bcda7300 100644 --- a/lib/pleroma/web/templates/o_auth/o_auth/consumer.html.eex +++ b/lib/pleroma/web/templates/o_auth/o_auth/consumer.html.eex @@ -1,6 +1,6 @@

Sign in with external provider

-<%= form_for @conn, o_auth_path(@conn, :prepare_request), [method: "get"], fn f -> %> +<%= form_for @conn, o_auth_path(@conn, :prepare_request), [as: "authorization", method: "get"], fn f -> %> <%= render @view_module, "_scopes.html", Map.put(assigns, :form, f) %> <%= hidden_input f, :client_id, value: @client_id %> diff --git a/lib/pleroma/web/templates/o_auth/o_auth/register.html.eex b/lib/pleroma/web/templates/o_auth/o_auth/register.html.eex index 126390391..facedc8db 100644 --- a/lib/pleroma/web/templates/o_auth/o_auth/register.html.eex +++ b/lib/pleroma/web/templates/o_auth/o_auth/register.html.eex @@ -8,8 +8,7 @@

Registration Details

If you'd like to register a new account, please provide the details below.

- -<%= form_for @conn, o_auth_path(@conn, :register), [], fn f -> %> +<%= form_for @conn, o_auth_path(@conn, :register), [as: "authorization"], fn f -> %>
<%= label f, :nickname, "Nickname" %> @@ -25,8 +24,8 @@

Alternatively, sign in to connect to existing account.

- <%= label f, :auth_name, "Name or email" %> - <%= text_input f, :auth_name %> + <%= label f, :name, "Name or email" %> + <%= text_input f, :name %>
<%= label f, :password, "Password" %> diff --git a/lib/pleroma/web/templates/o_auth/o_auth/show.html.eex b/lib/pleroma/web/templates/o_auth/o_auth/show.html.eex index 87278e636..3e360a52c 100644 --- a/lib/pleroma/web/templates/o_auth/o_auth/show.html.eex +++ b/lib/pleroma/web/templates/o_auth/o_auth/show.html.eex @@ -17,7 +17,7 @@ <%= password_input f, :password %>
-<%= render @view_module, "_scopes.html", Map.merge(assigns, %{form: f, scope_param: "authorization[scope][]"}) %> +<%= render @view_module, "_scopes.html", Map.merge(assigns, %{form: f}) %> <%= hidden_input f, :client_id, value: @client_id %> <%= hidden_input f, :response_type, value: @response_type %> diff --git a/test/web/oauth/oauth_controller_test.exs b/test/web/oauth/oauth_controller_test.exs index ac7843f9b..fb505fab3 100644 --- a/test/web/oauth/oauth_controller_test.exs +++ b/test/web/oauth/oauth_controller_test.exs @@ -68,10 +68,12 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", % "/oauth/prepare_request", %{ "provider" => "twitter", - "scope" => "read follow", - "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, - "state" => "a_state" + "authorization" => %{ + "scope" => "read follow", + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "a_state" + } } ) @@ -104,7 +106,7 @@ test "with user-bound registration, GET /oauth//callback redirects to } with_mock Pleroma.Web.Auth.Authenticator, - get_registration: fn _, _ -> {:ok, registration} end do + get_registration: fn _ -> {:ok, registration} end do conn = get( conn, @@ -134,7 +136,7 @@ test "with user-unbound registration, GET /oauth//callback renders reg } with_mock Pleroma.Web.Auth.Authenticator, - get_registration: fn _, _ -> {:ok, registration} end do + get_registration: fn _ -> {:ok, registration} end do conn = get( conn, @@ -193,12 +195,14 @@ test "GET /oauth/registration_details renders registration details form", %{ conn, "/oauth/registration_details", %{ - "scopes" => app.scopes, - "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, - "state" => "a_state", - "nickname" => nil, - "email" => "john@doe.com" + "authorization" => %{ + "scopes" => app.scopes, + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "a_state", + "nickname" => nil, + "email" => "john@doe.com" + } } ) @@ -221,12 +225,14 @@ test "with valid params, POST /oauth/register?op=register redirects to `redirect "/oauth/register", %{ "op" => "register", - "scopes" => app.scopes, - "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, - "state" => "a_state", - "nickname" => "availablenick", - "email" => "available@email.com" + "authorization" => %{ + "scopes" => app.scopes, + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "a_state", + "nickname" => "availablenick", + "email" => "available@email.com" + } } ) @@ -244,17 +250,23 @@ test "with invalid params, POST /oauth/register?op=register renders registration params = %{ "op" => "register", - "scopes" => app.scopes, - "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, - "state" => "a_state", - "nickname" => "availablenickname", - "email" => "available@email.com" + "authorization" => %{ + "scopes" => app.scopes, + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "a_state", + "nickname" => "availablenickname", + "email" => "available@email.com" + } } for {bad_param, bad_param_value} <- [{"nickname", another_user.nickname}, {"email", another_user.email}] do - bad_params = Map.put(params, bad_param, bad_param_value) + bad_registration_attrs = %{ + "authorization" => Map.put(params["authorization"], bad_param, bad_param_value) + } + + bad_params = Map.merge(params, bad_registration_attrs) conn = conn @@ -281,12 +293,14 @@ test "with valid params, POST /oauth/register?op=connect redirects to `redirect_ "/oauth/register", %{ "op" => "connect", - "scopes" => app.scopes, - "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, - "state" => "a_state", - "auth_name" => user.nickname, - "password" => "testpassword" + "authorization" => %{ + "scopes" => app.scopes, + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "a_state", + "name" => user.nickname, + "password" => "testpassword" + } } ) @@ -304,12 +318,14 @@ test "with invalid params, POST /oauth/register?op=connect renders registration_ params = %{ "op" => "connect", - "scopes" => app.scopes, - "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, - "state" => "a_state", - "auth_name" => user.nickname, - "password" => "wrong password" + "authorization" => %{ + "scopes" => app.scopes, + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "a_state", + "name" => user.nickname, + "password" => "wrong password" + } } conn =