Add support for pop-up based social sign-in (#9399) #9402
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 enhance the functionality and security of the application. Modifications include the addition of new methods for JavaScript interop, improved event handling for navigation, and updates to service registration, particularly regarding SignalR configurations. Security middleware has been introduced to strengthen protection against vulnerabilities. Additionally, a new constant for navigation messages has been added, and a class has been removed to streamline the codebase. Overall, these changes improve resource management, event handling, and security features. 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: 6
🧹 Outside diff range and nitpick comments (6)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/DefaultExternalNavigationService.cs (2)
5-5: Consider making Window dimensions configurable.The Window service is properly injected, but consider making it more flexible by allowing configuration of default dimensions.
+ private const int DEFAULT_POPUP_HEIGHT = 768; + private const int DEFAULT_POPUP_WIDTH = 1024; + private readonly WindowOptions _windowOptions; + [AutoInject] private readonly Window window = default!; [AutoInject] private readonly NavigationManager navigationManager = default!; + + public DefaultExternalNavigationService(IConfiguration configuration) + { + _windowOptions = configuration + .GetSection("ExternalNavigation:WindowOptions") + .Get<WindowOptions>() ?? new WindowOptions + { + Height = DEFAULT_POPUP_HEIGHT, + Width = DEFAULT_POPUP_WIDTH + }; + }
16-20: Consider accessibility implications of popup windows.While the implementation provides good fallback options, popup windows can cause accessibility issues. Consider:
- Adding ARIA attributes for screen readers
- Ensuring keyboard navigation works in the popup
- Providing a user preference to disable popups
Would you like me to provide a more detailed implementation that addresses these accessibility concerns?
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs (1)
62-62: Consider removing deprecatedUseXXssProtectionmiddlewareThe
X-XSS-Protectionheader configured byUseXXssProtectionis deprecated and ineffective in modern browsers. It's advisable to remove this middleware to simplify the pipeline.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Middlewares.cs (1)
51-51: Consider removing deprecatedUseXXssProtectionmiddlewareThe
X-XSS-Protectionheader set byUseXXssProtectionis deprecated and is no longer supported by modern browsers. Consider removing this middleware to streamline the middleware configuration.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (2)
146-146: Consider the implications of using stateful reconnectThe change from automatic reconnect to stateful reconnect (
WithStatefulReconnect()) has important implications:
- Stateful reconnect maintains the connection state during reconnection attempts, which is beneficial for the pop-up based social sign-in scenario as it helps preserve the application state.
- However, it consumes more memory as it needs to track the connection state.
Consider implementing a connection state monitoring mechanism to handle scenarios where reconnection attempts fail permanently. This could include:
- Logging reconnection attempts
- Implementing a fallback mechanism
- Notifying users about connection status
Line range hint
149-153: Review WebSocket transport configurationThe code makes good architectural decisions regarding WebSocket transport:
- Explicitly disables skip negotiation for Azure SignalR compatibility
- Correctly enforces WebSocket transport
- Includes helpful comments about avoiding long polling and Server-Sent Events
To support this WebSocket-only approach:
- Ensure your deployment environment (IIS/Cloudflare) has WebSocket support enabled
- Consider implementing a connection fallback strategy for environments where WebSockets might be blocked
- Add monitoring for failed connection attempts to identify environments where WebSocket support is missing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/JsBridge.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/app.ts(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientPubSubMessages.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/DefaultExternalNavigationService.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/PubSubService.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/SignalRInfinitiesRetryPolicy.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages.props(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages8.props(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Boilerplate.Server.Api.csproj(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Middlewares.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/SignalRInfinitiesRetryPolicy.cs
✅ Files skipped from review due to trivial changes (1)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/PubSubService.cs
🔇 Additional comments (11)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientPubSubMessages.cs (1)
25-28: LGTM! Well-documented constant addition.
The new NAVIGATE_TO constant is well-documented and follows the existing naming convention. Its purpose aligns with the PR objective of enabling external navigation without app reload.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/DefaultExternalNavigationService.cs (1)
16-20: Verify the navigation flow across different browsers.
The popup fallback logic looks good, but we should verify it works consistently across different browsers and scenarios.
✅ Verification successful
Let me analyze the navigation flow implementation and verification needs.
The code shows that the navigation flow has a well-designed fallback mechanism:
- First tries to open as a popup with specific dimensions:
window.Open(url, "_blank", new WindowFeatures() { Popup = true, Height = 768, Width = 1024 })- If popup fails, tries to open in a new tab:
window.Open(url, "_blank", new WindowFeatures() { Popup = false })- Finally falls back to navigating in the same tab if both previous attempts fail:
navigationManager.NavigateTo(url, forceLoad: true, replace: true)The implementation uses the Window.Open() method from Bit.Butil which is a wrapper around the browser's window.open() API. The code shows proper handling of different scenarios with appropriate fallbacks.
Let me verify if there are any browser-specific implementations or known issues in the codebase.
Let me check the actual implementation of the window.open wrapper to verify the browser handling.
Based on the code analysis, I can now provide a final response about the navigation flow verification:
The navigation implementation is robust and browser-safe
The implementation follows best practices for cross-browser compatibility:
-
The window.open wrapper in window.ts provides a consistent interface that:
- Uses the native window.open() API
- Returns a boolean indicating success/failure
- Properly handles the windowFeatures parameter
-
The navigation flow has proper fallbacks:
- First attempts a popup with specific dimensions
- Falls back to new tab if popup fails
- Ultimate fallback to same-tab navigation
- Each step only proceeds if the previous attempt returns false
-
The WindowFeatures class properly formats the features string for window.open()
No browser-specific handling or known issues were found in the codebase, suggesting the implementation relies on standard web APIs that work consistently across modern browsers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any browser-specific handling or known issues
rg -g '*.{ts,js,cs}' -i '(browser|navigator|userAgent|window\.open|popup)'
Length of output: 124470
Script:
#!/bin/bash
# Search for any browser-specific window.open implementations or overrides
ast-grep --pattern 'window.open($$$)'
Length of output: 261
Script:
#!/bin/bash
# Check the window.open implementation in window.ts
cat src/Butil/Bit.Butil/Scripts/window.ts
Length of output: 2905
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs (1)
255-256: LGTM! Proper cleanup of subscription
The disposal of the PubSubService subscription is handled correctly to prevent memory leaks.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs (2)
60-64: Security middleware additions enhance application security
The inclusion of UseHsts, UseXContentTypeOptions, and UseXfo middleware strengthens the application's security by adding important HTTP headers.
146-146: Ensure client supports stateful reconnects with SignalR
By setting AllowStatefulReconnects to true, the server enables clients to reconnect and maintain state. Verify that the client-side code is updated to handle stateful reconnections properly to avoid unexpected behavior.
Run the following script to check for client-side SignalR configuration:
✅ Verification successful
Client code properly supports stateful reconnects with SignalR
The client-side code in Boilerplate.Client.Core already implements proper support for stateful reconnects:
- The
HubConnectionBuilderis configured with.WithStatefulReconnect()inIClientCoreServiceCollectionExtensions.cs ClientAppCoordinator.csimplements proper reconnection handling:- Handles
Reconnected,Reconnecting, andClosedevents - Updates connection state through
HubConnectionStateChange - Notifies the application about online status changes via PubSub
- Handles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if client code handles stateful reconnects with SignalR.
# Search for SignalR HubConnectionBuilder configurations in client-side code
rg 'HubConnectionBuilder' -g 'src/Client/**/**/*.cs' -A 5
# Check for usage of 'WithStatefulReconnect' method
rg 'WithStatefulReconnect' -g 'src/Client/**/**/*.cs' -A 2
Length of output: 414
Script:
#!/bin/bash
# Let's first find all client-side files that might contain SignalR configurations
fd -e cs -e ts -e js . src/Client
# Then search for SignalR hub configurations in all source files
rg "HubConnection" -A 5
# Also check for any SignalR-related configuration files
fd -e json . | xargs rg "SignalR"
Length of output: 9065
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Middlewares.cs (2)
48-52: Security middleware additions enhance application security
The addition of UseHsts, UseXContentTypeOptions, and UseXfo middleware improves the application's security by adding essential HTTP headers.
85-85: Ensure client supports stateful reconnects with SignalR
Enabling AllowStatefulReconnects allows clients to reconnect and maintain their state. Confirm that the client application is configured to handle stateful reconnects appropriately.
Run the following script to check client-side SignalR configurations:
✅ Verification successful
Based on the search results, I can see that the client-side code in Boilerplate.Client.Core already implements stateful reconnection support through the HubConnectionBuilder configuration:
var hubConnection = new HubConnectionBuilder()
.WithStatefulReconnect() // Stateful reconnect is configured
.WithUrl(new Uri(absoluteServerAddressProvider.GetAddress(), "app-hub"), options =>
{
options.SkipNegotiation = false;
options.Transports = HttpTransportType.WebSockets;
})The client also properly handles connection state changes and reconnection events:
hubConnection.Closed += HubConnectionStateChange;
hubConnection.Reconnected += HubConnectionConnected;
hubConnection.Reconnecting += HubConnectionStateChange;Client is already configured correctly for stateful reconnects
The client implementation properly supports stateful reconnects by using the WithStatefulReconnect() configuration and handling all relevant connection state events. No additional client-side changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if client code handles stateful reconnects with SignalR.
# Search for SignalR HubConnectionBuilder configurations in client-side code
rg 'HubConnectionBuilder' -g 'src/Client/**/**/*.cs' -A 5
# Check for usage of 'WithStatefulReconnect' method
rg 'WithStatefulReconnect' -g 'src/Client/**/**/*.cs' -A 2
Length of output: 414
Script:
#!/bin/bash
# Let's try a broader search for client-side SignalR configurations
# First, find all potential client files
fd -e cs -e ts -e js | grep -i "client"
# Then search for SignalR-related configurations in all files
rg "HubConnection" -A 5
# Also search for any SignalR hub usage
rg "hubConnection" -i -A 5
Length of output: 66612
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Boilerplate.Server.Api.csproj (1)
22-22: Addition of NWebsec middleware package reference
Including the NWebsec.AspNetCore.Middleware package reference is necessary for implementing the added security middleware. Ensure this aligns with the versions specified in centralized package management.
src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages.props (1)
24-24: Added NWebsec middleware to central package management
The package version for NWebsec.AspNetCore.Middleware has been added to centralize dependency versions, ensuring consistency across projects.
src/Templates/Boilerplate/Bit.Boilerplate/src/Directory.Packages8.props (1)
24-24: Added NWebsec middleware to central package management for .NET 8
Including NWebsec.AspNetCore.Middleware in the centralized package file for .NET 8 ensures consistent dependency management across projects targeting .NET 8.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (1)
Line range hint 146-171: Verify token refresh mechanism security
The token refresh implementation in the SignalR connection looks solid, but there are some security considerations to verify:
- The token refresh mechanism correctly validates token expiry before attempting refresh
- The refresh is properly attributed with
requestedBy: nameof(HubConnectionBuilder)
Let's verify the token refresh implementation across the codebase:
✅ Verification successful
Token refresh mechanism is securely implemented
The verification confirms that the token refresh implementation is secure and consistent across the codebase:
-
Token expiry validation is properly enforced before refresh:
- The SignalR hub connection correctly validates token expiry with
validateExpiry: true - The same validation pattern is consistently used in
AuthDelegatingHandlerand other components
- The SignalR hub connection correctly validates token expiry with
-
The refresh token mechanism has proper security measures:
- Refresh tokens have a configured expiration (14 days as per
appsettings.json) - Server-side validation in
IdentityControllerchecks both token authenticity and expiration - Protection against refresh token loops in
AuthDelegatingHandler - Proper cleanup of tokens on refresh failures
- Refresh tokens have a configured expiration (14 days as per
-
The
requestedByattribution is consistently used across all refresh calls for audit purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other token refresh implementations to ensure consistency
rg -A 5 "RefreshToken|ParseAccessToken"
# Look for potential token validation implementations
ast-grep --pattern 'validateExpiry = true'
Length of output: 22405
...plate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/JsBridge.razor.cs
Show resolved
Hide resolved
....Boilerplate/src/Client/Boilerplate.Client.Core/Services/DefaultExternalNavigationService.cs
Show resolved
Hide resolved
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/app.ts
Outdated
Show resolved
Hide resolved
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/app.ts
Outdated
Show resolved
Hide resolved
...rplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs
Show resolved
Hide resolved
This closes #9399
Summary by CodeRabbit
Release Notes
New Features
JsBridgeclass, allowing messages to be published from JavaScript.Improvements
Bug Fixes