-
-
Notifications
You must be signed in to change notification settings - Fork 254
Add ThrowIfContainsExpiredAccessToken method to Boilerplate (#10951) #10952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes introduce two new extension methods to the HTTP context for detecting expired access tokens and update relevant authentication checks in API and SignalR controllers to use these methods. This centralizes and simplifies the logic for handling expired or invalid tokens in requests that allow anonymous access. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server (API/SignalR)
participant HttpContextExtensions
Client->>Server (API/SignalR): Sends request with/without Authorization header
Server (API/SignalR)->>HttpContextExtensions: Call ContainsExpiredAccessToken/ThrowIfContainsExpiredAccessToken
alt Expired Access Token Detected
HttpContextExtensions-->>Server (API/SignalR): Throws UnauthorizedException/HubException
Server (API/SignalR)-->>Client: Responds with Unauthorized error
else Valid or No Token
HttpContextExtensions-->>Server (API/SignalR): No exception
Server (API/SignalR)-->>Client: Proceeds with request
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a centralized mechanism to detect and handle expired access tokens by adding helper methods in the HTTP context extensions and updating related components to use this logic.
- Removed inline authentication checks in AppHub and PushNotificationController in favor of using new helper methods.
- Added two extension methods, ContainsExpiredAccessToken and ThrowIfContainsExpiredAccessToken, to encapsulate expired token detection and handling.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs | Updated token validation logic using ContainsExpiredAccessToken with a HubException for expired tokens. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/HttpContextExtensions.cs | Added extension methods for token checks and centralized exception throwing. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/PushNotification/PushNotificationController.cs | Replaced inline token validation with a call to ThrowIfContainsExpiredAccessToken. |
Comments suppressed due to low confidence (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs:30
- There is an inconsistency in the exception types thrown: AppHub.cs throws a HubException with additional connection data, whereas ThrowIfContainsExpiredAccessToken in HttpContextExtensions throws an UnauthorizedException. Consider unifying the exception type for consistent error handling across your API and SignalR endpoints.
if (Context.GetHttpContext()?.ContainsExpiredAccessToken() is true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/HttpContextExtensions.cs (1)
41-41: Fix XML documentation syntax error.There's an extra closing tag in the inheritdoc reference.
- /// <inheritdoc cref="ThrowIfContainsExpiredAccessToken(HttpContext)"/>/> + /// <inheritdoc cref="ThrowIfContainsExpiredAccessToken(HttpContext)"/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/PushNotification/PushNotificationController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/HttpContextExtensions.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/HttpContextExtensions.cs (1)
ContainsExpiredAccessToken(43-46)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/PushNotification/PushNotificationController.cs (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/HttpContextExtensions.cs (1)
ThrowIfContainsExpiredAccessToken(34-38)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/HttpContextExtensions.cs (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ClaimsPrincipalExtensions.cs (1)
IsAuthenticated(5-8)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/PushNotification/PushNotificationController.cs (1)
17-17: LGTM! Clean centralization of expired token detection.The use of the new extension method effectively centralizes the expired access token detection logic, improving code maintainability and consistency across the API.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs (1)
30-31: LGTM! Appropriate SignalR-specific implementation.Good use of the boolean extension method variant with SignalR-specific exception handling. The null-conditional operator provides proper safety, and including the ConnectionId in the exception data aids debugging.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/HttpContextExtensions.cs (2)
26-38: Excellent design with comprehensive documentation.The method design follows good practices:
- Clear separation between boolean check and exception throwing
- Comprehensive XML documentation explaining the use case
- Appropriate exception type for the scenario
The logic for detecting expired tokens (unauthenticated user + present Authorization header) is a reasonable heuristic that covers the common case where clients have expired tokens.
43-46: Solid implementation of expired token detection logic.The boolean logic correctly identifies the scenario where a client likely has an expired access token. The combination of an unauthenticated user with a present Authorization header is a good heuristic for this detection.
closes #10951
Summary by CodeRabbit