feat(services-bff): My pages bff setup #16543
Conversation
… better in comments what encrypt/decrypt is doing and update crypto test to not use mock
… query param in client
…rl to be defensive, i.e. no undefined possible.
… scope field which was due to backwards compatibility
…Also deletes the logout callback
…d proxy service and a little refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
charts/islandis/values.dev.yaml(3 hunks)charts/islandis/values.prod.yaml(3 hunks)charts/islandis/values.staging.yaml(3 hunks)charts/services/services-bff-portals-admin/values.dev.yaml(1 hunks)charts/services/services-bff-portals-admin/values.prod.yaml(1 hunks)charts/services/services-bff-portals-admin/values.staging.yaml(1 hunks)charts/services/services-bff-portals-my-pages/values.dev.yaml(1 hunks)charts/services/services-bff-portals-my-pages/values.prod.yaml(1 hunks)charts/services/services-bff-portals-my-pages/values.staging.yaml(1 hunks)infra/src/dsl/bff.ts(2 hunks)infra/src/dsl/feature-values.spec.ts(2 hunks)infra/src/dsl/portal-env.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- charts/services/services-bff-portals-admin/values.dev.yaml
- charts/services/services-bff-portals-admin/values.prod.yaml
- charts/services/services-bff-portals-admin/values.staging.yaml
- charts/services/services-bff-portals-my-pages/values.dev.yaml
- charts/services/services-bff-portals-my-pages/values.staging.yaml
- infra/src/dsl/feature-values.spec.ts
- infra/src/dsl/portal-env.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
infra/src/dsl/bff.ts (1)
Pattern infra/src/dsl/**/*: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
🔇 Additional comments (22)
infra/src/dsl/bff.ts (4)
10-16: LGTM! Well-structured constants and utility function.
The constants improve code maintainability, and the sanitizePath utility function is well-documented with a robust regex implementation.
20-34: LGTM! Improved scope handling with proper deduplication.
The implementation effectively addresses previous review comments by:
- Using Set for scope deduplication
- Properly wrapping switch cases in blocks
- Clear separation of concerns between different portal types
107-112: LGTM! Well-structured callback path handling.
The callback paths are properly constructed with:
- Consistent use of sanitized global prefix
- Environment-specific base URLs
- Clear separation between environments
96-98:
Security: Restrict production redirect URIs to specific paths.
The production redirect URIs are currently too permissive, allowing any path under the domain. This could potentially lead to security vulnerabilities.
Apply this change to restrict the URIs:
-prod: json([ref((ctx) => addProtocol(ctx.env.domain))]),
+prod: ref((ctx) => json(getRedirectUris(getBaseUrl(ctx), key))),charts/services/services-bff-portals-my-pages/values.prod.yaml (5)
64-76: Add rate limiting and SSL configuration
The ingress configuration is missing several security measures:
- Rate limiting configuration to prevent abuse
- TLS configuration for secure communication
- Proper redirection between www and non-www domains
87-93: Review memory allocation for Node.js process
The current memory configuration might be insufficient with Node.js --max-old-space-size=460 setting.
8-18: Verify audit logging configuration across services
The audit logging path /island-is/audit-log should be consistent with other services in the cluster.
✅ Verification successful
Audit logging path configuration is consistent
The audit logging path /island-is/audit-log is correctly configured and consistent with most services in the cluster. The only variation is for identity-server related services which appropriately use /identity-server/audit-log path, indicating proper domain separation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check audit log path configuration across services
rg "AUDIT_GROUP_NAME" --type yaml charts/
Length of output: 16881
33-35: Review internal service exposure and scope list
Two concerns:
- Internal service URL
web-api.islandis.svc.cluster.localis exposed in configuration - Large number of scopes might violate principle of least privilege
✅ Verification successful
Based on the search results, I can now provide a final response:
Internal service URLs and scopes are appropriately configured
The findings show that:
- The internal service URL pattern (
*.svc.cluster.local) is consistently used across all environments (dev/staging/prod) for internal service communication within the Kubernetes cluster, which is a standard practice - The scope list for
services-bff-portals-my-pagesis identical across all environments and matches other BFF services in terms of granularity, with each scope following a clear domain-specific pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if internal service URL is exposed in other configurations
rg "svc.cluster.local" --type yaml charts/
# Compare scope list with other BFF services
rg "IDENTITY_SERVER_CLIENT_SCOPES" --type yaml charts/services/
Length of output: 31146
101-105: Verify IAM role permissions
The service account uses IAM role arn:aws:iam::251502586493:role/services-bff. Ensure this role follows the principle of least privilege.
charts/islandis/values.staging.yaml (4)
1543-1543: LGTM: Namespace addition is correct
The addition of 'portals-my-pages' namespace follows the established pattern and is properly formatted.
2121-2124: Path configuration needs service context
The BFF paths and ingress configuration could lead to routing conflicts:
- BFF_CALLBACKS_BASE_PATH uses '/bff/callbacks' but should include the service context '/minarsidur'
- BFF_GLOBAL_PREFIX uses '/bff' but should be more specific
- Ingress path '/bff' is too generic and could conflict with other BFF services
Also applies to: 2169-2170
2141-2148: Health check paths need to align with service context
The health check paths should include the full service context path:
- Current liveness path: '/bff/liveness'
- Current readiness path: '/bff/health/check'
2192-2194: LGTM: Security configuration is robust
The service configuration includes:
- Proper security context preventing privilege escalation
- Comprehensive list of client scopes with proper access control
- Appropriate resource limits and constraints
Also applies to: 2130-2131
charts/islandis/values.prod.yaml (4)
1669-1669: LGTM: Namespace addition follows conventions
The new namespace 'portals-my-pages' follows the established naming pattern used for other portal services.
2317-2322: LGTM: Resource configuration is well-defined
The resource limits and requests are appropriately configured:
- CPU and memory limits are reasonable
- Resource requests ensure proper scheduling
- HPA configuration enables proper scaling
2254-2256: Verify callback and base path configuration
The following settings need verification:
- BFF_CALLBACKS_BASE_PATH points to '/bff/callbacks' which differs from admin portal pattern
- BFF_CLIENT_BASE_URL is set to '[null]' which might cause issues
✅ Verification successful
Based on the search results, I can now provide a final response as the patterns are clear across different environments and services.
BFF configuration is correctly set according to service patterns
The configuration is consistent with the established patterns across environments:
- For admin portal (
/stjornbord), callbacks include/stjornbord/bff/callbacksin the path - For my-pages portal (
/minarsidur), callbacks use/bff/callbacksin the path BFF_CLIENT_BASE_URLbeing[null]in production is intentional and matches other production configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent base path patterns across BFF services
rg "BFF_CALLBACKS_BASE_PATH|BFF_CLIENT_BASE_URL" -A 1
Length of output: 6581
2331-2334:
Dedicated IAM role needed for My Pages BFF
The service account is using a shared IAM role 'services-bff'. For better security isolation, create a dedicated IAM role for the My Pages BFF service.
Apply this change:
serviceAccount:
annotations:
- eks.amazonaws.com/role-arn: 'arn:aws:iam::251502586493:role/services-bff'
+ eks.amazonaws.com/role-arn: 'arn:aws:iam::251502586493:role/services-bff-my-pages'Likely invalid or redundant comment.
charts/islandis/values.dev.yaml (5)
1805-1805: LGTM: Namespace addition follows convention
The addition of 'portals-my-pages' namespace aligns with the existing namespace structure.
2383-2383:
Security concern: Inconsistent global prefix
The BFF paths should be namespaced consistently with other BFF services (like admin portal which uses '/stjornbord/bff'). Current configuration could lead to routing conflicts.
Apply these changes:
- BFF_CALLBACKS_BASE_PATH: 'https://beta.dev01.devland.is/bff/callbacks'
+ BFF_CALLBACKS_BASE_PATH: 'https://beta.dev01.devland.is/minarsidur/bff/callbacks'
- BFF_GLOBAL_PREFIX: '/bff'
+ BFF_GLOBAL_PREFIX: '/minarsidur/bff'Also applies to: 2386-2386
2403-2410: 🛠️ Refactor suggestion
Health check paths should be namespaced
The health check paths should follow the same namespacing pattern as other endpoints.
Apply these changes:
liveness:
initialDelaySeconds: 3
- path: '/bff/liveness'
+ path: '/minarsidur/bff/liveness'
timeoutSeconds: 3
readiness:
initialDelaySeconds: 3
- path: '/bff/health/check'
+ path: '/minarsidur/bff/health/check'2431-2432:
Security concern: Ingress path should be namespaced
The ingress path should be namespaced to avoid conflicts with other services.
Apply this change:
- - '/bff'
+ - '/minarsidur/bff'2444-2449: 🛠️ Refactor suggestion
Review memory limits against Node.js settings
The container memory limits might need adjustment:
- NODE_OPTIONS sets max-old-space-size=460MB
- Container memory limit is 512MB
- Memory request is 256MB
This leaves little headroom for the Node.js process and system operations.
Consider increasing the memory limits:
resources:
limits:
- memory: '512Mi'
+ memory: '768Mi'
requests:
- memory: '256Mi'
+ memory: '384Mi'
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
infra/src/dsl/bff.ts (1)
13-16: Consider enhancing path sanitization.While the current implementation handles basic path sanitization, consider enhancing it to handle additional edge cases:
- Multiple consecutive slashes within the path
- URL-encoded characters
- Special characters that might need escaping
-const sanitizePath = (path: string) => path.replace(/^\/+|\/+$/g, '') +const sanitizePath = (path: string) => { + // Remove leading/trailing slashes and normalize internal ones + return path + .replace(/\/+/g, '/') // Replace multiple slashes with single + .replace(/^\/|\/$/g, '') // Remove leading/trailing slash + .replace(/[^\w\-\/]/g, '-') // Replace special chars with dash +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
charts/islandis/values.prod.yaml(3 hunks)charts/services/services-bff-portals-admin/values.prod.yaml(1 hunks)charts/services/services-bff-portals-my-pages/values.prod.yaml(1 hunks)infra/src/dsl/bff.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/services/services-bff-portals-admin/values.prod.yaml
🧰 Additional context used
📓 Path-based instructions (1)
infra/src/dsl/bff.ts (1)
Pattern infra/src/dsl/**/*: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
🔇 Additional comments (14)
infra/src/dsl/bff.ts (6)
6-11: LGTM! Well-structured constants and imports.
The addition of typed constants improves code maintainability and reduces the risk of typos in portal key references.
20-33: LGTM! Proper scope deduplication implemented.
The implementation effectively:
- Uses Set for scope deduplication (addressing previous feedback)
- Properly blocks switch cases to prevent scope leakage
- Clearly separates concerns between different portal types
49-56: Simplify domain handling logic.
The domain construction logic could be simplified since ctx.env.domain already contains 'island.is' in production.
const getBaseUrl = (ctx: Context) => {
- const domain = ctx.featureDeploymentName
- ? `${ctx.featureDeploymentName}-beta.${ctx.env.domain}`
- : ctx.env.type === 'prod'
- ? ctx.env.domain
- : `beta.${ctx.env.domain}`
+ const domain = ctx.featureDeploymentName
+ ? `${ctx.featureDeploymentName}-beta.${ctx.env.domain}`
+ : ctx.env.type === 'prod'
+ ? ctx.env.domain
+ : `beta.${ctx.env.domain}`
return `https://${domain}`
}105-105: Consider using different ports for different apps.
To allow running multiple apps simultaneously in local development, consider using different ports for each app.
-local: `http://localhost:3010/${sanitizeGlobalPrefix}/callbacks`,
+local: `http://localhost:${key === MINAR_SIDUR ? '3010' : '3011'}/${sanitizeGlobalPrefix}/callbacks`,89-96:
Security: Restrict production redirect URIs.
The production redirect URIs are currently too permissive (https://island.is). This could potentially lead to security vulnerabilities.
BFF_ALLOWED_REDIRECT_URIS: {
local: json([
`http://localhost:4200/${key}`,
...(key === MINAR_SIDUR ? ['http://localhost:4242/umsoknir'] : []),
]),
dev: ref((ctx) => json(getRedirectUris(getBaseUrl(ctx), key))),
staging: ref((ctx) => json(getRedirectUris(getBaseUrl(ctx), key))),
- prod: json('https://island.is'),
+ prod: ref((ctx) => json(getRedirectUris(getBaseUrl(ctx), key))),
},Line range hint 40-126: Verify configuration structure across environments.
Let's verify that the configuration structure is consistent across all environments and properly handles the new BFF setup.
✅ Verification successful
Based on the search results, let me verify the configuration structure by searching for more specific patterns.
Configuration structure is consistent and properly handles BFF setup
The verification shows that:
- The
globalPrefixparameter is correctly used and sanitized across environments - Callback paths are consistently constructed using the sanitized prefix
- Base URL generation is uniform across all environments (dev, staging, prod)
- Environment-specific configurations maintain structural consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify configuration consistency across environments
# Check for any hardcoded URLs or environment-specific values
rg -l "island\.is" --type ts
# Check for consistent usage of the new globalPrefix parameter
ast-grep --pattern 'globalPrefix: $_'
# Verify BFF configuration usage across the codebase
rg -A 3 "bffConfig\(" --type ts
Length of output: 100284
Script:
#!/bin/bash
# Check for BFF configuration usage and structure
ast-grep --pattern 'BFF_GLOBAL_PREFIX: $_'
# Check for sanitizePath usage
ast-grep --pattern 'sanitizePath($_)'
# Check for getBaseUrl usage
ast-grep --pattern 'getBaseUrl($_)'
# Check for callback path construction
rg -A 2 "callbacks" --type ts
Length of output: 13779
charts/services/services-bff-portals-my-pages/values.prod.yaml (6)
8-18: LGTM: Global configuration follows best practices
The global configuration properly sets up AWS region, audit logging, and runtime parameters.
44-61: LGTM: Health checks and scaling configuration are well-defined
The health check endpoints and HPA configuration are properly set up with reasonable thresholds and scaling limits.
63-77: SSL and domain configuration needs attention
The ingress configuration accepts traffic from both island.is and www.island.is without TLS configuration.
87-93: Align memory limits with Node.js settings
The memory limits might be insufficient given the Node.js --max-old-space-size=460 setting.
98-105: LGTM: Security configuration follows best practices
The security context properly restricts privileges and the service account is correctly configured with the appropriate AWS IAM role.
21-40: LGTM: Environment variables are properly configured for production
The configuration properly restricts external API URLs and redirect URIs to production domains. The extensive list of identity server scopes is well-defined and appropriate for the service portal use case.
✅ Verification successful
Environment configuration is properly isolated across environments
The configuration correctly maintains environment isolation:
- Production uses
island.isdomains - Staging uses
staging01.devland.isdomains - Development uses
dev01.devland.isdomains
Each environment has properly configured and matching URLs for:
- External API access
- Redirect URIs
- Client base URL
- Identity server
- Logout redirects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that no development or staging URLs are accidentally included
rg -i "(localhost|staging|dev|test).*island\.is" charts/services/services-bff-portals-my-pages/
# Check for any hardcoded secrets
rg -i "(password|secret|key).*['\"]([a-zA-Z0-9+/]{32,}|[0-9a-f]{32,})['\"]" charts/services/services-bff-portals-my-pages/
Length of output: 4785
Script:
#!/bin/bash
# Let's check the URLs and environment variables in dev and staging environments
cat charts/services/services-bff-portals-my-pages/values.dev.yaml charts/services/services-bff-portals-my-pages/values.staging.yaml | grep -E "BFF_ALLOWED_EXTERNAL_API_URLS|BFF_ALLOWED_REDIRECT_URIS|BFF_CLIENT_BASE_URL|BFF_LOGOUT_REDIRECT_URI|IDENTITY_SERVER_ISSUER_URL"
# Also check if there are any other environment files that might contain URLs
fd -e yaml -e yml . charts/services/services-bff-portals-my-pages/ --exec grep -l "island.is"
Length of output: 1325
charts/islandis/values.prod.yaml (2)
1669-1669: LGTM: Namespace addition follows established pattern
The new namespace 'portals-my-pages' is correctly added to the namespaces list.
2331-2333: Dedicated IAM role needed for My Pages BFF
The service account is using a shared IAM role 'services-bff'. For better security isolation, create a dedicated IAM role for the My Pages BFF service.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
charts/services/services-bff-portals-my-pages/values.prod.yaml (3)
10-10: Consider enhancing audit log securityThe audit log group is configured, but consider adding:
- Log retention period
- Encryption settings
- Access controls
44-52: Adjust health check timeouts for productionThe 3-second timeout might be too aggressive for production, especially during high load:
- Consider increasing
timeoutSecondsto 5-10 seconds- Consider adding
periodSecondsandfailureThresholdconfigurations
98-105: Enhance container securityConsider adding these security enhancements:
- Set
readOnlyRootFilesystem: true- Add
runAsNonRoot: true- Specify
runAsUserandrunAsGroup
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
charts/islandis/values.prod.yaml(3 hunks)charts/services/services-bff-portals-admin/values.prod.yaml(1 hunks)charts/services/services-bff-portals-my-pages/values.prod.yaml(1 hunks)infra/src/dsl/bff.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/services/services-bff-portals-admin/values.prod.yaml
🧰 Additional context used
📓 Path-based instructions (1)
infra/src/dsl/bff.ts (1)
Pattern infra/src/dsl/**/*: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
🔇 Additional comments (10)
infra/src/dsl/bff.ts (4)
6-11: LGTM! Good use of constants for portal keys
The addition of named constants improves code maintainability and reduces the risk of typos when referencing portal keys.
20-33: Well-implemented scope management with proper deduplication
The implementation effectively uses Set for scope deduplication and properly handles different portal types with blocked switch cases.
105-110: LGTM! Well-structured callback path configuration
The callback paths are properly configured with sanitized global prefix across all environments, maintaining consistency and preventing path traversal issues.
89-96:
Security: Restrict redirect URIs in production environment
The production redirect URIs should be more restrictive to enhance security. Consider limiting them to specific paths instead of allowing the entire domain.
charts/services/services-bff-portals-my-pages/values.prod.yaml (3)
38-38:
Memory configuration needs adjustment
The memory limits might be insufficient:
- Node.js is configured with
--max-old-space-size=460but container memory limit is 512Mi - This leaves only ~50Mi for the rest of the process
- Memory requests (256Mi) might be too low for the configured heap size
Consider either:
- Increasing memory limits to at least 768Mi
- Or reducing the max old space size to ~350MB
Also applies to: 87-93
63-76:
SSL and domain configuration needs attention
Critical security configurations are missing:
- Add TLS configuration using cert-manager cluster issuer
- Configure redirect from www.island.is to island.is
- Add security headers
Example annotations to add:
nginx.ingress.kubernetes.io/ssl-redirect: "true"
cert-manager.io/cluster-issuer: "letsencrypt-prod"
nginx.ingress.kubernetes.io/configuration-snippet: |
return 301 https://island.is$request_uri;22-23: Verify external API and redirect URI restrictions
The allowed external APIs and redirect URIs are properly restricted to production domains, which is good. However:
- Consider adding pattern validation for subpaths
- Implement rate limiting for external API calls
charts/islandis/values.prod.yaml (3)
1669-1669: LGTM: Namespace addition is correct
The new namespace 'portals-my-pages' is properly added to the namespaces list, maintaining consistency with other portal namespaces.
2254-2254:
Fix callback path inconsistency
The BFF_CALLBACKS_BASE_PATH should include the '/minarsidur' prefix to maintain consistency with other BFF services and prevent potential callback handling issues.
Apply this change:
- BFF_CALLBACKS_BASE_PATH: 'https://island.is/bff/callbacks'
+ BFF_CALLBACKS_BASE_PATH: 'https://island.is/minarsidur/bff/callbacks'Likely invalid or redundant comment.
2331-2334:
Dedicated IAM role needed for My Pages BFF
The service account is using a shared IAM role 'services-bff'. For better security isolation, create a dedicated IAM role for the My Pages BFF service.
Apply this change:
serviceAccount:
annotations:
- eks.amazonaws.com/role-arn: 'arn:aws:iam::251502586493:role/services-bff'
+ eks.amazonaws.com/role-arn: 'arn:aws:iam::251502586493:role/services-bff-my-pages'Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/auth/react/src/lib/auth/AuthContext.tsx (2)
32-36: Consider enhancing the deprecation message with migration steps.While the current message clearly indicates the alternative, it could be more helpful to include a link to migration documentation or specific steps.
const warnDeprecated = (hookName: string, alternative: string) => { console.warn( - `[Deprecation Warning] "${hookName}" is being replaced by BFF auth pattern Please use "${alternative}" from "libs/react-spa/bff".`, + `[Deprecation Warning] "${hookName}" is being replaced by BFF auth pattern.\n` + + `Please use "${alternative}" from "libs/react-spa/bff".\n` + + `For migration steps, see: <link-to-migration-docs>`, ) }
60-61: Enhance error message with authentication guidance.The error message could be more helpful by suggesting how to ensure authentication.
if (!userInfo) { - throw new Error('User info is not available. Is the user authenticated?') + throw new Error('User info is not available. Please ensure the user is authenticated by calling signIn() or signInSilent() first.') }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/auth/react/src/lib/auth/AuthContext.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/auth/react/src/lib/auth/AuthContext.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
libs/auth/react/src/lib/auth/AuthContext.tsx (3)
41-42: Consider the naming feedback for the replacement hook.
There was previous feedback suggesting that useAuth is a better name than useBff. While this hook is being deprecated, consider renaming the replacement hook in libs/react-spa/bff to maintain semantic clarity.
Line range hint 6-13: LGTM: Compliant with coding guidelines.
The code follows the required guidelines for libs/**/* files:
- Uses TypeScript for interface definitions
- Implements reusable context and hooks
- Exports types properly
38-50: Verify the deprecation impact across the codebase.
Let's check the usage of useAuth to ensure all consumers are aware of the deprecation.
✅ Verification successful
Deprecation of useAuth appears safe to proceed
The deprecation of useAuth hook appears to have minimal impact on the codebase:
- The hook is only exported through the main index file of the auth/react package
- No direct usages of
useAuth()were found in the codebase - The deprecation notice properly guides developers to use
useBfffromlibs/react-spa/bffinstead - The implementation maintains backward compatibility by continuing to work while showing deprecation warnings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useAuth usage across the codebase
rg "useAuth\(" --type ts --type tsx -C 2
Length of output: 70
Script:
#!/bin/bash
# Search for useAuth usage across the codebase with ripgrep
rg "useAuth" --type typescript --type-add 'typescript:*.{ts,tsx}' -C 2
# Also search for any imports of useAuth
rg "import.*useAuth" --type typescript --type-add 'typescript:*.{ts,tsx}'
Length of output: 38835
Script:
#!/bin/bash
# Search for files importing from AuthContext.tsx
rg "from.*auth/AuthContext" --type typescript --type-add 'typescript:*.{ts,tsx}'
# Search for any direct usage of useAuth hook in the codebase
ast-grep --pattern 'useAuth()' --lang typescript
Length of output: 201
|
@coderabbitai ignore |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
libs/auth/react/src/lib/auth/AuthContext.tsx (1)
31-35: Consider enhancing the deprecation warning message.While the warning message is clear, it could be more helpful by including:
- Expected timeline for deprecation
- Link to migration guide or documentation
const warnDeprecated = (hookName: string, alternative: string) => { console.warn( - `[Deprecation Warning] "${hookName}" is being replaced by BFF auth pattern Please use "${alternative}" from "libs/react-spa/bff".`, + `[Deprecation Warning] "${hookName}" is being replaced by BFF auth pattern.\n` + + `Please migrate to "${alternative}" from "libs/react-spa/bff".\n` + + `This hook will be removed in the next major version.\n` + + `See migration guide: <link-to-docs>`, ) }apps/services/bff/src/app/modules/user/user.controller.spec.ts (1)
122-125: LGTM! Well-structured cache key format.The new cache key format using double colons as separators and including the config name provides better isolation between different configurations. This is consistently applied across all test cases.
Consider adding a comment explaining the cache key format convention for future maintainers:
// Cache key format: <type>::<config_name>::<sid> // where type is either 'attempt' or 'current'Also applies to: 155-158, 172-175, 197-200, 214-217
apps/services/bff/src/app/modules/proxy/proxy.service.ts (1)
231-250: Enhance error handling and loggingWhile the method effectively centralizes validation logic, the error message could be more descriptive and logging could include more context.
Apply this diff:
private prepareApiProxyRequest({ req, res, url, }: { req: Request res: Response url: string }): Promise<string> { if (!validateUri(url, this.config.allowedExternalApiUrls)) { - this.logger.error('Invalid external api url provided:', url) + this.logger.error({ + message: 'Invalid external API URL', + url, + allowedUrls: this.config.allowedExternalApiUrls, + }) - throw new BadRequestException('Proxing url failed!') + throw new BadRequestException( + `URL '${url}' is not in the allowed external API URLs list` + ) } return this.getAccessToken({ req, res }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(4 hunks)apps/services/bff/src/app/modules/auth/auth.service.ts(6 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts(1 hunks)apps/services/bff/src/app/modules/proxy/proxy.service.ts(3 hunks)apps/services/bff/src/app/modules/user/user.controller.spec.ts(6 hunks)apps/services/bff/src/app/utils/get-cookie-options.ts(1 hunks)libs/auth/react/src/lib/auth/AuthContext.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/services/bff/src/app/modules/auth/auth.controller.spec.ts
- apps/services/bff/src/app/modules/auth/auth.service.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/proxy/proxy.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/utils/get-cookie-options.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/auth/react/src/lib/auth/AuthContext.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (11)
apps/services/bff/src/app/utils/get-cookie-options.ts (2)
Line range hint 4-10: Strong security configuration for cookies
The cookie configuration implements robust security measures with httpOnly, secure, and sameSite: 'lax' settings, providing good protection against XSS and CSRF attacks while maintaining usability.
11-11: Verify the cookie path configuration across the codebase
The change from keyPath to globalPrefix needs verification to ensure consistent cookie path configuration across the BFF service.
✅ Verification successful
Let me check the cookie configuration usage in the identified files to ensure consistency.
Let me check the BffEnvironment interface definition to understand the globalPrefix configuration.
Let me check the environment schema and configuration to verify the globalPrefix setup.
Cookie path configuration is properly validated and consistent
The change from keyPath to globalPrefix is correctly implemented and validated:
- The
globalPrefixis properly defined in the environment schema with strict validation ensuring it ends with '/bff' - It's consistently used in cookie options and application bootstrap
- The cookie configuration is uniformly applied across all services using
getCookieOptions()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the cookie path configuration and environment variables
# Check for any remaining usage of keyPath in environment files
echo "Checking for keyPath usage in environment files..."
rg -l "keyPath" apps/services/bff/src/environment/
# Check the environment.ts file for globalPrefix configuration
echo "Checking globalPrefix configuration..."
rg "globalPrefix" apps/services/bff/src/environment/environment.ts -A 5
# Look for other cookie-related configurations that might need updating
echo "Checking for other cookie configurations..."
rg -l "getCookieOptions|CookieOptions" apps/services/bff/src/
Length of output: 784
Script:
#!/bin/bash
# Check the implementation and usage of cookie options in the identified files
echo "Checking cookie options implementation in get-cookie-options.ts..."
cat apps/services/bff/src/app/utils/get-cookie-options.ts
echo -e "\nChecking cookie usage in error.service.ts..."
rg "getCookieOptions|CookieOptions" apps/services/bff/src/app/services/error.service.ts -A 3
echo -e "\nChecking cookie usage in auth.service.ts..."
rg "getCookieOptions|CookieOptions" apps/services/bff/src/app/modules/auth/auth.service.ts -A 3
echo -e "\nChecking BffEnvironment interface..."
rg "interface BffEnvironment" -A 10 apps/services/bff/src/environment/
Length of output: 2079
Script:
#!/bin/bash
# Search for BffEnvironment interface definition
echo "Searching for BffEnvironment interface..."
fd -e ts -x grep -l "interface BffEnvironment" {} \;
echo -e "\nChecking the content of found files..."
fd -e ts -x sh -c 'if grep -l "interface BffEnvironment" {} > /dev/null; then echo "=== {} ==="; cat {}; fi' \;
echo -e "\nChecking for any environment variable usage in main.ts or similar bootstrap files..."
fd -e ts -x grep -l "BFF_GLOBAL_PREFIX" {} \;
Length of output: 1123
Script:
#!/bin/bash
echo "Checking environment schema..."
cat apps/services/bff/src/environment/environment.schema.ts
echo -e "\nChecking environment configuration..."
cat apps/services/bff/src/environment/environment.ts
echo -e "\nChecking main.ts for prefix usage..."
fd main.ts -t f -x cat {} \; apps/services/bff/src/
Length of output: 1872
libs/auth/react/src/lib/auth/AuthContext.tsx (2)
37-49: Consider the naming implications of the transition.
The deprecation message suggests using useBff, but this might not be the most semantically clear name as noted in a previous review. Consider if useAuth is actually a better name for the new hook in the BFF library.
Let's check the usage patterns of these hooks:
#!/bin/bash
# Description: Check usage patterns of auth hooks
# Look for all variations of the hook names to understand current usage
echo "Current useAuth usage:"
rg "useAuth" --type ts --type tsx
echo "\nCurrent useBff usage:"
rg "useBff" --type ts --type tsx
echo "\nCurrent useUserInfo usage:"
rg "useUserInfo" --type ts --type tsxLine range hint 1-63: Implementation follows library coding guidelines.
The code successfully meets the requirements for libs/**/*:
- ✅ Components and hooks are reusable across NextJS apps
- ✅ TypeScript is used effectively for type definitions
- ✅ Named exports enable proper tree-shaking
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
64-64: LGTM! Well-structured helper function.
The function follows TypeScript best practices and properly constructs the originUrl using configuration parameters.
Line range hint 77-234: Excellent test coverage and structure.
The test suite follows NestJS testing best practices with:
- Comprehensive coverage of success and error scenarios
- Clean test setup and teardown
- Good use of mocks and test data
- Clear test case organization
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (2)
37-37: LGTM: Identity server URL addition
The addition of the iss (issuer) property with the identity server URL is correct and follows OpenID Connect standards. This matches the development environment URL pattern.
Line range hint 1-241: Excellent test coverage and implementation
The test suite demonstrates comprehensive coverage of the TokenRefreshService:
- ✓ Follows NestJS testing best practices
- ✓ Covers all critical paths including edge cases
- ✓ Properly mocks dependencies
- ✓ Implements clear arrange/act/assert patterns
- ✓ Handles concurrent requests effectively
Let's verify the test coverage:
✅ Verification successful
Excellent test coverage confirmed with comprehensive implementation
The test suite in the spec file perfectly aligns with the service implementation:
- Tests cover the only public method
refreshToken - All critical paths are tested including:
- Successful token refresh
- Concurrent request handling
- Polling timeout scenarios
- Error handling
- Private helper methods are indirectly tested through the public interface
- No TODOs or missing test scenarios identified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any untested public methods in the TokenRefreshService
ast-grep --pattern 'export class TokenRefreshService {
$$$
public $method($_) {
$$$
}
$$$
}'
# Search for any TODO/FIXME comments in the test file
rg -i "todo|fixme" "apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts"
Length of output: 196
Script:
#!/bin/bash
# Let's first check the actual service implementation
cat apps/services/bff/src/app/modules/auth/token-refresh.service.ts
# Then check test descriptions to match against implementation
rg "describe\(.*TokenRefreshService" apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts -A 1
Length of output: 7001
apps/services/bff/src/app/modules/proxy/proxy.service.ts (3)
276-300: Consider consolidating GET and POST request handling
The forwardGetApiRequest and forwardPostApiRequest methods share significant code. Consider combining them into a single flexible method.
Here's a suggested implementation:
- public async forwardGetApiRequest({
- req,
- res,
- query,
- }: {
- req: Request
- res: Response
- query: ApiProxyDto
- }): Promise<void> {
- const { url } = query
- const accessToken = await this.prepareApiProxyRequest({ req, res, url })
-
- this.executeStreamRequest({
- accessToken,
- targetUrl: url,
- req,
- res,
- })
- }
-
- async forwardPostApiRequest({
- req,
- res,
- query,
- body,
- }: {
- req: Request
- res: Response
- query: ApiProxyDto
- body: Record<string, unknown>
- }) {
- const { url } = query
- const accessToken = await this.prepareApiProxyRequest({ req, res, url })
-
- this.executeStreamRequest({
- accessToken,
- targetUrl: url,
- req,
- res,
- body,
- })
- }
+ public async forwardApiRequest({
+ req,
+ res,
+ query,
+ body,
+ }: {
+ req: Request
+ res: Response
+ query: ApiProxyDto
+ body?: Record<string, unknown>
+ }): Promise<void> {
+ const { url } = query
+ const accessToken = await this.prepareApiProxyRequest({ req, res, url })
+
+ this.executeStreamRequest({
+ accessToken,
+ targetUrl: url,
+ req,
+ res,
+ body,
+ method: req.method,
+ })
+ }268-273:
Fix HTTP method handling in executeStreamRequest
The executeStreamRequest method always uses POST regardless of the original request method. This is incorrect for GET requests.
Add method parameter to executeStreamRequest and use it appropriately:
public async executeStreamRequest({
targetUrl,
accessToken,
req,
res,
body,
+ method = req.method,
}: {
targetUrl: string
accessToken: string
req: Request
res: Response
body?: Record<string, unknown>
+ method?: string
}) {
try {
const reqHeaderContentType = req.headers['content-type']
const finalBody = body ?? req.body
const response = await fetch(targetUrl, {
- method: 'POST',
+ method,
headers: {149-157:
Add request validation and security measures
The proxy implementation should include additional security measures to protect against potential attacks.
Consider implementing:
- Request body size limits
- Content-type validation
- Response size limits
- Explicit error handling for malformed bodies
Example implementation provided in the previous review still applies.
✅ Actions performedReviews paused. |
My pages bff setup
What
This PR creates new BFF server for our service-portal.
It updates all necessary files in the service portal to use the BFF service portal server.
Why
Because we are moving our SPA's to have their own dedicated BFF server
Checklist:
Summary by CodeRabbit
Release Notes
New Features
BffProviderfor improved user authentication management.services-bff-portals-my-pages.Improvements
useAuthwithuseUserInfo.CacheServiceto include BFF name.Bug Fixes
Documentation