#3233 Fixed change incorrect GET endpoints to POST #3240
Conversation
Updated REST endpoints for state-changing operations and moved auth credentials into JSON body, then aligned frontend callers, security rules, and docs to match the new methods and paths.
- Backend mappings switched to POST/PUT where needed and auth/logout paths simplified in src/org/scada_lts/web/mvc/api/AuthenticationAPI.java, src/org/scada_lts/web/mvc/api/DataSourceAPI.java, src/org/scada_lts/web/mvc/api/
PointValueAPI.java, src/org/scada_lts/web/mvc/api/ReportsAPI.java, src/org/scada_lts/web/mvc/api/SystemSettingsAPI.java, src/org/scada_lts/web/mvc/api/ResourcesAPI.java, src/org/scada_lts/web/mvc/api/ViewHierarchyAPI.java
- Report endpoint method guards updated in WebContent/WEB-INF/spring-security.xml
- Vuex/JSP/Cypress callers updated for new methods and auth body in scadalts-ui/src/store/dataSource/index.js, scadalts-ui/src/store/reports/index.js, scadalts-ui/src/store/systemSettings/index.js, WebContent/WEB-INF/jsp/
systemSettings.jsp, scadalts-ui/cypress/support/commands.js, scadalts-ui/cypress/integration/ScadaLTS_Views/modern_charts.cy.js, WebContent/resources/vue-mixins/shared/mixins-export-import.js
- OpenAPI and migration notes updated in doc/RESTAPI/ScadaLTS_API_OAS3.yaml and doc/RESTAPI/ENDPOINT_MIGRATION.md, plus a small README tweak in scadalts-ui/README.md
|
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
📝 WalkthroughWalkthroughThis pull request systematically converts multiple API endpoints from GET to POST/PUT methods, restructures authentication to accept credentials via request body instead of URL path parameters, and updates corresponding client-side implementations, tests, and API documentation to align with the new HTTP semantics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
WebContent/resources/vue-mixins/shared/mixins-export-import.js (2)
14-24: Critical: Hardcoded credentials and localhost URL in client-side code.This code has multiple serious issues:
- Security vulnerability: Hardcoded
admin/admincredentials in client-side JavaScript are visible to anyone and should never be committed.- Hardcoded localhost URL:
http://localhost:8080/ScadaBR/api/authwill fail in any non-local environment. Use a relative URL (e.g.,/ScadaBR/api/auth) or a configurable base URL.- Debug artifacts: Multiple
alert()calls should be removed before merging.Proposed fix
try { axios({ method: 'post', - url: 'http://localhost:8080/ScadaBR/api/auth', - data: { - username: 'admin', - password: 'admin', - }, + url: '/ScadaBR/api/auth', + // TODO: Replace with actual user credentials from auth context + data: { + username: this.username, + password: this.password, + }, }).then(function (response) { - alert(response.data); - alert(response.status); - alert(response.statusText); - alert(response.headers); - alert(response.config); + console.log('Auth successful:', response.status); + }).catch(function (error) { + console.error('Auth failed:', error); });
26-28: Ineffective error handling for async operation.The synchronous
try-catchwon't catch axios promise rejections. Add.catch()to the promise chain:Proposed fix
- try { axios({ method: 'post', url: '/ScadaBR/api/auth', data: { username: 'admin', password: 'admin', }, }).then(function (response) { // handle success + }).catch(function (error) { + console.error('Auth request failed:', error); }); - } catch( err ) { - alert( err ); - }doc/RESTAPI/ScadaLTS_API_OAS3.yaml (1)
6-8: Bump API version to 3.0.0 to signal breaking changes per semantic versioning.The project follows Semantic Versioning Guidelines (as stated in README.md). Since this PR introduces breaking changes (HTTP method changes, credential location changes), the API version must be bumped from 2.6.0 to 3.0.0 (major version) per semver rules, not a minor increment.
🤖 Fix all issues with AI agents
In @scadalts-ui/README.md:
- Line 27: Fix the typo in the README sentence: replace "propably" with
"probably" in the sentence that instructs pasting the cookie into your DevServer
(the line that currently reads "Because you were propably prompted to input a
auth cookie there").
🧹 Nitpick comments (7)
src/org/scada_lts/web/mvc/api/ViewHierarchyAPI.java (4)
122-147: Inconsistency:editFoldershould use PUT, not GET.This endpoint modifies an existing resource, which is a state-changing operation. Per the PR's objective of fixing incorrect GET endpoints, this should use
RequestMethod.PUT(orPATCH) for consistency with REST semantics.♻️ Suggested fix
- @RequestMapping(value = "/api/view_hierarchy/editFolder/{name}/{key}", method = RequestMethod.GET) + @RequestMapping(value = "/api/view_hierarchy/editFolder/{name}/{key}", method = RequestMethod.PUT)
149-172: Inconsistency:deleteFoldershould use DELETE, not GET.This endpoint deletes a resource, which is a state-changing operation. It should use
RequestMethod.DELETEfor proper REST semantics.♻️ Suggested fix
- @RequestMapping(value = "/api/view_hierarchy/deleteFolder/{id}", method = RequestMethod.GET) + @RequestMapping(value = "/api/view_hierarchy/deleteFolder/{id}", method = RequestMethod.DELETE)
174-197: Inconsistency:deleteViewshould use DELETE, not GET.Same issue as
deleteFolder- this is a destructive operation that should useRequestMethod.DELETE.♻️ Suggested fix
- @RequestMapping(value = "/api/view_hierarchy/deleteView/{id}", method = RequestMethod.GET) + @RequestMapping(value = "/api/view_hierarchy/deleteView/{id}", method = RequestMethod.DELETE)
199-247: Inconsistency:moveFolderandmoveViewshould use PUT or POST, not GET.Both endpoints perform state-changing operations (relocating resources). Consider using
PUT(idempotent move) orPOSTfor consistency with the PR's objective.♻️ Suggested fix
- @RequestMapping(value = "/api/view_hierarchy/moveFolder/{id}/{newParentId}", method = RequestMethod.GET) + @RequestMapping(value = "/api/view_hierarchy/moveFolder/{id}/{newParentId}", method = RequestMethod.PUT) public ResponseEntity<String> moveFolder(@PathVariable("id") int id, @PathVariable("newParentId") int newParentId, HttpServletRequest request) { ... } - @RequestMapping(value = "/api/view_hierarchy/moveView/{id}/{newParentId}", method = RequestMethod.GET) + @RequestMapping(value = "/api/view_hierarchy/moveView/{id}/{newParentId}", method = RequestMethod.PUT) public ResponseEntity<String> moveView(@PathVariable("id") int id, @PathVariable("newParentId") int newParentId, HttpServletRequest request) {src/org/scada_lts/web/mvc/api/PointValueAPI.java (1)
501-506: Misleading method name after HTTP method change.The method is named
setValueGetbut now uses@PutMapping. Consider renaming tosetValuePutor simplysetValueto avoid confusion for future maintainers.♻️ Suggested rename
@PutMapping(value = "/api/point_value/setValue/{xid}/{type}/{value}") -public ResponseEntity<String> setValueGet( +public ResponseEntity<String> setValuePut( @PathVariable("xid") String xid, @PathVariable("type") int type, @PathVariable("value") String value, HttpServletRequest request) {src/org/scada_lts/web/mvc/api/ReportsAPI.java (1)
42-68: Pre-existing: Inconsistent log messages vs HTTP methods.Several log messages don't match their actual HTTP methods:
- Line 44: Logs
GET::but uses@PostMapping- Line 68: Logs
GET::but uses@PostMapping- Line 115: Logs
GET::but uses@DeleteMappingConsider fixing these for consistency with the updated endpoints.
src/org/scada_lts/web/mvc/api/AuthenticationAPI.java (1)
65-77: Consider validating empty strings in addition to null checks.The null checks are good, but empty strings (
"") would pass validation and proceed to authentication. While the authentication logic might handle this, it's better to fail fast with a clear 400 response.♻️ Suggested improvement
- if (authRequest == null || authRequest.getUsername() == null || authRequest.getPassword() == null) { + if (authRequest == null + || authRequest.getUsername() == null || authRequest.getUsername().isEmpty() + || authRequest.getPassword() == null || authRequest.getPassword().isEmpty()) { return new ResponseEntity<>(HttpStatus.BAD_REQUEST); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
WebContent/WEB-INF/jsp/systemSettings.jspWebContent/WEB-INF/spring-security.xmlWebContent/resources/vue-mixins/shared/mixins-export-import.jsdoc/RESTAPI/ScadaLTS_API_OAS3.yamlscadalts-ui/README.mdscadalts-ui/cypress/integration/ScadaLTS_Views/modern_charts.cy.jsscadalts-ui/cypress/support/commands.jsscadalts-ui/src/store/dataSource/index.jsscadalts-ui/src/store/reports/index.jsscadalts-ui/src/store/systemSettings/index.jssrc/org/scada_lts/web/mvc/api/AuthenticationAPI.javasrc/org/scada_lts/web/mvc/api/DataSourceAPI.javasrc/org/scada_lts/web/mvc/api/PointValueAPI.javasrc/org/scada_lts/web/mvc/api/ReportsAPI.javasrc/org/scada_lts/web/mvc/api/ResourcesAPI.javasrc/org/scada_lts/web/mvc/api/SystemSettingsAPI.javasrc/org/scada_lts/web/mvc/api/ViewHierarchyAPI.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/org/scada_lts/web/mvc/api/AuthenticationAPI.java (1)
src/com/serotonin/mango/Common.java (1)
Common(69-584)
🪛 LanguageTool
scadalts-ui/README.md
[grammar] ~27-~27: Ensure spelling is correct
Context: ... inside your DevServer because you were propably prompted to input a auth cookie there. ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (1)
- GitHub Check: Build WAR archive
🔇 Additional comments (26)
WebContent/WEB-INF/jsp/systemSettings.jsp (1)
499-509: LGTM! POST is the correct HTTP method for this state-changing operation.The switch from GET to POST for the
imagesRefreshendpoint is appropriate since this operation modifies server-side state (refreshes the image cache).scadalts-ui/src/store/systemSettings/index.js (1)
286-309: LGTM! POST is the correct HTTP method for purge operations.Both
purgeDataandpurgeNoware state-changing operations that modify server data, making POST the appropriate HTTP verb. The implementation correctly matches the backend API changes.WebContent/WEB-INF/spring-security.xml (1)
367-369: LGTM! Security rules correctly updated to match new HTTP methods.
PUTforpreventPurgeis appropriate as it's an idempotent state updatePOSTforrunis appropriate as it triggers a new report execution (non-idempotent)The access control expressions remain correctly configured with the guard permission checks.
src/org/scada_lts/web/mvc/api/ViewHierarchyAPI.java (1)
85-86: LGTM! POST is correct for creating a new folder resource.scadalts-ui/cypress/integration/ScadaLTS_Views/modern_charts.cy.js (1)
13-13: LGTM - Authentication request correctly updated to POST.The change aligns with the new
/api/authendpoint that accepts credentials via JSON body instead of URL path parameters.Note: This REST login call appears after the UI-based login in the
before()hook (lines 3-8). If this is intentional for session refresh purposes, consider adding a brief comment explaining why both authentication methods are used.src/org/scada_lts/web/mvc/api/DataSourceAPI.java (2)
67-75: LGTM - Correct use of POST for state-changing toggle operation.Changing from GET to POST for the
/api/datasource/toggleendpoint correctly follows REST semantics, as this operation modifies server state.
88-96: LGTM - Correct use of PUT for idempotent enable operation.The PUT method is appropriate here since enabling all data points is an idempotent operation (calling it multiple times yields the same result).
scadalts-ui/src/store/reports/index.js (2)
52-58: LGTM - Correctly updated to use PUT for preventPurge toggle.The change from GET to PUT aligns with REST semantics for this idempotent state modification, and matches the backend endpoint change.
72-74: LGTM - Correctly updated to use POST for running reports.The change from GET to POST is appropriate since running a report is a non-idempotent operation that triggers server-side processing.
scadalts-ui/cypress/support/commands.js (1)
52-62: LGTM - Authentication request correctly updated to POST with JSON body.This change improves security by moving credentials from the URL path to the request body. Credentials in URL paths can be inadvertently logged in server access logs, browser history, and network proxies.
scadalts-ui/src/store/dataSource/index.js (2)
341-354: LGTM - Correctly updated to use PUT for enabling data points.The change aligns with the backend
@PutMappingannotation on/api/datasource/datapoints/enableand follows REST semantics for idempotent operations.
357-370: LGTM - Correctly updated to use POST for toggling data source.The change aligns with the backend
@PostMappingannotation on/api/datasource/toggleand follows REST semantics for state-changing operations.src/org/scada_lts/web/mvc/api/PointValueAPI.java (1)
683-746: LGTM! HTTP method changes align with REST semantics.Changing these metadata update endpoints from GET to PUT correctly reflects their state-modifying behavior. PUT is appropriate since these operations are idempotent updates.
src/org/scada_lts/web/mvc/api/ResourcesAPI.java (1)
46-61: LGTM! Correct HTTP method for state-changing operation.Changing
imagesRefreshfrom GET to POST is appropriate since this endpoint triggers a refresh operation with side effects rather than simply retrieving data.src/org/scada_lts/web/mvc/api/SystemSettingsAPI.java (2)
305-323: LGTM! Critical fix for destructive operation.Changing
purgeNowfrom GET to POST is essential. GET requests for data-destructive operations are a security risk (can be triggered by link prefetching, crawlers, or accidental URL sharing).
487-505: LGTM! Correct HTTP method for purge operation.The
purgeDataendpoint correctly uses POST now, preventing accidental data deletion via GET requests.src/org/scada_lts/web/mvc/api/ReportsAPI.java (2)
87-92: LGTM! Correct HTTP method for action endpoint.Running a report is a state-changing operation that triggers processing, so POST is the appropriate method.
120-125: LGTM! PUT is appropriate for updating a resource property.Setting
preventPurgeis an idempotent update operation, making PUT the correct choice.doc/RESTAPI/ScadaLTS_API_OAS3.yaml (6)
17-43: LGTM! Authentication endpoints properly documented.The
/authendpoint now correctly uses POST with a JSON request body containing credentials, which is more secure than passing credentials in URL path parameters.
2516-2525: LGTM! AuthenticationRequest schema is well-defined.The schema correctly marks both
usernameandpasswordas required fields.
793-816: LGTM! PUT operation documented for setValue endpoint.The PUT method is correctly added alongside the existing (deprecated) POST method, providing a proper idempotent update operation.
948-979: LGTM! Metadata update endpoints correctly use PUT.All three
updateMetaDataPoint*endpoints are properly documented with PUT method.
980-1010: LGTM! Reports endpoints correctly documented.
/reports/run/{id}uses POST (action/trigger)/reports/instances/{id}/preventPurge/{preventPurge}uses PUT (idempotent update)
2313-2340: LGTM! Purge endpoints correctly use POST.Both
/systemSettings/purgeDataand/systemSettings/purgeNoware now documented as POST operations, aligning with the backend changes and REST best practices for destructive operations.src/org/scada_lts/web/mvc/api/AuthenticationAPI.java (2)
44-63: LGTM - Clean DTO for authentication credentials.The inner class properly encapsulates authentication credentials. Moving credentials from URL path parameters to a request body is a security improvement as it prevents credentials from being logged in server access logs and browser history.
101-111: Good security improvement - logout now uses session context.Deriving the user from
Common.getUser(request)instead of a path variable prevents a potential security issue where an attacker could attempt to log out other users by specifying their username. The POST method is also appropriate for this state-changing operation.
scadalts-ui/README.md
Outdated
| ## Using the developmnet server | ||
| When you start a development server to instant application building you have to log into the Scada-LTS. Dev server uses `"http-proxy-middleware-secure-cookies"` dependency to handle the connection within frontend and core application. When your server is ready you have to open your browser on the `http://localhost:3000/#/` and you should be redirected to login page. There you have to open the DevTools (F11 in Chrome) and open "Network" tab to monitor the HTTP requestes. Try to log into the application using the default password. | ||
| When you receive the `xhr response` with RequestURL `*/api/auth/...` from server open that request details and find the **Response Headers** section. Then find a "set-cookie" header and copy that value. Then paste that cookie inside your DevServer because you were propably prompted to input a auth cookie there. If everything is ok you should see the | ||
| When you receive the `xhr response` with RequestURL `*/api/auth` from server open that request details and find the **Response Headers** section. Then find a "set-cookie" header and copy that value. Then paste that cookie inside your DevServer because you were propably prompted to input a auth cookie there. If everything is ok you should see the |
There was a problem hiding this comment.
Fix typo: "propably" → "probably"
📝 Suggested fix
-When you receive the `xhr response` with RequestURL `*/api/auth` from server open that request details and find the **Response Headers** section. Then find a "set-cookie" header and copy that value. Then paste that cookie inside your DevServer because you were propably prompted to input a auth cookie there. If everything is ok you should see the
+When you receive the `xhr response` with RequestURL `*/api/auth` from server open that request details and find the **Response Headers** section. Then find a "set-cookie" header and copy that value. Then paste that cookie inside your DevServer because you were probably prompted to input a auth cookie there. If everything is ok you should see the 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When you receive the `xhr response` with RequestURL `*/api/auth` from server open that request details and find the **Response Headers** section. Then find a "set-cookie" header and copy that value. Then paste that cookie inside your DevServer because you were propably prompted to input a auth cookie there. If everything is ok you should see the | |
| When you receive the `xhr response` with RequestURL `*/api/auth` from server open that request details and find the **Response Headers** section. Then find a "set-cookie" header and copy that value. Then paste that cookie inside your DevServer because you were probably prompted to input a auth cookie there. If everything is ok you should see the |
🧰 Tools
🪛 LanguageTool
[grammar] ~27-~27: Ensure spelling is correct
Context: ... inside your DevServer because you were propably prompted to input a auth cookie there. ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In @scadalts-ui/README.md at line 27, Fix the typo in the README sentence:
replace "propably" with "probably" in the sentence that instructs pasting the
cookie into your DevServer (the line that currently reads "Because you were
propably prompted to input a auth cookie there").
Updated REST endpoints for state-changing operations and moved auth credentials into JSON body, then aligned frontend callers, security rules, and docs to match the new methods and paths.
Summary by CodeRabbit
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.