adessoSE / budgeteer

Simple Project Budget Monitoring via Web Browser
MIT License
31 stars 18 forks source link

Missing option to administer users #141

Open tinne opened 7 years ago

tinne commented 7 years ago

There should be a user administration menu which would allow to remove users entirely from an installation. The user administration menu could for reasons of simplicity be implemented as a special project.

maximAtanasov commented 6 years ago

@tinne Would you like this to be implemented as an additional (small) web app that is distributed along with the main war and simply connects to the database and allows the administrator to delete any user?

tinne commented 6 years ago

A minimum complete solution could come as a separately deployed or separately accessible web app. Else, administrator could be a special role to every user (in a in-memory setting, the first user created only would automatically become administrator) and user administration a separate page in the normal application.

Ideally, an administrator could enable and disable administratorship on other users, delete users and set (overwrite) user passwords.

maximAtanasov commented 5 years ago

@tinne I have implemented the required functionality in a separate web app. It is possible for you to try it out and tell me if it anything should be adjusted or changed? It can be compiled from this PR #298 or I can also send a .war file of the administration menu if that's more convenient :).

tinne commented 5 years ago

I tried the PR: it runs into an error due to an incompatible database schema - kindly provide a flyway script. See the attached schema-fix-issue in budgeteer-admin.log.

maximAtanasov commented 5 years ago

I have added a migration script that will update the database (as it is in branch release commit 73c7cc5370e7682de563d0fbac3b4bcd204438e1 on an Oracle db) . Everything should be working now.

tinne commented 5 years ago

see my comment in https://github.com/adessoAG/budgeteer/pull/298

tinne commented 5 years ago

Creating a new test database, I now got a running user admin installation, thanks. I try the productive database administration later.

The role options is somewhat clumsy to use and not easy to understand. Especially the drop-down menu with the checkboxes is hard to use and it takes time to understand that you cannot untick both boxes. I recommend to use two separate checkboxes without any pull-down around.

Who is allowed to log into the admin menu? Is this just the first user ever to register? This should be explained somewhere.

As concerning our other discussion, one could as well make the user name edible in the user list. But what is the use of having the role “admin only” for a user and project? It seems to behave as if the user and both roles anyway.

The default action in “Are you sure you want to delete this project?” should not be “Yes.” Nor should it be in any other these security dialogues. Also, add a “irrevocably.“

The security question for a user is ill-formulated. Instead of “Are you sure you want to remove this user from the project?” it should be “Are you sure to irrevocably remove this user from Budgeteer?”

If I fail to select a user (or there is none), the error message for a click on Add User to this Project / Add User should rather be “You need to select a user to perform this action.”

No verification mail is ever sent, nor do I find any error regarding this in the logs (using standard log settings).

What is the anticipated functionality considered to be removed from the Project Administration? Is there any?

maximAtanasov commented 5 years ago

The role options is somewhat clumsy to use and not easy to understand. Especially the drop-down menu with the checkboxes is hard to use and it takes time to understand that you cannot untick both boxes. I recommend to use two separate checkboxes without any pull-down around.

I see, I will change the drop-down to simply checkboxes.

Who is allowed to log into the admin menu? Is this just the first user ever to register? This should be explained somewhere.

The first user to log into the admin menu becomes the only one allowed to access it (untill other users are also made admins).

As concerning our other discussion, one could as well make the user name edible in the user list. But what is the use of having the role “admin only” for a user and project? It seems to behave as if the user and both roles anyway.

Yes, the project roles work a bit differently. A user can be both admin and user or only an admin, there is no difference. I have made this possible because of the way roles work in keycloak. (users can have many different roles). I also thought this design would be good if additional roles are to be added in the future (for example something between user and admin for example). But I agree that this is a bit confusing. Also, I will make the usernames editable.

The default action in “Are you sure you want to delete this project?” should not be “Yes.” Nor should it be in any other these security dialogues. Also, add a “irrevocably.“

The security question for a user is ill-formulated. Instead of “Are you sure you want to remove this user from the project?” it should be “Are you sure to irrevocably remove this user from Budgeteer?”

If I fail to select a user (or there is none), the error message for a click on Add User to this Project / Add User should rather be “You need to select a user to perform this action.”

I will adjust everything accordingly,

No verification mail is ever sent, nor do I find any error regarding this in the logs (using standard log settings).

In order for the email functionality to work, a mail server must be set up. How to do this is explained here: https://github.com/adessoAG/budgeteer/wiki/Set-up-local-mail-server-for-development-(Windows) and in a README in the project repo. However, these instructions are Windows-only and I'm guessing the Budgeteer runs on a Linux based server. I'm not sure how to handle this. (but I don't think we would want to publish any adesso specific data on github).

What is the anticipated functionality considered to be removed from the Project Administration? Is there any?

