feat: login get to post - #6409
Conversation
|
do you fix shenyu frontend? |
yes. pr: apache/shenyu-dashboard#589 |
Aias00
left a comment
There was a problem hiding this comment.
Findings:
-
(blocker) The committed admin UI still calls GET /platform/login. In
shenyu-admin/src/main/resources/static/index.814edc73.jsthe login call is.../platform/login?${stringify(t)}with{method:"GET"}. After this PR the controller only maps@PostMapping("/login"), so the bundled dashboard login request returns 405 and the UI cannot log in. The credentials also keep landing in the query string, which is exactly what this change is meant to stop. The PR updates the.httpdebug file but not the actual committed frontend bundle (there's no dashboard submodule or frontend build step inshenyu-admin/pom.xml, andstatic/*.jsare committed verbatim). Please regenerate thestatic/bundle from an updatedshenyu-dashboardand commit it here, or coordinate a simultaneous frontend release and note the dependency. Issue #6397 explicitly asks to "Update the Admin frontend to send login requests as POST." -
(should_fix) No compat path or migration note for the GET→POST break. In-repo clients are all converted (RegisterUtils, AccessTokenManager, AdminClient), but
shenyu-register-client-http(gateway) andshenyu-sync-data-http(bootstrap) are separate deployables that call/platform/loginagainst admin — a rolling upgrade of admin ahead of gateway/bootstrap breaks registration and sync (GET → 405). #6397's acceptance says "Maintain backward compatibility or provide a migration note in the release changelog," and I didn't find either in the repo. Consider keeping a deprecated GET handler for one release or adding an explicit release note that admin, gateway, and bootstrap must be upgraded together. -
(should_fix) Add a negative test for the new
@RequestBody @Validpath.LoginDashboardUserDTOnow has@NotBlankonuserName/password, butPlatformControllerTestonly covers the happy path. Please add a case posting{}(or blank fields) and asserting a 4xx so the fail-closed validation is actually covered. -
(nit)
mergeStateStatusis BEHIND — please rebase onto current master before merge.
Positive: server-side move from GET query to POST JSON body is the right call and removes credentials from access logs/referrer for the API clients. The DTO field names match Constants.LOGIN_NAME/PASS_WORD, so binding is consistent across register-client and sync-data.
Aias00
left a comment
There was a problem hiding this comment.
Correction on the blocker I raised above — the frontend bundle update is tracked in a separate repo: apache/shenyu-dashboard#589, which changes src/services/api.js from GET /platform/login?${stringify(params)} to POST /platform/login (no query string). So the "committed UI still calls GET, author forgot the frontend" concern was based on incomplete evidence on my part — the source fix is coordinated there. Retracting that framing.
What remains is a cross-repo merge-ordering requirement, not a code gap in this PR:
- The shenyu repo commits
static/*.jsverbatim and has no frontend build step inshenyu-admin/pom.xml(no dashboard submodule, no npm/node plugin). So even after shenyu-dashboard#589 merges, thestatic/*.jsin this repo will still be the GET bundle until someone rebuilds the dashboard and commits the regenerated bundle here. A release cut from this PR's shenyu state alone would ship the broken GET bundle and login returns 405. - Please either (a) commit the regenerated
static/*.jsinto this PR once #589 merges, or (b) state explicitly in the PR body that shenyu#6409 + apache/shenyu-dashboard#589 + the bundle rebuild must ship together, so the release process doesn't cut a half-upgraded admin.
The two earlier should-fix items still stand:
- Compat/migration for the GET→POST contract break. In-repo clients are converted (RegisterUtils, AccessTokenManager, AdminClient), but
shenyu-register-client-http(gateway) andshenyu-sync-data-http(bootstrap) are separate deployables that call/platform/loginagainst admin. A rolling upgrade of admin ahead of gateway/bootstrap breaks registration and sync (GET → 405). #6397's acceptance says "Maintain backward compatibility or provide a migration note in the release changelog" — please keep a deprecated GET handler for one release or add an explicit release note that admin, gateway, and bootstrap must be upgraded together. - Negative test for the new
@RequestBody @Validpath.LoginDashboardUserDTOnow has@NotBlankonuserName/password, butPlatformControllerTestonly covers the happy path. A case posting{}(or blank fields) asserting a 4xx would cover the fail-closed validation.
Backend change itself (POST controller + @Valid DTO + in-repo client updates, field names matching Constants.LOGIN_NAME/PASS_WORD) is correct. Switching this to comment — no code blocker remaining once the merge-ordering note is added.
Fixes: #6397
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.