Skip to content
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

Add unique user session check feature to Boilerplate (#9308) #9309

Conversation

ysmoradi
Copy link
Member

@ysmoradi ysmoradi commented Nov 23, 2024

This closes #9308

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced template configurability with new parameters for storage options, deployment configurations, and user verification methods.
    • Added SignalR functionality for real-time communication, including session uniqueness checks during profile updates.
    • Introduced a new Ping method for client communication in the SignalR hub.
  • Bug Fixes

    • Improved error handling for user session management and authentication checks.
  • Localization

    • Added new messages for handling concurrent user sessions and reCAPTCHA errors in multiple languages.
  • Documentation

    • Updated XML documentation for new constants and methods related to SignalR functionality.

@ysmoradi ysmoradi requested a review from msynk November 23, 2024 11:38
Copy link

coderabbitai bot commented Nov 23, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes in this pull request involve significant updates to the Bit Boilerplate template and its associated components, focusing on enhancing configurability and SignalR integration. A new parameter filesStorage is introduced, along with updates to existing parameters like api, captcha, and pipeline. The ClientAppCoordinator class is modified to improve SignalR connection handling, while new methods and fields are added to support user session uniqueness checks. Additionally, localization files are updated to include messages relevant to new features, ensuring comprehensive support for user interactions.

Changes

File Change Summary
src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json Added filesStorage, updated api, captcha, pipeline, added normalizedServerUrl, and modified generated parameters for port configurations.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs Updated SignalR connection handling, renamed method to SubscribeToSignalREventsMessages, added StartSignalR, and managed disposables for subscriptions.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/ProfileSection.razor.cs Added SignalR integration for session uniqueness checks in SaveProfile method.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs Introduced IsUserSessionUnique extension method for HubConnection.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs Modified AddClientCoreProjectServices to conditionally include SignalR services.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs Simplified session ID retrieval in SignOut and RevokeSession, updated GetUserSessions for validity checks.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Signalr/AppHub.cs Added Ping method for client communication, improved authentication checks in OnConnectedAsync.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fa.resx Added new data entries for concurrent user sessions and reCAPTCHA error messages.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.nl.resx Similar updates as above for Dutch localization.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.resx Added new data entry for concurrent user sessions.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs Introduced PONG constant with detailed documentation regarding its use in SignalR interactions.

Assessment against linked issues

Objective Addressed Explanation
Feature to detect if user session is unique is implemented (9308)

🐰 In the land of code, where changes bloom,
New features sprout, dispelling the gloom.
With SignalR's magic, sessions unique,
A template so flexible, it's truly sleek!
From ports to storage, all tailored with care,
Hop along, dear coder, new paths to share! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (13)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs (2)

5-7: Improve method documentation

The current documentation is incomplete and contains a typo. Consider adding comprehensive XML documentation that explains:

  • The purpose of the method
  • The return value meaning (true/false cases)
  • The cancellation token usage
  • Potential exceptions
-    /// <summary>
-    /// <inheritdoc cref="SignalREvents.PONG"/>"/>
-    /// </summary>
+    /// <summary>
+    /// Checks if the current user session is unique by performing a ping-pong test with the server.
+    /// </summary>
+    /// <param name="hubConnection">The SignalR hub connection</param>
+    /// <param name="cancellationToken">Cancellation token to cancel the operation</param>
+    /// <returns>
+    /// True if the session is unique (no PONG received within timeout),
+    /// False if another session exists (PONG received within timeout)
+    /// </returns>
+    /// <exception cref="ServerConnectionException">Thrown when hub connection is not in Connected state</exception>

12-30: Enhance error handling and cleanup

The current error handling could be more robust:

  1. Consider catching and wrapping exceptions from InvokeAsync
  2. Add timeout exception handling
  3. Consider implementing IAsyncDisposable for better cleanup
         try
         {
             if (hubConnection.State is not HubConnectionState.Connected)
                 throw new ServerConnectionException(AppStrings.ServerConnectionException);

             // ... existing code ...

-            await hubConnection.InvokeAsync("Ping", cancellationToken);
+            try
+            {
+                await hubConnection.InvokeAsync("Ping", sessionId, cancellationToken);
+            }
+            catch (Exception ex) when (ex is not OperationCanceledException)
+            {
+                throw new ServerConnectionException(AppStrings.ServerConnectionException, ex);
+            }

             // ... existing code ...
         }
+        catch (OperationCanceledException)
+        {
+            throw;
+        }
+        catch (Exception ex) when (ex is not ServerConnectionException)
+        {
+            throw new ServerConnectionException(AppStrings.ServerConnectionException, ex);
+        }
         finally
         {
-            disposable?.Dispose();
+            if (disposable is not null)
+            {
+                await hubConnection.Remove(SignalREvents.PONG);
+                disposable.Dispose();
+            }
         }
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Signalr/AppHub.cs (1)

37-39: Enhance method documentation

The current documentation only references the PONG event. Consider adding more context about the method's purpose in session uniqueness verification.

Apply this diff:

    /// <summary>
-   /// <inheritdoc cref="SignalREvents.PONG"/>
+   /// Handles ping requests for verifying user session uniqueness.
+   /// Responds with a PONG event to the specific client identified by their session ID.
+   /// This method is used as part of the session uniqueness verification mechanism.
+   /// </summary>
+   /// <remarks>
+   /// The client must be authenticated to use this endpoint.
+   /// The response is sent only to the requesting client using their session ID.
+   /// </remarks>
+   /// <exception cref="HubException">Thrown when the user context is not available.</exception>
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs (1)

30-43: Enhance documentation with implementation details and security considerations.

While the documentation is comprehensive, consider adding:

  1. A reference to where the 3-second timeout is implemented
  2. The expected format/signature of the Ping method
  3. A security warning about proper token handling
    /// <summary>
    /// While users are allowed to have multiple concurrent user sessions by **signing in** on multiple devices or browsers,  
    /// copying access and refresh tokens from one device / browser to another is prohibited.
    ///   
    /// This method uses a SignalR mechanism to check for potential misuse of access and refresh tokens across different app instances:  
-   /// - The client calls a `Ping` method on the server.
+   /// - The client calls a `Ping()` method on the server (implemented in AppHub).
    /// - The server sends a `PONG` to the specific user session.  
    /// - Because SignalR's connection id is the same is user session's Id (Thanks to AppHubConnectionHandler's implementation),
    /// - Only one user session will receive the `PONG` event.
-   /// - If the validation event is not received within a 3-second timeout, it indicates that there's something wrong.
+   /// - If the validation event is not received within a 3-second timeout (configured in HubConnectionExtensions), it indicates that there's something wrong.
    ///   - The user might have illegally copied tokens to another device or browser.
    ///   - The user may have lost their SignalR connection to the server.
    ///   - It could also be another open tab in the same browser or another instance of the app.
+   /// 
+   /// SECURITY WARNING: Ensure proper token validation and revocation mechanisms are in place to prevent token reuse across sessions.
    /// </summary>
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (1)

134-134: Consider documenting the retry policy behavior.

The SignalRInfinitiesRetryPolicy is registered as a singleton, but its behavior and configuration are not immediately clear. Consider adding XML documentation to explain the retry intervals and maximum attempts.

src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (2)

Line range hint 70-85: Remove duplicate session removal operations and add transaction scope

There are several issues in the SignOut method:

  1. The session is being removed twice from DbContext
  2. The second removal creates a new entity unnecessarily
  3. Multiple database operations should be wrapped in a transaction

Apply this diff to fix the issues:

 public async Task SignOut(CancellationToken cancellationToken)
 {
     var currentSessionId = User.GetSessionId();
 
     var userSession = await DbContext.UserSessions
         //#if (notification == true)
         .Include(us => us.PushNotificationSubscription)
         //#endif
         .FirstOrDefaultAsync(us => us.Id == currentSessionId, cancellationToken) ?? throw new ResourceNotFoundException();
 
-    DbContext.UserSessions.Remove(userSession);
-    await DbContext.SaveChangesAsync(cancellationToken);
-
-    DbContext.UserSessions.Remove(new() { Id = currentSessionId });
-    await DbContext.SaveChangesAsync(cancellationToken);
+    await using var transaction = await DbContext.Database.BeginTransactionAsync(cancellationToken);
+    try 
+    {
+        DbContext.UserSessions.Remove(userSession);
+        await DbContext.SaveChangesAsync(cancellationToken);
+        await transaction.CommitAsync(cancellationToken);
+    }
+    catch
+    {
+        await transaction.RollbackAsync(cancellationToken);
+        throw;
+    }
 
     SignOut();
 }

Add transaction scope but keep the current error message

The review comment's suggestion to add a transaction scope is valid, but the suggestion to change the error message isn't necessary since:

  1. The current comment-style error message ("Call SignOut instead") is clear and follows the codebase's pattern of using simple BadRequestException for validation errors
  2. The search results don't show a consistent pattern of using localized strings for all error messages, and many BadRequestException usages are without messages
  3. The suggested localization key "CannotRevokeCurrentSession" doesn't exist in the codebase

Here's the recommended change:

 public async Task RevokeSession(Guid id, CancellationToken cancellationToken)
 {
     var userId = User.GetUserId();
     var currentSessionId = User.GetSessionId();
 
     if (id == currentSessionId)
         throw new BadRequestException(); // "Call SignOut instead"
 
     var userSession = await DbContext.UserSessions
         //#if (notification == true)
         .Include(us => us.PushNotificationSubscription)
         //#endif
         .FirstOrDefaultAsync(us => us.Id == id, cancellationToken) ?? throw new ResourceNotFoundException();
 
+    await using var transaction = await DbContext.Database.BeginTransactionAsync(cancellationToken);
+    try 
+    {
         DbContext.UserSessions.Remove(userSession);
         await DbContext.SaveChangesAsync(cancellationToken);
 
         //#if (signalR == true)
         await appHubContext.Clients.Client(userSession.Id.ToString()).SendAsync(SignalREvents.PUBLISH_MESSAGE, SharedPubSubMessages.SESSION_REVOKED, cancellationToken);
         //#endif
+        await transaction.CommitAsync(cancellationToken);
+    }
+    catch
+    {
+        await transaction.RollbackAsync(cancellationToken);
+        throw;
+    }
 }
🔗 Analysis chain

Line range hint 92-108: Add transaction scope and improve error message

The RevokeSession method needs some improvements:

  1. Database operations should be wrapped in a transaction
  2. The error message for attempting to revoke current session could be more descriptive

Apply this diff to improve the implementation:

 public async Task RevokeSession(Guid id, CancellationToken cancellationToken)
 {
     var userId = User.GetUserId();
     var currentSessionId = User.GetSessionId();
 
     if (id == currentSessionId)
-        throw new BadRequestException(); // "Call SignOut instead"
+        throw new BadRequestException(Localizer[nameof(AppStrings.CannotRevokeCurrentSession)]);
 
     var userSession = await DbContext.UserSessions
         //#if (notification == true)
         .Include(us => us.PushNotificationSubscription)
         //#endif
         .FirstOrDefaultAsync(us => us.Id == id, cancellationToken) ?? throw new ResourceNotFoundException();
 
+    await using var transaction = await DbContext.Database.BeginTransactionAsync(cancellationToken);
+    try 
+    {
         DbContext.UserSessions.Remove(userSession);
         await DbContext.SaveChangesAsync(cancellationToken);
 
         //#if (signalR == true)
         await appHubContext.Clients.Client(userSession.Id.ToString()).SendAsync(SignalREvents.PUBLISH_MESSAGE, SharedPubSubMessages.SESSION_REVOKED, cancellationToken);
         //#endif
+        await transaction.CommitAsync(cancellationToken);
+    }
+    catch
+    {
+        await transaction.RollbackAsync(cancellationToken);
+        throw;
+    }
 }

Let's verify if the localization key exists:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the localization key exists in the resource files
rg -l "CannotRevokeCurrentSession" --type cs

Length of output: 44


Script:

#!/bin/bash
# Let's check for any string resource files or constants that might contain error messages
fd "AppStrings" --type f --extension cs

# Also check for any similar error messages about session revocation
rg -i "session.*revok" --type cs
rg -i "current.*session" --type cs

Length of output: 6456


Script:

#!/bin/bash
# Let's check for AppStrings class definition and any session-related string constants
fd "AppStrings" --type f --extension cs --exec ast-grep --pattern 'class AppStrings {
  $$$
}'

# Also check for any BadRequestException usage with custom messages
rg -A 2 "BadRequestException\(" --type cs

Length of output: 26865

src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json (1)

Line range hint 89-95: Consider enhancing SignalR configuration options.

Given the addition of the unique user session check feature, consider:

  1. Adding documentation about this feature in the SignalR parameter description
  2. Potentially making the unique session check configurable independently for cases where users want SignalR without session uniqueness checks

Apply this diff to enhance the documentation:

     "signalR": {
         "displayName": "Add SignalR?",
         "type": "parameter",
         "datatype": "bool",
         "defaultValue": "false",
-        "description": "Add SignalR sample."
+        "description": "Add SignalR sample with unique user session check capability."
     },
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fa.resx (1)

1154-1157: Consider improving the concurrent session message formatting.

While the translation is accurate, the double quotes in the message value could be simplified for better maintainability.

Apply this diff to improve the formatting:

    <data name="ConcurrentUserSessionOnTheSameDevice" xml:space="preserve">
-    <value>"لطفاً اطمینان حاصل کنید که این اپلیکیشن بیش از یک‌بار، به طور هم‌زمان روی یک دستگاه یا مرورگر باز نیست، و دوباره تلاش کنید."</value>
+    <value>لطفاً اطمینان حاصل کنید که این اپلیکیشن بیش از یک‌بار، به طور هم‌زمان روی یک دستگاه یا مرورگر باز نیست، و دوباره تلاش کنید.</value>
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs (1)

Line range hint 149-182: Incorrect initialization of signalROnDisposables

The signalROnDisposables list is initialized incorrectly using [], which is not valid in C#. It should be initialized using new List<IDisposable>().

Apply this diff to fix the initialization:

-private List<IDisposable> signalROnDisposables = [];
+private List<IDisposable> signalROnDisposables = new List<IDisposable>();
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/ProfileSection.razor.cs (3)

16-17: Be explicit with access modifiers for consistency.

Consider adding the private access modifier to the hub field to maintain consistency with other field declarations and improve code readability.

Apply this diff to add the private access modifier:

-[AutoInject] HubConnection hub = default!;
+[AutoInject] private HubConnection hub = default!;

Line range hint 30-31: Correct the lifecycle method name to OnInitializedAsync.

The method OnInitAsync does not exist in the Blazor component lifecycle. It should be OnInitializedAsync. Ensure you're overriding the correct method to initialize your component asynchronously.

Apply this diff to fix the method name:

-protected override async Task OnInitAsync()
+protected override async Task OnInitializedAsync()

60-62: Provide user-friendly feedback when the session is not unique.

Throwing a ForbiddenException may not provide the best user experience. Consider displaying a user-friendly message or handling the situation in a way that guides the user on how to resolve the issue of concurrent sessions.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d1726b0 and 3fcc429.

📒 Files selected for processing (11)
  • src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json (1 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs (8 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/ProfileSection.razor.cs (3 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs (1 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (2 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (2 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Signalr/AppHub.cs (1 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fa.resx (2 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.nl.resx (2 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.resx (1 hunks)
  • src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs (1 hunks)
🔇 Additional comments (18)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs (1)

17-24: ⚠️ Potential issue

Review the session uniqueness logic and timeout handling

Several concerns with the current implementation:

  1. The return value seems counterintuitive - a timeout (no PONG) indicates a unique session
  2. The 3-second timeout is hardcoded and might not be suitable for all network conditions
  3. The ping-pong exchange doesn't include any session-specific data for validation

Consider these improvements:

+        private const int DefaultTimeoutSeconds = 3;
+
         public static async Task<bool> IsUserSessionUnique(this HubConnection hubConnection, 
-            CancellationToken cancellationToken)
+            CancellationToken cancellationToken, int timeoutSeconds = DefaultTimeoutSeconds)
         {
-            disposable = hubConnection.On(SignalREvents.PONG, () =>
+            var sessionId = Guid.NewGuid().ToString();
+            disposable = hubConnection.On<string>(SignalREvents.PONG, (responseSessionId) =>
             {
+                if (responseSessionId == sessionId)
                 pongTcs!.SetResult();
             });

-            await hubConnection.InvokeAsync("Ping", cancellationToken);
+            await hubConnection.InvokeAsync("Ping", sessionId, cancellationToken);

-            return await Task.WhenAny(Task.Delay(TimeSpan.FromSeconds(3), cancellationToken), pongTcs!.Task) != pongTcs.Task;
+            var timeoutTask = Task.Delay(TimeSpan.FromSeconds(timeoutSeconds), cancellationToken);
+            var completedTask = await Task.WhenAny(timeoutTask, pongTcs.Task);
+            return completedTask == pongTcs.Task;

Let's verify the server-side implementation matches these changes:

src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Signalr/AppHub.cs (1)

36-44: Implementation aligns well with session uniqueness check requirements

The Ping method implementation provides a secure way to verify session uniqueness by:

  1. Requiring authentication
  2. Using session-specific client communication
  3. Integrating with the broader SignalR infrastructure

Let's verify the complete session check implementation:

✅ Verification successful

Session uniqueness check implementation is correctly implemented and verified

The implementation is well-designed and complete:

  • Server-side AppHub.Ping() uses session-specific client communication via GetSessionId()
  • AppHubConnectionHandler enforces unique session IDs by setting ConnectionId to user's session ID
  • Client-side HubConnectionExtensions.IsUserSessionUnique() implements timeout-based uniqueness check:
    • Sets up PONG handler
    • Invokes Ping
    • Returns true if no PONG received within 3 seconds (indicating session is unique)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the complete session uniqueness check implementation
# Looking for the HubConnectionExtensions class that uses this Ping method

# Search for HubConnectionExtensions implementation
ast-grep --pattern 'class HubConnectionExtensions {
  $$$
  IsUserSessionUnique($$$) {
    $$$
  }
  $$$
}'

# Search for any other session uniqueness related code
rg -l "IsUserSessionUnique|GetSessionId"

Length of output: 1536


Script:

#!/bin/bash
# Let's examine the client-side implementation and the hub connection handler

# Check HubConnectionExtensions implementation
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs

# Check AppHubConnectionHandler implementation
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Signalr/AppHubConnectionHandler.cs

# Check ClaimsPrincipalExtensions for GetSessionId implementation
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ClaimsPrincipalExtensions.cs

Length of output: 4685

src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs (2)

44-44: LGTM! Constant declaration follows best practices.

The constant declaration is well-implemented:

  • Uses nameof operator to prevent string literals
  • Maintains consistency with other SignalR event constants
  • Has appropriate visibility level

29-44: Verify the complete implementation chain for session uniqueness check.

Let's verify that all components required for the session uniqueness check are properly implemented:

✅ Verification successful

Let me gather more information about the implementation chain, particularly focusing on the HubConnectionExtensions and AppHub implementations.


Let me verify one final aspect about the session ID handling to complete the verification chain.


Complete implementation chain for session uniqueness check is properly implemented

The verification confirms that all components are correctly implemented and connected:

  1. Server-side (AppHub):

    • Correctly implements Ping method that sends PONG to specific client using session ID
    • AppHubConnectionHandler properly sets connection ID to user's session ID
  2. Client-side:

    • HubConnectionExtensions implements IsUserSessionUnique with proper timeout handling
    • PONG event handler is properly registered and managed
  3. Session Management:

    • Session IDs are consistently managed through ClaimsPrincipal extensions
    • Database properly tracks user sessions with appropriate relationships
    • Session revocation and cleanup are properly implemented

The implementation ensures that only one device/browser instance can validate its session through the SignalR ping-pong mechanism, effectively preventing token misuse across different app instances.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the complete implementation chain for session uniqueness check

# Check for Ping method implementation
echo "Checking for Ping method implementation..."
ast-grep --pattern 'class AppHub {
  $$$
  Ping() {
    $$$
  }
  $$$
}'

# Check for IsUserSessionUnique implementation
echo "Checking for IsUserSessionUnique extension method..."
ast-grep --pattern 'IsUserSessionUnique($$$) {
  $$$
}'

# Check for timeout configuration
echo "Checking for timeout configuration..."
rg -A 2 "3.*second.*timeout" 

# Check for PONG usage in connection handling
echo "Checking for PONG constant usage..."
rg "PONG" --type cs

Length of output: 2334


Script:

#!/bin/bash
# Check for the complete implementation of IsUserSessionUnique
echo "Checking HubConnectionExtensions implementation..."
rg -A 10 "IsUserSessionUnique" --type cs

# Check for AppHub's Ping implementation
echo "Checking AppHub's Ping implementation..."
rg -A 5 "public.*Ping" --type cs

# Check for AppHubConnectionHandler implementation
echo "Checking AppHubConnectionHandler implementation..."
rg -A 5 "class AppHubConnectionHandler" --type cs

Length of output: 5914


Script:

#!/bin/bash
# Check for GetSessionId implementation and usage
echo "Checking GetSessionId implementation..."
rg -A 5 "GetSessionId" --type cs

# Check for any session ID related configuration
echo "Checking session ID configuration..."
rg -A 5 "SessionId|UserSession" --type cs --type json

Length of output: 85837

src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (3)

14-17: LGTM: SignalR imports are properly configured.

The required SignalR client packages are correctly imported and appropriately wrapped in conditional compilation.


133-164: Verify unique session check implementation.

While the SignalR infrastructure is set up, the actual unique session check implementation is not visible in this file.

Let's verify the implementation exists:

✅ Verification successful

Unique session check implementation is properly implemented

The unique session check is implemented in HubConnectionExtensions.cs through the IsUserSessionUnique method and is being utilized in ProfileSection.razor.cs to prevent concurrent user sessions on the same device.

  • HubConnectionExtensions.cs implements the check using SignalR's hub connection
  • ProfileSection.razor.cs enforces the check by throwing a ForbiddenException when concurrent sessions are detected
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for unique session check implementation
rg -A 10 "IsUserSessionUnique|CheckUserSession|ValidateSession"

# Search for hub methods related to session checks
ast-grep --pattern 'public $_ $_($$) {
  $$$
  // Check for unique session related code
  $$$
}'

Length of output: 3839


144-147: Review WebSocket-only configuration.

While WebSockets offer better performance, skipping negotiation and disabling fallback transports might cause connectivity issues in environments where WebSocket connections are blocked (e.g., certain corporate proxies).

Consider running this script to check if the codebase handles connection failures:

Consider implementing a fallback mechanism using Server-Sent Events (SSE) when WebSocket connections fail.

✅ Verification successful

Let's search for connection handling patterns with broader terms and also look for any SignalR configuration code that might include error handling or reconnection logic.


Based on the search results, I can see that the application has proper error handling and reconnection logic for SignalR:

  1. The client uses WithAutomaticReconnect() when configuring the hub connection
  2. The ClientAppCoordinator class implements comprehensive connection state handling:
    • Tracks connection state changes via HubConnectionStateChange
    • Handles reconnection events via HubConnectionConnected
    • Publishes online/offline status changes
    • Logs connection errors and attempts to refresh tokens when unauthorized
    • Has retry logic built in

WebSocket-only configuration is appropriate with the implemented fallback mechanisms.

The code already has robust error handling and automatic reconnection capabilities built in. While WebSocket is preferred for performance, the automatic reconnection will handle any connection issues gracefully.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for connection error handling patterns
rg -A 5 "HubConnectionState\.Disconnected|OnReconnecting|OnReconnected|catch.*HubException"

Length of output: 93


Script:

#!/bin/bash
# Search for SignalR connection handling patterns with broader context
rg -i -A 5 "hubconnection|signalr.*connect|withautomaticreconnect|connection.*error"

# Search for any error handling in client-side code
rg -i -A 5 "catch.*exception.*signalr|onerror.*signalr|connection.*failed"

# Look for SignalR configuration and connection setup
ast-grep --pattern 'WithAutomaticReconnect($$$)'

Length of output: 165738

src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (1)

Line range hint 1-1: Verify implementation of session uniqueness check feature

While the session management improvements are good, I don't see the implementation of the unique user session check feature mentioned in the PR objectives. This might be implemented in other files, but we should verify its integration with these session management methods.

Let's check for the implementation:

✅ Verification successful

Let me check the implementation of the unique session check by examining the hub files.


Session uniqueness check is properly implemented

The unique session check feature is correctly implemented through SignalR:

  • Server assigns user's session ID as the SignalR connection ID in AppHubConnectionHandler
  • Client's IsUserSessionUnique method in HubConnectionExtensions performs a ping-pong check
  • If multiple sessions exist, the ping response won't be received within timeout, indicating non-unique session
  • The feature is integrated with session management as seen in ProfileSection.razor.cs
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for unique session check implementation
echo "Searching for unique session check implementation..."
rg -A 5 "IsUserSessionUnique|UniqueSession" --type cs

# Search for hub connection related files
echo "Searching for hub connection related files..."
fd -e cs -e razor | rg -i "hub|connection"

Length of output: 3296


Script:

#!/bin/bash
# Check AppHub implementation
echo "Checking AppHub implementation..."
cat "src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Signalr/AppHub.cs"

# Check AppHubConnectionHandler implementation
echo "Checking AppHubConnectionHandler implementation..."
cat "src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Signalr/AppHubConnectionHandler.cs"

# Check HubConnectionExtensions implementation
echo "Checking HubConnectionExtensions implementation..."
cat "src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/HubConnectionExtensions.cs"

Length of output: 5178

src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fa.resx (2)

Line range hint 1145-1152: LGTM: reCAPTCHA localization strings properly implemented.

The Google reCAPTCHA error messages are well-structured and appropriately placed within conditional compilation directives.


1154-1157: Implementation aligns well with PR objectives.

The added localization strings effectively support the unique user session check feature by providing clear Persian language feedback to users when concurrent sessions are detected.

src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.resx (1)

1154-1158: LGTM! The string resource aligns with the unique session check feature.

The addition of this localized string resource:

  • Supports the PR's objective of implementing unique session checks
  • Is properly scoped within SignalR conditional compilation
  • Provides clear user guidance when concurrent sessions are detected
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.nl.resx (1)

Line range hint 1145-1150: LGTM!

The Google reCAPTCHA error messages are properly formatted and correctly localized in Dutch.

src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs (5)

22-22: Injection of HubConnection is correctly implemented

The HubConnection is properly auto-injected for SignalR functionality.


36-38: Injection of IPushNotificationService is correctly implemented

The IPushNotificationService is appropriately auto-injected when notifications are enabled.


72-74: SignalR event subscription initialized appropriately

The SubscribeToSignalREventsMessages() method is correctly invoked when SignalR is enabled.


134-134: SignalR connection started after user authentication

The StartSignalR() method is appropriately called after user ID propagation to establish the SignalR connection.


267-270: Proper disposal of event handlers and subscriptions

The event handlers attached to hubConnection are correctly detached, and signalROnDisposables are disposed of to prevent memory leaks.

src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Settings/ProfileSection.razor.cs (1)

2-4: Verify conditional compilation directives.

The use of //#if and //#endif for conditional compilation may not be recognized by the standard C# preprocessor. Ensure that your build system or templating engine correctly processes these directives to include or exclude SignalR code as intended.

If you're using a custom preprocessing step or templating system, please confirm that it handles these directives appropriately.

@msynk msynk merged commit 8614b77 into bitfoundation:develop Nov 23, 2024
3 checks passed
@ysmoradi ysmoradi deleted the 9308-boilerplate-project-template-feature-to-detect-is-user-session-unique-is-missing branch November 23, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Boilerplate project template feature to detect is user session unique is missing
2 participants