From c32965af067cb2583a09fe62241ba221e67ec18b Mon Sep 17 00:00:00 2001 From: Tor Colvin Date: Fri, 14 Mar 2025 08:13:44 -0400 Subject: [PATCH] [3.2.6 backport] CBG-4694B do not return invalid sessions with GET --- auth/session.go | 15 ++++++++++++--- auth/session_test.go | 3 +-- docs/api/paths/admin/db-_session.yaml | 2 +- rest/session_api.go | 26 ++++++++------------------ rest/session_test.go | 23 ++++++++++++++++++++--- rest/utilities_testing.go | 7 +++++++ 6 files changed, 49 insertions(+), 27 deletions(-) diff --git a/auth/session.go b/auth/session.go index 6ca022e46d..21ec836e2c 100644 --- a/auth/session.go +++ b/auth/session.go @@ -114,15 +114,24 @@ func (auth *Authenticator) CreateSession(ctx context.Context, user User, ttl tim return session, nil } +// GetSession returns a session by ID. Return a not found error if the session is not found, or is invalid. func (auth *Authenticator) GetSession(sessionID string) (*LoginSession, error) { var session LoginSession _, err := auth.datastore.Get(auth.DocIDForSession(sessionID), &session) if err != nil { - if base.IsDocNotFoundError(err) { - err = nil - } return nil, err } + user, err := auth.GetUser(session.Username) + if err != nil { + return nil, err + } + if user == nil { + return nil, base.ErrNotFound + } + if session.SessionUUID != user.GetSessionUUID() { + return nil, base.ErrNotFound + } + return &session, nil } diff --git a/auth/session_test.go b/auth/session_test.go index cbb575b571..fc69da4d34 100644 --- a/auth/session_test.go +++ b/auth/session_test.go @@ -91,10 +91,9 @@ func TestDeleteSession(t *testing.T) { assert.NoError(t, dataStore.Set(auth.DocIDForSession(mockSession.ID), noSessionExpiry, nil, mockSession)) assert.NoError(t, auth.DeleteSession(ctx, mockSession.ID, "")) - // Just to verify the session has been deleted gracefully. session, err := auth.GetSession(mockSession.ID) assert.Nil(t, session) - assert.NoError(t, err) + base.RequireDocNotFoundError(t, err) } // Coverage for MakeSessionCookie. The MakeSessionCookie should create a cookie diff --git a/docs/api/paths/admin/db-_session.yaml b/docs/api/paths/admin/db-_session.yaml index 1776d97e9b..2eca1dc5c2 100644 --- a/docs/api/paths/admin/db-_session.yaml +++ b/docs/api/paths/admin/db-_session.yaml @@ -40,7 +40,7 @@ post: description: User name to generate the session for. type: string ttl: - description: Time until the session expires. Uses default value of 24 hours if left blank. + description: Time until the session expires. Uses default value of 24 hours if left blank. This value must be greater or equal to 1. type: integer responses: '200': diff --git a/rest/session_api.go b/rest/session_api.go index ababefb4d6..68b43d2a2f 100644 --- a/rest/session_api.go +++ b/rest/session_api.go @@ -242,14 +242,9 @@ func (h *handler) getUserSession() error { h.assertAdminOnly() session, err := h.db.Authenticator(h.ctx()).GetSession(h.PathVar("sessionid")) - - if session == nil { - if err == nil { - err = kNotFoundError - } + if err != nil { return err } - return h.respondWithSessionInfoForSession(session) } @@ -292,22 +287,17 @@ func (h *handler) deleteUserSessionWithValidation(sessionId string, userName str // Validate that the session being deleted belongs to the user. This adds some // overhead - for user-agnostic session deletion should use deleteSession session, getErr := h.db.Authenticator(h.ctx()).GetSession(sessionId) - if session == nil { - if getErr == nil { - getErr = kNotFoundError - } + if getErr != nil { return getErr } - if getErr == nil { - if session.Username == userName { - delErr := h.db.Authenticator(h.ctx()).DeleteSession(h.ctx(), sessionId, userName) - if delErr != nil { - return delErr - } - } else { - return kNotFoundError + if session.Username == userName { + delErr := h.db.Authenticator(h.ctx()).DeleteSession(h.ctx(), sessionId, userName) + if delErr != nil { + return delErr } + } else { + return kNotFoundError } return nil } diff --git a/rest/session_test.go b/rest/session_test.go index 0ab76ac888..f172452365 100644 --- a/rest/session_test.go +++ b/rest/session_test.go @@ -447,7 +447,7 @@ func TestSessionAPI(t *testing.T) { // Attempt to GET the deleted session and make sure it's not found response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user1sessions[0]), "") - RequireStatus(t, response, 404) + RequireNotFoundError(t, response) // 2. DELETE a session with user validation response = rt.SendAdminRequest("DELETE", fmt.Sprintf("/db/_user/%s/_session/%s", "user1", user1sessions[1]), "") @@ -455,11 +455,11 @@ func TestSessionAPI(t *testing.T) { // Attempt to GET the deleted session and make sure it's not found response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user1sessions[1]), "") - RequireStatus(t, response, 404) + RequireNotFoundError(t, response) // 3. DELETE a session not belonging to the user (should fail) response = rt.SendAdminRequest("DELETE", fmt.Sprintf("/db/_user/%s/_session/%s", "user1", user2sessions[0]), "") - RequireStatus(t, response, 404) + RequireNotFoundError(t, response) // GET the session and make sure it still exists response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user2sessions[0]), "") @@ -469,6 +469,23 @@ func TestSessionAPI(t *testing.T) { response = rt.SendAdminRequest("DELETE", "/db/_user/user2/_session", "") RequireStatus(t, response, 200) + // Validate that all sessions were deleted + for i := range 5 { + response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user2sessions[i]), "") + RequireNotFoundError(t, response) + } + + // 5. DELETE sessions when password is changed + // Change password for user3 + response = rt.SendAdminRequest("PUT", "/db/_user/user3", `{"password":"5678"}`) + RequireStatus(t, response, 200) + + // Validate that all sessions were deleted + for i := range 5 { + response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user3sessions[i]), "") + RequireNotFoundError(t, response) + } + // DELETE the users RequireStatus(t, rt.SendAdminRequest("DELETE", "/db/_user/user1", ""), 200) RequireStatus(t, rt.SendAdminRequest("GET", "/db/_user/user1", ""), 404) diff --git a/rest/utilities_testing.go b/rest/utilities_testing.go index 5e668adf30..b0ec765200 100644 --- a/rest/utilities_testing.go +++ b/rest/utilities_testing.go @@ -2827,3 +2827,10 @@ func (sc *ServerContext) reloadDatabaseWithConfigLoadFromBucket(nonContextStruct defer sc.lock.Unlock() return sc._reloadDatabaseWithConfig(nonContextStruct.Ctx, config, true, true) } + +func RequireNotFoundError(t *testing.T, response *TestResponse) { + RequireStatus(t, response, http.StatusNotFound) + var body db.Body + require.NoError(t, base.JSONUnmarshal(response.Body.Bytes(), &body)) + require.Equal(t, db.Body{"error": "not_found", "reason": "missing"}, body) +}