-
Notifications
You must be signed in to change notification settings - Fork 3
chore: Add token revocation, non-public auth documentation + update Next.js architecture documentation #619
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
44c8392
chore: enhance Next.js architecture documentation and introduce API a…
8lane 1ee6f73
chore: mermaid
8lane c91c896
feat: sign out with revoke
8lane 93f7f66
test: add unit tests for signOut function to verify token revocation …
8lane eab5223
Update docs/auth/cognito-session-persistence-issue.md
8lane File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,107 @@ | ||
| # Future Plans for Next.js Architecture | ||
|
|
||
| We are planning to enhance our Next.js architecture by removing CloudFront and re-enabling request-level caching. This change will optimise performance while allowing us to leverage Next.js features more effectively. | ||
| We are planning to enhance our Next.js architecture by removing CloudFront and re-enabling request-level caching for the public dashboard. The non-public dashboard is already using this architecture, which has proven effective for handling authenticated requests and dynamic content. | ||
|
|
||
| ## Key Changes | ||
| ## Current State (Non-Public Dashboard) | ||
|
|
||
| The non-public dashboard currently uses this architecture: | ||
|
|
||
| 1. **Direct Next.js Access:** | ||
|
|
||
| - Requests go directly to the Next.js server | ||
| - No CDN layer between user and server | ||
| - Immediate authentication validation | ||
|
|
||
| 2. **Request-Level Caching:** | ||
|
|
||
| - Due to dynamic functions (cookies, headers, etc.), we currently only have request-level caching for public endpoints | ||
| - Endpoints requiring authorization headers must not be cached to ensure security and data freshness | ||
| - Allows for per-user content and authentication state | ||
| - Maintains security while providing performance benefits | ||
| - We do not use page-level caching because: | ||
| - Pages contain dynamic data that requires fresh fetches | ||
| - Authentication state needs to be checked on every request | ||
| - User-specific content must be generated dynamically | ||
| - Dynamic functions (cookies, headers) prevent static page generation | ||
| - **Redis Integration:** Currently used to handle cached data across the load balancer, ensuring consistent performance across multiple containers | ||
|
|
||
| 3. **Dynamic Content Handling:** | ||
|
|
||
| - Efficient handling of authenticated requests | ||
| - Immediate content updates without cache invalidation | ||
| - Better user experience for authenticated users | ||
|
|
||
| 4. **Current Performance Considerations:** | ||
| - Whilst effective for security and dynamic content, there is a significant performance hit on page load times | ||
| - This is expected given the minimal caching strategy currently in place | ||
| - Future optimization efforts should focus on: | ||
| - Implementing ISR (Incremental Static Regeneration) for static content | ||
| - Exploring PPR (Partial Prerendering) once stable in Next.js 14 | ||
| - Optimizing data fetching patterns and component boundaries | ||
| - References: | ||
| - [Next.js ISR Documentation](https://nextjs.org/docs/app/building-your-application/data-fetching/revalidating) | ||
| - [Next.js PPR RFC](https://github.com/vercel/next.js/discussions/54908) | ||
| - [Next.js Performance Optimization Guide](https://nextjs.org/docs/app/building-your-application/optimizing) | ||
|
|
||
| ## Planned Changes (Public Dashboard) | ||
|
|
||
| 1. **Removal of CloudFront:** | ||
|
|
||
| - Simplifies our architecture and reduces complexity. | ||
| - Eliminates CDN latency, allowing direct requests to Next.js. | ||
| - Simplifies our architecture and reduces complexity | ||
| - Eliminates CDN latency, allowing direct requests to Next.js | ||
| - Matches the successful architecture of the non-public dashboard | ||
|
|
||
| 2. **Re-enabling Request-Level Caching:** | ||
|
|
||
| - Utilises Next.js's built-in caching mechanisms to improve response times for dynamic routes. | ||
| - **Redis Integration:** Employing Redis for shared caching across multiple containers in the load balancer to ensure consistent performance and reduced latency. | ||
| - Utilises Next.js's built-in caching mechanisms to improve response times for dynamic routes | ||
| - **Redis Integration:** Will use the same Redis setup as the non-public dashboard for shared caching across multiple containers in the load balancer | ||
| - Endpoints requiring authorization headers will not be cached, following the same security principles as the non-public dashboard | ||
| - Page-level caching will be selectively implemented for truly static content only | ||
|
|
||
| 3. **Incremental Static Regeneration (ISR):** | ||
|
|
||
| - ISR will be reactivated, allowing us to update static content without a full rebuild. | ||
| - Allows for caching of dynamic pages, enabling efficient content delivery. | ||
| - ISR will be reactivated, allowing us to update static content without a full rebuild | ||
| - Allows for caching of dynamic pages, enabling efficient content delivery | ||
| - Will help mitigate the performance impact of removing CloudFront | ||
|
|
||
| 4. **CMS Integration for On-Demand Purges:** | ||
|
|
||
| - Hooking up the caching strategy to our CMS for instant purging of outdated content, now managed directly without CloudFront. | ||
| - Allows us to manage content updates dynamically based on CMS events. | ||
| - Hooking up the caching strategy to our CMS for instant purging of outdated content, now managed directly without CloudFront | ||
| - Allows us to manage content updates dynamically based on CMS events | ||
|
|
||
| 5. **Optimised Data Fetching with App Router:** | ||
| - Leveraging the App Router in Next.js 14 for better routing and data fetching strategies. | ||
| - Utilising **React Server Components** for improved data handling and rendering without relying on traditional data fetching methods. | ||
| - Leveraging the App Router in Next.js 14 for better routing and data fetching strategies | ||
| - Utilising **React Server Components** for improved data handling and rendering without relying on traditional data fetching methods | ||
|
|
||
| ## Benefits | ||
|
|
||
| - **On-Demand Revalidation:** The main benefit of this architecture is the ability to perform on-demand revalidation of cached content, ensuring that users receive the most up-to-date information without delays. | ||
| - **Dynamic Content Handling:** Efficiently manage dynamic content updates with ISR and request-level caching. | ||
| - **Easier Content Management:** Immediate updates from the CMS streamline our workflow, without the need for manual cache purges. | ||
| - **On-Demand Revalidation:** The main benefit of this architecture is the ability to perform on-demand revalidation of cached content, ensuring that users receive the most up-to-date information without delays | ||
| - **Dynamic Content Handling:** Efficiently manage dynamic content updates with ISR and request-level caching | ||
| - **Easier Content Management:** Immediate updates from the CMS streamline our workflow, without the need for manual cache purges | ||
| - **Consistent Architecture:** Both public and non-public dashboards will use the same proven architecture | ||
| - **Security First:** Ensures authenticated endpoints are never cached, maintaining data security and freshness | ||
|
|
||
| ## Performance Optimization Roadmap | ||
|
|
||
| 1. **Short Term:** | ||
|
|
||
| - Implement ISR for static content | ||
| - Optimize data fetching patterns | ||
| - Improve component boundaries for better streaming | ||
|
|
||
| 2. **Medium Term:** | ||
|
|
||
| - Evaluate and implement PPR once stable | ||
| - Add Redis caching for shared data | ||
| - Optimize bundle sizes and loading strategies | ||
|
|
||
| 3. **Long Term:** | ||
| - Monitor and optimize based on real-world performance metrics | ||
| - Consider edge caching strategies | ||
| - Implement progressive enhancement where appropriate | ||
|
|
||
| ## Further Investigation | ||
|
|
||
| For more details on this transition, please refer to the investigation in the [Pull Request #525](https://github.com/UKHSA-Internal/data-dashboard-frontend/pull/525). | ||
|
|
||
| This updated architecture will ensure better performance, scalability, and ease of maintenance while fully leveraging Next.js capabilities. | ||
| This updated architecture will ensure better performance, scalability, and ease of maintenance while fully leveraging Next.js capabilities, as demonstrated by the non-public dashboard's current implementation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,32 @@ | ||
| # Next.js 14 Architecture Overview | ||
|
|
||
| This architecture is optimized for aggressive caching to enhance performance, using Next.js 14 with AWS deployment in a Docker container and AWS CloudFront CDN for efficient delivery. Here’s a brief overview: | ||
| This architecture is optimized for aggressive caching to enhance performance, using Next.js 14 with AWS deployment in a Docker container. The public dashboard uses AWS CloudFront CDN for efficient delivery, while the non-public dashboard uses direct Next.js access. Here's a brief overview: | ||
|
|
||
| 1. **Next.js 14 with App Router:** | ||
| - `forceDynamic`: Applied on all routes to prevent caching at the origin level, ensuring that cache-control settings rely entirely on CloudFront. | ||
| - `forceDynamic`: Applied on all routes to prevent caching at the origin level, ensuring that cache-control settings rely entirely on CloudFront (for public dashboard). | ||
| 2. **Docker & AWS:** | ||
| - **Docker:** Provides a consistent, portable runtime environment. | ||
| - **AWS Deployment:** Hosting in AWS allows scalability and tight integration with CloudFront. | ||
| 3. **CloudFront CDN:** | ||
| - **AWS Deployment:** Hosting in AWS allows scalability and tight integration with CloudFront (for public dashboard). | ||
| 3. **CloudFront CDN (Public Dashboard Only):** | ||
| - **Caching Strategy:** | ||
| - **Default TTL:** A 30-day TTL for all routes ensures that cached responses are aggressively reused. | ||
| - **Manual Cache Flush:** CMS changes require manual cache invalidation to reflect updates across cached content. | ||
| - **Specific TTL Adjustments:** Landing page and weather alerts are set to shorter TTLs for selective, more frequent updates. | ||
|
|
||
| ## Workflow Summary | ||
|
|
||
| ### Public Dashboard | ||
|
|
||
| 1. **User Request → CloudFront:** If cached, the response is delivered immediately. If not, it forwards the request to Next.js. | ||
| 2. **Next.js processes uncached requests dynamically,** delivering them back to CloudFront. | ||
| 3. **CloudFront Caches the response** for the configured TTL, requiring manual purges for any CMS-driven content updates. | ||
|
|
||
| This setup ensures performance optimisation via aggressive caching, reducing server load and latency while relying on manual cache management for selective updates. | ||
| ### Non-Public Dashboard | ||
|
|
||
| 1. **User Request → Next.js:** Requests go directly to the Next.js server | ||
| 2. **Next.js processes requests dynamically:** | ||
| - Public endpoints can use request-level caching where appropriate | ||
| - Endpoints requiring authorization headers must not be cached to ensure security and data freshness | ||
| 3. **Dynamic Functions:** Due to the use of dynamic functions (cookies, headers, etc.), we currently only have request-level caching for public endpoints | ||
|
|
||
| This setup ensures performance optimisation via aggressive caching for the public dashboard while allowing for more dynamic content handling in the non-public dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| # API Authentication Forwarding | ||
|
|
||
| ## Current State | ||
|
|
||
| Currently, none of the API endpoints consumed by the dashboard forward authentication tokens to the backend services. This means that while users are authenticated in the frontend, backend services are not receiving the necessary authentication information to validate requests. | ||
|
|
||
| ## Technical Requirements | ||
|
|
||
| ### Token Forwarding | ||
|
|
||
| We need to ensure that all API requests include: | ||
|
|
||
| 1. The access token in the Authorization header when authentication is required | ||
| 2. Any necessary Cognito-specific headers | ||
| 3. Proper error handling for token-related issues | ||
|
|
||
| ### Implementation Pattern | ||
|
|
||
| Our existing `client` function in `src/api/utils/api.utils.ts` should be updated to handle authentication state. The function already has an `isPublic` flag which we can use to determine when to include authentication headers. | ||
|
|
||
| ```typescript | ||
| // src/api/utils/api.utils.ts | ||
| import { auth } from '@/auth' | ||
|
|
||
| export async function client<T>( | ||
| endpoint: string, | ||
| { | ||
| body, | ||
| // Defaulting all requests to public (non-authenticated) for now. | ||
| // This may change to an opt-in approach as we build out the authenticated dashboard. | ||
| isPublic = true, | ||
| searchParams, | ||
| baseUrl = getApiBaseUrl(), | ||
| ...customConfig | ||
| }: Options = {} | ||
| ): Promise<{ data: T | null; status: number; error?: Error; headers?: Headers }> { | ||
| const headers: HeadersInit = { | ||
| 'content-type': 'application/json', | ||
| // Only include API key for public requests | ||
| ...(isPublic ? { Authorization: process.env.API_KEY ?? '' } : {}), | ||
| } | ||
|
|
||
| // Get auth token for authenticated requests | ||
| if (!isPublic) { | ||
| const session = await auth() | ||
| if (!session?.accessToken) { | ||
| throw new Error('No access token available') | ||
| } | ||
| headers.Authorization = `Bearer ${session.accessToken}` | ||
| } | ||
|
|
||
| // Rest of existing client function... | ||
| const fetchOptions: RequestInit & { next?: { revalidate: number; tags: string[] } } = { | ||
| method: body ? 'POST' : 'GET', | ||
| body: body ? JSON.stringify(body) : undefined, | ||
| ...customConfig, | ||
| next: { | ||
| revalidate: authEnabled && isPublic ? customConfig.next?.revalidate ?? cacheRevalidationInterval : 0, | ||
| tags: authEnabled && isPublic ? [cacheFetchTags.public] : [], | ||
| }, | ||
| headers: { | ||
| ...headers, | ||
| ...customConfig.headers, | ||
| }, | ||
| } | ||
|
|
||
| // ... rest of existing implementation | ||
| } | ||
| ``` | ||
|
|
||
| Usage remains the same, but now includes auth option: | ||
|
|
||
| ```typescript | ||
| // Public endpoint (existing behavior) | ||
| const response = await client('/api/public') | ||
|
|
||
| // Authenticated endpoint | ||
| const response = await client('/api/protected', { isPublic: false }) | ||
| ``` | ||
|
|
||
| ## Public vs Non-Public Dashboard Authentication | ||
|
|
||
| The `isPublic` flag is opt-in (defaults to `true`) because: | ||
|
|
||
| 1. **Shared Endpoints**: Many API endpoints are shared between public and non-public dashboards. | ||
|
|
||
| 2. **Selective Authentication**: Only specific endpoints in the non-public dashboard will require authentication. | ||
|
|
||
| 3. **Backward Compatibility**: Keeping `isPublic = true` as default ensures: | ||
| - Existing public dashboard endpoints continue to work | ||
| - No breaking changes for shared endpoints | ||
| - Gradual migration to authenticated endpoints | ||
|
|
||
| Example of endpoint categorization: | ||
|
|
||
| ```typescript | ||
| // Shared endpoints (isPublic = true) | ||
| const geographicData = await client('/api/geographic-data') | ||
| const visualizationData = await client('/api/visualization-data') | ||
|
|
||
| // Non-public only endpoints (isPublic = false) | ||
| const userPreferences = await client('/api/user-preferences', { isPublic: false }) | ||
| const adminSettings = await client('/api/admin-settings', { isPublic: false }) | ||
| ``` | ||
|
|
||
| ## Required Changes | ||
|
|
||
| ### API Utilities | ||
|
|
||
| We need to update our existing `client` function to: | ||
|
|
||
| 1. Use the existing `isPublic` flag to determine auth requirements | ||
| 2. Add auth headers conditionally based on `isPublic` | ||
| 3. Handle auth-related errors appropriately | ||
|
|
||
| ## Testing Requirements | ||
|
|
||
| 1. **Authentication Tests** | ||
|
|
||
| - Verify token forwarding | ||
| - Test unauthorized scenarios | ||
| - Validate error handling | ||
|
|
||
| 2. **Integration Tests** | ||
|
|
||
| - Test with backend services | ||
| - Verify token refresh flow | ||
| - Check error propagation | ||
|
|
||
| ## Future Considerations | ||
|
|
||
| 1. **Monitoring** | ||
|
|
||
| - Track authentication failures | ||
| - Monitor token refresh patterns | ||
| - Log security-related events | ||
|
|
||
| 2. **Performance** | ||
|
|
||
| - Optimize token handling | ||
| - Implement request caching where appropriate |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| # Cognito Session Persistence Issue | ||
|
|
||
| ## Overview | ||
|
|
||
| When implementing sign out functionality, we discovered that Cognito's token revocation process is separate from NextAuth.js's session management. This means that even after clearing NextAuth.js's internal session and cookies, the Cognito tokens can remain valid, allowing automatic re-authentication. | ||
|
|
||
| To address this, we added a direct token revocation request to Cognito's `/revoke` endpoint in our sign out flow. This request attempts to invalidate the refresh token on Cognito's side. However, we're still seeing cases where users can automatically re-authenticate after signing out, suggesting that either: | ||
|
|
||
| 1. The token revocation isn't fully effective | ||
| 2. There's another mechanism at play that we haven't identified yet | ||
|
|
||
| ## The Problem | ||
|
|
||
| Our current sign out flow: | ||
|
|
||
| 1. Revokes the refresh token with Cognito | ||
| 2. Clears NextAuth.js session and cookies | ||
| 3. Redirects to the start page | ||
|
|
||
| However, Cognito's token revocation process is independent of NextAuth.js's session management. This leads to: | ||
|
|
||
| - Users being automatically re-authenticated after signing out | ||
| - Inconsistent state between NextAuth.js's session and Cognito's token validity | ||
| - Potential security concerns with lingering valid tokens | ||
|
|
||
| ## Technical Details | ||
|
|
||
| - NextAuth.js manages its session through cookies and internal state | ||
| - Cognito manages authentication through: | ||
| - Access tokens | ||
| - Refresh tokens | ||
| - Token revocation endpoints | ||
| - These systems operate independently, requiring explicit token revocation on both sides | ||
|
|
||
| ## Current Solution | ||
|
|
||
| Our `signOut` function now: | ||
|
|
||
| 1. Attempts to revoke the refresh token with Cognito first | ||
| 2. Only proceeds with NextAuth.js sign out after successful token revocation | ||
| 3. Includes error handling to ensure NextAuth.js cleanup even if Cognito revocation fails | ||
|
|
||
| ## Future Considerations | ||
|
|
||
| 1. Investigate additional Cognito token revocation methods | ||
| 2. Consider implementing a more comprehensive sign out flow that: | ||
| - Revokes all tokens (access and refresh) | ||
| - Ensures complete token invalidation on both sides | ||
| 3. Add monitoring for failed sign out attempts | ||
|
|
||
| ## Testing Implications | ||
|
|
||
| When writing tests for authentication flows: | ||
|
|
||
| 1. Ensure both NextAuth.js session and Cognito tokens are properly revoked | ||
| 2. Verify that automatic re-authentication doesn't occur after sign out | ||
| 3. Test error scenarios during sign out | ||
| 4. Consider adding specific test cases for token revocation issues | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.