[#923] OAuth consumer mode refactoring, new tests, tests adjustments, readme.

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
 

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://<your_host>/oauth/twitter/callback
+
+* For Facebook, [register an app](https://developers.facebook.com/apps), configure callback URL to https://<your_host>/oauth/facebook/callback, enable Facebook Login service at https://developers.facebook.com/apps/<app_id>/fb-login/settings/
+
+* For Google, [register an app](https://console.developers.google.com), configure callback URL to https://<your_host>/oauth/google/callback
+
+* For Microsoft, [register an app](https://portal.azure.com), configure callback URL to https://<your_host>/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: []]}
+  ]
+```

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

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"

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")

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,23 +62,25 @@ 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
+    with {:ok, auth} <- do_create_authorization(conn, params, opts[:user]) do
+      after_create_authorization(conn, auth, auth_params)
+    else
+      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)
 
-      cond do
-        redirect_uri == "urn:ietf:wg:oauth:2.0:oob" ->
+    if redirect_uri == "urn:ietf:wg:oauth:2.0:oob" do
       render(conn, "results.html", %{
         auth: auth
       })
-
-        true ->
+    else
       connector = if String.contains?(redirect_uri, "?"), do: "&", else: "?"
       url = "#{redirect_uri}#{connector}"
       url_params = %{:code => auth.token}
@@ -94,26 +96,29 @@ def create_authorization(
 
       redirect(conn, external: url)
     end
-    else
-      {scopes_issue, _} when scopes_issue in [:unsupported_scopes, :missing_scopes] ->
+  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
 
-      {:auth_active, false} ->
+  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)
-
-      error ->
-        Authenticator.handle_error(conn, error)
   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
@@ -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
+      {:create_authorization, error} ->
+        {:register, handle_create_authorization_error(conn, error, create_authorization_params)}
+
       _ ->
-        params = Map.delete(params, "password")
-
-        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
 

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 %>

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 %>

test/registration_test.exs

@@ -0,0 +1,59 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2019 Pleroma Authors <https://pleroma.social/>
+# 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

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/<provider>/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/<provider>/callback redirects to
       end
     end
 
-    test "with user-unbound registration, GET /oauth/<provider>/callback redirects to registration_details page",
+    test "with user-unbound registration, GET /oauth/<provider>/callback renders registration_details page",
          %{app: app, conn: conn} do
       registration = insert(:registration, user: nil)
 
@@ -177,20 +147,41 @@ test "with user-unbound registration, GET /oauth/<provider>/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, 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/<provider>/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) ==
-                 o_auth_path(conn, :registration_details, expected_redirect_params)
-      end
+      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", %{
@@ -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"
       }
 
+      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)
+
         conn =
           conn
           |> put_session(:registration_id, registration.id)
-        |> post("/oauth/register", params)
+          |> post("/oauth/register", bad_params)
 
-      assert response = html_response(conn, 302)
-
-      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