Add the ability to add and remove users from the webui #307

jere0500 · 2024-06-04T09:25:47Z

Idea

In Addition to managing user by script for testing purposes. I thought it would be easier to also allow creation and deletion of users directly in the webui.

I therefore added a button to the list menu.

The button opens a form where users can be simply generated. Either create a random user or completely/ partially filling form.

In order to delete users again I also added delete buttons to the end of the users in the list.

Deletion can also be done in the user action dropdown menu.

Addition/ Deletion is only visible and usable by accounts with system permission.

(Unfortunately the icons of Fontawesome do only partially work, probably due to the wrong version)

Current problems:

I have problems with the delete query raising errors as some tables didn't exist in my setup (e.g. telemetry). I therefore switched to Ecto.Adapters.SQL.query instead of Ecto.Adapters.SQL.query! to not raise errors.
I also split up the deletion from moderation_reports, because the combined query would fail due to certain columns not existing.
This might not be a problem on other setups.

Also deletion of user like the spadsbot user does not work, because the spadsbot creates lobbies and more foreign key dependencies.

There might be some better ways to route to the correct form for user creation.

I would be happy to get some feedback.

geekingfrog

Very nice to have an API to create users. This may get extended later, for testing purposes (both integration and load testing).
Perhaps this would be worth mentioning quickly in the testing guide as well.

I'm more concerned about the delete user button. There's a confirmation popup, but that's still a big irrevocable action and I'm not super sure we want that.
Perhaps someone with more experience using the admin website can comment on this @StanczakDominik (or ping someone else)?

I think without the delete and my suggested changes that would be a nice addition. Not even two days ago I ran into trouble trying to create some random local accounts to test something, and that would have helped.

geekingfrog · 2024-06-04T17:25:49Z

lib/teiserver/account/tasks/delete_user_task.ex

    end)

    # And now the users
    query = "DELETE FROM account_users WHERE id = ANY($1)"
-    Ecto.Adapters.SQL.query!(Repo, query, [id_list])
+    Ecto.Adapters.SQL.query!(Repo, query, [int_id_list])


I have problems with the delete query raising errors as some tables didn't exist in my setup (e.g. telemetry). I therefore switched to Ecto.Adapters.SQL.query instead of Ecto.Adapters.SQL.query! to not raise errors.

You should ensure your setup is correct first. There are some guide in the repo, and you might be interested in postgres setup

I have the telemetry tables locally and I didn't recall doing anything fancy to get them.

The main problem with swapping query!/3 for query/3 is that it doesn't raise an error and returns the exception as a value, but that doesn't fix the underlying problem.

If anything, one thing to fix here would be to wrap all the deletes inside a transaction, but that's irrelevant for your changes.

I did recreate my setup with the guide once again to make sure I did not miss anything.
However, the some columns still do not exist.
I observed that all telemetry_\*_event_types only have ["id", "name"].
That makes sense, considering they only define the event types, not when events happen to users.
The telemetry_\*_events contain the user_id, therefore I think this was originally meant.

I also changed some queries, because the columns are named differently.

I don't have any problem using query! now

To look at the columns I used something like this:

[ "SELECT * FROM moderation_actions" ] |> Enum.each(fn query -> Ecto.Adapters.SQL.query!(Repo, query) |> IO.inspect() end)

geekingfrog · 2024-06-04T17:26:09Z

lib/teiserver/account/tasks/delete_user_task.ex

-      "DELETE FROM moderation_actions WHERE target_id = ANY($1)"
+      "DELETE FROM moderation_reports WHERE reporter_id = ANY($1)",
+      "DELETE FROM moderation_reports WHERE target_id = ANY($1)",
+      "DELETE FROM moderation_actions WHERE responder_id = ANY($1)"


👍 good catch updating these missing relations.

geekingfrog · 2024-06-04T17:29:57Z

lib/teiserver_web/controllers/admin/user_controller.ex

