prevent re-using refresh token in Boilerplate (#9335) #9336
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 changes in this pull request focus on removing support for the Cosmos database across various components of the Bit platform CI and its associated templates. This includes the elimination of Cosmos-related test steps, configuration options, and references in multiple files. The modifications streamline the database options to SqlServer, Sqlite, PostgreSQL, and MySQL, while enhancing session management and security features in the identity controller. Additionally, several UI components and configuration files have been updated to reflect these changes, ensuring consistency throughout the application. Changes
Assessment against linked issues
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: 8
🧹 Outside diff range and nitpick comments (11)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AuthPolicies.cs (1)
43-48: Consider adding rate limiting for refresh token attemptsWhile the session stamp effectively prevents token reuse, consider enhancing security further by implementing rate limiting for refresh token attempts to prevent brute force attacks.
Would you like me to provide an example implementation of rate limiting for refresh token endpoints?
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/SessionsSection.razor (2)
23-29: Consider adding tooltips for privilege indicatorsThe visual indicators (✓/✘) for privileged sessions are good, but might benefit from tooltips to explain their meaning to users.
Consider adding a tooltip attribute:
-ImageInitials="@(currentSession.Privileged ? "✓" : "✘")" +ImageInitials="@(currentSession.Privileged ? "✓" : "✘")" +Tooltip="@(currentSession.Privileged ? Localizer[nameof(AppStrings.PrivilegedSession)] : Localizer[nameof(AppStrings.StandardSession)])"
Line range hint
1-67: Review security considerations for session managementWhile the UI improvements enhance visibility of session information, please ensure:
- The component is only accessible to authenticated users with appropriate permissions
- Consider masking sensitive information (e.g., partial IP addresses) for security
- Implement rate limiting for session revocation to prevent abuse
- Add confirmation dialog before session revocation to prevent accidental termination
Consider implementing these security enhancements:
- Add rate limiting middleware for session management endpoints
- Implement audit logging for session revocation events
- Use secure session storage mechanisms
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/AppDbContext.cs (2)
Line range hint
48-63: Enhance error handling for refresh token operationsWhile the current concurrency exception handling is good, consider adding specific handling for refresh token operations to provide clearer error messages and proper audit logging.
public override async Task<int> SaveChangesAsync(bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = new CancellationToken()) { try { return await base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken); } catch (DbUpdateConcurrencyException exception) { + // Check if the concurrency exception is related to refresh token + if (exception.Entries.Any(e => e.Entity is UserSession)) + { + throw new ConflictException(nameof(AppStrings.RefreshTokenAlreadyUsed), exception); + } throw new ConflictException(nameof(AppStrings.UpdateConcurrencyException), exception); } }
Line range hint
1-171: Consider implementing a dedicated refresh token serviceTo better handle refresh token security across different database providers, consider extracting token management logic into a dedicated service that can:
- Implement consistent token invalidation across all database providers
- Provide proper audit logging for security events
- Handle token cleanup and expiration
- Implement rate limiting for token refresh attempts
This would help ensure consistent security regardless of the underlying database provider.
Would you like me to provide a detailed design for this service?
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (3)
Line range hint
66-82: Prevent race conditions in session terminationThe current implementation might allow refresh token reuse during the brief window between token validation and session removal. Consider marking the session as used first, then removing it.
Here's a suggested implementation using a transaction:
[HttpPost] public async Task SignOut(CancellationToken cancellationToken) { var currentSessionId = User.GetSessionId(); + await using var transaction = await DbContext.Database.BeginTransactionAsync(cancellationToken); var userSession = await DbContext.UserSessions - .FirstOrDefaultAsync(us => us.Id == currentSessionId, cancellationToken) ?? throw new ResourceNotFoundException(); + .FirstOrDefaultAsync(us => us.Id == currentSessionId && !us.IsUsed, cancellationToken) + ?? throw new ResourceNotFoundException("Session not found or already terminated"); + + userSession.IsUsed = true; + await DbContext.SaveChangesAsync(cancellationToken); DbContext.UserSessions.Remove(userSession); await DbContext.SaveChangesAsync(cancellationToken); + await transaction.CommitAsync(cancellationToken); SignOut(); }
Line range hint
397-398: Enhance elevated access token securityThe comment indicates that elevated access token claims are added during refresh token requests, but there's no mechanism to prevent replay attacks or ensure the refresh token hasn't been reused.
Consider implementing the following security measures:
- Add a nonce to the elevated access token claim
- Store the nonce in the UserSession record
- Validate the nonce during token refresh
- Invalidate the nonce after successful validation
This ensures that even if a refresh token is somehow reused, the elevated access token claim cannot be replayed.
Line range hint
1-1: Summary of Required Security ImprovementsTo fully prevent refresh token reuse as per PR objectives, the following changes are needed:
- Add
IsUsedflag to UserSessions table- Implement token rotation or single-use mechanism
- Use transactions for atomic session updates
- Add nonce validation for elevated access tokens
Would you like assistance in implementing these security improvements?
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20241126123435_InitialMigration.Designer.cs (1)
744-745: Security enhancement: Proper cleanup of push notification subscriptionsThe change to
OnDelete(DeleteBehavior.SetNull)for the UserSession-PushNotificationSubscription relationship is a good security practice. When a user session ends, the push notification subscription is not immediately deleted but rather unlinked, which:
- Prevents orphaned push notification channels
- Maintains audit trail
- Allows for proper cleanup of notification tokens
Consider implementing a background job to clean up unused push notification subscriptions after a certain period to prevent accumulation of stale records.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs (2)
Line range hint
284-288: Avoid possible race conditions inSendOtpmethodThe
resendDelaycalculation may result in negative values ifOtpRequestedOnis in the future ornull. Additionally, there is a potential for race conditions if multiple requests are made simultaneously. Implement proper checks to handle these scenarios and ensure thread safety.Consider updating the code as follows:
-var resendDelay = (DateTimeOffset.Now - user.OtpRequestedOn) - AppSettings.Identity.OtpTokenLifetime; +var timeSinceLastRequest = DateTimeOffset.Now - (user.OtpRequestedOn ?? DateTimeOffset.MinValue); +var resendDelay = AppSettings.Identity.OtpTokenLifetime - timeSinceLastRequest; -if (resendDelay < TimeSpan.Zero) +if (resendDelay > TimeSpan.Zero) throw new TooManyRequestsExceptions(Localizer[nameof(AppStrings.WaitForOtpRequestResendDelay), resendDelay.Humanize(culture: CultureInfo.CurrentUICulture)]);
Line range hint
327-331: Correct the logic forresendDelaycalculation inSendTwoFactorTokenSimilar to the
SendOtpmethod, the calculation ofresendDelaymay produce incorrect results ifTwoFactorTokenRequestedOnisnull. Adjust the logic to handle null values and ensure accurate delay calculation.Update the code to handle null values:
-var resendDelay = (DateTimeOffset.Now - user.TwoFactorTokenRequestedOn) - AppSettings.Identity.TwoFactorTokenLifetime; +var timeSinceLastRequest = DateTimeOffset.Now - (user.TwoFactorTokenRequestedOn ?? DateTimeOffset.MinValue); +var resendDelay = AppSettings.Identity.TwoFactorTokenLifetime - timeSinceLastRequest; -if (resendDelay < TimeSpan.Zero) +if (resendDelay > TimeSpan.Zero) throw new TooManyRequestsExceptions(Localizer[nameof(AppStrings.WaitForTwoFactorTokenRequestResendDelay), resendDelay.Humanize(culture: CultureInfo.CurrentUICulture)]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
.github/workflows/bit.full.ci.yml(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/ProfileSection.razor.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/SessionsSection.razor(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages.props(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages8.props(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Boilerplate.Server.Api.csproj(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/AppDbContext.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Identity/RoleConfiguration.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Identity/UserConfiguration.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/PushNotification/PushNotificationSubscriptionConfiguration.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20241126123435_InitialMigration.Designer.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20241126123435_InitialMigration.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/AppDbContextModelSnapshot.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/appsettings.json(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/appsettings.json(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/Identity/UserSessionDto.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AuthPolicies.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs(0 hunks)src/Websites/Platform/src/Bit.Websites.Platform.Client/Pages/Templates/Templates05CreateProjectPage.razor(1 hunks)src/Websites/Platform/src/Bit.Websites.Platform.Client/Pages/Templates/Templates05CreateProjectPage.razor.cs(0 hunks)
💤 Files with no reviewable changes (15)
- src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/ProfileSection.razor.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages.props
- src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages8.props
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Boilerplate.Server.Api.csproj
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Identity/RoleConfiguration.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Identity/UserConfiguration.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/appsettings.json
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/appsettings.json
- src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/Identity/UserSessionDto.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs
- src/Websites/Platform/src/Bit.Websites.Platform.Client/Pages/Templates/Templates05CreateProjectPage.razor.cs
🔇 Additional comments (16)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/PushNotification/PushNotificationSubscriptionConfiguration.cs (2)
13-13: LGTM! Improved security through automatic FK nullification
The change from ClientSetNull to SetNull enhances security by ensuring database-level cleanup of push notification subscriptions when user sessions are invalidated, which aligns with the goal of preventing refresh token reuse.
13-13: Verify session invalidation behavior
Let's ensure this change doesn't affect the session invalidation logic elsewhere in the codebase.
✅ Verification successful
Setting UserSession to null is safe for session invalidation
The change to use DeleteBehavior.SetNull for the PushNotificationSubscription's UserSession relationship is safe and consistent with the session invalidation logic in the codebase. Here's why:
- The UserSession deletion is properly handled in the UserController where sessions are explicitly removed using
DbContext.UserSessions.Remove()during sign-out and session revocation. - The SetNull behavior only affects the PushNotificationSubscription table by nulling the UserSessionId foreign key when the associated session is deleted, which is the desired behavior.
- The session invalidation flow remains intact as the UserSession entity itself is still deleted with Cascade behavior from its parent User entity.
- Push notification subscriptions are designed to outlive sessions, as evidenced by the nullable UserSessionId and SetNull delete behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for session invalidation logic that might be affected
# Look for session invalidation patterns
rg -A 5 "InvalidateSession|DeleteSession|RemoveSession"
# Check for UserSession deletion handling
ast-grep --pattern 'UserSession.Delete$$$'
# Look for refresh token invalidation logic
rg -A 5 "RefreshToken.*invalid|invalid.*RefreshToken"
Length of output: 5548
Script:
#!/bin/bash
# Let's check for session and token related configurations and handlers
# Check for session and token related configurations
ast-grep --pattern 'class $_Configuration : IEntityTypeConfiguration<$_> {
$$$
}'
# Look for session deletion or token invalidation in controllers/services
rg -A 10 "UserSession|RefreshToken" --type cs
# Check for cascade delete configurations
rg -A 5 "DeleteBehavior" --type cs
Length of output: 101019
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AuthPolicies.cs (1)
43-48: Implementation effectively prevents refresh token reuse!
The addition of SESSION_STAMP claim type with its concurrency stamp mechanism is a robust approach to prevent refresh token reuse. This implementation aligns well with security best practices by ensuring tokens are single-use and tied to specific sessions.
Let's verify the complete implementation of this security measure:
✅ Verification successful
SESSION_STAMP is properly implemented to prevent refresh token reuse
The verification confirms that SESSION_STAMP is correctly integrated into the authentication flow:
- During sign-in: A new session stamp is created using the session's start time
- During refresh: The system validates the session stamp from the refresh token against the stored session timestamp (RenewedOn or StartedOn)
- If timestamps don't match, the system throws UnauthorizedException, effectively preventing token reuse
- The session stamp is updated (RenewedOn) after successful refresh, ensuring single-use tokens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the SESSION_STAMP is properly integrated in the authentication flow
# Expected: Find usage in SignIn/Refresh methods of IdentityController
# Check for SESSION_STAMP usage in authentication-related files
rg -l "SESSION_STAMP" --type cs
# Look for refresh token handling in IdentityController
ast-grep --pattern 'class IdentityController {
$$$
Refresh($$$) {
$$$
}
$$$
}'
Length of output: 437
Script:
#!/bin/bash
# Let's examine the implementation details in IdentityController
rg "SESSION_STAMP" -B 5 -A 10 src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
# Also check for refresh token related methods
ast-grep --pattern 'RefreshToken($$$) {
$$$
}'
Length of output: 3012
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/SessionsSection.razor (1)
46-52: Verify session revocation implementation
While the UI changes for displaying sessions look good, it's crucial to ensure that the session revocation functionality (triggered by the delete button) properly invalidates the refresh token to prevent reuse.
Let's verify the session revocation implementation:
✅ Verification successful
Let's gather more specific information about the session revocation implementation.
Let's search for more details about the session handling and token invalidation:
Based on the gathered information, I can now generate the final response:
Session revocation implementation is properly secured
The session revocation implementation is well-designed with multiple security measures in place:
- Server-side validation prevents revoking the current session (must use SignOut instead)
- Session revocation triggers client-side token cleanup via PubSub notifications
- Refresh tokens are protected and validated with security stamps
- Client properly handles token cleanup during SignOut
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the RevokeSession implementation and related token invalidation logic
ast-grep --pattern 'RevokeSession($$$) {
$$$
}'
# Look for refresh token handling in the codebase
rg -l 'refresh.*token|token.*refresh' --type cs
Length of output: 1276
Script:
#!/bin/bash
# Check UserController for session revocation endpoint
rg -A 10 "RevokeSession|RevokeSessions?" "src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs"
# Check AuthManager for client-side session handling
rg -A 10 "RevokeSession|RevokeSessions?" "src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs"
# Look for refresh token invalidation in IdentityController
rg -A 10 "InvalidateRefreshToken|RevokeToken" "src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs"
Length of output: 1316
Script:
#!/bin/bash
# Check UserSessions table structure and handling
rg -A 10 "class UserSession" src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Models/Identity/
# Look for refresh token handling in IdentityController
rg -A 10 "RefreshToken|SignOut" src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
# Check for token validation/invalidation in AuthManager
rg -A 10 "SignOut|InvalidateToken" src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs
Length of output: 3664
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/AppDbContext.cs (1)
Line range hint 19-19: Verify UserSession entity configuration for refresh token security
Given the PR's objective to prevent refresh token reuse, ensure that the UserSession entity has proper configuration in the Configurations folder to enforce token invalidation constraints.
✅ Verification successful
Let me gather more information about the UserSession model and its configuration.
Let me check the IdentityController implementation to understand how refresh tokens are handled with UserSessions.
Based on the code review, I can now provide the final response:
Refresh token reuse prevention is properly implemented
The UserSession entity and refresh token handling in IdentityController.cs effectively prevents refresh token reuse through:
- Session stamp validation: Each refresh token contains a session stamp that must match the current session's RenewedOn/StartedOn timestamp
- Session invalidation: If a refresh token is reused, the session stamp validation will fail and the session is deleted
- Automatic session renewal: RenewedOn timestamp is updated with each successful refresh, invalidating previous refresh tokens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UserSession configuration
ast-grep --pattern 'class UserSessionConfiguration : IEntityTypeConfiguration<UserSession>
{
$$$
}'
# Check for any existing refresh token handling
rg -l "RefreshToken|UserSession" src/
Length of output: 4090
Script:
#!/bin/bash
# Check UserSession model
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Models/Identity/UserSession.cs
# Check migrations for UserSession table configuration
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20241126123435_InitialMigration.cs
# Check if there's any specific configuration in Data/Configurations
fd -e cs -p "UserSessionConfiguration.cs" src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Identity/
# Check IdentityController for refresh token handling
rg -A 5 "RefreshToken" src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
Length of output: 26557
Script:
#!/bin/bash
# Check IdentityController for sign-in and refresh token handling
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
# Check if there are any other files handling refresh tokens
rg -l "RefreshToken.*UserSession" src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/
Length of output: 20735
.github/workflows/bit.full.ci.yml (1)
141-141: LGTM: Database options test step updated correctly
The test step has been updated to remove Cosmos DB tests while maintaining coverage for PostgreSQL, MySql, and Other database options. This aligns with the architectural changes to streamline database support.
Let's verify the database options are consistently defined across the codebase:
✅ Verification successful
Database support changes verified successfully
Based on the codebase analysis, the removal of Cosmos DB and streamlining of database options to PostgreSQL, MySQL, SQLite, and SQL Server is consistently implemented. The search results confirm:
- No remaining Cosmos DB or DocumentDB references in configuration files or code
- Database providers are properly configured in
Program.Services.csfor:- SQLite
- SQL Server
- PostgreSQL
- MySQL
- Connection strings in
appsettings.jsonare updated accordingly for the supported databases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining Cosmos DB references in configuration files
rg -i 'cosmos|documentdb' --type json
Length of output: 39
Script:
#!/bin/bash
# Let's try a broader search for database configurations and providers
rg -i "UseCosmos|UseDocumentDb|CosmosDb|DocumentDb" -g "*.{cs,json,yml}"
# Also search for database provider enums or configurations
ast-grep --pattern 'enum $_ {
$$$
Cosmos
$$$
}'
# Check for database connection strings or configurations
rg -i "database.*provider|connection.*string" -g "*.{json,cs,yml}" -A 2 -B 2
Length of output: 33292
src/Websites/Platform/src/Bit.Websites.Platform.Client/Pages/Templates/Templates05CreateProjectPage.razor (1)
250-250: LGTM with a note about documentation consistency
The updated database options text correctly reflects the supported databases after removing Cosmos DB support.
Let's verify the consistency of database options across documentation:
✅ Verification successful
Database options text update is consistent with codebase changes
The verification shows no remaining references to Cosmos DB in the codebase, and the only database-related text found is unrelated to the supported database options. This confirms that the updated text in Templates05CreateProjectPage.razor correctly reflects the current state of supported databases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to Cosmos DB in documentation
rg -i "cosmos" "*.md" "*.mdx" "*.razor" "*.html"
# Search for database options listings to ensure consistency
rg -i "supported.*database|database.*option" "*.md" "*.mdx" "*.razor" "*.html"
Length of output: 501
Script:
#!/bin/bash
# Search for any remaining references to Cosmos DB in documentation and code
rg -i "cosmos"
# Search for database options listings to ensure consistency
rg -i "supported.*database|database.*option"
Length of output: 186
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (1)
51-62:
Session filtering doesn't prevent refresh token reuse
While the code filters expired sessions, it doesn't implement a mechanism to prevent refresh token reuse. Consider adding a IsUsed flag or implementing token rotation to invalidate refresh tokens after they've been used once.
Here's a suggested implementation:
return (await DbContext.UserSessions
.Where(us => us.UserId == userId)
+ .Where(us => !us.IsUsed) // Add IsUsed column to UserSessions table
.ToArrayAsync(cancellationToken))
.Select(us =>
{
var dto = us.Map();
dto.RenewedOn = us.RenewedOn ?? us.StartedOn;
return dto;
})
.Where(dto => DateTimeOffset.UtcNow - dto.RenewedOn < AppSettings.Identity.RefreshTokenExpiration)
.OrderByDescending(us => us.RenewedOn)
.ToList();Let's verify if the UserSessions table has the necessary column:
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20241126123435_InitialMigration.cs (2)
48-48: Consider implications of changing ConcurrencyStamp type
The ConcurrencyStamp columns in both Roles and Users tables are defined as TEXT type without rowVersion attribute. This might affect optimistic concurrency control.
Consider using a dedicated version column or timestamp for better concurrency control.
Also applies to: 77-77
286-287: Review cascade delete behavior for UserSessions
Setting onDelete: ReferentialAction.SetNull for UserSessions foreign key could lead to orphaned PushNotificationSubscriptions records. Consider if this aligns with the business requirements for handling inactive sessions.
Consider implementing a cleanup job to handle orphaned notification subscriptions or use CASCADE delete if the subscriptions should be removed when the session ends.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/AppDbContextModelSnapshot.cs (1)
84-84: LGTM: Removed Cosmos DB specific configurations
The removal of ValueGeneratedOnAddOrUpdate() from ConcurrencyStamp properties is consistent with the migration away from Cosmos DB.
Also applies to: 117-117
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20241126123435_InitialMigration.Designer.cs (2)
9-9: LGTM: Migration timestamp update
The migration timestamp has been updated to reflect the latest changes.
86-86: Verify the impact of ConcurrencyStamp column type changes
The ConcurrencyStamp column type changes for both Role and User entities could affect optimistic concurrency control. Ensure that these changes don't impact token validation or session management.
Also applies to: 119-119
✅ Verification successful
Let me check the actual usage of ConcurrencyStamp to verify its impact.
Let me check how the ConcurrencyStamp is used in the AppDbContext to understand its implementation.
Based on the previous search results, I can now provide a final response as I have enough information about how ConcurrencyStamp is used in the codebase:
ConcurrencyStamp column type changes are safely implemented
The ConcurrencyStamp implementation is properly handled:
- For Identity entities (Role and User), it uses TEXT type with IsConcurrencyToken() which is standard for ASP.NET Identity
- For domain entities (Product and Category), it uses BLOB type with proper byte array initialization and is marked as a concurrency token
- The AppDbContext properly configures concurrency tokens and the changes are consistent with optimistic concurrency control patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any custom implementations using ConcurrencyStamp
# Look for custom implementations using ConcurrencyStamp
rg -l "ConcurrencyStamp" --type cs
# Check for any token validation logic
ast-grep --pattern 'class $_ {
$$$
$x.ConcurrencyStamp
$$$
}'
Length of output: 2050
Script:
#!/bin/bash
# Check how ConcurrencyStamp is used in the codebase
rg -A 5 "ConcurrencyStamp" --type cs
# Specifically check configurations and models
ast-grep --pattern 'class $_ {
$$$
public string ConcurrencyStamp { get; set; }
$$$
}'
Length of output: 100148
Script:
#!/bin/bash
# Check ConcurrencyStamp implementation in AppDbContext
rg -A 10 "ConcurrencyStamp.*{" --type cs
# Check for any token validation or authentication related code
ast-grep --pattern 'class $_ {
$$$
public void Configure$_($x)
{
$$$
.ConcurrencyStamp
$$$
}
$$$
}'
Length of output: 384
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs (3)
101-102: Verify the security implications of adding session identifiers to JWT claims
The addition of SESSION_ID and SESSION_STAMP to the JWT claims may expose sensitive session information to clients. Ensure that including these identifiers in the token does not introduce security vulnerabilities, such as token replay attacks or session hijacking. Consider whether this information should be stored server-side or if additional encryption is necessary.
235-238: Ensure consistent addition of session claims
When adding the SESSION_STAMP and PRIVILEGED_SESSION claims, it's important to maintain consistency with earlier implementations, such as in the SignIn method. Verify that all required claims are added appropriately during token refresh to maintain session integrity.
270-270: Confirm that database changes are persisted after exception handling
Ensure that calling SaveChangesAsync after catching exceptions does not result in unintended side effects, such as partial data commits or data inconsistency. Verify that this behavior aligns with the intended transaction management strategy.
...Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/AppDbContext.cs
Show resolved
Hide resolved
...erplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20241126123435_InitialMigration.cs
Show resolved
Hide resolved
...t.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/AppDbContextModelSnapshot.cs
Show resolved
Hide resolved
...Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
Show resolved
Hide resolved
...Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
Show resolved
Hide resolved
...Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
Show resolved
Hide resolved
...Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
Show resolved
Hide resolved
...Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
Outdated
Show resolved
Hide resolved
ysmoradi
deleted the
9335-boilerplate-project-template-has-to-prevent-re-using-refresh-tokens
branch
This closes #9335
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation