diff --git a/pkg/server/authentication.go b/pkg/server/authentication.go index 1cfc4f149fc9..6e4748062cfa 100644 --- a/pkg/server/authentication.go +++ b/pkg/server/authentication.go @@ -58,9 +58,6 @@ const ( secretLength = 16 // SessionCookieName is the name of the cookie used for HTTP auth. SessionCookieName = "session" - // MultiTenantSessionCookieName is the name of the cookie used for HTTP auth - // when the cluster is multitenant. - MultitenantSessionCookieName = "multitenant-session" // DemoLoginPath is the demo shell auto-login URL. DemoLoginPath = "/demologin" ) @@ -804,69 +801,82 @@ func createAggregatedSessionCookieValue(sessionCookieValue []sessionCookieValue) // findAndDecodeSessionCookie looks for multitenant-session and session cookies // in the cookies slice. If they are found the value will need to be processed if -// it is a multitenant-session cookie (see findSessionCookieValue for details) -// and then decoded. Multitenant-session cookies are prioritized over session cookies. -// If neither cookie types are found or there is an error in decoding or processing, -// the function will return an error. +// it is a multitenant-session cookie (see findSessionCookieValueForTenant for details) +// and then decoded. If there is an error in decoding or processing, the function +// will return an error. func findAndDecodeSessionCookie( ctx context.Context, st *cluster.Settings, cookies []*http.Cookie, ) (*serverpb.SessionCookie, error) { - // Validate the returned cookie. found := false - var cookie *serverpb.SessionCookie - var err error - var sessionCookies []*http.Cookie - // Prioritize multitenant session cookies. - for _, c := range cookies { - if c.Name == MultitenantSessionCookieName { - sessionCookies = append([]*http.Cookie{c}, sessionCookies...) - } else if c.Name == SessionCookieName { - sessionCookies = append(sessionCookies, c) + var sessionCookie *serverpb.SessionCookie + tenantSelectCookieVal := findTenantSelectCookieValue(cookies) + for _, cookie := range cookies { + if cookie.Name != SessionCookieName { + continue } - } - for _, sessionCookie := range sessionCookies { found = true - // This case is if the multitenant session cookie is set. - if sessionCookie.Name == MultitenantSessionCookieName { - mtSessionVal, err := findSessionCookieValue(st, cookies) - if err != nil { - return cookie, apiInternalError(ctx, err) - } - if mtSessionVal != "" { - sessionCookie.Value = mtSessionVal - } + mtSessionVal, err := findSessionCookieValueForTenant( + st, + cookie, + tenantSelectCookieVal) + if err != nil { + return sessionCookie, apiInternalError(ctx, err) + } + if mtSessionVal != "" { + cookie.Value = mtSessionVal } - cookie, err = decodeSessionCookie(sessionCookie) + sessionCookie, err = decodeSessionCookie(cookie) if err != nil { // Multiple cookies with the same name may be included in the // header. We continue searching even if we find a matching - // name with an invalid value + // name with an invalid value. log.Infof(ctx, "found a matching cookie that failed decoding: %v", err) found = false continue } break } - if err != nil || !found { + if !found { return nil, http.ErrNoCookie } - return cookie, nil + return sessionCookie, nil } -// findSessionCookieValue finds the encoded session in an aggregated -// session cookie value established in multi-tenant clusters. If the -// method cannot find a match between the tenant name and session -// or cannot find the tenant cookie value, it will return an empty -// string to indicate this. -// e.g. tenant cookie value is "system" and multitenant-session cookie -// value is "abcd1234,system,efgh5678,app" the output will be "abcd1234". -func findSessionCookieValue(st *cluster.Settings, cookies []*http.Cookie) (string, error) { - if mtSessionStr := findMultitenantSessionCookieValue(cookies); mtSessionStr != "" { - tenantName := findTenantCookieValue(cookies) +// findSessionCookieValueForTenant finds the encoded session in the provided +// aggregated session cookie value established in multi-tenant clusters that's +// associated with the provided tenant name. If an empty tenant name is provided, +// we default to the defaultTenantSelect cluster setting value. +// +// If the method cannot find a match between the tenant name and session, or +// if the provided session cookie is nil, it will return an empty string. +// +// e.g. tenant name is "system" and session cookie's value is +// "abcd1234,system,efgh5678,app" the output will be "abcd1234". +// +// In the case of legacy session cookies, where tenant names are not encoded +// into the cookie value, we assume that the session belongs to defaultTenantSelect. +// Note that these legacy session cookies only contained a single session string +// as the cookie's value. +func findSessionCookieValueForTenant( + st *cluster.Settings, sessionCookie *http.Cookie, tenantName string, +) (string, error) { + if sessionCookie == nil { + return "", nil + } + if mtSessionStr := sessionCookie.Value; sessionCookie.Value != "" { + sessionSlice := strings.Split(mtSessionStr, ",") + if len(sessionSlice) == 1 { + // If no separator was found in the cookie value, this is likely + // a cookie from a previous CRDB version where the cookie value + // contained a single session string without any tenant names encoded. + // To maintain backwards compatibility, assume this session belongs + // to the default tenant. In this case, the entire cookie value is + // the session string. + return mtSessionStr, nil + } if tenantName == "" { tenantName = defaultTenantSelect.Get(&st.SV) } - sessionSlice := strings.Split(mtSessionStr, ",") var encodedSession string for idx, val := range sessionSlice { if val == tenantName && idx > 0 { @@ -881,10 +891,10 @@ func findSessionCookieValue(st *cluster.Settings, cookies []*http.Cookie) (strin return "", nil } -// findTenantCookieValue iterates through all request cookies in order -// to find the value of the tenant cookie. If the tenant cookie is not -// found, it returns the empty string. -func findTenantCookieValue(cookies []*http.Cookie) string { +// findTenantSelectCookieValue iterates through all request cookies in order +// to find the value of the tenant select cookie. If the tenant select cookie +// is not found, it returns the empty string. +func findTenantSelectCookieValue(cookies []*http.Cookie) string { for _, c := range cookies { if c.Name == TenantSelectCookieName { return c.Value @@ -892,15 +902,3 @@ func findTenantCookieValue(cookies []*http.Cookie) string { } return "" } - -// findMultitenantSessionCookieValue iterates through all request -// cookies to find the value of the multitenant-session cookie. -// If not found, it return an empty string. -func findMultitenantSessionCookieValue(cookies []*http.Cookie) string { - for _, c := range cookies { - if c.Name == MultitenantSessionCookieName { - return c.Value - } - } - return "" -} diff --git a/pkg/server/authentication_test.go b/pkg/server/authentication_test.go index 08bf918d0ff5..06752be5c091 100644 --- a/pkg/server/authentication_test.go +++ b/pkg/server/authentication_test.go @@ -593,7 +593,7 @@ func TestLogoutClearsCookies(t *testing.T) { require.Equal(t, "", c.Value) cNames[i] = c.Name } - require.ElementsMatch(t, cNames, []string{SessionCookieName, MultitenantSessionCookieName, TenantSelectCookieName}) + require.ElementsMatch(t, cNames, []string{SessionCookieName, TenantSelectCookieName}) } func TestLogout(t *testing.T) { @@ -906,66 +906,78 @@ func TestFindSessionCookieValue(t *testing.T) { defer log.Scope(t).Close(t) normalSessionStr := "abcd1234,system,efgh5678,app" tests := []struct { - name string - cookieArg []*http.Cookie - resExpected string - errorExpected bool + name string + sessionCookie *http.Cookie + tenantSelectValue string + resExpected string + errorExpected bool }{ - {"standard args", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + { + name: "standard args", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - { - Name: TenantSelectCookieName, - Value: "system", - Path: "/", - }, - }, "abcd1234", false}, - {"no multitenant session cookie", []*http.Cookie{ - { - Name: TenantSelectCookieName, - Value: "system", - Path: "/", - }, - }, "", false}, - {"no tenant cookie", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + tenantSelectValue: "system", + resExpected: "abcd1234", + errorExpected: false, + }, + { + name: "no multitenant session cookie", + sessionCookie: nil, + tenantSelectValue: "system", + resExpected: "", + errorExpected: false, + }, + { + name: "no tenant cookie", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - }, "abcd1234", false}, - {"empty string tenant cookie", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + resExpected: "abcd1234", + errorExpected: false, + }, + { + name: "empty string tenant cookie", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - { - Name: TenantSelectCookieName, - Value: "", - Path: "/", - }, - }, "abcd1234", false}, - {"no tenant name match", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + tenantSelectValue: "", + resExpected: "abcd1234", + errorExpected: false, + }, + { + name: "no tenant name match", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - { - Name: TenantSelectCookieName, - Value: "app2", + tenantSelectValue: "app2", + resExpected: "", + errorExpected: true, + }, + { + name: "legacy session cookie", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, + Value: "aaskjhf218==", Path: "/", }, - }, "", true}, + tenantSelectValue: "", + resExpected: "aaskjhf218==", + errorExpected: false, + }, } for _, test := range tests { t.Run(fmt.Sprintf("find-session-cookie/%s", test.name), func(t *testing.T) { st := cluster.MakeClusterSettings() - res, err := findSessionCookieValue(st, test.cookieArg) + res, err := findSessionCookieValueForTenant(st, test.sessionCookie, test.tenantSelectValue) require.Equal(t, test.resExpected, res) require.Equal(t, test.errorExpected, err != nil) }) diff --git a/pkg/server/server_controller_http.go b/pkg/server/server_controller_http.go index 8933c6326c00..751a2c0b0eaa 100644 --- a/pkg/server/server_controller_http.go +++ b/pkg/server/server_controller_http.go @@ -72,7 +72,7 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) { log.Warningf(ctx, "unable to find server for tenant %q: %v", tenantName, err) // Clear session and tenant cookies since it appears they reference invalid state. http.SetCookie(w, &http.Cookie{ - Name: MultitenantSessionCookieName, + Name: SessionCookieName, Value: "", Path: "/", HttpOnly: true, @@ -193,7 +193,7 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler { if len(tenantNameToSetCookieSlice) > 0 { sessionsStr := createAggregatedSessionCookieValue(tenantNameToSetCookieSlice) cookie := http.Cookie{ - Name: MultitenantSessionCookieName, + Name: SessionCookieName, Value: sessionsStr, Path: "/", HttpOnly: false, @@ -282,15 +282,7 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler { } // Clear session and tenant cookies after all logouts have completed. cookie := http.Cookie{ - Name: MultitenantSessionCookieName, - Value: "", - Path: "/", - HttpOnly: true, - Expires: timeutil.Unix(0, 0), - } - http.SetCookie(w, &cookie) - cookie = http.Cookie{ - Name: TenantSelectCookieName, + Name: SessionCookieName, Value: "", Path: "/", HttpOnly: false, @@ -298,7 +290,7 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler { } http.SetCookie(w, &cookie) cookie = http.Cookie{ - Name: SessionCookieName, + Name: TenantSelectCookieName, Value: "", Path: "/", HttpOnly: false, diff --git a/pkg/ui/workspaces/db-console/src/redux/cookies.ts b/pkg/ui/workspaces/db-console/src/redux/cookies.ts index d29a12f5dd0e..972dbef21824 100644 --- a/pkg/ui/workspaces/db-console/src/redux/cookies.ts +++ b/pkg/ui/workspaces/db-console/src/redux/cookies.ts @@ -8,7 +8,7 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. -export const MULTITENANT_SESSION_COOKIE_NAME = "multitenant-session"; +export const MULTITENANT_SESSION_COOKIE_NAME = "session"; export const getAllCookies = (): Map => { const cookieMap: Map = new Map(); @@ -26,7 +26,7 @@ export const getCookieValue = (cookieName: string): string => { return cookies.get(cookieName) || null; }; -// selectTenantsFromMultitenantSessionCookie formats the multitenant-session +// selectTenantsFromMultitenantSessionCookie formats the session // cookie value and returns only the tenant names. export const selectTenantsFromMultitenantSessionCookie = (): string[] => { const cookies = getAllCookies(); diff --git a/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx b/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx index c55b07b9865a..53a9ed880be7 100644 --- a/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx +++ b/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx @@ -21,7 +21,7 @@ jest.mock("src/redux/cookies", () => ({ })); describe("TenantDropdown", () => { - it("returns null if there are no tenants in the multitenant-session cookie", () => { + it("returns null if there are no tenants in the session cookie", () => { ( selectTenantsFromMultitenantSessionCookie as jest.MockedFn< typeof selectTenantsFromMultitenantSessionCookie @@ -30,7 +30,7 @@ describe("TenantDropdown", () => { const wrapper = shallow(); expect(wrapper.isEmptyRender()); }); - it("returns a dropdown list of tenant options if there are tenant in the multitenant-session cookie", () => { + it("returns a dropdown list of tenant options if there are tenant in the session cookie", () => { ( selectTenantsFromMultitenantSessionCookie as jest.MockedFn< typeof selectTenantsFromMultitenantSessionCookie