+    (params["name"] || "" == "")
+
+    passwordfn = fn -> if is_nil(params["password"]) or String.trim(params["password"]) == "" do "password" else params["password"] end end
+    emailfn = fn -> if is_nil(params["email"]) or String.trim(params["email"]) == "" do UUID.uuid1() else params["email"] end end


you're going to want to setup the formatting tool, either mix format or your IDE.

should be done now

geekingfrog · 2024-06-04T17:32:28Z

lib/teiserver_web/controllers/admin/user_controller.ex

+    user_params = %{
+      "name" => params["name"],
+      "password" => passwordfn.(),
+      "email" => emailfn.(),


Is there any reason you're invoking the lambdas you just created above instead of getting values directly?
I'm thinking of

password = if ... do: "password", else: ... end

geekingfrog · 2024-06-04T17:34:31Z

lib/teiserver_web/router.ex

@@ -561,6 +561,8 @@ defmodule TeiserverWeb.Router do

    # User stuff
    put("/users/gdpr_clean/:id", UserController, :gdpr_clean)
+    get("/users/delete_user/:id", UserController, :delete_user)
+    put("/users/delete_user/:id", UserController, :delete_user)


The routing is roughly REST-ish, so it would be better to use the delete function (and associated http verb) to delete the user.

should be correct now

geekingfrog · 2024-06-04T17:39:09Z

lib/teiserver_web/controllers/admin/user_controller.ex

+  end
+
+  @spec create_post(Plug.Conn.t(), map) :: Plug.Conn.t()
+  def create_post(conn, params \\ %{}) do


You should add a guard to these functions first, that checks the user has indeed the correct permissions to perform that. Currently, the only check is done in the template rendering, but that is trivially bypassable.
Something along the lines of:

def create_post(conn, _) when not Teiserver.Account.Authlib.allow?(conn, "Server") do # flash + redirect elsewhere end def create_post(conn, params \\ %{}) do # your function end

This idea did not compile. I used the other approach mentioned further down.

Ah yes indeed, because the guards must be macros since they generate special bytecode for dispatch.

geekingfrog · 2024-06-04T17:39:27Z

lib/teiserver_web/controllers/admin/user_controller.ex

+
+
+  @spec delete_user(Plug.Conn.t(), map()) :: Plug.Conn.t()
+  def delete_user(conn, %{"id" => id}) do


Similar comment regarding authentication.

Also, if the given parameters don't have an id key, the pattern match will fail and you're going to get a server error.
It would be better to add another catch all clause after this one to return back a 400 with a pre-rendered form (optional but nice to have)

I added a catch case using a put_flash and a redirect, hope this checks out

geekingfrog · 2024-06-04T17:46:13Z

lib/teiserver_web/controllers/admin/user_controller.ex

+
+    case Teiserver.Account.UserLib.has_access(user, conn) do
+      {true, _} ->
+        Teiserver.manually_delete_user(id)


That's my biggest problem in this change. manually_delete_user is a mix task, and is supposed to be invoked through the terminal with the mix command, or some script, but definitely not from application code.

There is Teiserver.Account.delete_user that is more suitable, although it doesn't remove all the linked rows.
A quick grep shows that it's likely a generated stub and isn't really used. I'd rather have this function taking care of deleting the various entities and then you can call it from there.

manually_delete_user uses Teiserver.Admin.DeleteUserTask.delete_users internally, which I now also use.
I could also use Teiserver.Account.delete_user if that is better.

geekingfrog · 2024-06-04T17:46:52Z

lib/teiserver_web/controllers/admin/user_controller.ex

+  def delete_user(conn, %{"id" => id}) do
+    user = Account.get_user_by_id(id)
+
+    case Teiserver.Account.UserLib.has_access(user, conn) do


👍 for the access control here. I'd recommend using that approach or the one I suggested in both functions.

geekingfrog · 2024-06-04T17:48:19Z

lib/teiserver_web/templates/admin/user/index.html.heex

@@ -1,5 +1,6 @@
 <% bsname = view_colour()
-is_moderator = allow?(@conn, "Moderator") %>
+is_moderator = allow?(@conn, "Moderator")
+is_system = allow?(@conn, "System") %>


Why is this permission different than the Server you used elsewhere? I'm not on top of the permission system, so I may very well have missed something.

I don't know what I originally thought here. I switched to is_server to have it the same way as the other.

L-e-x-o-n · 2024-06-04T19:33:35Z

In my opinion, this shouldn't be creating users directly and should instead go through the existing CacheUser.register_user. That one goes through the whole process with a few more checks and verifications e.g. checking if name is available and clean, email isn't already registered, password is valid....

jauggy · 2024-06-05T00:09:45Z

If you want this feature purely for testing then you could check for a config variable. For example, there are certain endpoints that only are enabled by this flag:

  enable_hailstorm: true,

that is set inside dev.exs
So you could also set up some similar flag to tell if we are in a test/dev environment where adding/deleting users is safe.

geekingfrog · 2024-06-05T17:30:55Z

In my opinion, this shouldn't be creating users directly and should instead go through the existing CacheUser.register_user. That one goes through the whole process with a few more checks and verifications e.g. checking if name is available and clean, email isn't already registered, password is valid....

oh you're right, it's much better there.
There is some logic around sending email as well, that is bypassed for some special domains (hailstorm related). So I'd suggest you default the emails of these new user to use the @agents domain, so you get a problem with emails.
One side effect of that is that you don't get the Verified role, but that may not be a problem, and that can be manually edited from the UI with an admin account, so no big deal.

jere0500 · 2024-06-06T16:56:44Z

Thanks for all the feedback!
I will look into fixing the parts.

I tried it with Teiserver.CacheUser.register_user in a previous version,
but had some problems with signing in to the created user with chobby, because I could not get the verification code.
Giving the role manually could have probably done the trick.
I will look into using it again!

I also like the Idea of locking add/ delete user behind a flag for now, will check that one out.

jere0500 · 2024-06-07T19:21:07Z

I now use Teiserver.CacheUser.register_user.
When no Mailer is defined, no mail will be sent and the verification code will be set to empty. This allows signing with created users through chobby, bypassing the verification.
I also moved from manually_delete_user to Teiserver.Account.delete_user.

Next, I will look at using delete and post to improve routing.

jere0500 · 2024-06-08T16:54:57Z

I now added the guards, changed to using delete and post for correct rooting.

The formatting (done with mix format) should now be correct too.

Idea

I also added a commit, which locks delete and add behind :test_mode which can be set in config/config.exs. If this should be done differently or generally be available without setting :test_mode I will change that, or add some documentation to the local testing guide.

geekingfrog

I think you can drop the whole configuration bits and put a disclaimer on the templates like "this is for testing/local dev only, proceed with care in production" for both create&update endpoints. That should simplify the code.

The delete endpoint could just be deleted in my opinion.

Also, I'm very sorry for the super late feedback. It slipped my mind and then I was away. Don't hesitate to ping me here or on discord (channel teiserver-spads, I'm blackmelon on discord) if you need further feedback on this one.

geekingfrog · 2024-06-26T15:45:02Z

lib/teiserver/data/cache_user.ex

-            case EmailHelper.new_user(user) do
-              {:error, error} ->
-                Logger.error("Error sending new user email - #{user.email} - #{error}")
+            case Application.fetch_env(:teiserver, Teiserver.Mailer) do


I don't think this check should live here. This file deals with users, and shouldn't need to know about the internals of the email system. This logic should be moved inside EmailHelper.new_user instead. The deletion of the cache key related to the verification code should stay in this file though. You can do that when you get back a no_verify or perhaps a new atom that could be returned.

I noticed that EmailHelper.new_user already implements some logic to disable the verification in the webui. So I decided to remove my workaround all together.

geekingfrog · 2024-06-26T15:46:15Z

lib/teiserver_web/controllers/admin/user_controller.ex

+  end
+
+  @spec create_post(Plug.Conn.t(), map) :: Plug.Conn.t()
+  def create_post(conn, params \\ %{}) do


Ah yes indeed, because the guards must be macros since they generate special bytecode for dispatch.

geekingfrog · 2024-06-26T15:49:31Z

lib/teiserver_web/controllers/admin/user_controller.ex

+      conn
+      |> put_flash(:danger, "Invalid user name")
+      |> redirect(to: ~p"/teiserver/admin/user")
+    end


This redirect doesn't actually do anything, because everything is immutable and there is no early return in elixir.
If you want to guard against missing or empty params, the usual way is to have a private function that actually do the action once you checked things like:

if is_nil(params["name"]) do ... else do_create_post(conn, params) end defp do_create_post(conn, params) do # do the actual logic here. end

Thanks for the hint. I now implemented it that way.

geekingfrog · 2024-06-26T15:51:16Z

lib/teiserver_web/controllers/admin/user_controller.ex

+          "password"
+        else
+          params["password"]
+        end


A slightly simpler way to do that is password = Map.get(params, "password", "password"). Map.get/3 allows you to provide a default value if not present.

I have tested it and this works well. In earlier iterations I had problems detecting "" as empty to trigger the default value.

geekingfrog · 2024-06-26T15:51:59Z

lib/teiserver_web/controllers/admin/user_controller.ex

+      conn
+      |> put_flash(:danger, "not in testmode")
+      |> redirect(to: ~p"/teiserver/admin/user")
+    end


Similar comment regarding this expression having no effect.

geekingfrog · 2024-06-26T15:55:25Z

lib/teiserver_web/router.ex

+
+    if Application.compile_env(:teiserver, Teiserver)[:test_mode] do
+      delete("/users/delete_user/:id", UserController, :delete_user)
+    end


I am not a big fan of adding more config flags, especially for such a feature. I feel this is better served by permissions.
Since this feature doesn't already exist and hasn't been required (I think) for a time, I would say it's safe to assume we don't need it. If you want to delete users locally, the mix task should be fine.
I personally would prefer not having this delete user feature at all, until we get some feedback from the mods/admins that are using teiserver in anger to add it.

I now removed that delete endpoint all together (for now).

geekingfrog · 2024-06-26T16:01:37Z

lib/teiserver_web/router.ex

@@ -617,6 +621,11 @@ defmodule TeiserverWeb.Router do
    get("/tools/falist", ToolController, :falist)
    get("/tools/test_page", ToolController, :test_page)

+    if Application.compile_env(:teiserver, Teiserver)[:test_mode] do


compile_env gets the value at compile time, and it's generally a bad idea. Because you need to recompile the app to change it. Also, this complicate matters when compiling the app from a different machine.

If you want to conditionally have these endpoints + template, a better way is to put that into the config files under config/config.exs set to false, and inside config/test.exs config/dev.exs put that to true.

But, tbh, I'm not sure all of this is warranted, requiring admin/server privilege to be able to create users manually may just be enough.

I have removed that for now. I currently think relying on the admin/ server privilege should be sufficient.

jauggy · 2024-07-01T11:49:34Z

I do agree that the delete feature is not needed. But to be honest even the add user feature is not super useful too since mix teiserver.fakedata adds a whole bunch of fake users. What was the original driving force behind this PR?

jere0500 · 2024-07-09T14:36:44Z

Hey there,
the original idea behind the feature was to simplify adding and removing of test users for testing purposes.
I understand that there are other ways to achieve the same. It was more meant as a shortcut to do it more quickly, e.g. when you need a quick test user with a short name.

I plan to include the proposed requested changes in the next weeks. (remove the delete user endpoint, ...)

However, I understand if there is no need for this feature, then the PR can probably get closed for now.

jauggy · 2024-07-09T19:48:30Z

EDIT: Actually I have now a reason to use the add user functionality: I can add my own username and password to the local dev database so I don't have to keep switching users when go from server4 to localhost.

Adds delete buttons to user list, and to the "user action" drop down menu. Added form for user creation.

Moving user creation to Teiserver.CacheUser.register_user If no mailer defined, unset the verification code Added guards Changed the Router methods to the correct type delete: for deleting the user post: for creating a new user

L-e-x-o-n · 2024-08-21T18:48:23Z

lib/teiserver_web/templates/admin/user/section_menu.html.heex

@@ -40,6 +40,17 @@
      ) %>
    <% end %>

+    <%= if allow?(@current_user, "admin") do %>


You check for "admin" here...

L-e-x-o-n · 2024-08-21T18:48:38Z

lib/teiserver_web/controllers/admin/user_controller.ex

@@ -210,6 +210,57 @@ defmodule TeiserverWeb.Admin.UserController do
    |> render("new.html")
  end

+  @spec create_form(Plug.Conn.t(), map) :: Plug.Conn.t()
+  def create_form(conn, _) do
+    if allow?(conn, "Server") do


... but "Server" here. I recommend using "Server" in both.

geekingfrog · 2024-08-21T19:00:25Z

I actually had the need for this recently. I ended up using the internal function in a repl, but I think this would be a nice addition.
The deletion feature, I think it's a bit dangerous and not really required locally.
But otherwise I'm up to get that in. Putting it behind Server permission is probably good enough.

L-e-x-o-n · 2024-08-21T19:01:53Z

I am thinking about putting it behind dev env check as well, thoughts?

geekingfrog · 2024-08-21T20:08:40Z

no strong opinions, bit more complex, but not much.

geekingfrog reviewed Jun 4, 2024

View reviewed changes

jere0500 force-pushed the addRemoveWebUI branch from e95d7a3 to 42c4b76 Compare June 8, 2024 16:43

geekingfrog reviewed Jun 26, 2024

View reviewed changes

jere0500 added 6 commits July 17, 2024 11:24

Implemented adding/ removing users from WebUI

1dfcae0

Adds delete buttons to user list, and to the "user action" drop down menu. Added form for user creation.

Experimental: Changed query code to make deletion from WebUI work

e50a4db

Updated delete queries

eba6741

Removed the Delete User Feature

0675d4a

Removed the duplicate Email overwrite

ac304d2

jere0500 force-pushed the addRemoveWebUI branch from 42c4b76 to f1cfd44 Compare July 17, 2024 09:33

Guarding with do_create_post function

bc3164a

jere0500 force-pushed the addRemoveWebUI branch from f1cfd44 to bc3164a Compare July 17, 2024 09:56

L-e-x-o-n requested a review from StanczakDominik August 6, 2024 16:28

L-e-x-o-n reviewed Aug 21, 2024

View reviewed changes



		@spec delete_user(Plug.Conn.t(), map()) :: Plug.Conn.t()
		def delete_user(conn, %{"id" => id}) do

Add the ability to add and remove users from the webui #307

Are you sure you want to change the base?

Add the ability to add and remove users from the webui #307

Conversation

jere0500 commented Jun 4, 2024

Idea

Current problems:

geekingfrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-e-x-o-n commented Jun 4, 2024

jauggy commented Jun 5, 2024

geekingfrog commented Jun 5, 2024

jere0500 commented Jun 6, 2024

jere0500 commented Jun 7, 2024

jere0500 commented Jun 8, 2024

Idea

geekingfrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jauggy commented Jul 1, 2024

jere0500 commented Jul 9, 2024

jauggy commented Jul 9, 2024 • edited Loading

L-e-x-o-n Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

L-e-x-o-n Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

geekingfrog commented Aug 21, 2024

L-e-x-o-n commented Aug 21, 2024

geekingfrog commented Aug 21, 2024

jauggy commented Jul 9, 2024 •

edited

Loading

L-e-x-o-n Aug 21, 2024 •

edited

Loading

L-e-x-o-n Aug 21, 2024 •

edited

Loading