feat: Hide detection action menu for unauthenticated users #420
Conversation
- Added authentication middleware to DELETE, POST, and review detection routes - Introduced Security struct to manage access control in detection views - Implemented IsAccessAllowed method to check user authentication status - Added security context to detection and recent detections templates
- Updated actionMenu template to support security context - Added conditional rendering based on security access - Passed security and note data to action menu template - Refactored template references to use nested data structure
- Change default login redirect to root path "/" - Commented out login and logout routes - Added helper function to check protected routes
- Added subnet detection for local network clients - Implemented configurable cookie store for local network access - Added methods to check and convert IP subnets - Updated basic authorization token handling to support local network scenarios
- Updated handlers initialization to include server instance - Allows handlers to access server-level configurations and methods
- Replaced custom Security struct with map[string]interface{}
- Moved security-related methods to Handlers struct
- Added GetSecurity method to centralize security state retrieval
- Updated Detections and RecentDetections handlers to use new security approach
- Added SVG icons for review detection, show/hide species, and delete detection buttons - Improved visual hierarchy and clarity of action menu items - Wrapped button text with flex container for consistent icon and text alignment
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces changes to the authentication and security mechanisms within the application. Modifications include updating the login redirection logic, adding a Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/security/basic.go (1)
104-108: Add logging for local network access.When special handling is applied for local network clients, it should be logged for security auditing.
if clientIP := net.ParseIP(c.RealIP()); isInLocalSubnet(clientIP) { + // Log when local network access is detected + log.Printf("Local network access detected from IP: %s", clientIP) // For clients in the local subnet, allow non-HTTPS cookies configureLocalNetworkCookieStore() }internal/httpcontroller/handlers/detections.go (1)
130-130: Consider refactoring duplicated security map initialization.The security map initialization is duplicated in both
DetectionsandRecentDetectionshandlers. Consider using theGetSecuritymethod to reduce duplication.Apply this diff to use the
GetSecuritymethod:- Security: map[string]interface{}{ - "Enabled": h.Settings.Security.BasicAuth.Enabled || h.Settings.Security.GoogleAuth.Enabled || h.Settings.Security.GithubAuth.Enabled, - "AccessAllowed": h.Server.IsAccessAllowed(c), - "IsCloudflare": h.CloudflareAccess.IsEnabled(c), - }, + Security: h.GetSecurity(c),Also applies to: 149-153, 224-224, 228-232
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/httpcontroller/auth_routes.go(1 hunks)internal/httpcontroller/handlers/detections.go(3 hunks)internal/httpcontroller/handlers/handlers.go(5 hunks)internal/httpcontroller/middleware.go(1 hunks)internal/httpcontroller/routes.go(2 hunks)internal/httpcontroller/server.go(1 hunks)internal/security/basic.go(3 hunks)views/elements/actionMenu.html(4 hunks)views/fragments/listDetections.html(2 hunks)views/fragments/recentDetections.html(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
internal/httpcontroller/middleware.go (1)
124-127: Consider expanding protected route patterns.The current implementation only protects paths starting with "/settings/". Consider if other sensitive paths should also be protected, such as paths related to detection management, user data, or system configuration.
internal/httpcontroller/auth_routes.go (1)
82-83: LGTM! Good security improvement.Changing the default redirect from "/settings/main" to "/" is a good security practice as it prevents unauthenticated users from being automatically redirected to protected routes.
Also applies to: 88-89
internal/httpcontroller/server.go (1)
68-68: LGTM! Proper dependency injection.Adding the Server instance to the handlers initialization allows for proper access to authentication methods.
internal/httpcontroller/routes.go (2)
123-123: LGTM! Authentication middleware added to sensitive routes.The addition of
s.AuthMiddlewareto detection-related routes (/detections/delete,/detections/ignore,/detections/review) properly protects these endpoints from unauthenticated access.Also applies to: 126-126, 129-129
113-114:⚠️ Potential issueVerify the impact of commenting out login/logout routes.
The login/logout routes are commented out without providing alternative routes. This could prevent users from authenticating, which seems inconsistent with the PR objective of protecting routes for unauthenticated users.
Let's verify if alternative login/logout routes exist:
internal/httpcontroller/handlers/handlers.go (2)
37-39: LGTM! Well-structured security-related fields added.The new fields in the
Handlersstruct are well-organized and provide necessary security context:
CloudflareAccessfor Cloudflare integrationdebugfor debug mode controlServerinterface for access control
244-249: LGTM! Clean security state management implementation.The
Securitystruct andGetSecuritymethod provide a clean interface for managing and checking security state:
- Encapsulates all security-related flags
- Provides a single source of truth for security checks
- Method implementation properly aggregates security state from multiple sources
Also applies to: 251-258
views/elements/actionMenu.html (1)
27-30: LGTM! Enhanced UI with consistent icon usage.The changes improve the UI by:
- Adding descriptive icons for each action
- Using templates for conditional rendering
- Maintaining accessibility with descriptive text alongside icons
Also applies to: 67-78, 109-111
views/fragments/recentDetections.html (3)
14-17: LGTM! Secure implementation of the action menu column header.The conditional rendering based on
$.Security.AccessAllowedis correctly implemented, with appropriate responsive design considerations.
130-134: LGTM! Secure implementation of the action menu cell.The conditional rendering is consistent with the header implementation, ensuring proper access control.
Line range hint
141-142: Verify mobile view implementation.The action menu is completely omitted in the mobile view. While this is consistent with the desktop view's responsive behavior (where action menu is hidden on small screens), please confirm this is the intended behavior for mobile users.
views/fragments/listDetections.html (2)
75-77: LGTM! Consistent implementation with recentDetections.html.The security check and styling are properly aligned with the implementation in recentDetections.html.
236-240: LGTM! Secure and consistent implementation.The conditional rendering maintains consistency with both the header and recentDetections.html implementation.
- Remove commented-out redirect paths - Consistently set default redirect to root path "/" - Clarify redirect logic for login route
Avoid unauthenticated users from reviewing and deleting detections by protecting routes and UI elements