pkg/server: fix /demologin to properly redirect to home page
#98319
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
pkg/server/server_controller_http.go
Outdated
| // In the case of /demologin, we want to redirect to the home page. | ||
| // If we get back a cookie along with a 302, be sure to transfer that | ||
| // into the ResponseWriter later. | ||
| if sw.code == 302 { |
There was a problem hiding this comment.
http.StatusTemporaryRedirect is assigned to code 307. Seems like that's what we should be using anyway, so I went ahead and switched both places (the /demologin handler, and the quoted code above) to use http.StatusTemporaryRedirect.
pkg/server/server_controller_http.go
Outdated
| @@ -217,7 +224,12 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler { | |||
| return | |||
| } | |||
| } | |||
| w.WriteHeader(http.StatusOK) | |||
| if redirect { | |||
| w.Header()["Location"] = []string{"/"} | |||
There was a problem hiding this comment.
http.Redirect(w, r, "/", http.StatusTemporaryRedirect) worked well. Thanks!
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/server/server_controller_http.go line 228 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
http.Redirect(w, r, "/", http.StatusTemporaryRedirect)worked well. Thanks!
Should we also grab the Location from the fanout response?
There was a problem hiding this comment.
Reviewable status:
pkg/server/server_controller_http.go line 228 at r1 (raw file):
Previously, dhartunian (David Hartunian) wrote…
Should we also grab the Location from the fanout response?
Good idea! Added this, with a default value of /
With the introduction of the server controller, we introduced a layer between the HTTP handler and the HTTP server. When this was introduced, the logic to attempt a login to all tenants forgot to handle the case for `/demologin` where the status code is set to a 307 redirect, instead of a 200 status OK. This broke the redirect piece of the `/demologin` endpoint. This patch updates the `attemptLoginToAllTenants` HTTP handler to properly set the 307 response code in the case where the underlying login function does so on the sessionWriter. Release note: none
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
|
TFTR! bors r=dhartunian |
|
Build failed (retrying...): |
|
Build failed: |
|
sorry for the noise, wrong PR |
|
bors retry |
|
Build succeeded: |
With the introduction of the server controller, we introduced a layer between the HTTP handler and the HTTP server. When this was introduced, the logic to attempt a login to all tenants forgot to handle a specific case for
/demologin, where the status code is set to a 302 redirect, instead of a 200 status OK.This broke the redirect piece of the
/demologinendpoint.This patch updates the
attemptLoginToAllTenantsHTTP handler to properly set the 302 response code in the case where the underlying login function does so on the sessionWriter.Release note: none
Epic: CRDB-12100
Fixes: #98253