-
-
Notifications
You must be signed in to change notification settings - Fork 395
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
base: master
Are you sure you want to change the base?
Conversation
src/main/java/de/rwth/idsg/steve/repository/dto/WebUserOverview.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java
Outdated
Show resolved
Hide resolved
<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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/main/resources/webapp/WEB-INF/views/data-man/webuserDetails.jsp
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/repository/dto/WebUserOverview.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
Outdated
Show resolved
Hide resolved
<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> |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
…eryForm form) to WebUserService
@goekay @juherr Found the Problem: |
…")" at the end of the SecurityFilterChain. Otherwise a USER get's an Error 500 on login
…rPassword.jsp removed error-message on pwError
…sion on reslove conflict)
@AssertFalse(message = "The repeated password did not match!") | ||
private Boolean pwError; | ||
|
||
private String apiPassword = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding websites, controller and some methodes for multi Web-Users