-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
Implement database-based multi user system for Web UI #1539
Conversation
9736c15
to
d50ca8f
Compare
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.
LGTM, it will be an awesome feature.
I've just shared some ideas.
src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.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/repository/impl/WebUserRepositoryImpl.java
Outdated
Show resolved
Hide resolved
@fnkbsi how do you want to continue from here. we have your solution/branch and my solution/branch. would you be okay with me merging this first, and you could rebase your branch to add the missing and web-related stuff on top? |
One more idea to discuss. Do we want a optional link (foreign_key) between user and web_user? |
@fnkbsi I don't see value add to link user and webuser, do you? |
Maybe in some businesses cases. If a user gets also an web_user, the user can be managed on one website. If the user will be deleted, the web_user could be automatically deleted too. I just want to address this idea before the db is changed. |
In my understanding, Steve serves as a CPO layer that manages the stations and where the web users act as the operators. Currently, Steve is not a multi-tenant software, meaning all users have the same permissions across all stations. However, if we were to link specific users or tags to web users, it would open the door to the EMSP world, making the system more intricate. |
@fnkbsi i think this is an interesting proposition. maybe we can do it in future, if we want to expand the capabilities of steve. i added the token column as a preparation in 93c9cc0, because i think web users and api clients are within the same realm of "technical/operator user". one is human, the other is computer. connecting a "web user" with a "user" would connect the technical world with the ocpp world (business). careful planning would be needed, if we want to. therefore, i would not want to rush it. @juherr i know what this means. it would be a big undertaking. i am not categorically against it. it is just a matter of resources (i.e. time, active developers etc.). |
IMO, if you go that way, you should start by linking web users to stations 🤷🏼♂️ |
93c9cc0
to
a1c201f
Compare
FYI to the readers of this PR from the future: if you are using a version of steve with this PR included, your i plan to remove these two fields from properties file in a later version. |
reason: our build matrix fails for mysql, but succeeds for mariadb. Jooq infers data type org.jooq.JSON for web_user.authorities for mysql. on the other hand, it is String for mariadb. example: https://github.com/steve-community/steve/actions/runs/10339451112
* add check for validating that "authorities" is an array * store a sorted set of authorities without duplicates
reason: to be used by web pages. a better way than doing with username, and is consistent with other delete operations we do.
a1c201f
to
163de00
Compare
src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
Outdated
Show resolved
Hide resolved
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.
LGTM 👍
…y#1539) * add UserDetailsService impl using Jooq * improve impl such that it is in a working condition * refactor: make github action checks happy * force data type JSON in Jooq for web_user.authorities reason: our build matrix fails for mysql, but succeeds for mariadb. Jooq infers data type org.jooq.JSON for web_user.authorities for mysql. on the other hand, it is String for mariadb. example: https://github.com/steve-community/steve/actions/runs/10339451112 * tighten json logic * add check for validating that "authorities" is an array * store a sorted set of authorities without duplicates * add method to delete web user by database id reason: to be used by web pages. a better way than doing with username, and is consistent with other delete operations we do. * PR feedback: skip default admin user creation, if "any" admin already exists * refactor: PR feedback * prepare database for steve-community#1540 * PR feedback * add license header where missing
my take on #991 to be compared with #1165
FYI @fnkbsi @juherr