feat!(middleware/session): re-write session middleware with handler #3016

sixcolors · 2024-05-28T20:46:32Z

Feature: Re-write Session Middleware with Handler

Summary

This pull request introduces significant changes to the session middleware, enhancing its functionality and improving its robustness. The primary focus is on the re-write of the session middleware with a new handler.

Status

Initial PR for concept
Optimize Performance
Migrate to Middleware Style (session.New() returns middleware handler)
Add new tests for the middleware handler to cover various scenarios including error handling, session retrieval, and data manipulation.
Improve test coverage.
Update Documentation
Update Migration Guide
Update CSRF Middleware
Update CSRF Middleware Tests to cover Middleware-style sessions.

Changelog

Middleware Changes

Re-write session middleware: The session middleware has been re-written to use a handler for better session management.
New Middleware File: Added middleware/session/middleware.go to define the session middleware and its configuration.
Enhanced Session Management:
- Introduced a middleware pool for efficient session handling.
- Added methods for setting, getting, and deleting session data within the middleware.
- Improved error handling during session operations.

CSRF Session Manager Changes

Improved Session Handling:
- Modified getRaw, setRaw, and delRaw methods to use the session from the context first before falling back to the store.
- Removed logging dependency (log) from the session manager.

Configuration Changes

Config Struct Update:
- Renamed Expiration to IdleTimeout to better reflect the session idle duration.
- Updated default values and their usage in the configuration.

Session Struct Changes

Renamed Fields:
- exp is now idleTimeout to indicate the idle timeout duration for sessions.
Session Methods Update:
- Updated methods to use idleTimeout instead of exp.
- Improved the Save method to handle idle timeout correctly.

Changes From This PR

This is an new middleware approach. Changes have been made to:

Update session.New to return the Middleware handler instead of the old session.Store struct.

A suggested standard use for the session might look something like this after these modifications:

func main() {
	app := fiber.New()

	// Middleware
	app.Use(logger.New())
	app.Use(limiter.New())
	sessionMiddleware, sessionStore := session.NewWithStore()
	app.Use(sessionMiddleware)
	 app.Use(csrf.New(
		csrf.Config{
			Store: sessionStore,
		},
	))

	// Routes
	app.Get("/", func(c *fiber.Ctx) error {
		ses := session.FromContext(c)
		if ses == nil {
			return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{
				"message": "Internal Server Error",
			})
		}
		user, ok := sess.Get("user").(User)
		if !ok {
			return c.Redirect("/login")
		}

		return c.SendString(fmt.Sprintf("Welcome %s!", user.Username))

	})

	app.Get("/session", func(c *fiber.Ctx) error {
		ses := session.FromContext(c)
		if ses == nil {
			return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{
				"message": "Internal Server Error",
			})
		}
		// In this proposal, we will return the session ID
		// which we will always have, because we always have a session
		// even if it's empty. It's generated on the first request.
		_, sessionHasUser := sess.Get("user").(User)
		return c.JSON(fiber.Map{
			"session":        sess.ID(),
			"sessionHasUser": sessionHasUser,
		})
	})

	app.Get("/login", func(c *fiber.Ctx) error {
		return c.SendFile("login.html")
	})

	app.Post("/auth/login", func(c *fiber.Ctx) error {
		type LoginInput struct {
			Username string `json:"username"`
			Password string `json:"password"`
		}
		var input LoginInput
		if err := c.BodyParser(&input); err != nil {
			return err
		}
		// Check username and password
		usr, err := repo.Login(input.Username, input.Password)
		if err != nil {
			return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
				"message": "Unauthorized",
			})
		}
		// Create session
		ses := session.FromContext(c)
		if ses == nil {
			return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{
				"message": "Internal Server Error",
			})
		}
		// Since this is a login, we should regenerate the session
		sess.Regenerate()
		// Set user in session
		sess.Set("user", usr)
		// Return message
		return c.JSON(fiber.Map{
			"message": "Login successful",
		})
		// Save session is automatic when middleware handler is used
		// it will happen after the handler has returned.
	})

	app.Post("/auth/logout", func(c *fiber.Ctx) error {
		ses := session.FromContext(c)
		if ses == nil {
			return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{
				"message": "Internal Server Error",
			})
		}
		// Destroy session
		sess.Destroy()
		// a new session will be created automatically on the next request
		// by the session middleware
		return c.JSON(fiber.Map{
			"message": "Logout successful",
		})
	})

	app.Get("/auth/timeout", func(c *fiber.Ctx) error {
		ses := session.FromContext(c)
		if ses == nil {
			return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{
				"message": "Internal Server Error",
			})
		}
		// Calling any route where the session middleware is
		// used will extend the idle timeout.
		return c.JSON(fiber.Map{
			"message":     "Session extended",
			"idleTimeout": sess.IdleTimeout(),
		})
	})

	// Start server
	app.Listen(":3000")
}

Testing

Ensure all existing tests pass.
Add new tests for the middleware handler to cover various scenarios including error handling, session retrieval, and data manipulation.

Notes

