Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/server: /demologin doesn't redirect, doesn't provide multitenant-encoded cookie #98253

Closed
abarganier opened this issue Mar 8, 2023 · 1 comment · Fixed by #98319
Closed
Assignees
Labels
A-observability-inf C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@abarganier
Copy link
Contributor

abarganier commented Mar 8, 2023

Describe the problem

When I use the /demologin endpoint, many times I am still returned to the login page. On the occasions that it does work, I noticed that the session cookie isn't in the multitenant format (e.g. no encoded tenant name(s), and only a single session).

To Reproduce

Run ./cockroach demo --multitenant=true and use the provided /demologin link. Note the above stated behavior.

Expected behavior
The user should be logged in with a multitenant session cookie.

Jira issue: CRDB-25153

Epic: CRDB-12100

@abarganier abarganier added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability-inf labels Mar 8, 2023
@abarganier abarganier self-assigned this Mar 8, 2023
@abarganier
Copy link
Contributor Author

abarganier commented Mar 9, 2023

The /demologin endpoint seems to work correctly (I think the original issue was likely conflated with #97786). It properly uses the HTTP mux to attempt logins to all available tenant servers, and sets the expected multitenant session cookie with the session,tenant_name encoding.

The problem is that it's the browser that issues the request, so when a 200 response is received, we don't have any JavaScript running to trigger a redirect to the home page.

One possible solution here would be to use the existing /login page instead. We could include URL query params to auto-fill the username & password form, and then either auto-submit the form or leave it up to the user to hit the submit button.

The login page has redirect functionality already, so the rest would be handled.

I'm not sure the history around why we have a separate /demologin endpoint in the first place. Is there something preventing us from using the existing /login endpoint instead? From what I can tell, the logic between the two [/demoLogin, /login] is very similar. Perhaps we can do away with /demologin and consolidate around /login.

cc @dhartunian @knz in case y'all have any history or context around /demologin's origin/purpose. If there are no objections, I'd like to take this approach.

EDIT: I figured I should also pose this angle... Is this actually a problem that needs solving? Is it acceptable for /demologin to not redirect to the home page? It's not the smoothest experience, but there may be higher priority work than this.

EDIT: Okay, I missed the 302 redirect piece of this. For some reason that doesn't seem to be triggering on the first request. Looks to be related to the request being intercepted by the server controller, which is returning a 200 instead of 302. I'll dig into this - sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability-inf C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant