beyond-all-reason / teiserver

Middleware server for online gaming
https://www.beyondallreason.info/
MIT License
46 stars 46 forks source link

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

Open jere0500 opened 1 month ago

jere0500 commented 1 month ago

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. AddButton

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

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

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

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.

L-e-x-o-n commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

jauggy commented 1 week ago

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 commented 1 day ago

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 commented 1 day ago

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.