Improve Boilerplate refresh token rotation (#9528)#9530
Improve Boilerplate refresh token rotation (#9528)#9530msynk merged 9 commits intobitfoundation:developfrom yasmoradi:9528-boilerplate-project-template-refresh-token-roation-needs-improvements
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several enhancements to the Bit Boilerplate project's authentication and token management system. The changes focus on improving refresh token handling, error management, and telemetry context integration. Key modifications include adding a new Changes
Assessment against linked issues
Poem
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 (
|
…mplate-refresh-token-roation-needs-improvements
…-needs-improvements' of https://github.com/ysmoradi/bitplatform into 9528-boilerplate-project-template-refresh-token-roation-needs-improvements
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/Identity/RefreshRequestDto.cs (1)
14-14: Add a validation attribute or clarify usage.
Adding the new DeviceInfo property is helpful for analytics and security purposes. If you intend to enforce specific formats or content (e.g., device fingerprint), consider adding a validation attribute or clarifying usage in the documentation.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs (1)
116-121: Ensure consistency of DeviceInfo usage.
telemetryContext.Platform is being passed as DeviceInfo, which implies all device-related metadata is contained in Platform. Verify that no relevant device fields are missing or implied. If you plan to extend device info, consider a more descriptive property name.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs (2)
221-221: Provide more context in UnauthorizedException.
Currently, throwing a generic UnauthorizedException offers limited feedback. Consider elaborating with a localized or descriptive message (e.g., user-friendly detail about token expiration) to aid in debugging and user support.
223-223: UnauthorizedException usage for security stamp mismatch.
Again, consider adding a more descriptive message or custom error type to distinguish security stamp mismatches from other 401 scenarios, helping frontends and logs differentiate root causes..github/workflows/bit.full.ci.yml (3)
146-146: “continue-on-error” can mask test failures.
By setting “continue-on-error” to true, failing steps will not stop the workflow. Ensure that important errors (e.g., migrations, major test regressions) do not proceed silently.
161-161: Consider using condition-based skipping instead of “continue-on-error”.
If certain tests are non-blocking or optional, a better alternative might be condition-based run logic, so essential steps remain mandatory.
192-192: Confirm necessity for ignoring sample config 2 errors.
Double-check whether skipping errors in this specialized matrix is truly warranted. Some failures may expose hidden compatibility issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/bit.full.ci.yml(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Dashboard/DashboardPage.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientPubSubMessages.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/Identity/RefreshRequestDto.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Exceptions/ReusedRefreshTokenException.cs(1 hunks)
🔇 Additional comments (12)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Exceptions/ReusedRefreshTokenException.cs (1)
1-40: Good clarity in error messaging and status code.
The dedicated exception clearly identifies a reused refresh token scenario and returns an HTTP 401 (Unauthorized) status, aligning with security best practices. This approach improves maintainability and readability.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientPubSubMessages.cs (2)
33-35: PROFILE_UPDATED constant usage.
The new PROFILE_UPDATED constant is conditionally added when signalR is disabled. Revisit usage across the codebase to ensure it’s invoked only when relevant.
✅ Verification successful
Let me search for the usage of PROFILE_UPDATED constant across the codebase.
No issues with PROFILE_UPDATED constant usage
The constant is correctly defined and used across the codebase. When SignalR is disabled (#if (signalR != true)), the constant is defined in ClientPubSubMessages.cs. When SignalR is enabled, the constant is used from SharedPubSubMessages.cs instead. The usage in components like UserMenu.razor.cs and ProfileSection.razor.cs is consistent with this pattern, as they reference the constant through the appropriate class based on the SignalR configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for PROFILE_UPDATED usage across the codebase
rg "PROFILE_UPDATED" -B 2 -A 2
Length of output: 5399
6-9: Conditional inheritance is appropriate; confirm shared messages usage.
The partial class conditionally inherits from SharedPubSubMessages based on the signalR compilation symbol. This design is flexible but ensure that references to SharedPubSubMessages don’t inadvertently break when signalR is disabled.
✅ Verification successful
Let me gather information about the usage of SharedPubSubMessages and related dependencies.
Let me check the implementation of SharedPubSubMessages to understand how it's used when signalR is disabled.
Based on the analysis of the code, I can now provide the final response:
Conditional inheritance is correctly implemented and safe
The design ensures proper message handling in both SignalR and non-SignalR scenarios. When SignalR is disabled, ClientPubSubMessages defines its own constants, and when SignalR is enabled, it inherits from SharedPubSubMessages which contains the shared message definitions. The codebase consistently checks for SignalR configuration before using any shared messaging features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for SharedPubSubMessages class definition and usages
rg "SharedPubSubMessages" -l
# Search for any references to ClientPubSubMessages
rg "ClientPubSubMessages" -l
# Look for any conditional compilation directives related to signalR
rg "#if.*signalR" -A 2
Length of output: 28872
Script:
#!/bin/bash
# Check the implementation of SharedPubSubMessages
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs
# Check how ClientPubSubMessages is used when signalR is disabled
rg "ClientPubSubMessages" -A 3 src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientPubSubMessages.cs
Length of output: 1355
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Dashboard/DashboardPage.razor.cs (1)
21-21: PubSub subscription reference updated to SharedPubSubMessages.
Switching to SharedPubSubMessages centralizes messaging, but double-check that no code still relies on ClientPubSubMessages for “DASHBOARD_DATA_CHANGED.” Confirm all references are consistently updated.
✅ Verification successful
Let me generate shell scripts to verify if there are any remaining references to "DASHBOARD_DATA_CHANGED" in ClientPubSubMessages or any inconsistent usage across the codebase.
Based on the search results, I can now generate the final response:
Verified: DASHBOARD_DATA_CHANGED is consistently using SharedPubSubMessages
All references to DASHBOARD_DATA_CHANGED in the codebase are using SharedPubSubMessages:
- Defined in SharedPubSubMessages.cs
- Used in DashboardPage.razor.cs for subscription
- Used in ProductController.cs and CategoryController.cs for SignalR broadcasts
No references to ClientPubSubMessages.DASHBOARD_DATA_CHANGED were found, confirming the migration is complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any references to DASHBOARD_DATA_CHANGED in the codebase
rg "DASHBOARD_DATA_CHANGED" -A 2 -B 2
# Search for ClientPubSubMessages class definition and its contents
ast-grep --pattern 'class ClientPubSubMessages {
$$$
}'
# Search for SharedPubSubMessages class definition and its contents
ast-grep --pattern 'class SharedPubSubMessages {
$$$
}'
Length of output: 3821
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs (2)
18-18: Consider validating telemetryContext before usage.
While using "default!" is acceptable in certain scenarios, it's good practice to confirm that dependency injection always populates telemetryContext. If there's any possibility that telemetryContext could be null, consider adding validations in your initialization logic.
131-131: nonInterrupting usage may mask critical errors.
Here, you are designating ReusedRefreshTokenException as non-interrupting. While this prevents user interruption, ensure that no vital security or usage metrics are being silently suppressed. Review logs and monitoring to confirm that repeated misuse is still caught.
✅ Verification successful
Let me search for more context about how the ReusedRefreshTokenException and non-interrupting errors are handled in the codebase.
Let me search for more context about the error handling and logging mechanisms to understand how these exceptions are processed.
Let me search for the specific handling of ReusedRefreshTokenException and how it's logged.
Based on the gathered information, I can now provide a final response:
Marking ReusedRefreshTokenException as non-interrupting is appropriate and secure.
The current implementation is correct because:
- The exception is properly logged and tracked through the
authLogger.LogInformationcall in the refresh token process - The system maintains security by clearing tokens via
ClearTokens()when anUnauthorizedExceptionoccurs - The ReusedRefreshTokenException is part of a robust token rotation security mechanism that prevents token reuse attacks
- The non-interrupting flag only affects UI behavior, not the security enforcement
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for ReusedRefreshTokenException definition and usage
rg "ReusedRefreshTokenException" -A 5
# Search for nonInterrupting parameter usage patterns
rg "nonInterrupting:" -A 5
# Look for error handling and logging patterns around these exceptions
ast-grep --pattern 'catch ($_ exp) {
$$$
nonInterrupting: $$$
$$$
}'
Length of output: 10746
Script:
#!/bin/bash
# Search for the RestException base class and its handling
rg "RestException" -A 5
# Look for the exception handler implementation
rg "IExceptionHandler" -A 10
# Search for logging patterns around exceptions
rg "LogError|LogWarning" -A 5
Length of output: 82680
Script:
#!/bin/bash
# Look for the context around where ReusedRefreshTokenException is thrown
rg -B 10 "throw new ReusedRefreshTokenException"
# Search for logging related to refresh tokens
rg -B 5 "refresh.*token" --ignore-case
# Look for the implementation of the exception handler that processes nonInterrupting exceptions
ast-grep --pattern 'Handle($_, nonInterrupting: $_)'
Length of output: 45114
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs (3)
228-228: Behavior upon deleted user session.
Throwing UnauthorizedException if the session is deleted is valid. Ensure user feedback is clear so they can re-login or refresh the session if needed.
231-231: Meaningful custom exception for reused token.
Replacing UnauthorizedException with ReusedRefreshTokenException is an excellent improvement, making logs more precise about this misuse scenario. Keep error messages consistent with the rest of your code to ensure uniform user feedback.
254-254: Potential security considerations for DeviceInfo.
Storing device info in userSession can expose user details if logs or analytics are not properly secured. Confirm that sensitive device facts (e.g., unique IDs) are properly sanitized or protected.
✅ Verification successful
Let me gather more information about how DeviceInfo is handled in the codebase.
Based on the search results, I can now provide a final response as I have enough context about how DeviceInfo is handled in the codebase:
DeviceInfo usage appears to be properly implemented with minimal security risk
The DeviceInfo field is:
- Only storing basic platform/OS information (e.g. "Samsung Android 14") for session tracking
- Used primarily for UI display purposes in the sessions management screen
- Populated from telemetry context on the client side, not arbitrary user input
- Stored alongside other session metadata like IP and location
- Not containing any sensitive device identifiers or unique hardware IDs
The implementation follows security best practices by:
- Only capturing high-level device platform details
- Using it for legitimate session management purposes
- Storing it with appropriate access controls as part of the user session data
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for DeviceInfo class/type definition
ast-grep --pattern 'class DeviceInfo {
$$$
}'
# Search for any usage or handling of DeviceInfo
rg "DeviceInfo" -A 3 -B 3
Length of output: 31729
.github/workflows/bit.full.ci.yml (2)
186-186: Potential confusion for advanced sample tests.
When advanced sample configurations fail, continuing the pipeline might mislead maintainers into thinking everything passed. Ensure results are clearly reported or that a separate job tracks these step outcomes.
172-172: Risk of partial coverage.
Marking specialized environment steps as “continue-on-error” may limit full coverage for multi-server or multi-environment scenarios. Evaluate whether partial coverage is acceptable for your project’s CI gating.
src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json (1)
348-348: Renaming solution file is a straightforward improvement.
Changing from “Boilerplate.Web.slnf” to “Boilerplate.sln” clarifies the solution’s primary entry point. Make sure any documentation or scripts referencing the old filename is updated accordingly.
msynk
changed the title
2 participants
closes #9528
Summary by CodeRabbit
New Features
DeviceInfoproperty to the refresh request to capture device-related information.Bug Fixes
Documentation
Chores