fix(notifications): allows alert pages to registered users only#1393
fix(notifications): allows alert pages to registered users only#1393ygrishajev merged 1 commit intomainfrom
Conversation
WalkthroughThis update introduces a new route protection mechanism that combines feature flag checks with user authentication, enforced both on the server and client. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant NextPage
participant RegisteredUsersOnly HOC
participant useUser
participant FallbackComponent
Browser->>NextPage: Request Page
NextPage->>RegisteredUsersOnly HOC: Render
RegisteredUsersOnly HOC->>useUser: Get user info
alt User is registered
RegisteredUsersOnly HOC->>NextPage: Render original page
else User not registered
RegisteredUsersOnly HOC->>FallbackComponent: Render fallback (e.g., 404)
end
sequenceDiagram
participant Browser
participant Next.js SSR
participant routeProtector.showToRegisteredUserIfEnabled
participant featureFlagService.isEnabledForCtx
participant getSession
Browser->>Next.js SSR: Request Page
Next.js SSR->>routeProtector.showToRegisteredUserIfEnabled: Call for SSR props
routeProtector.showToRegisteredUserIfEnabled->>getSession: Get user session
routeProtector.showToRegisteredUserIfEnabled->>featureFlagService.isEnabledForCtx: Check feature flag
alt Feature enabled and user authenticated
routeProtector.showToRegisteredUserIfEnabled->>Next.js SSR: Return { props: {} }
else
routeProtector.showToRegisteredUserIfEnabled->>Next.js SSR: Return { notFound: true }
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1393 +/- ##
==========================================
+ Coverage 36.07% 36.31% +0.24%
==========================================
Files 811 814 +3
Lines 19569 19602 +33
Branches 3615 3619 +4
==========================================
+ Hits 7059 7119 +60
+ Misses 11790 11764 -26
+ Partials 720 719 -1
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.spec.tsx (1)
58-58: Remove unused parameter from setup function interface.The
fallbackComponentparameter is defined in the interface but never used in the function.-function setup({ isRegistered = true, props = {} }: { isRegistered?: boolean; props?: Record<string, any>; fallbackComponent?: React.ComponentType }) { +function setup({ isRegistered = true, props = {} }: { isRegistered?: boolean; props?: Record<string, any> }) {apps/deploy-web/src/services/route-protector/route-protector.service.ts (1)
12-26: LGTM! Proper implementation of combined feature flag and authentication protection.The method correctly implements the PR objective by enforcing both feature flag enablement and user authentication. The logic is clear and the return values follow Next.js conventions.
Consider adding error handling for potential exceptions from
getSessionorisEnabledForCtx:showToRegisteredUserIfEnabled(name: string): GetServerSideProps { return async context => { + try { const session = await this.getSession(context.req, context.res); const isEnabled = await this.featureFlagService.isEnabledForCtx(name, context); if (isEnabled && session?.user) { return { props: {} }; } return { notFound: true }; + } catch (error) { + // Log error and return 404 for security + console.error('Route protection error:', error); + return { notFound: true }; + } }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/deploy-web/src/components/layout/Sidebar.tsx(4 hunks)apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.spec.tsx(1 hunks)apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.tsx(1 hunks)apps/deploy-web/src/pages/alerts/contact-points/index.tsx(1 hunks)apps/deploy-web/src/pages/alerts/contact-points/new.tsx(1 hunks)apps/deploy-web/src/pages/alerts/index.tsx(1 hunks)apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts(2 hunks)apps/deploy-web/src/services/feature-flag/feature-flag.service.ts(1 hunks)apps/deploy-web/src/services/route-protector/index.ts(1 hunks)apps/deploy-web/src/services/route-protector/route-protector.service.spec.ts(1 hunks)apps/deploy-web/src/services/route-protector/route-protector.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/deploy-web/src/components/layout/Sidebar.tsx (1)
apps/deploy-web/src/hooks/useUser.ts (1)
useUser(8-24)
apps/deploy-web/src/pages/alerts/index.tsx (5)
apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.tsx (1)
RegisteredUsersOnly(6-22)apps/deploy-web/src/components/alerts/AlertsPage.tsx (1)
AlertsPage(7-14)apps/deploy-web/src/pages/alerts/contact-points/new.tsx (1)
getServerSideProps(7-7)apps/deploy-web/src/pages/alerts/contact-points/index.tsx (1)
getServerSideProps(7-7)apps/deploy-web/src/services/route-protector/index.ts (1)
routeProtector(6-6)
apps/deploy-web/src/services/route-protector/route-protector.service.ts (2)
apps/deploy-web/src/services/feature-flag/index.ts (1)
featureFlagService(6-6)apps/deploy-web/src/services/feature-flag/feature-flag.service.ts (1)
FeatureFlagService(6-47)
apps/deploy-web/src/services/route-protector/index.ts (2)
apps/deploy-web/src/services/route-protector/route-protector.service.ts (1)
RouteProtectorService(6-26)apps/deploy-web/src/services/feature-flag/index.ts (1)
featureFlagService(6-6)
apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.tsx (1)
apps/deploy-web/src/hooks/useUser.ts (1)
useUser(8-24)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (35)
apps/deploy-web/src/components/layout/Sidebar.tsx (4)
42-42: LGTM: Proper import for user authentication check.The import of
useUserhook is correctly added to support the conditional rendering logic for the alerts route.
70-70: LGTM: User hook usage follows established pattern.The
useUserhook is properly called to retrieve the current user state, consistent with how it's used in theRegisteredUsersOnlyHOC.
108-115: LGTM: Dual-condition check ensures proper access control.The conditional logic correctly implements the access control requirements by checking both:
- Feature flag status (
isAlertsEnabled)- User registration status (
user?.userId)This aligns perfectly with the server-side protection implemented via
routeProtector.showToRegisteredUserIfEnabledand the client-side protection via theRegisteredUsersOnlyHOC.
118-118: LGTM: Dependency array correctly updated.The addition of
user?.userIdto the dependency array ensures that the routes are recomputed when the user's registration status changes, which is essential for the conditional alerts route rendering.apps/deploy-web/src/services/route-protector/index.ts (1)
1-7: LGTM: Clean dependency injection pattern for route protection.This module follows excellent architectural patterns:
- Singleton pattern: Appropriate for route protection as it ensures consistent behavior across the application
- Dependency injection: Properly injects
featureFlagServiceandgetSessiondependencies, making the service testable and loosely coupled- Centralized logic: Consolidates route protection logic in one place, promoting maintainability
The integration with Auth0's
getSessionand the existingfeatureFlagServicecreates a cohesive access control mechanism that combines feature flags with user authentication.apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.tsx (1)
6-22: LGTM: Well-implemented HOC following React best practices.This HOC demonstrates excellent implementation patterns:
- Type safety: Uses generic type
P extends objectfor proper prop forwarding- User registration check: Correctly uses
!!user?.userIdto determine registration status, consistent with theuseUserhook's behavior- Configurable fallback: Allows custom fallback component while defaulting to 404 page
- Debug support: Properly sets
displayNamefor better debugging experience- Prop forwarding: Correctly spreads props to the wrapped component
The logic aligns perfectly with the server-side protection via
routeProtector.showToRegisteredUserIfEnabled, creating a comprehensive access control mechanism.apps/deploy-web/src/pages/alerts/contact-points/new.tsx (3)
2-3: LGTM: Proper imports for dual-layer protection.The imports correctly bring in both the client-side (
RegisteredUsersOnlyHOC) and server-side (routeProtector) protection mechanisms.
5-5: LGTM: Client-side protection properly implemented.The
RegisteredUsersOnlyHOC wrapper ensures that even if server-side protection is bypassed, the component will only render for registered users on the client side.
7-7: LGTM: Server-side protection with combined checks.The
routeProtector.showToRegisteredUserIfEnabled("alerts")method provides server-side protection by checking both:
- The "alerts" feature flag status
- User session/registration status
This creates a robust defense-in-depth approach when combined with the client-side HOC protection.
apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.spec.tsx (6)
10-18: LGTM! Good test coverage for the registered user scenario.The test correctly verifies that the wrapped component renders when the user is registered and the fallback component doesn't render.
20-26: LGTM! Proper test coverage for the unregistered user scenario.The test correctly verifies the inverse behavior - fallback renders when user is not registered.
28-39: Excellent test for prop forwarding behavior.This test ensures the HOC correctly passes through props to the wrapped component, which is crucial for HOC functionality.
41-45: Good test for displayName behavior.Testing the displayName is important for debugging React components, especially when using HOCs.
47-56: Excellent edge case handling for anonymous components.This test ensures the HOC gracefully handles components without displayName or name properties, which is a common edge case.
69-76:Details
❌ Incorrect review comment
Consider using more realistic user data structure.
The mock user profile structure should match what the actual
useUserhook returns from Auth0. Consider verifying the expected structure.
🏁 Script executed:
#!/bin/bash # Search for the actual useUser hook usage to verify the expected user structure rg -A 10 "useUser.*from.*auth0" --type tsx --type tsLength of output: 82
I’ll retry searching for all
useUseroccurrences across both.tsand.tsxfiles using glob patterns:
🏁 Script executed:
#!/bin/bash rg -A10 "useUser" -g "*.ts*"Length of output: 37628
Gathering the HOC implementation, its test, and the
CustomUserProfileshape for verification:#!/bin/bash echo "=== HOC implementation ===" rg -n "RegisteredUsersOnly" -A20 apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.tsx || true echo echo "=== Test file ===" cat apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.spec.tsx echo echo "=== CustomUserProfile type ===" rg -n "export type CustomUserProfile" -A20 apps/deploy-web/src/types/user.ts || true
I need to inspect the HOC’s test and the exact shape of
CustomUserProfileto ensure our mock aligns. Running:
🏁 Script executed:
#!/bin/bash echo "=== registered-users-only.hoc.spec.tsx (first 200 lines) ===" sed -n '1,200p' apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.spec.tsx || true echo echo "=== Locate CustomUserProfile in types ===" rg -n "CustomUserProfile" -g "apps/deploy-web/src/types/*.ts" || true echo echo "=== CustomUserProfile definition (first 200 lines) ===" sed -n '1,200p' apps/deploy-web/src/types/user.ts || trueLength of output: 4432
Suggestion Unnecessary: Current Mock Covers Required Fields
The
RegisteredUsersOnlyHOC only usesuserIdto determine registration, so your mock returning{ userId, email }is sufficient for this test. There’s no need to mirror the entire Auth0UserProfileshape here.Likely an incorrect or invalid review comment.
apps/deploy-web/src/pages/alerts/index.tsx (3)
2-3: LGTM! Proper imports for the new protection mechanism.The imports correctly bring in both the client-side HOC and server-side route protector.
5-5: LGTM! Clean implementation of client-side protection.The default export is properly wrapped with the
RegisteredUsersOnlyHOC, which will handle client-side access control.
7-7: LGTM! Consistent server-side protection implementation.The
getServerSidePropscorrectly uses the route protector'sshowToRegisteredUserIfEnabledmethod, providing server-side access control that matches the client-side protection.apps/deploy-web/src/pages/alerts/contact-points/index.tsx (3)
2-3: LGTM! Consistent import pattern across alert pages.The imports match the pattern used in the alerts index page, maintaining consistency.
5-5: LGTM! Consistent HOC usage pattern.The ContactPointsPage is properly wrapped with the RegisteredUsersOnly HOC, following the same pattern as other alert pages.
7-7: LGTM! Consistent server-side protection.The getServerSideProps implementation matches the pattern used across other alert pages, ensuring consistent access control.
apps/deploy-web/src/services/route-protector/route-protector.service.spec.ts (6)
10-20: LGTM! Comprehensive happy path test.The test correctly verifies that when both conditions are met (feature enabled + user logged in), the service returns props for rendering.
22-32: Excellent coverage of all failure scenarios.These tests thoroughly cover all combinations where access should be denied, ensuring the service correctly returns
{ notFound: true }in each case.Also applies to: 34-44, 46-56
58-68: Good integration test for feature flag service.This test ensures the feature flag service is called with the correct parameters, verifying proper integration.
70-80: Good integration test for session service.This test verifies that the session service is called with the correct request/response objects from the context.
102-114: Well-structured mock context.The GetServerSidePropsContext mock includes the essential properties needed for testing, with realistic faker-generated data.
87-98:Details
❓ Verification inconclusive
Consider verifying the session structure matches Auth0's actual session format.
The mock session structure should align with what Auth0's
getSessionactually returns to ensure test accuracy.
🏁 Script executed:
#!/bin/bash # Search for Auth0 session usage patterns to verify the expected structure rg -A 15 "getSession.*from.*auth0" --type ts --type tsxLength of output: 85
🏁 Script executed:
#!/bin/bash # Locate the route protector service and inspect how getSession is used echo "Finding service file:" fd route-protector.service.ts || echo "File not found with fd; listing matching files:" rg "route-protector.service" --files echo -e "\nSearching for getSession usage in the repo:" rg "getSession" -nLength of output: 67312
Verify Auth0
getSessionreturn shape
The test currently stubsgetSessionto return only:{ user: { sub, email, name, picture } }Auth0’s
getSessionactually returns a fullerSessionobject—including fields likeaccessToken,idToken,expiresIn, etc.—in addition touser.
• If your route-protector service only readssession.user.sub, this mock is sufficient.
• Otherwise, extend the stub to include any additional properties your code uses.apps/deploy-web/src/services/feature-flag/feature-flag.service.ts (2)
33-39: LGTM! Good separation of concerns with the newisEnabledForCtxmethod.The extraction of feature flag evaluation logic into a dedicated method improves modularity and enables reuse by other services like the
RouteProtectorService. The method correctly handles both the global enable-all flag and context-specific evaluation.
41-46: LGTM! Clean refactoring ofshowIfEnabledmethod.The refactored method properly delegates to
isEnabledForCtxand maintains the same behavior while improving code reusability. The logic is clear and concise.apps/deploy-web/src/services/route-protector/route-protector.service.ts (1)
6-11: LGTM! Well-designed service with clear dependencies.The constructor properly injects the required dependencies for feature flag evaluation and user session management. The dependency injection pattern makes the service testable and follows good architectural practices.
apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts (5)
11-30: LGTM! Well-structured tests forgetFlagmethod with good coverage.The tests are properly organized and cover both the enable-all configuration scenario and the normal flag evaluation flow. The mocking strategy is appropriate and the assertions verify the expected behavior.
32-44: LGTM! Comprehensive tests forextractSessionIdmethod.The tests cover both the successful session ID extraction and the case where no session ID is present. This ensures the cookie parsing logic works correctly in various scenarios.
46-64: LGTM! Thorough tests for the newisEnabledForCtxmethod.The tests properly verify both the enable-all configuration path and the normal evaluation flow, including the interaction with
extractSessionIdandgetFlag. The use of spies ensures the method calls are verified correctly.
66-86: LGTM! Complete test coverage forshowIfEnabledmethod.The tests verify both the enabled and disabled scenarios, ensuring the method returns the correct server-side props structure. The mocking of
isEnabledForCtxproperly isolates the testing of this method's specific logic.
88-109: LGTM! Simplified and improved setup function.The removal of the
getFlagResultparameter and the addition of the spy onextractSessionIdmakes the setup function more focused and easier to use across different test scenarios.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor