Skip to content

feat: login get to post - #6409

Open
eye-gu wants to merge 4 commits into
apache:masterfrom
eye-gu:fix/6397-login-get-to-post
Open

feat: login get to post#6409
eye-gu wants to merge 4 commits into
apache:masterfrom
eye-gu:fix/6397-login-get-to-post

Conversation

@eye-gu

@eye-gu eye-gu commented Jun 27, 2026

Copy link
Copy Markdown
Member

Fixes: #6397

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

@moremind moremind added this to the 2.7.1 milestone Jul 6, 2026
@moremind

moremind commented Jul 6, 2026

Copy link
Copy Markdown
Member

do you fix shenyu frontend?

@eye-gu

eye-gu commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

do you fix shenyu frontend?

yes. pr: apache/shenyu-dashboard#589

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings:

  1. (blocker) The committed admin UI still calls GET /platform/login. In shenyu-admin/src/main/resources/static/index.814edc73.js the 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 .http debug file but not the actual committed frontend bundle (there's no dashboard submodule or frontend build step in shenyu-admin/pom.xml, and static/*.js are committed verbatim). Please regenerate the static/ bundle from an updated shenyu-dashboard and 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."

  2. (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) and shenyu-sync-data-http (bootstrap) are separate deployables that call /platform/login against 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.

  3. (should_fix) Add a negative test for the new @RequestBody @Valid path. LoginDashboardUserDTO now has @NotBlank on userName/password, but PlatformControllerTest only covers the happy path. Please add a case posting {} (or blank fields) and asserting a 4xx so the fail-closed validation is actually covered.

  4. (nit) mergeStateStatus is 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 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/*.js verbatim and has no frontend build step in shenyu-admin/pom.xml (no dashboard submodule, no npm/node plugin). So even after shenyu-dashboard#589 merges, the static/*.js in 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/*.js into 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:

  1. Compat/migration for the GET→POST contract break. In-repo clients are converted (RegisterUtils, AccessTokenManager, AdminClient), but shenyu-register-client-http (gateway) and shenyu-sync-data-http (bootstrap) are separate deployables that call /platform/login against 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.
  2. Negative test for the new @RequestBody @Valid path. LoginDashboardUserDTO now has @NotBlank on userName/password, but PlatformControllerTest only 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task] Change /platform/login from GET to POST to avoid credentials in URL

3 participants