From 5c028b8f92aacb296afbd59130d848883f0c3a10 Mon Sep 17 00:00:00 2001 From: Sachin Joshi Date: Fri, 17 May 2019 12:20:31 +0545 Subject: [PATCH 1/4] user creation admin api will create multiple users --- CHANGELOG.md | 1 + .../web/admin_api/admin_api_controller.ex | 37 +++++---- .../web/admin_api/views/account_view.ex | 46 ++++++++++ lib/pleroma/web/router.ex | 2 +- .../admin_api/admin_api_controller_test.exs | 83 ++++++++++++++++++- 5 files changed, 149 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0e849285..5ee853c76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Configuration: Added `extra_cookie_attrs` for setting non-standard cookie attributes. Defaults to ["SameSite=Lax"] so that remote follows work. - Timelines: Messages involving people you have blocked will be excluded from the timeline in all cases instead of just repeats. - Admin API: Move the user related API to `api/pleroma/admin/users` +- Admin API: `POST /api/pleroma/admin/users` will take list of users - Pleroma API: Support for emoji tags in `/api/pleroma/emoji` resulting in a breaking API change - Mastodon API: Support for `exclude_types`, `limit` and `min_id` in `/api/v1/notifications` - Mastodon API: Add `languages` and `registrations` to `/api/v1/instance` diff --git a/lib/pleroma/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index e00b33aba..6048ed35b 100644 --- a/lib/pleroma/web/admin_api/admin_api_controller.ex +++ b/lib/pleroma/web/admin_api/admin_api_controller.ex @@ -46,24 +46,31 @@ def user_unfollow(conn, %{"follower" => follower_nick, "followed" => followed_ni |> json("ok") end - def user_create( - conn, - %{"nickname" => nickname, "email" => email, "password" => password} - ) do - user_data = %{ - nickname: nickname, - name: nickname, - email: email, - password: password, - password_confirmation: password, - bio: "." - } + def users_create(conn, %{"users" => users}) do + result = + Enum.map(users, fn %{"nickname" => nickname, "email" => email, "password" => password} -> + user_data = %{ + nickname: nickname, + name: nickname, + email: email, + password: password, + password_confirmation: password, + bio: "." + } - changeset = User.register_changeset(%User{}, user_data, need_confirmation: false) - {:ok, user} = User.register(changeset) + changeset = User.register_changeset(%User{}, user_data, need_confirmation: false) + + case User.register(changeset) do + {:ok, user} -> + AccountView.render("created.json", %{user: user}) + + {:error, changeset} -> + AccountView.render("create-error.json", %{changeset: changeset}) + end + end) conn - |> json(user.nickname) + |> json(result) end def user_show(conn, %{"nickname" => nickname}) do diff --git a/lib/pleroma/web/admin_api/views/account_view.ex b/lib/pleroma/web/admin_api/views/account_view.ex index 28bb667d8..e1825c5f1 100644 --- a/lib/pleroma/web/admin_api/views/account_view.ex +++ b/lib/pleroma/web/admin_api/views/account_view.ex @@ -44,4 +44,50 @@ def render("invites.json", %{invites: invites}) do invites: render_many(invites, AccountView, "invite.json", as: :invite) } end + + def render("created.json", %{user: user}) do + %{ + type: "success", + code: 201, + data: %{ + nickname: user.nickname, + email: user.email + } + } + end + + def render("create-error.json", %{changeset: %Ecto.Changeset{changes: changes, errors: errors}}) do + %{ + type: "error", + code: 409, + error: parse_error(errors), + data: %{ + nickname: Map.get(changes, :nickname), + email: Map.get(changes, :email) + } + } + end + + defp parse_error([]), do: "" + + defp parse_error(errors) do + ## when nickname is duplicate ap_id constraint error is raised + nickname_error = Keyword.get(errors, :nickname) || Keyword.get(errors, :ap_id) + email_error = Keyword.get(errors, :email) + password_error = Keyword.get(errors, :password) + + cond do + nickname_error -> + "nickname #{elem(nickname_error, 0)}" + + email_error -> + "email #{elem(email_error, 0)}" + + password_error -> + "password #{elem(password_error, 0)}" + + true -> + "" + end + end end diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index 7fef82f82..bbc2fda9b 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -156,7 +156,7 @@ defmodule Pleroma.Web.Router do post("/user", AdminAPIController, :user_create) delete("/users", AdminAPIController, :user_delete) - post("/users", AdminAPIController, :user_create) + post("/users", AdminAPIController, :users_create) patch("/users/:nickname/toggle_activation", AdminAPIController, :user_toggle_activation) put("/users/tag", AdminAPIController, :tag_users) delete("/users/tag", AdminAPIController, :untag_users) diff --git a/test/web/admin_api/admin_api_controller_test.exs b/test/web/admin_api/admin_api_controller_test.exs index 6c1897b5a..a0c9fd56f 100644 --- a/test/web/admin_api/admin_api_controller_test.exs +++ b/test/web/admin_api/admin_api_controller_test.exs @@ -31,12 +31,87 @@ test "Create" do |> assign(:user, admin) |> put_req_header("accept", "application/json") |> post("/api/pleroma/admin/users", %{ - "nickname" => "lain", - "email" => "lain@example.org", - "password" => "test" + "users" => [ + %{ + "nickname" => "lain", + "email" => "lain@example.org", + "password" => "test" + } + ] }) - assert json_response(conn, 200) == "lain" + assert json_response(conn, 200) == [ + %{ + "code" => 201, + "data" => %{ + "email" => "lain@example.org", + "nickname" => "lain" + }, + "type" => "success" + } + ] + end + + test "Cannot create user with exisiting email" do + admin = insert(:user, info: %{is_admin: true}) + user = insert(:user) + + conn = + build_conn() + |> assign(:user, admin) + |> put_req_header("accept", "application/json") + |> post("/api/pleroma/admin/users", %{ + "users" => [ + %{ + "nickname" => "lain", + "email" => user.email, + "password" => "test" + } + ] + }) + + assert json_response(conn, 200) == [ + %{ + "code" => 409, + "data" => %{ + "email" => user.email, + "nickname" => "lain" + }, + "error" => "email has already been taken", + "type" => "error" + } + ] + end + + test "Cannot create user with exisiting nickname" do + admin = insert(:user, info: %{is_admin: true}) + user = insert(:user) + + conn = + build_conn() + |> assign(:user, admin) + |> put_req_header("accept", "application/json") + |> post("/api/pleroma/admin/users", %{ + "users" => [ + %{ + "nickname" => user.nickname, + "email" => "someuser@plerama.social", + "password" => "test" + } + ] + }) + + assert json_response(conn, 200) == [ + %{ + "code" => 409, + "data" => %{ + "email" => "someuser@plerama.social", + "nickname" => user.nickname + }, + "error" => "nickname has already been taken", + "type" => "error" + } + ] end end From 5534d4c67675901ab272ee47355ad43dfae99033 Mon Sep 17 00:00:00 2001 From: Sachin Joshi Date: Sat, 1 Jun 2019 11:17:53 +0545 Subject: [PATCH 2/4] make bulk user creation from admin works as a transaction --- lib/pleroma/user.ex | 8 ++- .../web/admin_api/admin_api_controller.ex | 45 +++++++++---- .../web/admin_api/views/account_view.ex | 2 +- .../admin_api/admin_api_controller_test.exs | 66 ++++++++++++++++++- 4 files changed, 104 insertions(+), 17 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index c6a562a61..722e8ff6b 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -276,7 +276,13 @@ defp autofollow_users(user) do @doc "Inserts provided changeset, performs post-registration actions (confirmation email sending etc.)" def register(%Ecto.Changeset{} = changeset) do with {:ok, user} <- Repo.insert(changeset), - {:ok, user} <- autofollow_users(user), + {:ok, user} <- post_register_action(user) do + {:ok, user} + end + end + + def post_register_action(%User{} = user) do + with {:ok, user} <- autofollow_users(user), {:ok, user} <- set_cache(user), {:ok, _} <- Pleroma.User.WelcomeMessage.post_welcome_message_to_user(user), {:ok, _} <- try_send_confirmation_email(user) do diff --git a/lib/pleroma/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index 6048ed35b..60fd4e571 100644 --- a/lib/pleroma/web/admin_api/admin_api_controller.ex +++ b/lib/pleroma/web/admin_api/admin_api_controller.ex @@ -47,7 +47,7 @@ def user_unfollow(conn, %{"follower" => follower_nick, "followed" => followed_ni end def users_create(conn, %{"users" => users}) do - result = + changesets = Enum.map(users, fn %{"nickname" => nickname, "email" => email, "password" => password} -> user_data = %{ nickname: nickname, @@ -58,19 +58,40 @@ def users_create(conn, %{"users" => users}) do bio: "." } - changeset = User.register_changeset(%User{}, user_data, need_confirmation: false) - - case User.register(changeset) do - {:ok, user} -> - AccountView.render("created.json", %{user: user}) - - {:error, changeset} -> - AccountView.render("create-error.json", %{changeset: changeset}) - end + User.register_changeset(%User{}, user_data, need_confirmation: false) + end) + |> Enum.reduce(Ecto.Multi.new(), fn changeset, multi -> + Ecto.Multi.insert(multi, Ecto.UUID.generate(), changeset) end) - conn - |> json(result) + case Pleroma.Repo.transaction(changesets) do + {:ok, users} -> + res = + users + |> Map.values() + |> Enum.map(fn user -> + {:ok, user} = User.post_register_action(user) + user + end) + |> Enum.map(&AccountView.render("created.json", %{user: &1})) + + conn + |> json(res) + + {:error, id, changeset, _} -> + res = + Enum.map(changesets.operations, fn + {current_id, {:changeset, _current_changeset, _}} when current_id == id -> + AccountView.render("create-error.json", %{changeset: changeset}) + + {_, {:changeset, current_changeset, _}} -> + AccountView.render("create-error.json", %{changeset: current_changeset}) + end) + + conn + |> put_status(:conflict) + |> json(res) + end end def user_show(conn, %{"nickname" => nickname}) do diff --git a/lib/pleroma/web/admin_api/views/account_view.ex b/lib/pleroma/web/admin_api/views/account_view.ex index e1825c5f1..cccdeff7e 100644 --- a/lib/pleroma/web/admin_api/views/account_view.ex +++ b/lib/pleroma/web/admin_api/views/account_view.ex @@ -48,7 +48,7 @@ def render("invites.json", %{invites: invites}) do def render("created.json", %{user: user}) do %{ type: "success", - code: 201, + code: 200, data: %{ nickname: user.nickname, email: user.email diff --git a/test/web/admin_api/admin_api_controller_test.exs b/test/web/admin_api/admin_api_controller_test.exs index a0c9fd56f..019905137 100644 --- a/test/web/admin_api/admin_api_controller_test.exs +++ b/test/web/admin_api/admin_api_controller_test.exs @@ -36,18 +36,31 @@ test "Create" do "nickname" => "lain", "email" => "lain@example.org", "password" => "test" + }, + %{ + "nickname" => "lain2", + "email" => "lain2@example.org", + "password" => "test" } ] }) assert json_response(conn, 200) == [ %{ - "code" => 201, + "code" => 200, "data" => %{ "email" => "lain@example.org", "nickname" => "lain" }, "type" => "success" + }, + %{ + "code" => 200, + "data" => %{ + "email" => "lain2@example.org", + "nickname" => "lain2" + }, + "type" => "success" } ] end @@ -70,7 +83,7 @@ test "Cannot create user with exisiting email" do ] }) - assert json_response(conn, 200) == [ + assert json_response(conn, 409) == [ %{ "code" => 409, "data" => %{ @@ -101,7 +114,7 @@ test "Cannot create user with exisiting nickname" do ] }) - assert json_response(conn, 200) == [ + assert json_response(conn, 409) == [ %{ "code" => 409, "data" => %{ @@ -113,6 +126,53 @@ test "Cannot create user with exisiting nickname" do } ] end + + test "Multiple user creation works in transaction" do + admin = insert(:user, info: %{is_admin: true}) + user = insert(:user) + + conn = + build_conn() + |> assign(:user, admin) + |> put_req_header("accept", "application/json") + |> post("/api/pleroma/admin/users", %{ + "users" => [ + %{ + "nickname" => "newuser", + "email" => "newuser@pleroma.social", + "password" => "test" + }, + %{ + "nickname" => "lain", + "email" => user.email, + "password" => "test" + } + ] + }) + + assert json_response(conn, 409) == [ + %{ + "code" => 409, + "data" => %{ + "email" => user.email, + "nickname" => "lain" + }, + "error" => "email has already been taken", + "type" => "error" + }, + %{ + "code" => 409, + "data" => %{ + "email" => "newuser@pleroma.social", + "nickname" => "newuser" + }, + "error" => "", + "type" => "error" + } + ] + + assert User.get_by_nickname("newuser") === nil + end end describe "/api/pleroma/admin/users/:nickname" do From e394fc2eefdd7a4c7edd5fb3c04b445215d4a86c Mon Sep 17 00:00:00 2001 From: Sachin Joshi Date: Sun, 2 Jun 2019 09:48:45 +0545 Subject: [PATCH 3/4] fix the flaky test for users creation by admin --- .../admin_api/admin_api_controller_test.exs | 20 ++----------------- 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/test/web/admin_api/admin_api_controller_test.exs b/test/web/admin_api/admin_api_controller_test.exs index 9721a4034..86b160246 100644 --- a/test/web/admin_api/admin_api_controller_test.exs +++ b/test/web/admin_api/admin_api_controller_test.exs @@ -47,24 +47,8 @@ test "Create" do ] }) - assert json_response(conn, 200) == [ - %{ - "code" => 200, - "data" => %{ - "email" => "lain@example.org", - "nickname" => "lain" - }, - "type" => "success" - }, - %{ - "code" => 200, - "data" => %{ - "email" => "lain2@example.org", - "nickname" => "lain2" - }, - "type" => "success" - } - ] + response = json_response(conn, 200) |> Enum.map(&Map.get(&1, "type")) + assert response == ["success", "success"] end test "Cannot create user with exisiting email" do From 37229af15fe6d540e7b4a0b6e89f548a100d51c7 Mon Sep 17 00:00:00 2001 From: Sachin Joshi Date: Thu, 22 Aug 2019 00:15:00 +0545 Subject: [PATCH 4/4] remove old user create and delete routes for admin --- lib/pleroma/web/router.ex | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index eb3ee03f3..445cf62e2 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -151,10 +151,6 @@ defmodule Pleroma.Web.Router do post("/users/follow", AdminAPIController, :user_follow) post("/users/unfollow", AdminAPIController, :user_unfollow) - # TODO: to be removed at version 1.0 - delete("/user", AdminAPIController, :user_delete) - post("/user", AdminAPIController, :user_create) - delete("/users", AdminAPIController, :user_delete) post("/users", AdminAPIController, :users_create) patch("/users/:nickname/toggle_activation", AdminAPIController, :user_toggle_activation)