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.

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

Added website stuff for Web-User administration and additional request… #1544

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

Conversation

fnkbsi
Copy link
Contributor

@fnkbsi fnkbsi commented Aug 16, 2024

Adding websites, controller and some methodes for multi Web-Users

@fnkbsi fnkbsi changed the title Added website stuff for Web-User administarion and additional request… Added website stuff for Web-User administration and additional request… Aug 16, 2024
<form><select id="myAuthoritiesList" name="authorities" path="authorities" title="List of roles/authoriies the web-user shall have.">
<option value="USER" >USER</option>
<option value="ADMIN" >ADMIN</option>
<option value="USER,ADMIN" >USER, ADMIN</option>
Copy link
Member

Choose a reason for hiding this comment

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

you are deciding and hardcoding business values in view layer. maybe this should be an java enum?

Copy link
Contributor

Choose a reason for hiding this comment

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

Admin is supposed to have more rights than user. What the purpose to have many roles for one user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Admin is supposed to have more rights than user. What the purpose to have many roles for one user?

In general yes, but in some cases the user has privileges a admin does not. To keep this freedom for the future, I added the option that one web-user can have both roles.

Copy link
Member

Choose a reason for hiding this comment

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

in some cases the user has privileges a admin does not.

can you give examples?

i think the common practice is that an admin is a superset of user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hypothetical: the USER authority could be to manage the users (read, write of ocpp_tag, user) and the ADMIN authority manages only the charge-boxes. In this scenario there are some web-users which are allowed to do both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on what I understand your goal, I would like to propose implementing an additional user profile: CPO_USER (chargebox), EMP_USER (tag/user), and ADMIN (with full rights and privileges).

Copy link
Member

@goekay goekay Aug 18, 2024

Choose a reason for hiding this comment

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

see #1544 (comment)

because of this, i think we should not try to find a golden solution. only 2 authorities (USER and ADMIN) will not be enough. the permission (what) distribution between authorities (who) will be different for each org.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the division of read and write permissions between user and admin roles is not widely used, a straightforward approach would be to remove role-based access control in the first implementation and grant all users access to all privileges.

WDYT?

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 added a java enum, if some one don't want a combination of USER&ADMIN just out-comment it in the enum. But it work's if needed.

@goekay goekay self-requested a review August 17, 2024 02:17
<form><select id="myAuthoritiesList" name="authorities" path="authorities" title="List of roles/authoriies the web-user shall have.">
<option value="USER" >USER</option>
<option value="ADMIN" >ADMIN</option>
<option value="USER,ADMIN" >USER, ADMIN</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Admin is supposed to have more rights than user. What the purpose to have many roles for one user?

.requestMatchers(prefix + "/ocppTags").hasAnyAuthority("USER", "ADMIN")
.requestMatchers(prefix + "/ocppTags" + "/details/**").hasAnyAuthority("USER", "ADMIN")
// chargepoints
.requestMatchers(prefix + "/chargepoints").hasAnyAuthority("USER", "ADMIN")
Copy link
Contributor

Choose a reason for hiding this comment

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

As user can't create, update ou delete charpoints (and some others), the UI should be updated (at least remove delete and update button)

Copy link
Member

Choose a reason for hiding this comment

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

As user can't create, update ou delete charpoints (and some others)

i will play devil's advocate (as setup for my next argument): why not?

your argument has a very specific understanding/model of what a user and admin should do or do not. i find it really hard to make decisions in steve about permissions and roles matrix because each customer/CPO/company would have different requirements depending on their organisation structure.

i have no solution for this. i dont think we can make some decisions (in this generic version of steve) that will make people happy.

Copy link
Member

Choose a reason for hiding this comment

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

the perfectionist in me says.. we should make the permissions and roles configurable in web UI or something, but this would be a big undertaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, why not?

Let me share my own experience where people (administrative persons but technical operators too) unintentionally break things because they can.

But what if we could give them a way to only "read" data and help drivers?

At the same time, "users" could be drivers in different contexts.

In this vision, users become helpful companions for drivers/them, providing information and simple commands that won't cause any damage. Meanwhile, admins will be in charge of managing the network of stations.

Now, the question arises: how would we differentiate between admins and users? Can you imagine any noticeable differences in the way they are currently implemented?

To the perfectionist: I think it is recommended that implementation of peripheral functionalities, such as the one in question or even IAM, be delegated to specialized software stacks. This ensures that the primary objective of the software solution remains the central focus, while benefiting from the seamless integration of a well-established stack that effectively addresses the requirement.
However, it is important to note that implementing this way will require additional effort and may introduce complexity to the existing system. This could potentially impact the ops part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the button are still there, but USER landing on the NoAccess site. If someone with more *jsp experience can hide them, I would welcome this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do some ui improvments but @geokay must define first what is expected for USER and ADMIN.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option can be ADMIN, SUPERVISOR, USER.
With
USER = readonly, except users,
SUPERVISOR = read+write, except users,
ADMIN = read+write all

Wdyt @goekay ? Please advice.

Copy link
Member

Choose a reason for hiding this comment

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

i wrote this in an earlier comment, and i still think that way: probably, we should not make any decisions about this.

we can create the basic framework (i.e. the preparation for decisions) and ability to create roles. what these roles can do and cannot do should be left to each user of steve.

there are so many aspects and functionalities, i.e. master data management (read and write) and various ocpp operations. i find it hard to come up with one matrix that will be the solution.

otherwise, we will get questions/issues like: why cant my USER role do X?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current state of the code creating roles is possible by adapting the enum.
Changing the privileges can be done by adapting the SecurityFilterChain, which of course could be tricky (#1544 (comment)).
Are comments in the source code sufficient?

So for the basic framework solution configuration I suggest:
User --> allowed to see all website/data
Admin --> allowed to see all websites and is allowed to change the data
[User, Admin] is commented out in the enum. I would like to keep it as a hint that multiple roles are technically possible

@goekay
Copy link
Member

goekay commented Aug 17, 2024

hey @fnkbsi i took the liberty of doing some small code improvements in d00c287 and hope you are okay with it 🙏

@fnkbsi
Copy link
Contributor Author

fnkbsi commented Aug 28, 2024

@goekay @juherr
I try to login the site as 'USER' and get an Error 500 >>java.lang.StackOverflowError<<
Login as 'ADMIN' works fine.
Q1: Can you confirm this behavior?
Q2: Any hints whats causing the error and how we can fix it?

Found the Problem:
The >>.requestMatchers(prefix + "/**").hasAuthority("ADMIN")<< entry must be the last one in the list of the SecurityFilterChain!

@AssertFalse(message = "The repeated password did not match!")
private Boolean pwError;

private String apiPassword = "";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goekay: The API password is missing validation rules. Would the rule "@SiZe(min=8) be enough?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants