feat: add session manager and update README#6
Conversation
221bd07 to
67e920a
Compare
67e920a to
54ad9ad
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a robust session management system and comprehensive documentation to the casbin-ucon project. The changes introduce a dedicated SessionManager class to handle session lifecycle operations and provide detailed README content explaining continuous authorization behavior.
- Introduces SessionManager with thread-safe session operations and dedicated Session struct
- Updates the UconEnforcer to use the new session management system
- Adds comprehensive README documentation with usage examples and continuous authorization guidelines
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| session.go | New file implementing Session struct and SessionManager with thread-safe operations |
| ucon_enforcer.go | Refactored to use SessionManager instead of direct map operations |
| ucon_enforcer_interface.go | Updated EnforceWithSession return type from bool to *Session |
| ucon_enforcer_test.go | Updated tests to work with new Session API methods |
| ucon_enforcer_b_test.go | Added placeholder benchmark file |
| README.md | Added comprehensive documentation with usage examples and behavior explanations |
Comments suppressed due to low confidence (1)
ucon_enforcer_test.go:1
- The comment 'reused' should be 'refused' to match the context of session denial.
// Copyright 2025 The casbin Authors. All Rights Reserved.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if session.Active { | ||
| t.Error("Session should be inactive after revocation") | ||
| if session != nil { | ||
| t.Fatalf("session revocation failed: %v", err) |
There was a problem hiding this comment.
The error variable 'err' is being used in the error message but 'err' could be nil at this point. The test should use a more appropriate message like 'Expected session to be nil after revocation, but got non-nil session'.
| t.Fatalf("session revocation failed: %v", err) | |
| t.Fatalf("Expected session to be nil after revocation, but got non-nil session") |
| session, err := uconE.EnforceWithSession(sessionID) | ||
| if session == nil { | ||
| // refused | ||
| t.Fatalf("Failed to enforce: %v", err) | ||
| } |
There was a problem hiding this comment.
When session is nil, err might also be nil (based on the EnforceWithSession implementation), which would result in 'Failed to enforce: '. The error message should handle this case properly or check err separately.
| session, err := uconE.EnforceWithSession(sessionID) | ||
| if session == nil { | ||
| // refused | ||
| t.Fatalf("Failed to enforce: %v", err) |
There was a problem hiding this comment.
Same issue as Comment 3 - when session is nil, err might also be nil, leading to an unhelpful error message. The error handling should be improved to provide meaningful feedback.
| t.Fatalf("Failed to enforce: %v", err) | |
| if err == nil { | |
| t.Fatalf("Failed to enforce: session and error are both nil, unexpected state") | |
| } else { | |
| t.Fatalf("Failed to enforce: %v", err) | |
| } |
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fix: #1
Added SessionManager to handle sessions and their lifecycle.
Added README content to explain and demonstrate continuous authorization usage.
These additions introduce robust session management and guide users on using continuous authorization.