Skip to content

Commit

Permalink
server: fix logout to always fanout to all tenants
Browse files Browse the repository at this point in the history
Previously, we added login fanout logic handled through an HTTP layer in
`server_controller_http.go` which lets us fanout logins to multiple tenants. In
the case of a single tenant cluster, this simply means that the
`multitenant-session` cookie contains only a single session, and `tenant` is
set to `system`. However, we run into problems during `/logout` calls because
our routing assumes that if you have a `tenant` cookie set that it should *not*
fanout and simply proxy the request to the correct tenant. In the special case
of Logout, this causes a problem because the tenant's Logout handler only
clears the `session` cookie and not the new `multitenant-session` and `tenant`
cookies.

Since these new cookies are owned by the code in `server_controller_http.go`
and because we don't claim to support per-tenant logout operations yet (#92855)
this commit enforces a fanout on *all* HTTP calls to `/logout`. This ensures
that sessions are cleared in the SQL layer. Then we also make sure to clear the
`session` cookie as well in case this code runs with a legacy session.

Epic: CRDB-12100

Release note: None
  • Loading branch information
dhartunian committed Feb 21, 2023
1 parent 8124f66 commit c82358c
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 3 deletions.
25 changes: 25 additions & 0 deletions pkg/server/authentication_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,31 @@ func TestAuthenticationAPIUserLogin(t *testing.T) {
}
}

func TestLogoutClearsCookies(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
s, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(context.Background())
ts := s.(*TestServer)

// Log in.
authHTTPClient, _, err := ts.getAuthenticatedHTTPClientAndCookie(authenticatedUserName(), true)
require.NoError(t, err)

// Log out.
resp, err := authHTTPClient.Get(ts.AdminURL() + logoutPath)
require.NoError(t, err)
defer resp.Body.Close()

cookies := resp.Cookies()
cNames := make([]string, len(cookies))
for i, c := range cookies {
require.Equal(t, "", c.Value)
cNames[i] = c.Name
}
require.ElementsMatch(t, cNames, []string{SessionCookieName, MultitenantSessionCookieName, TenantSelectCookieName})
}

func TestLogout(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
Expand Down
20 changes: 17 additions & 3 deletions pkg/server/server_controller_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,17 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) {
case loginPath, DemoLoginPath:
c.attemptLoginToAllTenants().ServeHTTP(w, r)
return
case logoutPath:
c.attemptLogoutFromAllTenants().ServeHTTP(w, r)
return
}
}
// Since we do not support per-tenant logout until
// https://github.com/cockroachdb/cockroach/issues/92855
// is completed, we should always fanout a logout
// request in order to clear the multi-tenant session
// cookies properly.
if r.URL.Path == logoutPath {
c.attemptLogoutFromAllTenants().ServeHTTP(w, r)
return
}
s, err := c.getServer(ctx, tenantName)
if err != nil {
log.Warningf(ctx, "unable to find server for tenant %q: %v", tenantName, err)
Expand Down Expand Up @@ -291,6 +297,14 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler {
Expires: timeutil.Unix(0, 0),
}
http.SetCookie(w, &cookie)
cookie = http.Cookie{
Name: SessionCookieName,
Value: "",
Path: "/",
HttpOnly: false,
Expires: timeutil.Unix(0, 0),
}
http.SetCookie(w, &cookie)
if r.Header.Get(AcceptHeader) == JSONContentType {
w.Header().Add(ContentTypeHeader, JSONContentType)
_, err = w.Write([]byte("{}"))
Expand Down

0 comments on commit c82358c

Please sign in to comment.