Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1dfcae0
Implemented adding/ removing users from WebUI
jere0500 May 17, 2024
e50a4db
Experimental: Changed query code to make deletion from WebUI work
jere0500 May 20, 2024
eba6741
Updated delete queries
jere0500 Jun 7, 2024
19a372a
Fixes for user creation, deletion, correct keywords, add guards
jere0500 Jun 7, 2024
0675d4a
Removed the Delete User Feature
jere0500 Jul 17, 2024
ac304d2
Removed the duplicate Email overwrite
jere0500 Jul 17, 2024
bc3164a
Guarding with do_create_post function
jere0500 Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions lib/teiserver/account/tasks/delete_user_task.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ defmodule Teiserver.Admin.DeleteUserTask do
id_list
|> Enum.each(&Account.decache_user/1)

int_id_list = Enum.map(id_list, fn x -> String.to_integer(x) end)

[
# Clan memberships
"DELETE FROM teiserver_clan_memberships WHERE user_id = ANY($1)",
Expand All @@ -24,14 +26,14 @@ defmodule Teiserver.Admin.DeleteUserTask do
"DELETE FROM account_friends WHERE user1_id = ANY($1) OR user2_id = ANY($1)",

# Telemetry
"DELETE FROM telemetry_complex_client_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_client_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_complex_match_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_match_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_complex_lobby_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_lobby_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_complex_server_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_server_event_types WHERE user_id = ANY($1)",
"DELETE FROM telemetry_complex_client_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_client_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_complex_match_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_match_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_complex_lobby_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_lobby_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_complex_server_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_simple_server_events WHERE user_id = ANY($1)",
"DELETE FROM telemetry_user_properties WHERE user_id = ANY($1)",

# User table extensions/stats
Expand All @@ -47,24 +49,24 @@ defmodule Teiserver.Admin.DeleteUserTask do
# Chat
"DELETE FROM teiserver_lobby_messages WHERE user_id = ANY($1)",
"DELETE FROM teiserver_room_messages WHERE user_id = ANY($1)",
"DELETE FROM direct_messages WHERE from_user_id = ANY($1) OR to_user_id = ANY($1)",
"DELETE FROM direct_messages WHERE from_id = ANY($1) OR to_id = ANY($1)",

# Match stuff
"DELETE FROM teiserver_battle_match_memberships WHERE user_id = ANY($1)",
"DELETE FROM teiserver_account_ratings WHERE user_id = ANY($1)",
"DELETE FROM teiserver_game_rating_logs WHERE user_id = ANY($1)",

# Moderation
"DELETE FROM moderation_reports WHERE reporter_id = ANY($1) OR target_id = ANY($1) OR responder_id = ANY($1)",
"DELETE FROM moderation_reports WHERE reporter_id = ANY($1) OR target_id = ANY($1)",
"DELETE FROM moderation_actions WHERE target_id = ANY($1)"
]
|> Enum.each(fn query ->
Ecto.Adapters.SQL.query!(Repo, query, [id_list])
Ecto.Adapters.SQL.query!(Repo, query, [int_id_list])
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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


# Delete our cache of them
id_list
Expand Down
6 changes: 4 additions & 2 deletions lib/teiserver/data/cache_user.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,16 @@ defmodule Teiserver.CacheUser do

case Account.script_create_user(params) do
{:ok, user} ->
verification_code = (:rand.uniform(899_999) + 100_000) |> to_string

Account.update_user_stat(user.id, %{
"verification_code" => (:rand.uniform(899_999) + 100_000) |> to_string
"verification_code" => verification_code
})

# Now add them to the cache
user
|> convert_user
|> Map.put(:password_hash, spring_md5_password(password))
|> Map.put(:password_hash, encrypt_password(spring_md5_password(password)))
|> Map.put(:spring_password, false)
|> add_user
|> update_user(persist: true)
Expand Down
55 changes: 53 additions & 2 deletions lib/teiserver_web/controllers/admin/user_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ defmodule TeiserverWeb.Admin.UserController do
user: {Teiserver.Account.AuthLib, :current_user}
)

plug(:add_breadcrumb, name: 'Admin', url: '/teiserver/admin')
plug(:add_breadcrumb, name: 'Users', url: '/teiserver/admin/user')
plug(:add_breadcrumb, name: "Admin", url: "/teiserver/admin")
plug(:add_breadcrumb, name: "Users", url: "/teiserver/admin/user")

@spec index(Plug.Conn.t(), map) :: Plug.Conn.t()
def index(conn, params) do
Expand Down Expand Up @@ -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
Copy link
Collaborator

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

conn
|> render("create_form.html")
else
conn
|> put_flash(:info, "No permission")
|> redirect(to: ~p"/teiserver/admin/user")
end
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if is_nil(params["name"]) or String.trim(params["name"]) == "" do
conn
|> put_flash(:danger, "Invalid user name")
|> redirect(to: ~p"/teiserver/admin/user")
else
if allow?(conn, "Server") do
do_create_post(conn, params)
else
conn
|> put_flash(:danger, "No access.")
|> redirect(to: ~p"/teiserver/admin/user")
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

@spec do_create_post(Plug.Conn.t(), map) :: Plug.Conn.t()
defp do_create_post(conn, params) do
password = Map.get(params, "password", "password")
email = Map.get(params, "email", UUID.uuid1())

case Teiserver.CacheUser.register_user(params["name"], email, password) do
:success ->
conn
|> put_flash(:info, "User created successfully.")
|> redirect(to: ~p"/teiserver/admin/user")

{:failure, str} ->
conn
|> put_flash(:error, "Problem creating user: " <> str)
|> redirect(to: ~p"/teiserver/admin/user")

_ ->
conn
|> redirect(to: ~p"/teiserver/admin/user")
end
end

@spec create(Plug.Conn.t(), map) :: Plug.Conn.t()
def create(conn, %{"user" => user_params}) do
user_params =
Expand Down
2 changes: 2 additions & 0 deletions lib/teiserver_web/router.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,8 @@ defmodule TeiserverWeb.Router do
get("/tools/falist", ToolController, :falist)
get("/tools/test_page", ToolController, :test_page)

get("/users/create_form", UserController, :create_form)
post("/users/create_post", UserController, :create_post)
get("/users/rename_form/:id", UserController, :rename_form)
put("/users/rename_post/:id", UserController, :rename_post)
get("/users/reset_password/:id", UserController, :reset_password)
Expand Down
107 changes: 107 additions & 0 deletions lib/teiserver_web/templates/admin/user/create_form.html.heex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
<% bsname = view_colour() %>

<%= render(
TeiserverWeb.Admin.GeneralView,
"sub_menu.html",
Map.merge(assigns, %{active: "users"})
) %>

<div class="row section-menu">
<div class="col-md-12">
<div class={"card border-#{bsname}"}>
<div class="card-body">
<%= render(
TeiserverWeb.Admin.UserView,
"section_menu.html",
Map.merge(assigns, %{
quick_search: "",
show_search: false,
active: "add_user"
})
) %>

<hr />

<div class="row mt-4">
<div class="col-sm-12 col-md-8 offset-md-2 col-lg-6 offset-lg-3 col-xl-4 offset-xl-4">
<h4>Create with default values:</h4>
<h5>(random name; random email; password = "password")</h5>
<form method="post" action={Routes.ts_admin_user_path(@conn, :create_post)} class="">
<% random_name = "User#{:rand.uniform(899_999_999) + 100_000_000}" %>
<input type="hidden" name="_csrf_token" value={get_csrf_token()} />
<input type="hidden" name="_method" value="POST" />
<input type="hidden" name="name" value={random_name} />

<button type="submit" class={"btn btn-#{bsname}2 btn-block"}>
Create: <%= random_name %>
</button>
</form>

<br />
<hr />
<br />

<h4>Create with chosen values:</h4>
<form method="post" action={Routes.ts_admin_user_path(@conn, :create_post)} class="">
<input type="hidden" name="_csrf_token" value={get_csrf_token()} />
<input type="hidden" name="_method" value="POST" />

<div class="row mt-2">
<label for="name" class="col-sm-2 control-label">User name:</label>
<div class="col-sm-10">
<input
type="text"
name="name"
id="name"
value=""
class="form-control"
autofocus="autofocus"
/>
</div>
</div>
<div class="row mt-2">
<label for="email" class="col-sm-2 control-label">Email:</label>
<div class="col-sm-10">
<input
type="text"
name="email"
id="email"
value=""
class="form-control"
autofocus="autofocus"
/>
</div>
</div>
<div class="row mt-2">
<label for="password" class="col-sm-2 control-label">Password:</label>
<div class="col-sm-10">
<input
type="password"
name="password"
id="password"
value=""
class="form-control"
autofocus="autofocus"
/>
</div>
</div>

<div class="row mt-4">
<div class="col-md-6">
<a href={~p"/teiserver/admin/user/"} class="btn btn-secondary btn-block">
Cancel
</a>
</div>
<div class="col-md-6">
<button type="submit" class={"btn btn-#{bsname} btn-block"}>
Create
</button>
</div>
</div>
</form>
</div>
</div>
</div>
</div>
</div>
</div>
11 changes: 11 additions & 0 deletions lib/teiserver_web/templates/admin/user/section_menu.html.heex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@
) %>
<% end %>

<%= if allow?(@current_user, "admin") do %>
Copy link
Collaborator

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You check for "admin" here...

<%= central_component("section_menu_button",
name: "add_user",
label: "Add user",
active: @active,
icon: "fa-regular fa-plus",
bsname: bsname,
url: ~p"/teiserver/admin/users/create_form"
) %>
<% end %>

<%= if @active == "show" do %>
<%= central_component("section_menu_button",
name: "show",
Expand Down
Loading