After an admin is made a user, they can no longer access the administration menu. After a project admin is made a user, they can no longer acess the Project Administration Menu (the button in the Dashboard disappears). This was somewhat bugged before (likely when you tested it), but I have fixed it since. I really did not want to give you a buggy build to test, but unfortunately some bugs only come up some of the time or things sometimes break after making changes, without me noticing (and Unit tests don't always catch these errros).

maximAtanasov commented 5 years ago

The default action in “Are you sure you want to delete this project?” should not be “Yes.” Nor should it be in any other these security dialogues. Also, add a “irrevocably.“

The security question for a user is ill-formulated. Instead of “Are you sure you want to remove this user from the project?” it should be “Are you sure to irrevocably remove this user from Budgeteer?”

If I fail to select a user (or there is none), the error message for a click on Add User to this Project / Add User should rather be “You need to select a user to perform this action.”

The role options is somewhat clumsy to use and not easy to understand. Especially the drop-down menu with the checkboxes is hard to use and it takes time to understand that you cannot untick both boxes. I recommend to use two separate checkboxes without any pull-down around.

As concerning our other discussion, one could as well make the user name edible in the user list.

These issues have been fixed. If you want we will also replace the dropdown in the Project Administration with checkboxes.

tinne commented 5 years ago

Yes, the project roles work a bit differently. A user can be both admin and user or only an admin, there is no difference. I have made this possible because of the way roles work in keycloak. (users can have many different roles). I also thought this design would be good if additional roles are to be added in the future (for example something between user and admin for example). But I agree that this is a bit confusing. Also, I will make the usernames editable.

Having specific roles like this is very much appreciated. I only would like to make a bit more explicit:

Introduce separate roles for system admins (system wide) and project admins (per project). Make this sort of stuff edible by the admin menu. I suggest to add an action link [turn into system admin] to all users in “All users in this Budgeteer install” and a link [revoke system admin rights] to all other system admins in the list.

If project admin implies project user, ok. Just add a handler that enables the user role once the admin role is activated, and deletes the admin role once the user role is deleted. Thus people will have visual feedback on this dependency. Else, also use action links for all actions [grant admin rights], [revoke admin rights], [remove from project].

With regards to the mail setup, I suggest to first explain the general setup options and - potentially as an addendum - explain how to install a test server, as enough people got a productive mail server around they could use, and even more so in production environments. Additionally, this information should be transformed to an example application.properties and duplicated to the new client.

maximAtanasov commented 5 years ago

Introduce separate roles for system admins (system wide) and project admins (per project). Make this sort of stuff edible by the admin menu.

That is how it is currently. Project roles and System admin roles are separate. A project admin only has access to the administration for a project. System wide admins have access to the administration app as well as access to all projects. So a system admin can set permissions on users in a project and set other users as system wide admins as well.

I suggest to add an action link [turn into system admin] to all users in “All users in this Budgeteer install” and a link [revoke system admin rights] to all other system admins in the list.

I will add this.

If project admin implies project user, ok. Just add a handler that enables the user role once the admin role is activated, and deletes the admin role once the user role is deleted. Thus people will have visual feedback on this dependency. Else, also use action links for all actions [grant admin rights], [revoke admin rights], [remove from project].

Yes, I didn't think about just having both checkboxes be checked. I will do it like that.

With regards to the mail setup, I suggest to first explain the general setup options and - potentially as an addendum - explain how to install a test server, as enough people got a productive mail server around they could use, and even more so in production environments. Additionally, this information should be transformed to an example application.properties and duplicated to the new client.

I will try to write instructions for setting up a mail server in a linux environment. But I will add this in another PR. Could you please clarify about this part:

enough people got a productive mail server around they could use,

This means setting up an additional mail server isn't nessecary?

tinne commented 5 years ago

This means setting up an additional mail server isn't nessecary?

You need an SMTP server / mail transport agent, which is integrated to the global e-mail infrastructure and would accept mail via the SMTP protocol from your Budgeteer server (via the Java Mail API). An additional mail server can be used if your productive mail infrastructure would not exist / allow you to dock in. E-Mail is traditionally split into several parts and the MTA should not be mixed up with a POP3/IMAP/proprietary mail storage/access server or mail user agents (vulgo e-mail clients).

maximAtanasov commented 5 years ago

I suggest to add an action link [turn into system admin] to all users in “All users in this Budgeteer install” and a link [revoke system admin rights] to all other system admins in the list.

I have implemented this. To me it looks and feels the best so far.

If project admin implies project user, ok. Just add a handler that enables the user role once the admin role is activated, and deletes the admin role once the user role is deleted. Thus people will have visual feedback on this dependency

This has been implemented as well.

As for the emails, it seems things aren't simple as I had thought. I will look into it.