diff --git a/config/options.go b/config/options.go index 0aa8bc33..a6d3b26b 100644 --- a/config/options.go +++ b/config/options.go @@ -27,12 +27,12 @@ func NewConfig(options ...ConfigOption) *models.Config { DisabledPaths: []string{}, Session: models.SessionConfig{ CookieName: "authula.session_token", - ExpiresIn: time.Hour * 24 * 7, // 7 days by default - UpdateAge: time.Hour * 24, // 24 hours update interval + ExpiresIn: time.Hour * 24 * 7, + UpdateAge: time.Hour * 24, Secure: false, HttpOnly: true, SameSite: "lax", - CookieMaxAge: 24 * time.Hour, + CookieMaxAge: time.Hour * 24 * 7, AutoCleanup: false, CleanupInterval: time.Minute, MaxSessionsPerUser: 5, @@ -184,8 +184,11 @@ func WithSession(config models.SessionConfig) ConfigOption { if config.UpdateAge != 0 { c.Session.UpdateAge = config.UpdateAge } - if config.CookieMaxAge != 0 { + switch { + case config.CookieMaxAge != 0: c.Session.CookieMaxAge = config.CookieMaxAge + case config.ExpiresIn != 0: + c.Session.CookieMaxAge = config.ExpiresIn } c.Session.Secure = config.Secure c.Session.HttpOnly = config.HttpOnly diff --git a/config/options_test.go b/config/options_test.go index 8c26d2e5..41e75a85 100644 --- a/config/options_test.go +++ b/config/options_test.go @@ -2,6 +2,7 @@ package config import ( "testing" + "time" "github.com/Authula/authula/models" ) @@ -14,6 +15,8 @@ func (l *customLogger) Warn(msg string, args ...any) {} func (l *customLogger) Error(msg string, args ...any) {} func TestWithLoggerConfiguresLevelAndCustomLogger(t *testing.T) { + t.Parallel() + logger := &customLogger{} config := NewConfig(WithLogger(models.LoggerConfig{ @@ -29,3 +32,39 @@ func TestWithLoggerConfiguresLevelAndCustomLogger(t *testing.T) { t.Fatal("expected custom logger to be configured") } } + +func TestWithSessionCookieMaxAge(t *testing.T) { + tests := []struct { + name string + session models.SessionConfig + wantCookieMaxAge time.Duration + }{ + { + name: "defaults keep the cookie alive for the whole session lifetime", + session: models.SessionConfig{}, + wantCookieMaxAge: time.Hour * 24 * 7, + }, + { + name: "follows a custom expires_in when cookie_max_age is unset", + session: models.SessionConfig{ExpiresIn: 2 * time.Hour}, + wantCookieMaxAge: 2 * time.Hour, + }, + { + name: "explicit cookie_max_age wins over expires_in", + session: models.SessionConfig{ExpiresIn: 2 * time.Hour, CookieMaxAge: time.Hour}, + wantCookieMaxAge: time.Hour, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + config := NewConfig(WithSession(tt.session)) + + if config.Session.CookieMaxAge != tt.wantCookieMaxAge { + t.Fatalf("expected cookie max age %s, got %s", tt.wantCookieMaxAge, config.Session.CookieMaxAge) + } + }) + } +} diff --git a/core/api.go b/core/api.go index e4447b45..14b39ced 100644 --- a/core/api.go +++ b/core/api.go @@ -10,8 +10,8 @@ import ( ) type CoreAPI interface { - GetMe(ctx context.Context, userID string) (*types.GetMeResult, error) - SignOut(ctx context.Context, userID string, sessionID *string, signOutAll *bool) (*types.SignOutResult, error) + GetMe(ctx context.Context, userID string, sessionID *string) (*types.GetMeResult, error) + SignOut(ctx context.Context, userID string, currentSessionID *string, requestedSessionID *string, signOutAll *bool) (*types.SignOutResult, error) } type coreAPI struct { @@ -25,12 +25,12 @@ func NewCoreAPI(logger models.Logger, userService services.UserService, sessionS } } -func (api *coreAPI) GetMe(ctx context.Context, userID string) (*types.GetMeResult, error) { - return api.useCases.GetMeUseCase.GetMe(ctx, userID) +func (api *coreAPI) GetMe(ctx context.Context, userID string, sessionID *string) (*types.GetMeResult, error) { + return api.useCases.GetMeUseCase.GetMe(ctx, userID, sessionID) } -func (api *coreAPI) SignOut(ctx context.Context, userID string, sessionID *string, signOutAll *bool) (*types.SignOutResult, error) { - return api.useCases.SignOutUseCase.SignOut(ctx, userID, sessionID, signOutAll) +func (api *coreAPI) SignOut(ctx context.Context, userID string, currentSessionID *string, requestedSessionID *string, signOutAll *bool) (*types.SignOutResult, error) { + return api.useCases.SignOutUseCase.SignOut(ctx, userID, currentSessionID, requestedSessionID, signOutAll) } func BuildUseCases(logger models.Logger, userService services.UserService, sessionService services.SessionService) *usecases.UseCases { diff --git a/core/errors/errors.go b/core/errors/errors.go index 32682270..e774d293 100644 --- a/core/errors/errors.go +++ b/core/errors/errors.go @@ -22,6 +22,9 @@ var ( // Token Errors ErrTokenRequired = errors.New("token is required") + + // Session Errors + ErrNoSessionToSignOut = errors.New("no session to sign out: provide session_id or sign_out_all") ) func HandleError(err error, reqCtx *models.RequestContext) { diff --git a/core/handlers/get_me_handler.go b/core/handlers/get_me_handler.go index a43a0872..eea09530 100644 --- a/core/handlers/get_me_handler.go +++ b/core/handlers/get_me_handler.go @@ -17,7 +17,12 @@ func (h *GetMeHandler) Handle() http.HandlerFunc { ctx := r.Context() reqCtx, _ := models.GetRequestContext(ctx) - result, err := h.UseCase.GetMe(ctx, reqCtx.Actor.ID) + var sessionID *string + if id, ok := reqCtx.Values[models.ContextSessionID.String()].(string); ok && id != "" { + sessionID = &id + } + + result, err := h.UseCase.GetMe(ctx, reqCtx.Actor.ID, sessionID) if err != nil { reqCtx.SetJSONResponse(http.StatusInternalServerError, map[string]any{ "message": err.Error(), diff --git a/core/handlers/sign_out_handler.go b/core/handlers/sign_out_handler.go index b0b99117..4166ab35 100644 --- a/core/handlers/sign_out_handler.go +++ b/core/handlers/sign_out_handler.go @@ -1,8 +1,10 @@ package handlers import ( + "errors" "net/http" + coreerrors "github.com/Authula/authula/core/errors" "github.com/Authula/authula/core/types" "github.com/Authula/authula/core/usecases" "github.com/Authula/authula/models" @@ -20,7 +22,6 @@ func (h *SignOutHandler) Handle() http.HandlerFunc { var request types.SignOutRequest if err := util.ParseJSON(r, &request); err != nil { - // If no body is provided, we'll default to using an empty request. request = types.SignOutRequest{} } if err := request.Validate(); err != nil { @@ -29,8 +30,17 @@ func (h *SignOutHandler) Handle() http.HandlerFunc { return } - result, err := h.UseCase.SignOut(ctx, reqCtx.Actor.ID, request.SessionID, request.SignOutAll) + var currentSessionID *string + if id, ok := reqCtx.Values[models.ContextSessionID.String()].(string); ok && id != "" { + currentSessionID = &id + } + + result, err := h.UseCase.SignOut(ctx, reqCtx.Actor.ID, currentSessionID, request.SessionID, request.SignOutAll) if err != nil { + if errors.Is(err, coreerrors.ErrNotFound) || errors.Is(err, coreerrors.ErrForbidden) || errors.Is(err, coreerrors.ErrNoSessionToSignOut) { + coreerrors.HandleError(err, reqCtx) + return + } reqCtx.SetJSONResponse(http.StatusInternalServerError, map[string]any{ "message": "failed to sign out", }) @@ -38,7 +48,9 @@ func (h *SignOutHandler) Handle() http.HandlerFunc { return } - reqCtx.Values[models.ContextAuthSignOut.String()] = true + if result.ClearedCurrentSession { + reqCtx.Values[models.ContextAuthSignOut.String()] = true + } reqCtx.SetJSONResponse(http.StatusOK, &types.SignOutResponse{ Message: result.Message, diff --git a/core/handlers/sign_out_handler_test.go b/core/handlers/sign_out_handler_test.go new file mode 100644 index 00000000..982cdbdc --- /dev/null +++ b/core/handlers/sign_out_handler_test.go @@ -0,0 +1,164 @@ +package handlers_test + +import ( + "errors" + "net/http" + "testing" + + "github.com/stretchr/testify/mock" + + "github.com/Authula/authula/core/handlers" + "github.com/Authula/authula/core/usecases" + internaltests "github.com/Authula/authula/internal/tests" + "github.com/Authula/authula/models" +) + +func TestSignOutHandler(t *testing.T) { + t.Parallel() + + const ( + userID = "user-1" + currentSessionID = "session-current" + requestedSessionID = "session-requested" + ) + + tests := []struct { + name string + body []byte + currentSessionID *string + setup func(sessionService *internaltests.MockSessionService) + expectedStatus int + expectedMessage string + expectSignOut bool + }{ + { + name: "no body signs out the current session and clears the cookie", + currentSessionID: new(currentSessionID), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("Delete", mock.Anything, currentSessionID).Return(nil).Once() + }, + expectedStatus: http.StatusOK, + expectSignOut: true, + }, + { + name: "signing out another own session by id does not clear the cookie", + body: []byte(`{"session_id":"` + requestedSessionID + `"}`), + currentSessionID: new(currentSessionID), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("GetByID", mock.Anything, requestedSessionID).Return(&models.Session{ID: requestedSessionID, UserID: userID}, nil).Once() + sessionService.On("Delete", mock.Anything, requestedSessionID).Return(nil).Once() + }, + expectedStatus: http.StatusOK, + expectSignOut: false, + }, + { + name: "signing out the current session by id clears the cookie", + body: []byte(`{"session_id":"` + currentSessionID + `"}`), + currentSessionID: new(currentSessionID), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("GetByID", mock.Anything, currentSessionID).Return(&models.Session{ID: currentSessionID, UserID: userID}, nil).Once() + sessionService.On("Delete", mock.Anything, currentSessionID).Return(nil).Once() + }, + expectedStatus: http.StatusOK, + expectSignOut: true, + }, + { + name: "sign out all clears the cookie", + body: []byte(`{"sign_out_all":true}`), + currentSessionID: new(currentSessionID), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("DeleteAllByUserID", mock.Anything, userID).Return(nil).Once() + }, + expectedStatus: http.StatusOK, + expectSignOut: true, + }, + { + name: "session owned by another user returns forbidden", + body: []byte(`{"session_id":"` + requestedSessionID + `"}`), + currentSessionID: new(currentSessionID), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("GetByID", mock.Anything, requestedSessionID).Return(&models.Session{ID: requestedSessionID, UserID: "user-2"}, nil).Once() + }, + expectedStatus: http.StatusForbidden, + expectedMessage: "forbidden", + }, + { + name: "unknown session id returns not found", + body: []byte(`{"session_id":"` + requestedSessionID + `"}`), + currentSessionID: new(currentSessionID), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("GetByID", mock.Anything, requestedSessionID).Return((*models.Session)(nil), nil).Once() + }, + expectedStatus: http.StatusNotFound, + expectedMessage: "not found", + }, + { + name: "unexpected error returns internal server error", + currentSessionID: new(currentSessionID), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("Delete", mock.Anything, currentSessionID).Return(errors.New("db down")).Once() + }, + expectedStatus: http.StatusInternalServerError, + expectedMessage: "failed to sign out", + }, + { + name: "no current session and no body is rejected", + setup: func(sessionService *internaltests.MockSessionService) {}, + expectedStatus: http.StatusBadRequest, + expectedMessage: "no session to sign out: provide session_id or sign_out_all", + }, + { + name: "sign out all works without a current session", + body: []byte(`{"sign_out_all":true}`), + setup: func(sessionService *internaltests.MockSessionService) { + sessionService.On("DeleteAllByUserID", mock.Anything, userID).Return(nil).Once() + }, + expectedStatus: http.StatusOK, + expectSignOut: true, + }, + { + name: "empty session id is rejected", + body: []byte(`{"session_id":" "}`), + setup: func(sessionService *internaltests.MockSessionService) {}, + expectedStatus: http.StatusUnprocessableEntity, + expectedMessage: "session_id cannot be empty if provided", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + sessionService := &internaltests.MockSessionService{} + tt.setup(sessionService) + + handler := &handlers.SignOutHandler{ + UseCase: &usecases.SignOutUseCase{ + Logger: &internaltests.MockLogger{}, + SessionService: sessionService, + }, + } + + req, w, reqCtx := internaltests.NewHandlerRequest(t, http.MethodPost, "/sign-out", tt.body, new(userID)) + if tt.currentSessionID != nil { + reqCtx.Values[models.ContextSessionID.String()] = *tt.currentSessionID + } + + handler.Handle()(w, req) + + if reqCtx.ResponseStatus != tt.expectedStatus { + t.Fatalf("expected status %d, got %d", tt.expectedStatus, reqCtx.ResponseStatus) + } + if tt.expectedMessage != "" { + internaltests.AssertErrorMessage(t, reqCtx, tt.expectedStatus, tt.expectedMessage) + } + + signedOut, _ := reqCtx.Values[models.ContextAuthSignOut.String()].(bool) + if signedOut != tt.expectSignOut { + t.Fatalf("expected auth.sign_out flag %v, got %v", tt.expectSignOut, signedOut) + } + + sessionService.AssertExpectations(t) + }) + } +} diff --git a/core/openapi/openapi_docs.go b/core/openapi/openapi_docs.go index 458e8880..6359fb16 100644 --- a/core/openapi/openapi_docs.go +++ b/core/openapi/openapi_docs.go @@ -24,7 +24,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/sign-out", openapi.WithOperationID("signOut"), openapi.WithSummary("Sign out"), - openapi.WithDescription("Signs out the authenticated user. Optionally sign out a specific session or all sessions."), + openapi.WithDescription("Signs out the authenticated user. With no body, revokes the session that authenticated the request (400 if the request carries no session id). Pass session_id to revoke one of your own sessions (404 if it does not exist, 403 if it belongs to another user), or sign_out_all to revoke every session."), openapi.WithTags("Core"), openapi.WithRequest(&types.SignOutRequest{}), openapi.WithResponseStatus(http.StatusOK, &types.SignOutResponse{}), diff --git a/core/types/api.go b/core/types/api.go index f45640ba..c629a72f 100644 --- a/core/types/api.go +++ b/core/types/api.go @@ -30,7 +30,8 @@ func (req *SignOutRequest) Validate() error { } type SignOutResult struct { - Message string + Message string + ClearedCurrentSession bool } type SignOutResponse struct { diff --git a/core/usecases/get_me_usecase.go b/core/usecases/get_me_usecase.go index 753a7466..d1faaa1c 100644 --- a/core/usecases/get_me_usecase.go +++ b/core/usecases/get_me_usecase.go @@ -15,7 +15,7 @@ type GetMeUseCase struct { SessionService services.SessionService } -func (uc *GetMeUseCase) GetMe(ctx context.Context, userID string) (*types.GetMeResult, error) { +func (uc *GetMeUseCase) GetMe(ctx context.Context, userID string, sessionID *string) (*types.GetMeResult, error) { var ( wg sync.WaitGroup user *models.User @@ -32,9 +32,13 @@ func (uc *GetMeUseCase) GetMe(ctx context.Context, userID string) (*types.GetMeR }) wg.Go(func() { - session, sessErr = uc.SessionService.GetByUserID(ctx, userID) + if sessionID != nil && *sessionID != "" { + session, sessErr = uc.SessionService.GetByID(ctx, *sessionID) + } else { + session, sessErr = uc.SessionService.GetByUserID(ctx, userID) + } if sessErr != nil { - uc.Logger.Error("failed to get session by user ID: %v", sessErr) + uc.Logger.Error("failed to get session: %v", sessErr) } }) diff --git a/core/usecases/get_me_usecase_test.go b/core/usecases/get_me_usecase_test.go index f6886b0b..188eb910 100644 --- a/core/usecases/get_me_usecase_test.go +++ b/core/usecases/get_me_usecase_test.go @@ -29,6 +29,7 @@ func TestGetMeUseCase(t *testing.T) { tests := []struct { name string + sessionID *string user *models.User userErr error session *models.Session @@ -37,7 +38,7 @@ func TestGetMeUseCase(t *testing.T) { wantErr string }{ { - name: "returns user and session when both lookups succeed", + name: "returns user and most recent session when no session id is known", user: expectedUser, session: expectedSession, want: &types.GetMeResult{ @@ -45,6 +46,23 @@ func TestGetMeUseCase(t *testing.T) { Session: expectedSession, }, }, + { + name: "returns the authenticating session when its id is known", + sessionID: new("session-123"), + user: expectedUser, + session: expectedSession, + want: &types.GetMeResult{ + User: expectedUser, + Session: expectedSession, + }, + }, + { + name: "returns session lookup error for a known session id", + sessionID: new("session-123"), + user: expectedUser, + sessionErr: errors.New("session lookup failed"), + wantErr: "session lookup failed", + }, { name: "returns user lookup error", userErr: errors.New("user lookup failed"), @@ -73,7 +91,11 @@ func TestGetMeUseCase(t *testing.T) { sessionService := &inttests.MockSessionService{} userService.On("GetByID", ctx, userID).Return(tt.user, tt.userErr).Once() - sessionService.On("GetByUserID", ctx, userID).Return(tt.session, tt.sessionErr).Once() + if tt.sessionID != nil { + sessionService.On("GetByID", ctx, *tt.sessionID).Return(tt.session, tt.sessionErr).Once() + } else { + sessionService.On("GetByUserID", ctx, userID).Return(tt.session, tt.sessionErr).Once() + } uc := &GetMeUseCase{ Logger: &inttests.MockLogger{}, @@ -81,7 +103,7 @@ func TestGetMeUseCase(t *testing.T) { SessionService: sessionService, } - result, err := uc.GetMe(ctx, userID) + result, err := uc.GetMe(ctx, userID, tt.sessionID) if tt.wantErr != "" { if err == nil { diff --git a/core/usecases/sign_out_usecase.go b/core/usecases/sign_out_usecase.go index 784c14fc..8306a8ef 100644 --- a/core/usecases/sign_out_usecase.go +++ b/core/usecases/sign_out_usecase.go @@ -3,6 +3,7 @@ package usecases import ( "context" + coreerrors "github.com/Authula/authula/core/errors" "github.com/Authula/authula/core/types" "github.com/Authula/authula/models" "github.com/Authula/authula/services" @@ -16,39 +17,60 @@ type SignOutUseCase struct { func (uc *SignOutUseCase) SignOut( ctx context.Context, userID string, - sessionID *string, + currentSessionID *string, + requestedSessionID *string, signOutAll *bool, ) (*types.SignOutResult, error) { - // If a specific session ID is provided, delete only that session - if sessionID != nil && *sessionID != "" { - if err := uc.SessionService.Delete(ctx, *sessionID); err != nil { - uc.Logger.Error("failed to delete session", "error", err, "session_id", *sessionID) - return nil, err - } - return &types.SignOutResult{Message: "signed out"}, nil - } - - // If signOutAll is true, delete all sessions for the user if signOutAll != nil && *signOutAll { if err := uc.SessionService.DeleteAllByUserID(ctx, userID); err != nil { uc.Logger.Error("failed to delete all sessions for user", "error", err, "user_id", userID) return nil, err } - return &types.SignOutResult{Message: "signed out from all sessions"}, nil + return &types.SignOutResult{Message: "signed out from all sessions", ClearedCurrentSession: true}, nil } - // Else fallback to deleting the most recent session for the user - if mostRecentSession, err := uc.SessionService.GetByUserID(ctx, userID); err != nil { - uc.Logger.Error("failed to get session for user", "error", err, "user_id", userID) - return nil, err - } else if mostRecentSession != nil { - if err := uc.SessionService.Delete(ctx, mostRecentSession.ID); err != nil { - uc.Logger.Error("failed to delete session", "error", err, "session_id", mostRecentSession.ID) + if requestedSessionID != nil && *requestedSessionID != "" { + return uc.signOutRequestedSession(ctx, userID, currentSessionID, *requestedSessionID) + } + + if currentSessionID != nil && *currentSessionID != "" { + if err := uc.SessionService.Delete(ctx, *currentSessionID); err != nil { + uc.Logger.Error("failed to delete current session", "error", err, "session_id", *currentSessionID) return nil, err } + return &types.SignOutResult{Message: "signed out", ClearedCurrentSession: true}, nil + } + + // The caller is authenticated but nothing identifies a session to revoke so + // therefore guessing would risk revoking the wrong session, so refuse. + return nil, coreerrors.ErrNoSessionToSignOut +} - return &types.SignOutResult{Message: "signed out"}, nil +func (uc *SignOutUseCase) signOutRequestedSession( + ctx context.Context, + userID string, + currentSessionID *string, + requestedSessionID string, +) (*types.SignOutResult, error) { + session, err := uc.SessionService.GetByID(ctx, requestedSessionID) + if err != nil { + uc.Logger.Error("failed to get session", "error", err, "session_id", requestedSessionID) + return nil, err + } + if session == nil { + return nil, coreerrors.ErrNotFound } + if session.UserID != userID { + uc.Logger.Warn("user attempted to sign out a session they do not own", "user_id", userID, "session_id", requestedSessionID) + return nil, coreerrors.ErrForbidden + } + + if err := uc.SessionService.Delete(ctx, requestedSessionID); err != nil { + uc.Logger.Error("failed to delete session", "error", err, "session_id", requestedSessionID) + return nil, err + } + + clearedCurrent := currentSessionID != nil && *currentSessionID == requestedSessionID - return &types.SignOutResult{Message: "no active session found"}, nil + return &types.SignOutResult{Message: "signed out", ClearedCurrentSession: clearedCurrent}, nil } diff --git a/core/usecases/sign_out_usecase_test.go b/core/usecases/sign_out_usecase_test.go index a0476eec..615f35e2 100644 --- a/core/usecases/sign_out_usecase_test.go +++ b/core/usecases/sign_out_usecase_test.go @@ -5,6 +5,7 @@ import ( "errors" "testing" + coreerrors "github.com/Authula/authula/core/errors" "github.com/Authula/authula/core/types" inttests "github.com/Authula/authula/internal/tests" "github.com/Authula/authula/models" @@ -17,85 +18,119 @@ func TestSignOutUseCase(t *testing.T) { userID := "user-123" const ( - sessionIDValue = "session-123" - recentSessionID = "session-456" + currentSessionID = "session-current" + requestedSessionID = "session-requested" ) tests := []struct { name string - sessionID string - sessionIDProvided bool - signOutAll bool - signOutAllProvided bool + currentSessionID *string + requestedSessionID *string + signOutAll *bool configure func(*inttests.MockSessionService) want *types.SignOutResult - wantErr string + wantErr error }{ { - name: "deletes a specific session when session id is provided", - sessionID: sessionIDValue, - sessionIDProvided: true, + name: "deletes all sessions when sign out all is requested", + signOutAll: new(true), configure: func(sessionService *inttests.MockSessionService) { - sessionService.On("Delete", ctx, sessionIDValue).Return(nil).Once() + sessionService.On("DeleteAllByUserID", ctx, userID).Return(nil).Once() }, - want: &types.SignOutResult{Message: "signed out"}, + want: &types.SignOutResult{Message: "signed out from all sessions", ClearedCurrentSession: true}, }, { - name: "returns delete session error when deleting a specific session fails", - sessionID: sessionIDValue, - sessionIDProvided: true, + name: "returns delete all error when deleting all sessions fails", + signOutAll: new(true), configure: func(sessionService *inttests.MockSessionService) { - sessionService.On("Delete", ctx, sessionIDValue).Return(errors.New("delete failed")).Once() + sessionService.On("DeleteAllByUserID", ctx, userID).Return(errors.New("delete all failed")).Once() }, - wantErr: "delete failed", + wantErr: errors.New("delete all failed"), }, { - name: "deletes all sessions when sign out all is requested", - signOutAll: true, - signOutAllProvided: true, + name: "sign out all takes precedence over a requested session id", + currentSessionID: new(currentSessionID), + requestedSessionID: new(requestedSessionID), + signOutAll: new(true), configure: func(sessionService *inttests.MockSessionService) { sessionService.On("DeleteAllByUserID", ctx, userID).Return(nil).Once() }, - want: &types.SignOutResult{Message: "signed out from all sessions"}, + want: &types.SignOutResult{Message: "signed out from all sessions", ClearedCurrentSession: true}, }, { - name: "returns delete all error when deleting all sessions fails", - signOutAll: true, - signOutAllProvided: true, + name: "deletes a requested session owned by the user without clearing the current session", + currentSessionID: new(currentSessionID), + requestedSessionID: new(requestedSessionID), configure: func(sessionService *inttests.MockSessionService) { - sessionService.On("DeleteAllByUserID", ctx, userID).Return(errors.New("delete all failed")).Once() + sessionService.On("GetByID", ctx, requestedSessionID).Return(&models.Session{ID: requestedSessionID, UserID: userID}, nil).Once() + sessionService.On("Delete", ctx, requestedSessionID).Return(nil).Once() + }, + want: &types.SignOutResult{Message: "signed out", ClearedCurrentSession: false}, + }, + { + name: "clears the current session when the requested session id is the current session", + currentSessionID: new(currentSessionID), + requestedSessionID: new(currentSessionID), + configure: func(sessionService *inttests.MockSessionService) { + sessionService.On("GetByID", ctx, currentSessionID).Return(&models.Session{ID: currentSessionID, UserID: userID}, nil).Once() + sessionService.On("Delete", ctx, currentSessionID).Return(nil).Once() + }, + want: &types.SignOutResult{Message: "signed out", ClearedCurrentSession: true}, + }, + { + name: "returns not found when the requested session does not exist", + currentSessionID: new(currentSessionID), + requestedSessionID: new(requestedSessionID), + configure: func(sessionService *inttests.MockSessionService) { + sessionService.On("GetByID", ctx, requestedSessionID).Return((*models.Session)(nil), nil).Once() }, - wantErr: "delete all failed", + wantErr: coreerrors.ErrNotFound, }, { - name: "deletes the most recent session when no explicit sign out target is provided", + name: "returns forbidden when the requested session belongs to another user", + currentSessionID: new(currentSessionID), + requestedSessionID: new(requestedSessionID), configure: func(sessionService *inttests.MockSessionService) { - sessionService.On("GetByUserID", ctx, userID).Return(&models.Session{ID: recentSessionID, UserID: userID}, nil).Once() - sessionService.On("Delete", ctx, recentSessionID).Return(nil).Once() + sessionService.On("GetByID", ctx, requestedSessionID).Return(&models.Session{ID: requestedSessionID, UserID: "other-user"}, nil).Once() }, - want: &types.SignOutResult{Message: "signed out"}, + wantErr: coreerrors.ErrForbidden, }, { - name: "returns get session error when loading the most recent session fails", + name: "returns get by id error when loading the requested session fails", + requestedSessionID: new(requestedSessionID), configure: func(sessionService *inttests.MockSessionService) { - sessionService.On("GetByUserID", ctx, userID).Return((*models.Session)(nil), errors.New("get failed")).Once() + sessionService.On("GetByID", ctx, requestedSessionID).Return((*models.Session)(nil), errors.New("get by id failed")).Once() }, - wantErr: "get failed", + wantErr: errors.New("get by id failed"), }, { - name: "returns no active session message when the user has no session", + name: "returns delete error when deleting the requested session fails", + requestedSessionID: new(requestedSessionID), configure: func(sessionService *inttests.MockSessionService) { - sessionService.On("GetByUserID", ctx, userID).Return((*models.Session)(nil), nil).Once() + sessionService.On("GetByID", ctx, requestedSessionID).Return(&models.Session{ID: requestedSessionID, UserID: userID}, nil).Once() + sessionService.On("Delete", ctx, requestedSessionID).Return(errors.New("delete failed")).Once() }, - want: &types.SignOutResult{Message: "no active session found"}, + wantErr: errors.New("delete failed"), }, { - name: "returns delete error when deleting the most recent session fails", + name: "deletes the current session when no explicit sign out target is provided", + currentSessionID: new(currentSessionID), configure: func(sessionService *inttests.MockSessionService) { - sessionService.On("GetByUserID", ctx, userID).Return(&models.Session{ID: recentSessionID, UserID: userID}, nil).Once() - sessionService.On("Delete", ctx, recentSessionID).Return(errors.New("delete recent failed")).Once() + sessionService.On("Delete", ctx, currentSessionID).Return(nil).Once() }, - wantErr: "delete recent failed", + want: &types.SignOutResult{Message: "signed out", ClearedCurrentSession: true}, + }, + { + name: "returns delete error when deleting the current session fails", + currentSessionID: new(currentSessionID), + configure: func(sessionService *inttests.MockSessionService) { + sessionService.On("Delete", ctx, currentSessionID).Return(errors.New("delete current failed")).Once() + }, + wantErr: errors.New("delete current failed"), + }, + { + name: "returns an error when there is no current session and no sign out target", + wantErr: coreerrors.ErrNoSessionToSignOut, }, } @@ -113,26 +148,14 @@ func TestSignOutUseCase(t *testing.T) { SessionService: sessionService, } - var sessionID *string - if tt.sessionIDProvided { - value := tt.sessionID - sessionID = &value - } - - var signOutAll *bool - if tt.signOutAllProvided { - value := tt.signOutAll - signOutAll = &value - } + result, err := uc.SignOut(ctx, userID, tt.currentSessionID, tt.requestedSessionID, tt.signOutAll) - result, err := uc.SignOut(ctx, userID, sessionID, signOutAll) - - if tt.wantErr != "" { + if tt.wantErr != nil { if err == nil { t.Fatalf("expected error %q, got nil", tt.wantErr) } - if err.Error() != tt.wantErr { - t.Fatalf("expected error %q, got %q", tt.wantErr, err.Error()) + if err.Error() != tt.wantErr.Error() { + t.Fatalf("expected error %q, got %q", tt.wantErr, err) } if result != nil { t.Fatalf("expected nil result on error, got %#v", result) @@ -143,11 +166,9 @@ func TestSignOutUseCase(t *testing.T) { } if result == nil { t.Fatal("expected result, got nil") - } else { - got := *result - if got.Message != tt.want.Message { - t.Fatalf("expected message %q, got %q", tt.want.Message, got.Message) - } + } + if *result != *tt.want { + t.Fatalf("expected result %#v, got %#v", *tt.want, *result) } } diff --git a/openapi.json b/openapi.json index 0d6fcda3..6d634c61 100644 --- a/openapi.json +++ b/openapi.json @@ -3505,7 +3505,7 @@ "Core" ], "summary": "Sign out", - "description": "Signs out the authenticated user. Optionally sign out a specific session or all sessions.", + "description": "Signs out the authenticated user. With no body, revokes the session that authenticated the request (400 if the request carries no session id). Pass session_id to revoke one of your own sessions (404 if it does not exist, 403 if it belongs to another user), or sign_out_all to revoke every session.", "operationId": "signOut", "requestBody": { "content": { diff --git a/plugins/admin/handlers/impersonation_handlers.go b/plugins/admin/handlers/impersonation_handlers.go index 04c5c34c..6710a991 100644 --- a/plugins/admin/handlers/impersonation_handlers.go +++ b/plugins/admin/handlers/impersonation_handlers.go @@ -226,6 +226,7 @@ func (h *StopImpersonationHandler) Handler() http.HandlerFunc { HttpOnly: sessionConfig.HttpOnly, Secure: sessionConfig.Secure, SameSite: sameSite, + MaxAge: int(sessionConfig.CookieMaxAge.Seconds()), }) http.SetCookie(w, &http.Cookie{ diff --git a/plugins/admin/handlers/impersonation_handlers_test.go b/plugins/admin/handlers/impersonation_handlers_test.go index a16e656c..005ff39b 100644 --- a/plugins/admin/handlers/impersonation_handlers_test.go +++ b/plugins/admin/handlers/impersonation_handlers_test.go @@ -20,10 +20,11 @@ import ( func testGlobalConfig() *models.Config { return &models.Config{ Session: models.SessionConfig{ - CookieName: "authula.session_token", - HttpOnly: true, - Secure: false, - SameSite: "lax", + CookieName: "authula.session_token", + CookieMaxAge: time.Hour, + HttpOnly: true, + Secure: false, + SameSite: "lax", }, } } @@ -339,6 +340,9 @@ func TestStopImpersonationHandler(t *testing.T) { if tt.expectedMessage != "" { internaltests.AssertErrorMessage(t, reqCtx, tt.expectedStatus, tt.expectedMessage) } + if tt.expectedStatus == http.StatusOK { + assertRestoredSessionCookie(t, w.Result().Cookies(), originalSessionToken) + } impRepo.AssertExpectations(t) sessionStateRepo.AssertExpectations(t) @@ -347,3 +351,21 @@ func TestStopImpersonationHandler(t *testing.T) { }) } } + +func assertRestoredSessionCookie(t *testing.T, cookies []*http.Cookie, wantToken string) { + t.Helper() + cookieName := testGlobalConfig().Session.CookieName + for _, c := range cookies { + if c.Name != cookieName { + continue + } + if c.Value != wantToken { + t.Fatalf("expected restored cookie value %q, got %q", wantToken, c.Value) + } + if c.MaxAge != int(time.Hour.Seconds()) { + t.Fatalf("expected restored cookie max age %d, got %d", int(time.Hour.Seconds()), c.MaxAge) + } + return + } + t.Fatalf("expected restored session cookie %q to be set", cookieName) +} diff --git a/plugins/csrf/hooks.go b/plugins/csrf/hooks.go index 649a5313..6362e0aa 100644 --- a/plugins/csrf/hooks.go +++ b/plugins/csrf/hooks.go @@ -65,7 +65,11 @@ func (p *CSRFPlugin) generateCSRFTokenHook(reqCtx *models.RequestContext) error return nil } - _, err := reqCtx.Request.Cookie(p.pluginConfig.CookieName) + cookie, err := reqCtx.Request.Cookie(p.pluginConfig.CookieName) + if err == nil { + p.exposeCSRFToken(reqCtx, cookie.Value) + return nil + } if err != http.ErrNoCookie { return nil } diff --git a/plugins/csrf/plugin.go b/plugins/csrf/plugin.go index 97492709..600294c6 100644 --- a/plugins/csrf/plugin.go +++ b/plugins/csrf/plugin.go @@ -1,6 +1,7 @@ package csrf import ( + "crypto/subtle" "fmt" "net/http" @@ -148,7 +149,7 @@ func (p *CSRFPlugin) validateCSRFToken(reqCtx *models.RequestContext) error { return nil } - if headerToken != cookie.Value { + if !tokensMatch(headerToken, cookie.Value) { reqCtx.SetJSONResponse(http.StatusForbidden, map[string]string{"message": "invalid csrf token"}) reqCtx.Handled = true return nil @@ -157,6 +158,11 @@ func (p *CSRFPlugin) validateCSRFToken(reqCtx *models.RequestContext) error { return nil } +// tokensMatch compares the header and cookie tokens in constant time. +func tokensMatch(headerToken, cookieToken string) bool { + return subtle.ConstantTimeCompare([]byte(headerToken), []byte(cookieToken)) == 1 +} + // Middleware returns a CSRF protection middleware that users can add to custom routes. func (p *CSRFPlugin) Middleware() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { @@ -170,8 +176,11 @@ func (p *CSRFPlugin) Middleware() func(http.Handler) http.Handler { } if r.Method == http.MethodGet || r.Method == http.MethodHead || r.Method == http.MethodOptions { - _, err := r.Cookie(p.pluginConfig.CookieName) - if err == http.ErrNoCookie { + cookie, err := r.Cookie(p.pluginConfig.CookieName) + switch err { + case nil: + p.exposeCSRFToken(reqCtx, cookie.Value) + case http.ErrNoCookie: token, err := p.tokenService.Generate() if err != nil { reqCtx.SetJSONResponse( @@ -201,7 +210,7 @@ func (p *CSRFPlugin) Middleware() func(http.Handler) http.Handler { return } - if headerToken != cookie.Value { + if !tokensMatch(headerToken, cookie.Value) { reqCtx.SetJSONResponse(http.StatusForbidden, map[string]string{"message": "invalid csrf token"}) reqCtx.Handled = true return @@ -243,6 +252,13 @@ func (p *CSRFPlugin) setCSRFCookie(reqCtx *models.RequestContext, token string) MaxAge: int(p.pluginConfig.MaxAge.Seconds()), }) - // Set token in response header so client can read it + p.exposeCSRFToken(reqCtx, token) +} + +// exposeCSRFToken places the token in the response header. Clients on another origin +// (for example an SPA on app.example.com talking to api.example.com) cannot read the +// cookie through document.cookie, so the header is their only way to obtain the token +// after a page reload. CORS restricts who can read it, so echoing an existing token is safe. +func (p *CSRFPlugin) exposeCSRFToken(reqCtx *models.RequestContext, token string) { reqCtx.ResponseWriter.Header().Set(p.pluginConfig.HeaderName, token) } diff --git a/plugins/csrf/plugin_test.go b/plugins/csrf/plugin_test.go index 49cf36fd..3da11dab 100644 --- a/plugins/csrf/plugin_test.go +++ b/plugins/csrf/plugin_test.go @@ -206,6 +206,7 @@ func TestCSRFPlugin_TokenGeneration(t *testing.T) { method string existingCookie bool wantToken bool + wantHeader string }{ { name: "first GET generates token", @@ -213,9 +214,10 @@ func TestCSRFPlugin_TokenGeneration(t *testing.T) { wantToken: true, }, { - name: "second GET with cookie skips generation", + name: "second GET with cookie skips generation but echoes token header", method: http.MethodGet, existingCookie: true, + wantHeader: "existing", }, { name: "unauthenticated GET generates token", @@ -223,9 +225,10 @@ func TestCSRFPlugin_TokenGeneration(t *testing.T) { wantToken: true, }, { - name: "unauthenticated GET with existing cookie skips", + name: "unauthenticated GET with existing cookie skips but echoes token header", method: http.MethodGet, existingCookie: true, + wantHeader: "existing", }, { name: "POST does not generate token", @@ -266,6 +269,16 @@ func TestCSRFPlugin_TokenGeneration(t *testing.T) { if !tt.wantToken && len(cookies) > 0 { t.Error("expected no CSRF cookie to be set") } + + // Cross-origin clients cannot read the cookie, so the token must always be + // discoverable from the response header on safe requests. + gotHeader := w.Header().Get("X-AUTHULA-CSRF-TOKEN") + if tt.wantToken && gotHeader == "" { + t.Error("expected CSRF token header on generation") + } + if tt.wantHeader != "" && gotHeader != tt.wantHeader { + t.Errorf("CSRF token header = %q, want %q", gotHeader, tt.wantHeader) + } }) } } @@ -509,6 +522,7 @@ func TestCSRFPlugin_Middleware(t *testing.T) { expectedStatus int handlerShouldRun bool expectCookie bool + expectHeader string }{ { name: "unauthenticated POST passes through", @@ -552,13 +566,14 @@ func TestCSRFPlugin_Middleware(t *testing.T) { expectCookie: true, }, { - name: "authenticated GET with existing cookie does not regenerate", + name: "authenticated GET with existing cookie does not regenerate but echoes token header", method: http.MethodGet, userID: "user-123", hasCookie: true, cookieValue: "existing", expectedStatus: http.StatusOK, handlerShouldRun: true, + expectHeader: "existing", }, } @@ -615,6 +630,11 @@ func TestCSRFPlugin_Middleware(t *testing.T) { t.Error("expected CSRF cookie to be set") } } + if tt.expectHeader != "" { + if got := w.Header().Get("X-AUTHULA-CSRF-TOKEN"); got != tt.expectHeader { + t.Errorf("CSRF token header = %q, want %q", got, tt.expectHeader) + } + } }) } } diff --git a/plugins/session/hooks.go b/plugins/session/hooks.go index 62c88617..6f01c2ae 100644 --- a/plugins/session/hooks.go +++ b/plugins/session/hooks.go @@ -92,7 +92,7 @@ func (p *SessionPlugin) validateSessionHook(reqCtx *models.RequestContext) error reqCtx.Values[models.ContextSessionID.String()] = session.ID if p.shouldRenewSession(session) { - p.renewSession(reqCtx.ResponseWriter, reqCtx.Request, session) + p.renewSession(reqCtx.ResponseWriter, reqCtx.Request, session, sessionToken) } return nil @@ -129,9 +129,9 @@ func (p *SessionPlugin) validateSessionHookOptional(reqCtx *models.RequestContex }) reqCtx.Values[models.ContextSessionID.String()] = session.ID - // Optionally renew session if it's past 50% of its max age + // Slide the expiry forward once the session enters its update window if p.shouldRenewSession(session) { - p.renewSession(reqCtx.ResponseWriter, reqCtx.Request, session) + p.renewSession(reqCtx.ResponseWriter, reqCtx.Request, session, sessionToken) } return nil @@ -147,7 +147,8 @@ func (p *SessionPlugin) issueSessionCookieHook(reqCtx *models.RequestContext) er return nil } - p.SetSessionCookie(reqCtx.ResponseWriter, sessionToken) + expiresAt := time.Now().UTC().Add(p.globalConfig.Session.ExpiresIn) + p.SetSessionCookie(reqCtx.ResponseWriter, sessionToken, expiresAt) return nil } diff --git a/plugins/session/plugin.go b/plugins/session/plugin.go index 6bda0cfb..dccc1d34 100644 --- a/plugins/session/plugin.go +++ b/plugins/session/plugin.go @@ -72,6 +72,13 @@ func (p *SessionPlugin) Init(ctx *models.PluginContext) error { } p.tokenService = tokenService + if globalConfig.Session.UpdateAge >= globalConfig.Session.ExpiresIn { + p.logger.Warn("session update_age is not shorter than expires_in; every authenticated request will extend the session", + "update_age", globalConfig.Session.UpdateAge, + "expires_in", globalConfig.Session.ExpiresIn, + ) + } + return nil } @@ -79,11 +86,10 @@ func (p *SessionPlugin) Hooks() []models.Hook { return p.buildHooks() } -// AuthMiddleware validates session cookie and extracts user ID func (p *SessionPlugin) AuthMiddleware() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - session, err := p.validateSessionCookie(r) + session, rawToken, err := p.validateSessionCookie(r) if err != nil { errorMsg := "unauthorized" statusCode := http.StatusUnauthorized @@ -92,7 +98,7 @@ func (p *SessionPlugin) AuthMiddleware() func(http.Handler) http.Handler { } if p.shouldRenewSession(session) { - p.renewSession(w, r, session) + p.renewSession(w, r, session, rawToken) } ctx := context.WithValue(r.Context(), models.ContextUserID, session.UserID) @@ -102,13 +108,12 @@ func (p *SessionPlugin) AuthMiddleware() func(http.Handler) http.Handler { } } -// OptionalAuthMiddleware validates session if present but doesn't require it func (p *SessionPlugin) OptionalAuthMiddleware() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if session, err := p.validateSessionCookie(r); err == nil && session != nil { + if session, rawToken, err := p.validateSessionCookie(r); err == nil && session != nil { if p.shouldRenewSession(session) { - p.renewSession(w, r, session) + p.renewSession(w, r, session, rawToken) } ctx := context.WithValue(r.Context(), models.ContextUserID, session.UserID) @@ -120,25 +125,30 @@ func (p *SessionPlugin) OptionalAuthMiddleware() func(http.Handler) http.Handler } } -func (p *SessionPlugin) validateSessionCookie(r *http.Request) (*models.Session, error) { +// validateSessionCookie resolves the session behind the request's cookie and returns it +// together with the raw cookie value so the cookie can be re-issued unchanged on renewal. +func (p *SessionPlugin) validateSessionCookie(r *http.Request) (*models.Session, string, error) { cookie, err := r.Cookie(p.globalConfig.Session.CookieName) if err != nil { - return nil, err + return nil, "", err } session, err := p.sessionService.GetByToken(r.Context(), p.tokenService.Hash(cookie.Value)) - if err != nil || session == nil { - return nil, err + if err != nil { + return nil, "", err + } + if session == nil { + return nil, "", fmt.Errorf("session not found") } if session.ExpiresAt.Before(time.Now().UTC()) { if err := p.sessionService.Delete(r.Context(), session.ID); err != nil { p.logger.Error("failed to delete expired session", "error", err) } - return nil, fmt.Errorf("session expired") + return nil, "", fmt.Errorf("session expired") } - return session, nil + return session, cookie.Value, nil } func (p *SessionPlugin) writeErrorResponse(w http.ResponseWriter, statusCode int, errorMsg string) { @@ -160,17 +170,15 @@ func (p *SessionPlugin) getSameSiteMode() http.SameSite { } } -func (p *SessionPlugin) SetSessionCookie(w http.ResponseWriter, sessionToken string) { - sameSite := p.getSameSiteMode() - +func (p *SessionPlugin) SetSessionCookie(w http.ResponseWriter, sessionToken string, expiresAt time.Time) { http.SetCookie(w, &http.Cookie{ Name: p.globalConfig.Session.CookieName, Value: sessionToken, Path: "/", HttpOnly: p.globalConfig.Session.HttpOnly, Secure: p.globalConfig.Session.Secure, - SameSite: sameSite, - MaxAge: int(p.globalConfig.Session.CookieMaxAge.Seconds()), + SameSite: p.getSameSiteMode(), + MaxAge: p.cookieMaxAge(expiresAt), }) } @@ -181,41 +189,41 @@ func (p *SessionPlugin) ClearSessionCookie(w http.ResponseWriter) { Path: "/", HttpOnly: p.globalConfig.Session.HttpOnly, Secure: p.globalConfig.Session.Secure, + SameSite: p.getSameSiteMode(), MaxAge: -1, }) } +func (p *SessionPlugin) cookieMaxAge(expiresAt time.Time) int { + remaining := time.Until(expiresAt) + maxAge := p.globalConfig.Session.CookieMaxAge + if maxAge <= 0 || remaining < maxAge { + maxAge = remaining + } + return int(maxAge.Seconds()) +} + func (p *SessionPlugin) shouldRenewSession(session *models.Session) bool { now := time.Now().UTC() timeToExpiry := session.ExpiresAt.Sub(now) return timeToExpiry <= p.globalConfig.Session.UpdateAge } -func (p *SessionPlugin) renewSession(w http.ResponseWriter, r *http.Request, session *models.Session) { - newToken, err := p.tokenService.Generate() - if err != nil { - p.logger.Error("session renewal failed: token generation error", "error", err) - return - } - - hashedToken := p.tokenService.Hash(newToken) +// renewSession slides the session's expiry forward in place and re-issues the same cookie +// with a refreshed Max-Age. The row and token are kept, so concurrent requests carrying the +// same cookie all keep validating while the renewal is in flight. +func (p *SessionPlugin) renewSession(w http.ResponseWriter, r *http.Request, session *models.Session, rawToken string) { + renewed := *session + renewed.ExpiresAt = time.Now().UTC().Add(p.globalConfig.Session.ExpiresIn) - if err := p.sessionService.Delete(r.Context(), session.ID); err != nil { - p.logger.Error("session renewal failed: delete error", "error", err) - return - } - - newSession, err := p.sessionService.Create(r.Context(), session.UserID, hashedToken, session.IPAddress, session.UserAgent, p.globalConfig.Session.ExpiresIn) + updated, err := p.sessionService.Update(r.Context(), &renewed) if err != nil { - p.logger.Error("session renewal failed: create error", "error", err) + p.logger.Error("session renewal failed: update error", "error", err) return } - session.ID = newSession.ID - session.Token = hashedToken - session.ExpiresAt = newSession.ExpiresAt - - p.SetSessionCookie(w, newToken) + session.ExpiresAt = updated.ExpiresAt + p.SetSessionCookie(w, rawToken, session.ExpiresAt) } func (p *SessionPlugin) Close() error { diff --git a/plugins/session/plugin_test.go b/plugins/session/plugin_test.go index ee47ac9a..5a76c0ab 100644 --- a/plugins/session/plugin_test.go +++ b/plugins/session/plugin_test.go @@ -15,6 +15,31 @@ import ( "github.com/Authula/authula/models" ) +const ( + testCookieName = "authula.session_token" + testExpiresIn = time.Hour * 24 * 7 + testUpdateAge = time.Hour * 24 +) + +func newTestPlugin(sessionSvc *internaltests.MockSessionService, tokenSvc *internaltests.MockTokenService) *SessionPlugin { + return &SessionPlugin{ + globalConfig: &models.Config{ + Session: models.SessionConfig{ + CookieName: testCookieName, + ExpiresIn: testExpiresIn, + UpdateAge: testUpdateAge, + CookieMaxAge: testExpiresIn, + HttpOnly: true, + Secure: true, + SameSite: "strict", + }, + }, + sessionService: sessionSvc, + tokenService: tokenSvc, + logger: new(internaltests.MockLogger), + } +} + func TestSessionPlugin_RenewSession(t *testing.T) { t.Parallel() @@ -23,79 +48,254 @@ func TestSessionPlugin_RenewSession(t *testing.T) { userAgent := "test-agent" tests := []struct { - name string - setup func(*internaltests.MockTokenService, *internaltests.MockSessionService) - session *models.Session - wantCookie bool - wantToken string - wantSessionID string + name string + setup func(*internaltests.MockSessionService) + session *models.Session + wantCookie bool }{ { - name: "rotates token and creates new session", - setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { - mockTokenSvc.On("Generate").Return("new-token", nil).Once() - mockTokenSvc.On("Hash", "new-token").Return("hashed-new-token").Once() - mockSessionSvc.On("Delete", mock.Anything, "session-id").Return(nil).Once() - mockSessionSvc.On("Create", mock.Anything, "user-id", "hashed-new-token", &clientIP, &userAgent, time.Hour*24*7). - Return(&models.Session{ID: "new-session-id", Token: "hashed-new-token", ExpiresAt: now.Add(time.Hour * 24 * 7)}, nil).Once() + name: "extends expiry in place and re-issues the same cookie value", + setup: func(mockSessionSvc *internaltests.MockSessionService) { + mockSessionSvc.On("Update", mock.Anything, mock.MatchedBy(func(s *models.Session) bool { + // The existing row must be extended, never replaced + return s.ID == "session-id" && + s.Token == "hashed-old-token" && + s.ExpiresAt.After(now.Add(testExpiresIn-time.Minute)) + })).Return(&models.Session{ + ID: "session-id", + UserID: "user-id", + Token: "hashed-old-token", + ExpiresAt: now.Add(testExpiresIn), + }, nil).Once() }, session: &models.Session{ ID: "session-id", UserID: "user-id", Token: "hashed-old-token", - ExpiresAt: now.Add(24 * time.Hour), + ExpiresAt: now.Add(time.Hour), IPAddress: &clientIP, UserAgent: &userAgent, }, - wantCookie: true, - wantToken: "new-token", - wantSessionID: "new-session-id", + wantCookie: true, }, { - name: "token generation failure aborts renewal", - setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { - mockTokenSvc.On("Generate").Return("", errors.New("generation failed")).Once() + name: "update failure aborts renewal and leaves the session untouched", + setup: func(mockSessionSvc *internaltests.MockSessionService) { + mockSessionSvc.On("Update", mock.Anything, mock.Anything). + Return(nil, errors.New("update failed")).Once() }, session: &models.Session{ ID: "session-id", + UserID: "user-id", Token: "hashed-old-token", + ExpiresAt: now.Add(time.Hour), IPAddress: &clientIP, UserAgent: &userAgent, }, wantCookie: false, }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockTokenSvc := new(internaltests.MockTokenService) + mockSessionSvc := new(internaltests.MockSessionService) + tt.setup(mockSessionSvc) + + plugin := newTestPlugin(mockSessionSvc, mockTokenSvc) + originalExpiry := tt.session.ExpiresAt + + r := httptest.NewRequest(http.MethodGet, "/", nil) + w := httptest.NewRecorder() + + plugin.renewSession(w, r, tt.session, "old-plaintext-token") + + cookies := w.Result().Cookies() + if tt.wantCookie { + require.Len(t, cookies, 1) + assert.Equal(t, testCookieName, cookies[0].Name) + assert.Equal(t, "old-plaintext-token", cookies[0].Value, "cookie value must not rotate") + assert.Equal(t, "session-id", tt.session.ID, "session ID must not change") + assert.True(t, tt.session.ExpiresAt.After(originalExpiry), "expiry should be extended") + } else { + assert.Empty(t, cookies) + assert.Equal(t, originalExpiry, tt.session.ExpiresAt, "expiry must not change on error") + } + + // Renewal must never delete or create rows, nor generate new tokens + mockSessionSvc.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything) + mockSessionSvc.AssertNotCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockTokenSvc.AssertNotCalled(t, "Generate") + mockTokenSvc.AssertExpectations(t) + mockSessionSvc.AssertExpectations(t) + }) + } +} + +func TestSessionPlugin_ShouldRenewSession(t *testing.T) { + t.Parallel() + + now := time.Now().UTC() + + tests := []struct { + name string + expiresAt time.Time + want bool + }{ + {name: "far from expiry does not renew", expiresAt: now.Add(testExpiresIn), want: false}, + {name: "just outside update window does not renew", expiresAt: now.Add(testUpdateAge + time.Minute), want: false}, + {name: "inside update window renews", expiresAt: now.Add(testUpdateAge - time.Minute), want: true}, + {name: "already expired still reports renewable", expiresAt: now.Add(-time.Minute), want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + plugin := newTestPlugin(new(internaltests.MockSessionService), new(internaltests.MockTokenService)) + + got := plugin.shouldRenewSession(&models.Session{ExpiresAt: tt.expiresAt}) + + assert.Equal(t, tt.want, got) + }) + } +} + +func TestSessionPlugin_SetSessionCookie(t *testing.T) { + t.Parallel() + + now := time.Now().UTC() + + tests := []struct { + name string + cookieMaxAge time.Duration + expiresAt time.Time + wantMaxAge int + }{ + { + name: "uses cookie max age when session outlives it", + cookieMaxAge: time.Hour, + expiresAt: now.Add(testExpiresIn), + wantMaxAge: int(time.Hour.Seconds()), + }, { - name: "delete failure aborts renewal", - setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { - mockTokenSvc.On("Generate").Return("new-token", nil).Once() - mockTokenSvc.On("Hash", "new-token").Return("hashed-new-token").Once() - mockSessionSvc.On("Delete", mock.Anything, "session-id").Return(errors.New("delete failed")).Once() + name: "clamps to remaining session lifetime when shorter than cookie max age", + cookieMaxAge: testExpiresIn, + expiresAt: now.Add(2 * time.Hour), + wantMaxAge: int((2 * time.Hour).Seconds()), + }, + { + name: "falls back to remaining session lifetime when cookie max age is unset", + cookieMaxAge: 0, + expiresAt: now.Add(3 * time.Hour), + wantMaxAge: int((3 * time.Hour).Seconds()), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + plugin := newTestPlugin(new(internaltests.MockSessionService), new(internaltests.MockTokenService)) + plugin.globalConfig.Session.CookieMaxAge = tt.cookieMaxAge + w := httptest.NewRecorder() + + plugin.SetSessionCookie(w, "plaintext-token", tt.expiresAt) + + cookies := w.Result().Cookies() + require.Len(t, cookies, 1) + cookie := cookies[0] + assert.Equal(t, testCookieName, cookie.Name) + assert.Equal(t, "plaintext-token", cookie.Value) + assert.Equal(t, "/", cookie.Path) + assert.True(t, cookie.HttpOnly) + assert.True(t, cookie.Secure) + assert.Equal(t, http.SameSiteStrictMode, cookie.SameSite) + // Allow a second of slack for the time elapsed between building expiresAt and the cookie + assert.InDelta(t, tt.wantMaxAge, cookie.MaxAge, 1) + }) + } +} + +func TestSessionPlugin_ClearSessionCookie(t *testing.T) { + t.Parallel() + + plugin := newTestPlugin(new(internaltests.MockSessionService), new(internaltests.MockTokenService)) + w := httptest.NewRecorder() + + plugin.ClearSessionCookie(w) + + cookies := w.Result().Cookies() + require.Len(t, cookies, 1) + cookie := cookies[0] + assert.Equal(t, testCookieName, cookie.Name) + assert.Empty(t, cookie.Value) + assert.Equal(t, "/", cookie.Path) + assert.True(t, cookie.HttpOnly) + assert.True(t, cookie.Secure) + assert.Equal(t, http.SameSiteStrictMode, cookie.SameSite, "clear must carry the same SameSite as the set") + assert.Equal(t, -1, cookie.MaxAge) +} + +func TestSessionPlugin_ValidateSessionHook(t *testing.T) { + t.Parallel() + + now := time.Now().UTC() + + tests := []struct { + name string + cookie *http.Cookie + setup func(*internaltests.MockTokenService, *internaltests.MockSessionService) + wantHandled bool + wantStatus int + wantActorID string + wantCookie bool + }{ + { + name: "missing cookie returns unauthorized", + setup: func(*internaltests.MockTokenService, *internaltests.MockSessionService) {}, + wantHandled: true, + wantStatus: http.StatusUnauthorized, + }, + { + name: "unknown token returns unauthorized without touching the cookie", + cookie: &http.Cookie{Name: testCookieName, Value: "plaintext"}, + setup: func(tokenSvc *internaltests.MockTokenService, sessionSvc *internaltests.MockSessionService) { + tokenSvc.On("Hash", "plaintext").Return("hashed").Once() + sessionSvc.On("GetByToken", mock.Anything, "hashed").Return(nil, nil).Once() }, - session: &models.Session{ - ID: "session-id", - Token: "hashed-old-token", - IPAddress: &clientIP, - UserAgent: &userAgent, + wantHandled: true, + wantStatus: http.StatusUnauthorized, + }, + { + name: "expired session returns unauthorized", + cookie: &http.Cookie{Name: testCookieName, Value: "plaintext"}, + setup: func(tokenSvc *internaltests.MockTokenService, sessionSvc *internaltests.MockSessionService) { + tokenSvc.On("Hash", "plaintext").Return("hashed").Once() + sessionSvc.On("GetByToken", mock.Anything, "hashed"). + Return(&models.Session{ID: "s1", UserID: "u1", ExpiresAt: now.Add(-time.Minute)}, nil).Once() }, - wantCookie: false, + wantHandled: true, + wantStatus: http.StatusUnauthorized, }, { - name: "create failure after delete aborts renewal", - setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { - mockTokenSvc.On("Generate").Return("new-token", nil).Once() - mockTokenSvc.On("Hash", "new-token").Return("hashed-new-token").Once() - mockSessionSvc.On("Delete", mock.Anything, "session-id").Return(nil).Once() - mockSessionSvc.On("Create", mock.Anything, "user-id", "hashed-new-token", &clientIP, &userAgent, time.Hour*24*7). - Return(nil, errors.New("create failed")).Once() + name: "valid session outside renewal window sets actor and no cookie", + cookie: &http.Cookie{Name: testCookieName, Value: "plaintext"}, + setup: func(tokenSvc *internaltests.MockTokenService, sessionSvc *internaltests.MockSessionService) { + tokenSvc.On("Hash", "plaintext").Return("hashed").Once() + sessionSvc.On("GetByToken", mock.Anything, "hashed"). + Return(&models.Session{ID: "s1", UserID: "u1", ExpiresAt: now.Add(testExpiresIn)}, nil).Once() }, - session: &models.Session{ - ID: "session-id", - UserID: "user-id", - Token: "hashed-old-token", - IPAddress: &clientIP, - UserAgent: &userAgent, + wantActorID: "u1", + }, + { + name: "valid session inside renewal window extends and re-issues cookie", + cookie: &http.Cookie{Name: testCookieName, Value: "plaintext"}, + setup: func(tokenSvc *internaltests.MockTokenService, sessionSvc *internaltests.MockSessionService) { + tokenSvc.On("Hash", "plaintext").Return("hashed").Once() + sessionSvc.On("GetByToken", mock.Anything, "hashed"). + Return(&models.Session{ID: "s1", UserID: "u1", Token: "hashed", ExpiresAt: now.Add(time.Hour)}, nil).Once() + sessionSvc.On("Update", mock.Anything, mock.Anything). + Return(&models.Session{ID: "s1", UserID: "u1", Token: "hashed", ExpiresAt: now.Add(testExpiresIn)}, nil).Once() }, - wantCookie: false, + wantActorID: "u1", + wantCookie: true, }, } @@ -103,43 +303,40 @@ func TestSessionPlugin_RenewSession(t *testing.T) { t.Run(tt.name, func(t *testing.T) { mockTokenSvc := new(internaltests.MockTokenService) mockSessionSvc := new(internaltests.MockSessionService) - tt.setup(mockTokenSvc, mockSessionSvc) + plugin := newTestPlugin(mockSessionSvc, mockTokenSvc) - plugin := &SessionPlugin{ - globalConfig: &models.Config{ - Session: models.SessionConfig{ - CookieName: "authula.session_token", - ExpiresIn: time.Hour * 24 * 7, - CookieMaxAge: time.Hour * 24 * 7, - HttpOnly: true, - SameSite: "lax", - }, - }, - sessionService: mockSessionSvc, - tokenService: mockTokenSvc, - logger: new(internaltests.MockLogger), + r := httptest.NewRequest(http.MethodGet, "/me", nil) + if tt.cookie != nil { + r.AddCookie(tt.cookie) } - - r := httptest.NewRequest(http.MethodGet, "/", nil) - r.AddCookie(&http.Cookie{ - Name: "authula.session_token", - Value: "old-plaintext-token", - }) w := httptest.NewRecorder() + reqCtx := &models.RequestContext{ + Request: r, + ResponseWriter: w, + Values: make(map[string]any), + ResponseHeaders: make(http.Header), + } - plugin.renewSession(w, r, tt.session) + err := plugin.validateSessionHook(reqCtx) + + require.NoError(t, err) + assert.Equal(t, tt.wantHandled, reqCtx.Handled) + if tt.wantHandled { + assert.Equal(t, tt.wantStatus, reqCtx.ResponseStatus) + assert.Nil(t, reqCtx.Actor) + } else { + require.NotNil(t, reqCtx.Actor) + assert.Equal(t, tt.wantActorID, reqCtx.Actor.ID) + assert.Equal(t, "s1", reqCtx.Values[models.ContextSessionID.String()]) + } cookies := w.Result().Cookies() if tt.wantCookie { require.Len(t, cookies, 1) - assert.Equal(t, "authula.session_token", cookies[0].Name) - assert.Equal(t, tt.wantToken, cookies[0].Value) - assert.Equal(t, tt.wantSessionID, tt.session.ID, "session ID should be updated") - assert.Equal(t, "hashed-new-token", tt.session.Token, "session token should be updated") + assert.Equal(t, "plaintext", cookies[0].Value) } else { - assert.Empty(t, cookies) - assert.Equal(t, "hashed-old-token", tt.session.Token, "session token should not change on error") + assert.Empty(t, cookies, "a rejected or non-renewed request must never emit Set-Cookie") } mockTokenSvc.AssertExpectations(t) @@ -147,3 +344,56 @@ func TestSessionPlugin_RenewSession(t *testing.T) { }) } } + +func TestSessionPlugin_IssueSessionCookieHook(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + actor *models.Actor + values map[string]any + wantCookie bool + }{ + { + name: "issues cookie bounded by session lifetime for user actor", + actor: &models.Actor{ID: "u1", Type: models.ActorUser}, + values: map[string]any{models.ContextSessionToken.String(): "plaintext"}, + wantCookie: true, + }, + { + name: "skips when no session token in context", + actor: &models.Actor{ID: "u1", Type: models.ActorUser}, + values: map[string]any{}, + }, + { + name: "skips when actor is nil", + values: map[string]any{models.ContextSessionToken.String(): "plaintext"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + plugin := newTestPlugin(new(internaltests.MockSessionService), new(internaltests.MockTokenService)) + plugin.globalConfig.Session.CookieMaxAge = 30 * 24 * time.Hour // longer than ExpiresIn + w := httptest.NewRecorder() + reqCtx := &models.RequestContext{ + Request: httptest.NewRequest(http.MethodPost, "/sign-in", nil), + ResponseWriter: w, + Actor: tt.actor, + Values: tt.values, + } + + err := plugin.issueSessionCookieHook(reqCtx) + + require.NoError(t, err) + cookies := w.Result().Cookies() + if !tt.wantCookie { + assert.Empty(t, cookies) + return + } + require.Len(t, cookies, 1) + assert.Equal(t, "plaintext", cookies[0].Value) + assert.InDelta(t, int(testExpiresIn.Seconds()), cookies[0].MaxAge, 1, "cookie must not outlive the session") + }) + } +}