From 47a236f7537ad4366d07361d184c84f3912648f1 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 5 Apr 2019 15:12:02 +0300 Subject: [PATCH] [#923] OAuth consumer mode refactoring, new tests, tests adjustments, readme. --- config/config.exs | 4 +- docs/config.md | 55 +++++++ lib/pleroma/config.ex | 4 + lib/pleroma/web/endpoint.ex | 2 +- lib/pleroma/web/oauth/fallback_controller.ex | 17 ++- lib/pleroma/web/oauth/oauth_controller.ex | 136 +++++++++--------- .../templates/o_auth/o_auth/consumer.html.eex | 2 +- .../web/templates/o_auth/o_auth/show.html.eex | 2 +- test/registration_test.exs | 59 ++++++++ test/web/oauth/oauth_controller_test.exs | 112 +++++++-------- 10 files changed, 258 insertions(+), 135 deletions(-) create mode 100644 test/registration_test.exs diff --git a/config/config.exs b/config/config.exs index 9bc79f939..05b164273 100644 --- a/config/config.exs +++ b/config/config.exs @@ -397,9 +397,7 @@ base_path: "/oauth", providers: ueberauth_providers -config :pleroma, :auth, - oauth_consumer_strategies: oauth_consumer_strategies, - oauth_consumer_enabled: oauth_consumer_strategies != [] +config :pleroma, :auth, oauth_consumer_strategies: oauth_consumer_strategies config :pleroma, Pleroma.Mailer, adapter: Swoosh.Adapters.Sendmail diff --git a/docs/config.md b/docs/config.md index 06d6fd757..36d7f1273 100644 --- a/docs/config.md +++ b/docs/config.md @@ -412,3 +412,58 @@ Pleroma account will be created with the same name as the LDAP user name. * `Pleroma.Web.Auth.PleromaAuthenticator`: default database authenticator * `Pleroma.Web.Auth.LDAPAuthenticator`: LDAP authentication + +## :auth + +Authentication / authorization settings. + +* `oauth_consumer_strategies`: lists enabled OAuth consumer strategies; by default it's set by OAUTH_CONSUMER_STRATEGIES environment variable. + +OAuth consumer mode allows sign in / sign up via external OAuth providers (e.g. Twitter, Facebook, Google, Microsoft, etc.). +Implementation is based on Ueberauth; see the list of [available strategies](https://github.com/ueberauth/ueberauth/wiki/List-of-Strategies). + +Note: each strategy is shipped as a separate dependency; in order to get the strategies, run `OAUTH_CONSUMER_STRATEGIES="..." mix deps.get`, +e.g. `OAUTH_CONSUMER_STRATEGIES="twitter facebook google microsoft" mix deps.get`. +The server should also be started with `OAUTH_CONSUMER_STRATEGIES="..." mix phx.server` in case you enable any strategies. + +Note: each strategy requires separate setup (on external provider side and Pleroma side). Below are the guidelines on setting up most popular strategies. + +* For Twitter, [register an app](https://developer.twitter.com/en/apps), configure callback URL to https:///oauth/twitter/callback + +* For Facebook, [register an app](https://developers.facebook.com/apps), configure callback URL to https:///oauth/facebook/callback, enable Facebook Login service at https://developers.facebook.com/apps//fb-login/settings/ + +* For Google, [register an app](https://console.developers.google.com), configure callback URL to https:///oauth/google/callback + +* For Microsoft, [register an app](https://portal.azure.com), configure callback URL to https:///oauth/microsoft/callback + +Once the app is configured on external OAuth provider side, add app's credentials and strategy-specific settings (if any — e.g. see Microsoft below) to `config/prod.secret.exs`, +per strategy's documentation (e.g. [ueberauth_twitter](https://github.com/ueberauth/ueberauth_twitter)). Example config basing on environment variables: + +``` +# Twitter +config :ueberauth, Ueberauth.Strategy.Twitter.OAuth, + consumer_key: System.get_env("TWITTER_CONSUMER_KEY"), + consumer_secret: System.get_env("TWITTER_CONSUMER_SECRET") + +# Facebook +config :ueberauth, Ueberauth.Strategy.Facebook.OAuth, + client_id: System.get_env("FACEBOOK_APP_ID"), + client_secret: System.get_env("FACEBOOK_APP_SECRET"), + redirect_uri: System.get_env("FACEBOOK_REDIRECT_URI") + +# Google +config :ueberauth, Ueberauth.Strategy.Google.OAuth, + client_id: System.get_env("GOOGLE_CLIENT_ID"), + client_secret: System.get_env("GOOGLE_CLIENT_SECRET"), + redirect_uri: System.get_env("GOOGLE_REDIRECT_URI") + +# Microsoft +config :ueberauth, Ueberauth.Strategy.Microsoft.OAuth, + client_id: System.get_env("MICROSOFT_CLIENT_ID"), + client_secret: System.get_env("MICROSOFT_CLIENT_SECRET") + +config :ueberauth, Ueberauth, + providers: [ + microsoft: {Ueberauth.Strategy.Microsoft, [callback_params: []]} + ] +``` diff --git a/lib/pleroma/config.ex b/lib/pleroma/config.ex index 21507cd38..189faa15f 100644 --- a/lib/pleroma/config.ex +++ b/lib/pleroma/config.ex @@ -57,4 +57,8 @@ def delete([parent_key | keys]) do def delete(key) do Application.delete_env(:pleroma, key) end + + def oauth_consumer_strategies, do: get([:auth, :oauth_consumer_strategies], []) + + def oauth_consumer_enabled?, do: oauth_consumer_strategies() != [] end diff --git a/lib/pleroma/web/endpoint.ex b/lib/pleroma/web/endpoint.ex index b85b95bf9..085f23159 100644 --- a/lib/pleroma/web/endpoint.ex +++ b/lib/pleroma/web/endpoint.ex @@ -59,7 +59,7 @@ defmodule Pleroma.Web.Endpoint do else: "pleroma_key" same_site = - if Pleroma.Config.get([:auth, :oauth_consumer_enabled]) do + if Pleroma.Config.oauth_consumer_enabled?() do # Note: "SameSite=Strict" prevents sign in with external OAuth provider # (there would be no cookies during callback request from OAuth provider) "SameSite=Lax" diff --git a/lib/pleroma/web/oauth/fallback_controller.ex b/lib/pleroma/web/oauth/fallback_controller.ex index f0fe3b578..afaa00242 100644 --- a/lib/pleroma/web/oauth/fallback_controller.ex +++ b/lib/pleroma/web/oauth/fallback_controller.ex @@ -6,8 +6,21 @@ defmodule Pleroma.Web.OAuth.FallbackController do use Pleroma.Web, :controller alias Pleroma.Web.OAuth.OAuthController - # No user/password - def call(conn, _) do + def call(conn, {:register, :generic_error}) do + conn + |> put_status(:internal_server_error) + |> put_flash(:error, "Unknown error, please check the details and try again.") + |> OAuthController.registration_details(conn.params) + end + + def call(conn, {:register, _error}) do + conn + |> put_status(:unauthorized) + |> put_flash(:error, "Invalid Username/Password") + |> OAuthController.registration_details(conn.params) + end + + def call(conn, _error) do conn |> put_status(:unauthorized) |> put_flash(:error, "Invalid Username/Password") diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index 404728899..108303eb2 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -16,7 +16,7 @@ defmodule Pleroma.Web.OAuth.OAuthController do import Pleroma.Web.ControllerHelper, only: [oauth_scopes: 2] - if Pleroma.Config.get([:auth, :oauth_consumer_enabled]), do: plug(Ueberauth) + if Pleroma.Config.oauth_consumer_enabled?(), do: plug(Ueberauth) plug(:fetch_session) plug(:fetch_flash) @@ -62,60 +62,65 @@ defp do_authorize(conn, params) do def create_authorization( conn, - %{ - "authorization" => %{"redirect_uri" => redirect_uri} = auth_params - } = params, + %{"authorization" => auth_params} = params, opts \\ [] ) do - with {:ok, auth} <- - (opts[:auth] && {:ok, opts[:auth]}) || - do_create_authorization(conn, params, opts[:user]) do - redirect_uri = redirect_uri(conn, redirect_uri) - - cond do - redirect_uri == "urn:ietf:wg:oauth:2.0:oob" -> - render(conn, "results.html", %{ - auth: auth - }) - - true -> - connector = if String.contains?(redirect_uri, "?"), do: "&", else: "?" - url = "#{redirect_uri}#{connector}" - url_params = %{:code => auth.token} - - url_params = - if auth_params["state"] do - Map.put(url_params, :state, auth_params["state"]) - else - url_params - end - - url = "#{url}#{Plug.Conn.Query.encode(url_params)}" - - redirect(conn, external: url) - end + with {:ok, auth} <- do_create_authorization(conn, params, opts[:user]) do + after_create_authorization(conn, auth, auth_params) else - {scopes_issue, _} when scopes_issue in [:unsupported_scopes, :missing_scopes] -> - # 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) - - {:auth_active, false} -> - # 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) - error -> - Authenticator.handle_error(conn, error) + handle_create_authorization_error(conn, error, auth_params) end end + def after_create_authorization(conn, auth, %{"redirect_uri" => redirect_uri} = auth_params) do + redirect_uri = redirect_uri(conn, redirect_uri) + + if redirect_uri == "urn:ietf:wg:oauth:2.0:oob" do + render(conn, "results.html", %{ + auth: auth + }) + else + connector = if String.contains?(redirect_uri, "?"), do: "&", else: "?" + url = "#{redirect_uri}#{connector}" + url_params = %{:code => auth.token} + + url_params = + if auth_params["state"] do + Map.put(url_params, :state, auth_params["state"]) + else + url_params + end + + url = "#{url}#{Plug.Conn.Query.encode(url_params)}" + + redirect(conn, external: url) + end + end + + defp handle_create_authorization_error(conn, {scopes_issue, _}, auth_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) + end + + defp handle_create_authorization_error(conn, {:auth_active, false}, auth_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) + end + + defp handle_create_authorization_error(conn, error, _auth_params) do + Authenticator.handle_error(conn, error) + end + def token_exchange(conn, %{"grant_type" => "authorization_code"} = params) do with %App{} = app <- get_app_from_request(conn, params), fixed_token = fix_padding(params["code"]), @@ -202,6 +207,7 @@ def token_revoke(conn, %{"token" => token} = params) do end end + @doc "Prepares OAuth request to provider for Ueberauth" def prepare_request(conn, %{"provider" => provider} = params) do scope = oauth_scopes(params, []) @@ -218,6 +224,7 @@ def prepare_request(conn, %{"provider" => provider} = params) do |> Map.drop(~w(scope scopes client_id redirect_uri)) |> Map.put("state", state) + # Handing the request to Ueberauth redirect(conn, to: o_auth_path(conn, :request, provider, params)) end @@ -266,7 +273,7 @@ def callback(conn, params) do conn |> put_session(:registration_id, registration.id) - |> redirect(to: o_auth_path(conn, :registration_details, registration_params)) + |> registration_details(registration_params) end else _ -> @@ -292,32 +299,28 @@ def registration_details(conn, params) do end def register(conn, %{"op" => "connect"} = params) do - create_authorization_params = %{ - "authorization" => Map.merge(params, %{"name" => params["auth_name"]}) - } + authorization_params = Map.put(params, "name", params["auth_name"]) + create_authorization_params = %{"authorization" => authorization_params} with registration_id when not is_nil(registration_id) <- get_session_registration_id(conn), %Registration{} = registration <- Repo.get(Registration, registration_id), - {:ok, auth} <- do_create_authorization(conn, create_authorization_params), + {_, {:ok, auth}} <- + {:create_authorization, do_create_authorization(conn, create_authorization_params)}, %User{} = user <- Repo.preload(auth, :user).user, {:ok, _updated_registration} <- Registration.bind_to_user(registration, user) do conn |> put_session_registration_id(nil) - |> create_authorization( - create_authorization_params, - auth: auth - ) + |> after_create_authorization(auth, authorization_params) else - _ -> - params = Map.delete(params, "password") + {:create_authorization, error} -> + {:register, handle_create_authorization_error(conn, error, create_authorization_params)} - conn - |> put_flash(:error, "Unknown error, please try again.") - |> redirect(to: o_auth_path(conn, :registration_details, params)) + _ -> + {:register, :generic_error} end end - def register(conn, params) do + def register(conn, %{"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 @@ -349,13 +352,12 @@ def register(conn, params) do ) conn + |> put_status(:forbidden) |> put_flash(:error, "Error: #{message}.") - |> redirect(to: o_auth_path(conn, :registration_details, params)) + |> registration_details(params) _ -> - conn - |> put_flash(:error, "Unknown error, please try again.") - |> redirect(to: o_auth_path(conn, :registration_details, params)) + {:register, :generic_error} end 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 002f014e6..9365c7c44 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 @@ -9,7 +9,7 @@ <%= hidden_input f, :redirect_uri, value: @redirect_uri %> <%= hidden_input f, :state, value: @state %> - <%= for strategy <- Pleroma.Config.get([:auth, :oauth_consumer_strategies], []) do %> + <%= for strategy <- Pleroma.Config.oauth_consumer_strategies() do %> <%= submit "Sign in with #{String.capitalize(strategy)}", name: "provider", value: strategy %> <% end %> <% end %> 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 0144675ab..87278e636 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 @@ -26,6 +26,6 @@ <%= submit "Authorize" %> <% end %> -<%= if Pleroma.Config.get([:auth, :oauth_consumer_enabled]) do %> +<%= if Pleroma.Config.oauth_consumer_enabled?() do %> <%= render @view_module, Pleroma.Web.Auth.Authenticator.oauth_consumer_template(), assigns %> <% end %> diff --git a/test/registration_test.exs b/test/registration_test.exs new file mode 100644 index 000000000..6143b82c7 --- /dev/null +++ b/test/registration_test.exs @@ -0,0 +1,59 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2019 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.RegistrationTest do + use Pleroma.DataCase + + import Pleroma.Factory + + alias Pleroma.Registration + alias Pleroma.Repo + + describe "generic changeset" do + test "requires :provider, :uid" do + registration = build(:registration, provider: nil, uid: nil) + + cs = Registration.changeset(registration, %{}) + refute cs.valid? + + assert [ + provider: {"can't be blank", [validation: :required]}, + uid: {"can't be blank", [validation: :required]} + ] == cs.errors + end + + test "ensures uniqueness of [:provider, :uid]" do + registration = insert(:registration) + registration2 = build(:registration, provider: registration.provider, uid: registration.uid) + + cs = Registration.changeset(registration2, %{}) + assert cs.valid? + + assert {:error, + %Ecto.Changeset{ + errors: [ + uid: + {"has already been taken", + [constraint: :unique, constraint_name: "registrations_provider_uid_index"]} + ] + }} = Repo.insert(cs) + + # Note: multiple :uid values per [:user_id, :provider] are intentionally allowed + cs2 = Registration.changeset(registration2, %{uid: "available.uid"}) + assert cs2.valid? + assert {:ok, _} = Repo.insert(cs2) + + cs3 = Registration.changeset(registration2, %{provider: "provider2"}) + assert cs3.valid? + assert {:ok, _} = Repo.insert(cs3) + end + + test "allows `nil` :user_id (user-unbound registration)" do + registration = build(:registration, user_id: nil) + cs = Registration.changeset(registration, %{}) + assert cs.valid? + assert {:ok, _} = Repo.insert(cs) + end + end +end diff --git a/test/web/oauth/oauth_controller_test.exs b/test/web/oauth/oauth_controller_test.exs index 75333f2d5..385896dc6 100644 --- a/test/web/oauth/oauth_controller_test.exs +++ b/test/web/oauth/oauth_controller_test.exs @@ -20,16 +20,11 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest do describe "in OAuth consumer mode, " do setup do - oauth_consumer_enabled_path = [:auth, :oauth_consumer_enabled] oauth_consumer_strategies_path = [:auth, :oauth_consumer_strategies] - oauth_consumer_enabled = Pleroma.Config.get(oauth_consumer_enabled_path) oauth_consumer_strategies = Pleroma.Config.get(oauth_consumer_strategies_path) - - Pleroma.Config.put(oauth_consumer_enabled_path, true) Pleroma.Config.put(oauth_consumer_strategies_path, ~w(twitter facebook)) on_exit(fn -> - Pleroma.Config.put(oauth_consumer_enabled_path, oauth_consumer_enabled) Pleroma.Config.put(oauth_consumer_strategies_path, oauth_consumer_strategies) end) @@ -42,7 +37,7 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest do ] end - test "GET /oauth/authorize also renders OAuth consumer form", %{ + test "GET /oauth/authorize renders auth forms, including OAuth consumer form", %{ app: app, conn: conn } do @@ -97,31 +92,6 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", % } = state_components end - test "on authentication error, redirects to `redirect_uri`", %{app: app, conn: conn} do - state_params = %{ - "scope" => Enum.join(app.scopes, " "), - "client_id" => app.client_id, - "redirect_uri" => app.redirect_uris, - "state" => "" - } - - conn = - conn - |> assign(:ueberauth_failure, %{errors: [%{message: "unknown error"}]}) - |> get( - "/oauth/twitter/callback", - %{ - "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM", - "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs", - "provider" => "twitter", - "state" => Poison.encode!(state_params) - } - ) - - assert response = html_response(conn, 302) - assert redirected_to(conn) == app.redirect_uris - end - test "with user-bound registration, GET /oauth//callback redirects to `redirect_uri` with `code`", %{app: app, conn: conn} do registration = insert(:registration) @@ -152,7 +122,7 @@ test "with user-bound registration, GET /oauth//callback redirects to end end - test "with user-unbound registration, GET /oauth//callback redirects to registration_details page", + test "with user-unbound registration, GET /oauth//callback renders registration_details page", %{app: app, conn: conn} do registration = insert(:registration, user: nil) @@ -177,22 +147,43 @@ test "with user-unbound registration, GET /oauth//callback redirects t } ) - expected_redirect_params = - state_params - |> Map.delete("scope") - |> Map.merge(%{ - "scope" => "read write", - "email" => Registration.email(registration), - "nickname" => Registration.nickname(registration) - }) - - assert response = html_response(conn, 302) - - assert redirected_to(conn) == - o_auth_path(conn, :registration_details, expected_redirect_params) + assert response = html_response(conn, 200) + assert response =~ ~r/name="op" type="submit" value="register"/ + assert response =~ ~r/name="op" type="submit" value="connect"/ + assert response =~ Registration.email(registration) + assert response =~ Registration.nickname(registration) end end + test "on authentication error, GET /oauth//callback redirects to `redirect_uri`", %{ + app: app, + conn: conn + } do + state_params = %{ + "scope" => Enum.join(app.scopes, " "), + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "" + } + + conn = + conn + |> assign(:ueberauth_failure, %{errors: [%{message: "(error description)"}]}) + |> get( + "/oauth/twitter/callback", + %{ + "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM", + "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs", + "provider" => "twitter", + "state" => Poison.encode!(state_params) + } + ) + + assert response = html_response(conn, 302) + assert redirected_to(conn) == app.redirect_uris + assert get_flash(conn, :error) == "Failed to authenticate: (error description)." + end + test "GET /oauth/registration_details renders registration details form", %{ app: app, conn: conn @@ -243,7 +234,7 @@ test "with valid params, POST /oauth/register?op=register redirects to `redirect assert redirected_to(conn) =~ ~r/#{app.redirect_uris}\?code=.+/ end - test "with invalid params, POST /oauth/register?op=register redirects to registration_details page", + test "with invalid params, POST /oauth/register?op=register renders registration_details page", %{ app: app, conn: conn @@ -257,19 +248,22 @@ test "with invalid params, POST /oauth/register?op=register redirects to registr "client_id" => app.client_id, "redirect_uri" => app.redirect_uris, "state" => "a_state", - "nickname" => another_user.nickname, - "email" => another_user.email + "nickname" => "availablenickname", + "email" => "available@email.com" } - conn = - conn - |> put_session(:registration_id, registration.id) - |> post("/oauth/register", params) + 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) - assert response = html_response(conn, 302) + conn = + conn + |> put_session(:registration_id, registration.id) + |> post("/oauth/register", bad_params) - assert redirected_to(conn) == - o_auth_path(conn, :registration_details, params) + assert html_response(conn, 403) =~ ~r/name="op" type="submit" value="register"/ + assert get_flash(conn, :error) == "Error: #{bad_param} has already been taken." + end end test "with valid params, POST /oauth/register?op=connect redirects to `redirect_uri` with `code`", @@ -300,7 +294,7 @@ test "with valid params, POST /oauth/register?op=connect redirects to `redirect_ assert redirected_to(conn) =~ ~r/#{app.redirect_uris}\?code=.+/ end - test "with invalid params, POST /oauth/register?op=connect redirects to registration_details page", + test "with invalid params, POST /oauth/register?op=connect renders registration_details page", %{ app: app, conn: conn @@ -323,10 +317,8 @@ test "with invalid params, POST /oauth/register?op=connect redirects to registra |> put_session(:registration_id, registration.id) |> post("/oauth/register", params) - assert response = html_response(conn, 302) - - assert redirected_to(conn) == - o_auth_path(conn, :registration_details, Map.delete(params, "password")) + assert html_response(conn, 401) =~ ~r/name="op" type="submit" value="connect"/ + assert get_flash(conn, :error) == "Invalid Username/Password" end end