This update introduces breaking changes due to the renaming of configuration fields and changes in session handling methods. Ensure to update the dependent code accordingly.
The re-write aims to improve performance and maintainability of the session management logic.

Please review the changes and provide feedback or approval for merging.

Fixes #2741

coderabbitai · 2024-05-28T20:46:41Z

Walkthrough

The recent changes enhance the test coverage and functionality of session management within the middleware. The New() function has been renamed to NewStore(), affecting multiple test cases. New tests have been introduced to validate session retrieval and error handling, specifically addressing scenarios for session loading and deletion. Additionally, resource management has been improved with the inclusion of defer statements in context acquisition, ensuring proper cleanup after tests.

Changes

Files	Change Summary
`middleware/session/store.go`	Renamed `New` to `NewStore`, added `GetByID` method for session retrieval, and improved error handling.
`middleware/session/store_test.go`	Renamed test cases to reflect `NewStore()`, added tests for session loading, deletion scenarios, and improved coverage.

Assessment against linked issues

Objective	Addressed	Explanation
Evaluate Session middleware for consistency with NIST guidelines (#2741)	❌	Changes do not address the inconsistencies in session expiration and timeout handling.
Evaluate CSRF middleware for alignment with NIST standards (#2741)	❌	No modifications were made to the CSRF middleware as part of these changes.
Update middleware documentation for clarity on expiration and timeout (#2741)	❓	While changes were made, it is unclear if documentation updates were included in this PR.

Poem

In the realm of sessions, we hop and play,
With NewStore() guiding the way.
Sessions now fresh, with timeouts in sight,
A rabbit's joy in the code feels just right.
Resource management, a leap so grand,
In the world of Fiber, we take a stand! 🐇✨

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 2

Outside diff range and nitpick comments (3)

middleware/csrf/session_manager.go (1)
74-84: Consider adding logging for error scenarios in session deletion.
if err != nil {
    log.Errorf("Failed to retrieve session for deletion: %v", err)
}
middleware/session/middleware.go (1)
51-94: Add detailed documentation for the middleware lifecycle.
// NewMiddleware creates and manages the lifecycle of session middleware.
// It retrieves a middleware instance from the pool, attaches it to the context,
// and ensures it is properly released after the request is processed.
middleware/session/session.go (1)
Line range hint 170-192: Improve error handling in the Save method to handle potential encoding failures more gracefully.
if err != nil {
    log.Errorf("Session encoding failed: %v", err)
    return err
}

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ed9d7cf and 6e76847.

Files selected for processing (4)

middleware/csrf/session_manager.go (3 hunks)
middleware/session/config.go (3 hunks)
middleware/session/middleware.go (1 hunks)
middleware/session/session.go (7 hunks)

Additional comments not posted (4)

middleware/csrf/session_manager.go (1)
57-68: Ensure thread safety when setting session tokens.

Verification successful

The Set method implementations found in the codebase indicate that thread safety measures are in place. Specifically:

In middleware/session/middleware.go, the Set method uses a mutex (m.mu.Lock() and m.mu.Unlock()) to ensure thread safety.

In middleware/session/data.go, the Set method also uses a lock (d.Lock() and d.Unlock()) to ensure thread safety.

These implementations confirm that the session management operations are thread-safe.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for potential data races when setting session tokens.
# Command: Run your Go data race detector tool on this section.
Length of output: 0

Script:
#!/bin/bash
# Description: Check if the session management library used in the codebase provides thread-safe operations.
# Command: Search for the session management library and inspect its documentation or implementation for thread safety.

# Find the session management library used in the codebase
session_lib=$(rg --files-with-matches 'session.FromContext' | head -n 1)

# Check if the session management library provides thread-safe operations
if [ -n "$session_lib" ]; then
  rg 'thread-safe' "$session_lib"
else
  echo "Session management library not found."
fi
Length of output: 208

Script:
#!/bin/bash
# Description: Extract and inspect the implementation of the `Set` method in the session management library to verify thread safety.

# Find the implementation of the `Set` method in the session management library
ast-grep --lang go --pattern 'func ($_ $_) Set($_, $_) $_' middleware/csrf/session_manager.go
Length of output: 95

Script:
#!/bin/bash
# Description: Search the entire codebase for the implementation of the `Set` method to verify thread safety.

# Find the implementation of the `Set` method in the entire codebase
ast-grep --lang go --pattern 'func ($_ $_) Set($_, $_) $_'
Length of output: 1417
middleware/session/config.go (1)

13-15: Renaming Expiration to IdleTimeout clarifies the purpose and aligns with best practices.

middleware/session/middleware.go (1)

31-37: Well-structured Middleware struct with appropriate use of synchronization for thread safety.

middleware/session/session.go (1)

16-22: The addition of idleTimeout in the Session struct is a positive change for better session management.

middleware/session/config.go

sixcolors · 2024-05-28T20:53:52Z

Known Issues

Storage Adapter UPSERT Behavior

There is an issue with the way Fiber's storage adapters handle setting keys. The current operation for setting a key is an UPSERT, which means it will insert a new key if it does not exist or update it if it does. This can lead to issues in scenarios with simultaneous requests altering the same session. For example:

One request destroys the session.
Another request makes an inconsequential change to the session.

If the session destruction completes first, the inconsequential change could recreate the session, potentially preventing actions like logging out and posing security risks.

Since this PR changes the session behavior to save and update on every request, it amplifies the problem if not addressed.

sixcolors · 2024-05-28T21:09:24Z

See: https://github.com/gofiber/recipes/tree/v3-csrf-with-session-PR-3016-update/csrf-with-session for usage example of this PR in current state.

gaby · 2024-05-29T04:27:00Z

Those new sequence diagrams are awesome

middleware/csrf/session_manager.go

middleware/session/middleware.go

renanbastos93

Wow, you've been doing a good job. I commented on a few details. Thanks for contributing here.

middleware/csrf/session_manager.go

middleware/session/middleware.go

Co-authored-by: Renan Bastos <[email protected]>

codecov · 2024-05-30T17:27:15Z

Codecov Report

Attention: Patch coverage is 83.81503% with 56 lines in your changes missing coverage. Please review.

Project coverage is 82.76%. Comparing base (0b6a26f) to head (b54c954).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
middleware/csrf/session_manager.go	57.14%	14 Missing and 7 partials ⚠️
middleware/session/middleware.go	90.65%	7 Missing and 3 partials ⚠️
middleware/session/store.go	84.61%	7 Missing and 3 partials ⚠️
middleware/session/session.go	87.83%	7 Missing and 2 partials ⚠️
middleware/session/config.go	64.28%	3 Missing and 2 partials ⚠️
middleware/session/data.go	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3016      +/-   ##
==========================================
+ Coverage   80.11%   82.76%   +2.65%     
==========================================
  Files         117      114       -3     
  Lines        9044    11149    +2105     
==========================================
+ Hits         7246     9228    +1982     
- Misses       1364     1521     +157     
+ Partials      434      400      -34

Flag	Coverage Δ
unittests	`82.76% <83.81%> (+2.65%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

feat!(middleware/session): re-write session middleware with handler

6e76847

sixcolors requested a review from a team as a code owner May 28, 2024 20:46

sixcolors requested review from ReneWerner87, efectn and gaby and removed request for a team May 28, 2024 20:46

github-actions bot added the ✏️ Feature label May 28, 2024

sixcolors added v3 SessionMW labels May 28, 2024

sixcolors marked this pull request as draft May 28, 2024 20:47

coderabbitai bot reviewed May 28, 2024

View reviewed changes

middleware/session/config.go Outdated Show resolved Hide resolved

sixcolors added 3 commits May 28, 2024 18:13

test(middleware/session): refactor to IdleTimeout

ac9a028

fix: lint errors

81f6789

test: Save session after setting or deleting raw data in CSRF middleware

28790cb

gaby reviewed May 29, 2024

View reviewed changes

middleware/csrf/session_manager.go Show resolved Hide resolved

efectn reviewed May 29, 2024

View reviewed changes

middleware/session/middleware.go Outdated Show resolved Hide resolved

renanbastos93 reviewed May 30, 2024

View reviewed changes

sixcolors and others added 2 commits May 30, 2024 13:51

Update middleware/session/middleware.go

7ffae3d

Co-authored-by: Renan Bastos <[email protected]>

fix: mutex and globals order

68f2739

coderabbitai bot mentioned this pull request May 30, 2024

Improve Error Handling in CSRF Middleware Storage #3020

Closed

gaby mentioned this pull request May 30, 2024

Fix(🩹): error handling in CSRF token storage retrieval #3021

Closed

20 tasks

sixcolors added 4 commits June 4, 2024 12:14

feat: Re-Add read lock to session Get method

92e6877

feat: Migrate New() to return middleware

239db00

chore: Refactor session middleware to improve session handling

0b93c5c

chore: Private get on store

7cb4a6e

This was referenced Oct 7, 2025

🩹 Fix: golangci-lint issue for go1.25.0 #3775

Merged

📒 docs: Auto generated framework documentation #3793

Open

chore: bump module paths for v3 gofiber/contrib#1427

Closed

Uh oh!

feat!(middleware/session): re-write session middleware with handler #3016

feat!(middleware/session): re-write session middleware with handler #3016

Uh oh!

Conversation

sixcolors commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Feature: Re-write Session Middleware with Handler

Summary

Status

Changelog

Middleware Changes

CSRF Session Manager Changes

Configuration Changes

Session Struct Changes

Changes From This PR

Testing

Notes

Uh oh!

coderabbitai bot commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Assessment against linked issues

Poem

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sixcolors commented May 28, 2024

Known Issues

Storage Adapter UPSERT Behavior

Uh oh!

sixcolors commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaby commented May 29, 2024

Uh oh!

Uh oh!

Uh oh!

renanbastos93 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sixcolors commented May 28, 2024 •

edited

Loading

coderabbitai bot commented May 28, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

sixcolors commented May 28, 2024 •

edited

Loading

codecov bot commented May 30, 2024 •

edited

Loading