Conversation
…eryForm form) to WebUserService
…")" 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)
* fnkbsi/MultiUser_4:
add website to set and change WebUser API password
Improvements on webUser password change. Only own password can be changed
WebUserForm.java: pwError changed from AssertTrue to AssertFalse
resolve style checks
GenericRepositoryImpl: re-add 'import org.jooq.impl.DSL;' (wrong decision on reslove conflict)
WebUserController removed password comparison; webuserAdd.jsp, webuserPassword.jsp removed error-message on pwError
BugFix: Set the ".requestMatchers(prefix + "/**").hasAuthority("ADMIN")" at the end of the SecurityFilterChain. Otherwise a USER get's an Error 500 on login
Adding Enum WebUserAuthority and adapt the code using it
WebUserOverview: changed String[] authorities to String
refactor
WebUserServive don't expose pw and apitoken to ui
WebUserController: using camel case
moved the mapping part of List<WebUserOverview> getOverview(WebUserQueryForm form) to WebUserService
webuser*.jsp: adapt to the using of camel case in the dto's
WebUserForm use camel case at webUsername
WebUserOverview moved to service/dto, use camel case and correct typo
WebUserForm, WebUserQueryForm use camel case
delete Statistics.java.bak
Added website stuff for Web-User administarion and additional requestMatchers
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds Web Users management: repository methods, service APIs, DTOs/forms, controllers, and JSP views. Enhances security with fine-grained access rules, user-scoped password endpoints, and a custom no-access page. Statistics now include web user count, surfaced on home and navigation. Introduces overview querying and PK-based updates/password changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant WC as WebUsersController
participant WS as WebUserService
participant WR as WebUserRepositoryImpl
participant DB as Database
User->>WC: GET /manager/webusers/query?filters
WC->>WS: getOverview(form)
WS->>WR: getOverview(form)
WR->>DB: SELECT pk, name, enabled, authorities WHERE filters
DB-->>WR: Result set
WR-->>WS: Result<Record4<...>>
WS-->>WC: List<WebUserOverview>
WC-->>User: Render webusers.jsp
sequenceDiagram
autonumber
actor U as Authenticated User
participant S as SecurityFilterChain
participant C as WebUsersController
participant NA as NoAccessController
U->>S: POST /manager/webusers/apipassword/{name}
S->>S: Check role/method and name == principal
alt Authorized
S->>C: Dispatch to apiPasswordChange
C-->>U: Redirect/Render result
else Access Denied
S-->>U: 403 -> /manager/noAccess
U->>NA: GET /manager/noAccess
NA-->>U: Render noAccess.jsp
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (35)
src/main/resources/webapp/WEB-INF/views/00-header.jsp (1)
60-60: Menu label nit: prefer “WEB USERS” over “WEBUSERS” for readabilitySmall UX/readability improvement to align with other multi-word labels (e.g., “CHARGING PROFILES”).
Apply this diff:
- <li><a href="${ctxPath}/manager/webusers">WEBUSERS</a></li> + <li><a href="${ctxPath}/manager/webusers">WEB USERS</a></li>Optional: If we already have role-based visibility in the navbar for other items, consider gating this link similarly to reduce “No Access” roundtrips for unauthorized users.
src/main/java/de/rwth/idsg/steve/web/dto/WebUserQueryForm.java (1)
1-7: Duplicate/stacked license header — remove the extra opening blockThere are two consecutive “/* …” starts; keep the newer 2013–2024 block and drop the older one to avoid confusion.
Apply this diff:
-/* - * SteVe - SteckdosenVerwaltung - https://github.com/steve-community/steve - * Copyright (C) 2013-2023 SteVe Community Team -/* +/*src/main/java/de/rwth/idsg/steve/web/dto/WebUserAuthority.java (1)
44-51: Make fromValue tolerant to spacing/case and common synonymsUser input or UI values might vary (e.g., “user, admin”, “USER_ADMIN”). Normalize before matching to reduce fragility.
Apply this diff:
public static WebUserAuthority fromValue(String v) { - for (WebUserAuthority c: WebUserAuthority.values()) { - if (c.value.equals(v)) { - return c; - } - } - throw new IllegalArgumentException(v); + String norm = v == null ? "" : v.replaceAll("[\\s_]", "").toUpperCase(java.util.Locale.ROOT); + switch (norm) { + case "USER": + return USER; + case "ADMIN": + return ADMIN; + case "USER,ADMIN": + case "ADMIN,USER": + case "USERADMIN": + return USER_ADMIN; + default: + throw new IllegalArgumentException(v); + } }src/main/java/de/rwth/idsg/steve/web/dto/WebUserBaseForm.java (1)
40-40: Naming nit: consider singular “authority” for a single enum-valued fieldThe field holds a single WebUserAuthority (which itself encodes combinations). Naming it “authority” can be clearer and less suggestive of a collection.
src/main/resources/webapp/WEB-INF/views/home.jsp (1)
36-39: Tile label nit: “Webusers” → “Web Users” for consistencyOther tiles use spaced, capitalized nouns (“Charge Points”, “OCPP Tags”, “Users”).
Apply this diff:
- <a class="tileRow1" href="${ctxPath}/manager/webusers"> - Number of<br>Webusers + <a class="tileRow1" href="${ctxPath}/manager/webusers"> + Number of<br>Web Userssrc/main/java/de/rwth/idsg/steve/web/controller/NoAccessController.java (1)
40-42: Restrict the handler to GET explicitlyTo avoid accidental handling of non-GET methods and for clarity, make the method mapping explicit.
Apply this diff within the method declaration:
- @RequestMapping() + @RequestMapping(method = RequestMethod.GET) public String accessDenied() { return "noAccess"; }Additionally, add this import at the top of the file (outside the selected lines):
import org.springframework.web.bind.annotation.RequestMethod;src/main/resources/webapp/WEB-INF/views/noAccess.jsp (2)
29-33: Fix typo and improve alt text
- "accessable" → "accessible"
- Improve image alt text for accessibility.
- Access denied for the requested page! For further information ask your administrator. - <a class="tooltip" href="#"><img src="${ctxPath}/static/images/info.png" style="vertical-align:middle" alt="info_image"> - <span>Some information and configuration pages are only accessable for administrators.</span> + Access denied for the requested page! For further information ask your administrator. + <a class="tooltip" href="#"><img src="${ctxPath}/static/images/info.png" style="vertical-align:middle" alt="Info"> + <span>Some information and configuration pages are only accessible for administrators.</span> </a>
36-38: Prefer a link over a form for simple navigationUsing an anchor avoids unnecessary form semantics and is simpler.
- <form action="${ctxPath}/manager/home"> - <input type="submit" value="Home"> - </form> + <a href="${ctxPath}/manager/home">Home</a>src/main/resources/webapp/WEB-INF/views/data-man/webuserPassword.jsp (2)
36-36: Close the table row in the theadInvalid HTML can lead to inconsistent rendering across browsers.
- <thead><tr><th>Webuser</th><th></th></thead> + <thead><tr><th>Webuser</th><th></th></tr></thead>
42-43: Hint browsers this is a new password fieldHelps browser behavior and password managers.
- <tr><td>Password:</td><td><form:password path="password" title="Set the password"/></td></tr> - <tr><td>Password confirmation:</td><td><form:password path="passwordComparison" title="Confirm the password"/></td></tr> + <tr><td>Password:</td><td><form:password path="password" title="Set the password" autocomplete="new-password"/></td></tr> + <tr><td>Password confirmation:</td><td><form:password path="passwordComparison" title="Confirm the password" autocomplete="new-password"/></td></tr>src/main/resources/webapp/WEB-INF/views/data-man/webuserApiPassword.jsp (4)
24-24: Minor wording: capitalize APIConsistency with UI wording.
- Error while trying to change api password of webuser: + Error while trying to change API password of webuser:
33-33: Clarify the section headerMake it clear this page changes the API password.
-<section><span>Webuser change password</span></section> +<section><span>Webuser change API password</span></section>
36-36: Close the table row in the theadFix invalid HTML for consistent rendering.
- <thead><tr><th>Webuser</th><th></th></thead> + <thead><tr><th>Webuser</th><th></th></tr></thead>
41-44: Confirm hidden password fields won’t inadvertently overwrite the real passwordSetting password/passwordComparison to "doNotChange" could be risky if the backend ever processes them. Prefer omitting these fields entirely for this endpoint and letting the controller/service ignore regular password updates when changing the API password.
If safe to change, remove these two hidden fields:
- <form:hidden path="password" value="doNotChange"/> - <form:hidden path="passwordComparison" value="doNotChange"/>Also, hint the browser for new password input:
- <tr><td>API Password:</td><td><form:password path="apiPassword" title="Set the API password"/></td></tr> + <tr><td>API Password:</td><td><form:password path="apiPassword" title="Set the API password" autocomplete="new-password"/></td></tr>src/main/resources/webapp/WEB-INF/views/data-man/webuserAdd.jsp (2)
22-26: Remove unused date picker includeThis page has no date fields. Loading
datePicker-past.jsadds unnecessary JS.Apply this diff:
-<script type="text/javascript"> - $(document).ready(function() { - <%@ include file="../snippets/datePicker-past.js" %> - }); -</script>
59-60: Remove unused variables
submitButtonNameandsubmitButtonValueare set but never used.Apply this diff:
- <c:set var="submitButtonName" value="add" /> - <c:set var="submitButtonValue" value="Add" />src/main/resources/webapp/WEB-INF/views/data-man/webuserDetails.jsp (2)
24-24: Correct the error heading textCopy-paste artifact: “charge point” should be “web user”.
Apply this diff:
- Error while trying to update a charge point: + Error while trying to update a web user:
64-68: Use<form:select>for enabled flag and avoid duplicate optionThis binds directly to the boolean property and avoids duplicating the current value as a separate option.
Apply this diff:
- <select id="myEnabledList" name="enabled" path="enabled" title="Enabled/Disable the user"> - <option value="${webuserForm.enabled}" >${webuserForm.enabled}</option> - <option value=false >false</option> - <option value=true >true</option> - </select> + <form:select id="myEnabledList" path="enabled" title="Enable/Disable the user"> + <form:option value="true" label="true"/> + <form:option value="false" label="false"/> + </form:select>src/main/resources/webapp/WEB-INF/views/data-man/webusers.jsp (1)
57-71: Optional: Show a friendly message when no users are foundImproves UX when the table is empty.
You can add after the
<tbody>:<c:if test="${empty webuserList}"> <tr><td colspan="4"><i>No web users found</i></td></tr> </c:if>src/main/java/de/rwth/idsg/steve/service/dto/WebUserOverview.java (2)
20-23: Avoid service-layer DTO depending on web-layer types
WebUserOverviewdepends onde.rwth.idsg.steve.web.dto.WebUserAuthority, which couples service and web layers. Prefer movingWebUserAuthorityto a shared package (e.g.,de.rwth.idsg.steve.commonorservice.dto) or define a service-level enum and map in the controller.
31-31: Name mismatch: plural field for singular type
authoritiesis singularWebUserAuthority. Consider renaming toauthorityfor clarity; adjust builder/getter accordingly.src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java (2)
55-55: Repository interface leaking jOOQ types and JSON
Result<Record4<... JSON>>ties the interface to jOOQ and a JSON serialization detail. Consider returning a repository-level DTO or a generated record type instead, and transform toWebUserOverviewin the service.
33-33: Clarify method responsibilities with Javadoc
updateUservsupdateUserByPkare ambiguous. Add short Javadoc to specify selection criteria (username vs PK) to avoid misuse.src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java (2)
66-66: Consider adding null-safe handling for the query parameter.The RequestMatcher checks if
backToOverviewparameter exists but doesn't verify its value. Consider making it more explicit by checking for specific values if needed.- RequestMatcher toOverview = (request) -> request.getParameter("backToOverview") != null; + RequestMatcher toOverview = (request) -> { + String param = request.getParameter("backToOverview"); + return param != null && !param.isEmpty(); + };
101-101: Potential typo in authorization pattern.Line 101 uses
/reservations/**which would match/reservations/and any subpath. This seems inconsistent with the pattern on line 100 which uses/reservations. Consider if this should be/reservations/**(note the missing space).- .requestMatchers(prefix + "/reservations" + "/**").hasAnyAuthority("ADMIN") + .requestMatchers(prefix + "/reservations/**").hasAuthority("ADMIN")src/main/java/de/rwth/idsg/steve/web/dto/WebUserForm.java (2)
3-3: Update copyright year to 2025.The copyright year should be 2025 to match the rest of the codebase.
- * Copyright (C) 2013-2024 SteVe Community Team + * Copyright (C) 2013-2025 SteVe Community Team
51-57: Password comparison logic could be simplified.The password comparison setter initializes
pwErrorto true then conditionally updates it. Consider a more straightforward approach.public void setPasswordComparison(String passwordComparison) { - this.pwError = true; this.passwordComparison = passwordComparison; - if (passwordComparison != null) { - this.pwError = !passwordComparison.equals(this.password); - } + this.pwError = passwordComparison == null || !passwordComparison.equals(this.password); }src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java (1)
183-183: Fix typo in comment.The comment mentions "Semicolon seperated" but should be "Comma separated" since the code splits on comma.
- String[] split = form.getRoles().split(","); //Semicolon seperated String to StringArray + String[] split = form.getRoles().split(","); // Comma separated String to StringArraysrc/main/java/de/rwth/idsg/steve/service/WebUserService.java (2)
220-226: Consider consistent null handling for API password.The method checks if
form.getApiPassword()is not null before encoding, but the form field has a default value of empty string. Consider whether empty strings should be treated as null.public void updateApiPassword(WebUserForm form) { String newPassword = null; - if (form.getApiPassword() != null) { + if (form.getApiPassword() != null && !form.getApiPassword().isEmpty()) { newPassword = encoder.encode(form.getApiPassword()); } webUserRepository.changeApiPassword(form.getWebUserPk(), newPassword); }
311-314: Remove commented-out code.Line 312 contains commented-out code that should be removed for cleanliness.
String encPw = ""; if (form.getPassword() != null) { - //encPw = form.getPassword(); encPw = encoder.encode(form.getPassword()); }src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java (5)
43-45: Prefer constructor injection over field injection.Constructor injection improves immutability, testability, and avoids reflection pitfalls.
Apply this diff:
- @Autowired private WebUserService webUserService; + private final WebUserService webUserService; + + @Autowired + public WebUsersController(WebUserService webUserService) { + this.webUserService = webUserService; + }
121-127: DRY: extract mapping fromWebUserBaseForm→WebUserForm.Both password GET handlers duplicate the same mapping logic.
Replace the duplicated blocks with:
- WebUserForm webUserForm = new WebUserForm(); - WebUserBaseForm webUserBaseForm = webUserService.getDetails(webUserName); - webUserForm.setWebUserPk(webUserBaseForm.getWebUserPk()); - webUserForm.setWebUsername(webUserBaseForm.getWebUsername()); - webUserForm.setAuthorities(webUserBaseForm.getAuthorities()); - webUserForm.setEnabled(webUserBaseForm.getEnabled()); + WebUserBaseForm base = webUserService.getDetails(webUserName); + WebUserForm webUserForm = fromBase(base);Add this helper method to the controller:
private WebUserForm fromBase(WebUserBaseForm base) { WebUserForm form = new WebUserForm(); form.setWebUserPk(base.getWebUserPk()); form.setWebUsername(base.getWebUsername()); form.setAuthorities(base.getAuthorities()); form.setEnabled(base.getEnabled()); return form; }Also applies to: 145-151
81-87: Consider graceful handling for missing users.If
webUserService.getDetails(webUserPk)throws or returns null for non-existent users, render a 404 or redirect with a flash message instead of throwing an error to the user.I can propose a small handler using
ResponseStatusExceptionor a redirect withRedirectAttributesif desired.
177-187: Method names “BackToOverview” return to details for password routes.Minor naming mismatch: both handlers named “BackToOverview” actually redirect to details. Consider renaming for clarity, or update the comment block.
64-171: Optionally adopt@GetMapping/@PostMappingfor brevity.Using composed annotations reduces noise and improves readability; no behavior change.
Example for one method:
- @RequestMapping(method = RequestMethod.GET) + @GetMapping public String getOverview(Model model) { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java(4 hunks)src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java(2 hunks)src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java(5 hunks)src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java(5 hunks)src/main/java/de/rwth/idsg/steve/service/WebUserService.java(6 hunks)src/main/java/de/rwth/idsg/steve/service/dto/WebUserOverview.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/controller/NoAccessController.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/dto/Statistics.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/dto/WebUserAuthority.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/dto/WebUserBaseForm.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/dto/WebUserForm.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/dto/WebUserQueryForm.java(1 hunks)src/main/resources/webapp/WEB-INF/views/00-header.jsp(1 hunks)src/main/resources/webapp/WEB-INF/views/data-man/webuserAdd.jsp(1 hunks)src/main/resources/webapp/WEB-INF/views/data-man/webuserApiPassword.jsp(1 hunks)src/main/resources/webapp/WEB-INF/views/data-man/webuserDetails.jsp(1 hunks)src/main/resources/webapp/WEB-INF/views/data-man/webuserPassword.jsp(1 hunks)src/main/resources/webapp/WEB-INF/views/data-man/webusers.jsp(1 hunks)src/main/resources/webapp/WEB-INF/views/home.jsp(1 hunks)src/main/resources/webapp/WEB-INF/views/noAccess.jsp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/main/java/de/rwth/idsg/steve/web/controller/NoAccessController.java (1)
src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java (1)
Controller(39-207)
src/main/java/de/rwth/idsg/steve/web/dto/WebUserBaseForm.java (1)
src/main/java/de/rwth/idsg/steve/web/dto/WebUserForm.java (1)
Getter(31-62)
src/main/java/de/rwth/idsg/steve/service/WebUserService.java (1)
src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-76)
src/main/java/de/rwth/idsg/steve/web/dto/WebUserForm.java (1)
src/main/java/de/rwth/idsg/steve/web/dto/WebUserBaseForm.java (1)
Getter(29-42)
src/main/java/de/rwth/idsg/steve/web/dto/WebUserAuthority.java (1)
src/main/java/de/rwth/idsg/steve/service/dto/WebUserOverview.java (1)
Getter(24-32)
src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java (2)
src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java (1)
Slf4j(44-136)src/main/java/de/rwth/idsg/steve/config/ApiAuthenticationManager.java (1)
Slf4j(50-115)
🪛 GitHub Check: checkstyle
src/main/java/de/rwth/idsg/steve/service/WebUserService.java
[failure] 219-219: [checkstyle] src/main/java/de/rwth/idsg/steve/service/WebUserService.java#L219 <com.puppycrawl.tools.checkstyle.checks.regexp.RegexpSinglelineCheck>
Line has trailing spaces.
[failure] 255-255: [checkstyle] src/main/java/de/rwth/idsg/steve/service/WebUserService.java#L255 <com.puppycrawl.tools.checkstyle.checks.regexp.RegexpSinglelineCheck>
Line has trailing spaces.
src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java
[failure] 46-46: [checkstyle] src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java#L46 <com.puppycrawl.tools.checkstyle.checks.regexp.RegexpSinglelineCheck>
Line has trailing spaces.
src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java
[failure] 204-204: [checkstyle] src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java#L204 <com.puppycrawl.tools.checkstyle.checks.naming.LocalVariableNameCheck>
Name 'redirect_str' must match pattern '^[a-z][a-zA-Z0-9]*$'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 17, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 17, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 17, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 17, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 17, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 17, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 17, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 17, mariadb:10.5.21)
🔇 Additional comments (13)
src/main/java/de/rwth/idsg/steve/web/controller/NoAccessController.java (1)
32-43: LGTM: Simple controller and view mapping look correctThe mapping to "noAccess" view under /manager/noAccess aligns with the security flow and view resolver conventions in this codebase.
src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java (2)
133-161: LGTM: Stats extended with numWebUsers cleanly
- Correct use of jOOQ subselect and aliasing.
- Import/Record arity updated appropriately (Record9).
- Builder population in order is consistent.
138-161: Confirmed — Statistics contains numWebUsers and Lombok builder/getter; no change neededVerified that Statistics.java declares a final Integer numWebUsers and is annotated with @builder and @Getter, so the builder method .numWebUsers(...) and the getter used by the JSP exist.
Files checked:
- src/main/java/de/rwth/idsg/steve/web/dto/Statistics.java — final field numWebUsers; @Builder/@Getter present
- src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java — uses .numWebUsers(gs.value9()) on Statistics.builder()
- src/main/resources/webapp/WEB-INF/views/home.jsp — references ${stats.numWebUsers}
No action required.
src/main/resources/webapp/WEB-INF/views/data-man/webuserDetails.jsp (2)
43-51: Review username-in-URL for password endpointsEmbedding
webUsernameinto the URL path can break routing if usernames contain special chars and may leak PII. Prefer usingwebUserPkor ensure strict username validation and URL-encoding.Would you like me to propose a change to use
webUserPkin the routes if the controller supports it, or to add encoding using<c:url>?
34-79: Ensure CSRF tokens on POST update formIf CSRF is enabled, include the token in the form.
Run the CSRF verification script provided in the add page comment. If enabled, add:
<form:form action="${ctxPath}/manager/webusers/update" modelAttribute="webuserForm"> <table class="userInput"> + <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>src/main/resources/webapp/WEB-INF/views/data-man/webusers.jsp (1)
64-66: Explicitly POST and include CSRF token for DeleteMake the intent explicit and include the CSRF token if enabled.
Apply this diff:
- <form:form action="${ctxPath}/manager/webusers/delete/${cr.webUserPk}"> - <input type="submit" class="redSubmit" value="Delete"> + <form:form action="${ctxPath}/manager/webusers/delete/${cr.webUserPk}" method="post"> + <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/> + <input type="submit" class="redSubmit" value="Delete" onclick="return confirm('Delete this web user?');">src/main/java/de/rwth/idsg/steve/web/dto/Statistics.java (1)
40-42: LGTM! Field addition for web user statistics.The addition of
numWebUsersto the Statistics DTO is properly integrated with the existing field group and leverages Lombok annotations for automatic generation of getter, builder, and toString methods.src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java (1)
77-87: Well-structured security rules for web user management.The authorization rules effectively implement role-based access control with proper separation between USER and ADMIN authorities. The use of
WebExpressionAuthorizationManagerfor user-specific password endpoints ensures users can only modify their own credentials.src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java (5)
119-130: Verify not-found handling for username-based lookups as well.
getDetails(webUserName)should explicitly handle “user not found” scenarios to avoid NPEs when mapping.If the service returns
Optional, preferorElseThrow(new ResponseStatusException(HttpStatus.NOT_FOUND)); otherwise, handle specific exceptions at the controller level. Do you want me to draft the exception handling scaffolding?Also applies to: 143-154
64-75: LGTM: Overview and query flow is clean and cohesive.The list initialization and query binding are tidy, and the view reuse keeps things simple.
64-206: All referenced JSP view templates exist — no action required.Verification checked these files; all are present:
- src/main/resources/webapp/WEB-INF/views/data-man/webusers.jsp
- src/main/resources/webapp/WEB-INF/views/data-man/webuserAdd.jsp
- src/main/resources/webapp/WEB-INF/views/data-man/webuserDetails.jsp
- src/main/resources/webapp/WEB-INF/views/data-man/webuserPassword.jsp
- src/main/resources/webapp/WEB-INF/views/data-man/webuserApiPassword.jsp
64-206: Verified — service method contracts match controller usageBoth overloads of getDetails(...) and all mutator methods invoked by WebUsersController exist with matching signatures.
- File: src/main/java/de/rwth/idsg/steve/service/WebUserService.java
- getDetails(Integer webUserPk) — line 241
- getDetails(String webUserName) — line 256
- add(WebUserForm) — line 202
- update(WebUserBaseForm) — line 206
- updatePassword(WebUserForm) — line 216
- updateApiPassword(WebUserForm) — line 220
- deleteUser(int webUserPk) — line 187
- deleteUser(String username) (override from UserDetailsManager) — line 125
No signature mismatches or missing methods found.
89-95: setAuthorities usage verified — no change requiredWebUserBaseForm defines a field
private WebUserAuthority authorities(with Lombok@Setter), andWebUserAuthorityis an enum (USER, ADMIN, USER_ADMIN). The call webUserForm.setAuthorities(WebUserAuthority.USER) matches the generated setter signature.
- src/main/java/de/rwth/idsg/steve/web/dto/WebUserBaseForm.java —
private WebUserAuthority authorities(Lombok @Setter)- src/main/java/de/rwth/idsg/steve/web/dto/WebUserAuthority.java — enum with
USER,ADMIN,USER_ADMIN- src/main/java/de/rwth/idsg/steve/web/controller/WebUsersController.java:92 —
webUserForm.setAuthorities(WebUserAuthority.USER)
src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
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/WebUserRepository.java
Outdated
Show resolved
Hide resolved
src/main/resources/webapp/WEB-INF/views/data-man/webuserDetails.jsp
Outdated
Show resolved
Hide resolved
src/main/resources/webapp/WEB-INF/views/data-man/webuserDetails.jsp
Outdated
Show resolved
Hide resolved
src/main/resources/webapp/WEB-INF/views/data-man/webuserPassword.jsp
Outdated
Show resolved
Hide resolved
src/main/resources/webapp/WEB-INF/views/data-man/webuserPassword.jsp
Outdated
Show resolved
Hide resolved
|
@fnkbsi I rebased your branch onto the latest upstream branch. What do you think about the IA review? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
juherr
force-pushed
the
fnkbsi/MultiUser_4
branch
from
8da3092 to
3a7b344
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request implements web user management functionality with role-based access controls. It adds comprehensive CRUD operations for web users, password management capabilities, and tightened security throughout the application.
Key changes include:
- Complete web user management system with overview, creation, update, delete, and password change functionality
- Enhanced role-based access control with USER and ADMIN authority levels
- New access denied page for insufficient permissions
- Updated home dashboard to display web user count
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/webapp/WEB-INF/views/noAccess.jsp | New access denied page displayed when users lack sufficient permissions |
| src/main/resources/webapp/WEB-INF/views/home.jsp | Added web users count tile to dashboard |
| src/main/resources/webapp/WEB-INF/views/data-man/*.jsp | Web user management pages for overview, details, add, and password changes |
| src/main/resources/webapp/WEB-INF/views/00-header.jsp | Added WEBUSERS navigation menu item |
| src/main/java/de/rwth/idsg/steve/web/dto/*.java | Data transfer objects for web user forms and queries |
| src/main/java/de/rwth/idsg/steve/web/controller/*.java | Controllers for web user management and access denial |
| src/main/java/de/rwth/idsg/steve/service/*.java | Service layer implementation for web user operations |
| src/main/java/de/rwth/idsg/steve/repository/*.java | Repository layer for web user data access |
| src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java | Enhanced security configuration with fine-grained access controls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,42 @@ | |||
| /* | |||
| * SteVe - SteckdosenVerwaltung - https://github.com/steve-community/steve | |||
| * Copyright (C) 2013-2024 SteVe Community Team | |||
There was a problem hiding this comment.
The copyright year should be updated to 2025 to match other files in the PR.
| * Copyright (C) 2013-2024 SteVe Community Team | |
| * Copyright (C) 2013-2025 SteVe Community Team |
| @@ -0,0 +1,32 @@ | |||
| /* | |||
| * SteVe - SteckdosenVerwaltung - https://github.com/steve-community/steve | |||
| * Copyright (C) 2013-2024 SteVe Community Team | |||
There was a problem hiding this comment.
The copyright year should be updated to 2025 to match other files in the PR.
| * Copyright (C) 2013-2024 SteVe Community Team | |
| * Copyright (C) 2013-2025 SteVe Community Team |
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Inconsistent spacing in Javadoc comment. There should be no space before the opening /**.
| /** | |
| /** |
| } | ||
|
|
||
| private static String toDetails(Integer userPk) { | ||
| return String.format("redirect:/manager/webusers/details/%s", userPk); |
There was a problem hiding this comment.
Inconsistent indentation. This line uses 6 spaces instead of the consistent 4 spaces used elsewhere in the method.
| return String.format("redirect:/manager/webusers/details/%s", userPk); | |
| return String.format("redirect:/manager/webusers/details/%s", userPk); |
| .webUserPk(r.value1()) | ||
| .webUsername(r.value2()) | ||
| .enabled(r.value3()) | ||
| //.authorities(fromJsonToString(r.value4())) |
There was a problem hiding this comment.
Remove commented-out code. This line appears to be leftover debug code that should be cleaned up.
| //.authorities(fromJsonToString(r.value4())) |
juherr
force-pushed
the
fnkbsi/MultiUser_4
branch
from
1728937 to
a845ce9
Compare
Rebase of steve-community#1544
Summary by CodeRabbit
New Features
Security