[#1985] Prevented force login on registration if account approval and/or email confirmation needed.

Refactored login code in OAuthController, reused in AccountController. Added tests.

lib/pleroma/web/api_spec/operations/account_operation.ex

@@ -449,21 +449,32 @@ defp create_request do
     }
   end
 
-  # TODO: This is actually a token respone, but there's no oauth operation file yet.
+  # Note: this is a token response (if login succeeds!), but there's no oauth operation file yet.
   defp create_response do
     %Schema{
       title: "AccountCreateResponse",
       description: "Response schema for an account",
       type: :object,
       properties: %{
+        # The response when auto-login on create succeeds (token is issued):
         token_type: %Schema{type: :string},
         access_token: %Schema{type: :string},
         refresh_token: %Schema{type: :string},
         scope: %Schema{type: :string},
         created_at: %Schema{type: :integer, format: :"date-time"},
         me: %Schema{type: :string},
-        expires_in: %Schema{type: :integer}
+        expires_in: %Schema{type: :integer},
+        #
+        # The response when registration succeeds but auto-login fails (no token):
+        identifier: %Schema{type: :string},
+        message: %Schema{type: :string}
       },
+      required: [],
+      # Note: example of successful registration with failed login response:
+      # example: %{
+      #   "identifier" => "missing_confirmed_email",
+      #   "message" => "You have been registered. Please check your email for further instructions."
+      # },
       example: %{
         "token_type" => "Bearer",
         "access_token" => "i9hAVVzGld86Pl5JtLtizKoXVvtTlSCJvwaugCxvZzk",

lib/pleroma/web/mastodon_api/controllers/account_controller.ex

@@ -27,8 +27,8 @@ defmodule Pleroma.Web.MastodonAPI.AccountController do
   alias Pleroma.Web.MastodonAPI.MastodonAPI
   alias Pleroma.Web.MastodonAPI.MastodonAPIController
   alias Pleroma.Web.MastodonAPI.StatusView
+  alias Pleroma.Web.OAuth.OAuthController
   alias Pleroma.Web.OAuth.OAuthView
-  alias Pleroma.Web.OAuth.Token
   alias Pleroma.Web.TwitterAPI.TwitterAPI
 
   plug(Pleroma.Web.ApiSpec.CastAndValidate)
@@ -101,10 +101,33 @@ def create(%{assigns: %{app: app}, body_params: params} = conn, _params) do
     with :ok <- validate_email_param(params),
          :ok <- TwitterAPI.validate_captcha(app, params),
          {:ok, user} <- TwitterAPI.register_user(params),
-         {:ok, token} <- Token.create_token(app, user, %{scopes: app.scopes}) do
+         {_, {:ok, token}} <-
+           {:login, OAuthController.login(user, app, app.scopes)} do
       json(conn, OAuthView.render("token.json", %{user: user, token: token}))
     else
-      {:error, error} -> json_response(conn, :bad_request, %{error: error})
+      {:login, {:account_status, :confirmation_pending}} ->
+        json_response(conn, :ok, %{
+          message: "You have been registered. Please check your email for further instructions.",
+          identifier: "missing_confirmed_email"
+        })
+
+      {:login, {:account_status, :approval_pending}} ->
+        json_response(conn, :ok, %{
+          message:
+            "You have been registered. You'll be able to log in once your account is approved.",
+          identifier: "awaiting_approval"
+        })
+
+      {:login, _} ->
+        json_response(conn, :ok, %{
+          message:
+            "You have been registered. Some post-registration steps may be pending. " <>
+              "Please log in manually.",
+          identifier: "manual_login_required"
+        })
+
+      {:error, error} ->
+        json_response(conn, :bad_request, %{error: error})
     end
   end
 

lib/pleroma/web/oauth/oauth_controller.ex

@@ -260,11 +260,8 @@ def token_exchange(
       ) do
     with {:ok, %User{} = user} <- Authenticator.get_user(conn),
          {:ok, app} <- Token.Utils.fetch_app(conn),
-         {:account_status, :active} <- {:account_status, User.account_status(user)},
+         requested_scopes <- Scopes.fetch_scopes(params, app.scopes),
-         {:ok, scopes} <- validate_scopes(app, params),
+         {:ok, token} <- login(user, app, requested_scopes) do
-         {:ok, auth} <- Authorization.create_authorization(app, user, scopes),
-         {:mfa_required, _, _, false} <- {:mfa_required, user, auth, MFA.require?(user)},
-         {:ok, token} <- Token.exchange_token(app, auth) do
       json(conn, OAuthView.render("token.json", %{user: user, token: token}))
     else
       error ->
@@ -522,6 +519,8 @@ def register(%Plug.Conn{} = conn, %{"authorization" => _, "op" => "register"} =
     end
   end
 
+  defp do_create_authorization(conn, auth_attrs, user \\ nil)
+
   defp do_create_authorization(
          %Plug.Conn{} = conn,
          %{
@@ -531,19 +530,37 @@ defp do_create_authorization(
                "redirect_uri" => redirect_uri
              } = auth_attrs
          },
-         user \\ nil
+         user
        ) do
     with {_, {:ok, %User{} = user}} <-
            {: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),
-         {:ok, scopes} <- validate_scopes(app, auth_attrs),
+         requested_scopes <- Scopes.fetch_scopes(auth_attrs, app.scopes),
-         {:account_status, :active} <- {:account_status, User.account_status(user)},
+         {:ok, auth} <- do_create_authorization(user, app, requested_scopes) do
-         {:ok, auth} <- Authorization.create_authorization(app, user, scopes) do
       {:ok, auth, user}
     end
   end
 
+  defp do_create_authorization(%User{} = user, %App{} = app, requested_scopes)
+       when is_list(requested_scopes) do
+    with {:account_status, :active} <- {:account_status, User.account_status(user)},
+         {:ok, scopes} <- validate_scopes(app, requested_scopes),
+         {:ok, auth} <- Authorization.create_authorization(app, user, scopes) do
+      {:ok, auth}
+    end
+  end
+
+  # Note: intended to be a private function but opened for AccountController that logs in on signup
+  @doc "If checks pass, creates authorization and token for given user, app and requested scopes."
+  def login(%User{} = user, %App{} = app, requested_scopes) when is_list(requested_scopes) do
+    with {:ok, auth} <- do_create_authorization(user, app, requested_scopes),
+         {:mfa_required, _, _, false} <- {:mfa_required, user, auth, MFA.require?(user)},
+         {:ok, token} <- Token.exchange_token(app, auth) do
+      {:ok, token}
+    end
+  end
+
   # Special case: Local MastodonFE
   defp redirect_uri(%Plug.Conn{} = conn, "."), do: auth_url(conn, :login)
 
@@ -560,12 +577,15 @@ defp build_and_response_mfa_token(user, auth) do
     end
   end
 
-  @spec validate_scopes(App.t(), map()) ::
+  @spec validate_scopes(App.t(), map() | list()) ::
           {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes}
-  defp validate_scopes(%App{} = app, params) do
+  defp validate_scopes(%App{} = app, params) when is_map(params) do
-    params
+    requested_scopes = Scopes.fetch_scopes(params, app.scopes)
-    |> Scopes.fetch_scopes(app.scopes)
+    validate_scopes(app, requested_scopes)
-    |> Scopes.validate(app.scopes)
+  end
+
+  defp validate_scopes(%App{} = app, requested_scopes) when is_list(requested_scopes) do
+    Scopes.validate(requested_scopes, app.scopes)
   end
 
   def default_redirect_uri(%App{} = app) do

test/web/mastodon_api/controllers/account_controller_test.exs

@@ -5,7 +5,6 @@
 defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do
   use Pleroma.Web.ConnCase
 
-  alias Pleroma.Config
   alias Pleroma.Repo
   alias Pleroma.User
   alias Pleroma.Web.ActivityPub.ActivityPub
@@ -16,8 +15,6 @@ defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do
   import Pleroma.Factory
 
   describe "account fetching" do
-    setup do: clear_config([:instance, :limit_to_local_content])
-
     test "works by id" do
       %User{id: user_id} = insert(:user)
 
@@ -42,7 +39,7 @@ test "works by nickname" do
     end
 
     test "works by nickname for remote users" do
-      Config.put([:instance, :limit_to_local_content], false)
+      clear_config([:instance, :limit_to_local_content], false)
 
       user = insert(:user, nickname: "user@example.com", local: false)
 
@@ -53,7 +50,7 @@ test "works by nickname for remote users" do
     end
 
     test "respects limit_to_local_content == :all for remote user nicknames" do
-      Config.put([:instance, :limit_to_local_content], :all)
+      clear_config([:instance, :limit_to_local_content], :all)
 
       user = insert(:user, nickname: "user@example.com", local: false)
 
@@ -63,7 +60,7 @@ test "respects limit_to_local_content == :all for remote user nicknames" do
     end
 
     test "respects limit_to_local_content == :unauthenticated for remote user nicknames" do
-      Config.put([:instance, :limit_to_local_content], :unauthenticated)
+      clear_config([:instance, :limit_to_local_content], :unauthenticated)
 
       user = insert(:user, nickname: "user@example.com", local: false)
       reading_user = insert(:user)
@@ -903,8 +900,10 @@ test "blocking / unblocking a user" do
       [valid_params: valid_params]
     end
 
-    test "Account registration via Application, no confirmation required", %{conn: conn} do
+    test "registers and logs in without :account_activation_required / :account_approval_required",
+         %{conn: conn} do
       clear_config([:instance, :account_activation_required], false)
+      clear_config([:instance, :account_approval_required], false)
 
       conn =
         conn
@@ -962,15 +961,16 @@ test "Account registration via Application, no confirmation required", %{conn: c
 
       token_from_db = Repo.get_by(Token, token: token)
       assert token_from_db
-      token_from_db = Repo.preload(token_from_db, :user)
+      user = Repo.preload(token_from_db, :user).user
-      assert token_from_db.user
+
-      refute token_from_db.user.confirmation_pending
+      assert user
+      refute user.confirmation_pending
+      refute user.approval_pending
     end
 
-    setup do: clear_config([:instance, :account_approval_required])
+    test "registers but does not log in with :account_activation_required", %{conn: conn} do
-
-    test "Account registration via Application", %{conn: conn} do
       clear_config([:instance, :account_activation_required], true)
+      clear_config([:instance, :account_approval_required], false)
 
       conn =
         conn
@@ -1019,23 +1019,18 @@ test "Account registration via Application", %{conn: conn} do
           agreement: true
         })
 
-      %{
+      response = json_response_and_validate_schema(conn, 200)
-        "access_token" => token,
+      assert %{"identifier" => "missing_confirmed_email"} = response
-        "created_at" => _created_at,
+      refute response["access_token"]
-        "scope" => ^scope,
+      refute response["token_type"]
-        "token_type" => "Bearer"
-      } = json_response_and_validate_schema(conn, 200)
 
-      token_from_db = Repo.get_by(Token, token: token)
+      user = Repo.get_by(User, email: "lain@example.org")
-      assert token_from_db
+      assert user.confirmation_pending
-      token_from_db = Repo.preload(token_from_db, :user)
-      assert token_from_db.user
-
-      assert token_from_db.user.confirmation_pending
     end
 
-    test "Account registration via app with account_approval_required", %{conn: conn} do
+    test "registers but does not log in with :account_approval_required", %{conn: conn} do
-      Pleroma.Config.put([:instance, :account_approval_required], true)
+      clear_config([:instance, :account_approval_required], true)
+      clear_config([:instance, :account_activation_required], false)
 
       conn =
         conn
@@ -1085,21 +1080,15 @@ test "Account registration via app with account_approval_required", %{conn: conn
           reason: "I'm a cool dude, bro"
         })
 
-      %{
+      response = json_response_and_validate_schema(conn, 200)
-        "access_token" => token,
+      assert %{"identifier" => "awaiting_approval"} = response
-        "created_at" => _created_at,
+      refute response["access_token"]
-        "scope" => ^scope,
+      refute response["token_type"]
-        "token_type" => "Bearer"
-      } = json_response_and_validate_schema(conn, 200)
 
-      token_from_db = Repo.get_by(Token, token: token)
+      user = Repo.get_by(User, email: "lain@example.org")
-      assert token_from_db
-      token_from_db = Repo.preload(token_from_db, :user)
-      assert token_from_db.user
 
-      assert token_from_db.user.approval_pending
+      assert user.approval_pending
-
+      assert user.registration_reason == "I'm a cool dude, bro"
-      assert token_from_db.user.registration_reason == "I'm a cool dude, bro"
     end
 
     test "returns error when user already registred", %{conn: conn, valid_params: valid_params} do
@@ -1153,11 +1142,9 @@ test "returns bad_request if missing required params", %{
       end)
     end
 
-    setup do: clear_config([:instance, :account_activation_required])
-
     test "returns bad_request if missing email params when :account_activation_required is enabled",
          %{conn: conn, valid_params: valid_params} do
-      Pleroma.Config.put([:instance, :account_activation_required], true)
+      clear_config([:instance, :account_activation_required], true)
 
       app_token = insert(:oauth_token, user: nil)