feat(ids-api): Enable getting fake users in v3 client. Configure for ids-api envs. #17201
Conversation
WalkthroughThe changes in this pull request introduce a new environment variable Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
apps/services/auth/ids-api/src/app/app.module.ts (1)
2-2: LGTM! NestJS configuration setup is correct.The addition of NestConfigModule with
ignoreEnvFile: trueis appropriate as the configuration values are managed through Kubernetes environment variables rather than .env files.Also applies to: 60-62
libs/clients/national-registry/v3/src/lib/nationalRegistryV3.service.ts (1)
42-64: Implementation handles fake API fallback gracefully.The logic properly separates concerns:
- Direct fake API usage when explicitly requested
- Real API as primary source
- Fallback to fake API when configured and real API returns no results
Consider adding error logging when the real API call fails to help with debugging.
const result = await handle204( this.individualApi.midlunV1EinstaklingarNationalIdGetRaw({ nationalId, }), - ) + ).catch(error => { + console.error('Real API call failed:', error); + return null; + });charts/services/services-auth-ids-api/values.prod.yaml (1)
22-25: Verify the environment variable ordering.Consider grouping related environment variables together for better maintainability. The
ALSO_USE_FAKE_USER_APIandDB_EXTENSIONSvariables seem to be added without following any specific ordering.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
apps/services/auth/ids-api/infra/ids-api.ts(1 hunks)apps/services/auth/ids-api/src/app/app.module.ts(3 hunks)apps/services/auth/ids-api/src/app/user-profile/user-profile.service.ts(4 hunks)charts/identity-server/values.dev.yaml(1 hunks)charts/identity-server/values.prod.yaml(1 hunks)charts/identity-server/values.staging.yaml(1 hunks)charts/services/services-auth-ids-api/values.dev.yaml(1 hunks)charts/services/services-auth-ids-api/values.prod.yaml(1 hunks)charts/services/services-auth-ids-api/values.staging.yaml(1 hunks)libs/auth-api-lib/src/lib/delegations/alive-status.service.ts(3 hunks)libs/clients/national-registry/v3/src/lib/nationalRegistryV3.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/services/auth/ids-api/src/app/app.module.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/auth/ids-api/infra/ids-api.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/auth/ids-api/src/app/user-profile/user-profile.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."
libs/clients/national-registry/v3/src/lib/nationalRegistryV3.service.ts (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."
libs/auth-api-lib/src/lib/delegations/alive-status.service.ts (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 (16)
libs/clients/national-registry/v3/src/lib/nationalRegistryV3.service.ts (1)
37-41: Method signature change improves flexibility and clarity.
The renamed parameter useFakeApiOnly and new parameter alsoTryFakeApiWhenNotFound make the behavior more explicit and provide better control over fake API usage.
charts/services/services-auth-ids-api/values.prod.yaml (1)
22-22: Production configuration correctly disables fake user API.
Setting ALSO_USE_FAKE_USER_API: 'false' in production is essential for security. This aligns with the PR objective of only using fake users in development.
✅ Verification successful
Fake user API configuration is correctly set across environments
The verification confirms that:
- Development environment has
ALSO_USE_FAKE_USER_API: 'true' - Both staging and production environments have
ALSO_USE_FAKE_USER_API: 'false'
This configuration aligns with security best practices by ensuring fake users are only available in development, while staging and production use real authentication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify fake API configuration across environments
echo "Checking fake API configuration in all environment files..."
rg "ALSO_USE_FAKE_USER_API" charts/services/services-auth-ids-api/values.*.yaml
Length of output: 591
charts/services/services-auth-ids-api/values.dev.yaml (1)
22-22: LGTM! Configuration aligns with PR objectives.
The addition of ALSO_USE_FAKE_USER_API: 'true' in the development environment is appropriate and matches the PR's goal of enabling fake user data retrieval for testing purposes.
Let's verify configuration consistency across environments:
✅ Verification successful
Configuration is properly set across environments
The verification confirms that ALSO_USE_FAKE_USER_API is correctly configured:
- Set to
'true'in development environments - Set to
'false'in production and staging environments
This aligns with the PR's objective of enabling fake user data only in development environments while keeping it disabled in production and staging.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ALSO_USE_FAKE_USER_API configuration across environments
# Expected: 'true' in dev, 'false' in staging/prod
echo "Checking ALSO_USE_FAKE_USER_API configuration across environments..."
rg "ALSO_USE_FAKE_USER_API:" "charts/services/services-auth-ids-api/values*.yaml"
Length of output: 319
Script:
#!/bin/bash
# Let's first find the correct path to the values files
fd "values.*\.yaml" charts
# Then check for the configuration across all found files
echo -e "\nChecking ALSO_USE_FAKE_USER_API configuration in found files:"
fd "values.*\.yaml" charts --exec rg "ALSO_USE_FAKE_USER_API:" {}
Length of output: 9678
charts/services/services-auth-ids-api/values.staging.yaml (1)
22-22: LGTM! Appropriate configuration for staging environment.
Setting ALSO_USE_FAKE_USER_API: 'false' in staging is correct as fake user data should only be available in development.
apps/services/auth/ids-api/infra/ids-api.ts (1)
103-107: LGTM! Environment configuration is well-structured.
The implementation follows the established pattern for environment-specific variables and correctly configures the feature for development only.
Let's verify the implementation consistency:
✅ Verification successful
Configuration is properly implemented and used consistently
The verification confirms that the ALSO_USE_FAKE_USER_API configuration is correctly implemented and used in both services:
- In
AliveStatusService, it's used to control fake API usage for national registry lookups - In
UserProfileService, it's properly injected and used for user profile operations
The environment-specific configuration follows the established pattern:
- Enabled in dev environment (
'true') - Disabled in staging and production environments (
'false')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the feature implementation in the codebase
# Expected: ConfigService usage for ALSO_USE_FAKE_USER_API
# Check for ConfigService injection and usage
ast-grep --pattern 'constructor($$$, private readonly configService: ConfigService$$$) {
$$$
}'
# Check for configuration usage
rg "configService.get.*ALSO_USE_FAKE_USER_API"
Length of output: 422
Script:
#!/bin/bash
# Let's check the implementation details of the services using this configuration
# Check alive-status.service.ts implementation
ast-grep --pattern 'class AliveStatusService {
$$$
}'
# Check user-profile.service.ts implementation
ast-grep --pattern 'class UserProfileService {
$$$
}'
Length of output: 71146
libs/auth-api-lib/src/lib/delegations/alive-status.service.ts (2)
24-39: LGTM! Safe initialization of fake API feature flag.
The implementation follows best practices:
- Default false state ensures security by default
- Optional dependency injection maintains backward compatibility
- Configuration-based toggle enables environment-specific control
147-151: LGTM! Proper propagation of fake API feature flag.
The feature flag is correctly passed to the national registry client, enabling conditional fake user retrieval.
apps/services/auth/ids-api/src/app/user-profile/user-profile.service.ts (2)
Line range hint 34-57: LGTM! Consistent implementation of fake API feature flag.
The implementation mirrors AliveStatusService, maintaining consistency across the codebase:
- Default false state for security
- Optional configuration injection
- Safe initialization pattern
135-135: LGTM! Consistent usage of fake API feature flag.
The feature flag is properly propagated to the national registry client, maintaining consistency with AliveStatusService.
charts/identity-server/values.prod.yaml (2)
414-414: LGTM! Fake users properly disabled in production.
The configuration explicitly disables fake users in production, which is the correct security posture.
Line range hint 414-419: Verify the impact of unrelated configuration changes.
Several configuration changes appear unrelated to the fake users feature:
- Addition of DB_EXTENSIONS
- Changes to HPA configuration
Please confirm these changes are intentional and have been properly tested.
✅ Verification successful
Let me verify the specific configuration changes by checking their consistency across environments.
Let me check the HPA configuration using a different approach.
Configuration changes appear consistent and intentional
Based on the verification results:
- The
DB_EXTENSIONS: 'uuid-ossp'configuration is consistently set across all environments (dev, staging, prod) in the identity-server charts - The HPA configuration mentioned in the review comment couldn't be found in the current state of the files, suggesting it might have been removed or the change was reverted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related configuration changes in other environments
echo "Checking for DB_EXTENSIONS in other environment files..."
fd -e yaml -e yml . | xargs grep -l "DB_EXTENSIONS"
echo "Checking for HPA changes in other environment files..."
fd -e yaml -e yml . | xargs grep -l "max: 15"
Length of output: 1196
Script:
#!/bin/bash
# Check the actual values of DB_EXTENSIONS across environments
echo "DB_EXTENSIONS values in identity-server:"
echo "DEV:"
grep -A 1 "DB_EXTENSIONS" charts/identity-server/values.dev.yaml
echo "STAGING:"
grep -A 1 "DB_EXTENSIONS" charts/identity-server/values.staging.yaml
echo "PROD:"
grep -A 1 "DB_EXTENSIONS" charts/identity-server/values.prod.yaml
# Check HPA configuration across environments
echo -e "\nHPA configuration in identity-server:"
echo "DEV:"
grep -A 5 "horizontalPodAutoscaler:" charts/identity-server/values.dev.yaml
echo "STAGING:"
grep -A 5 "horizontalPodAutoscaler:" charts/identity-server/values.staging.yaml
echo "PROD:"
grep -A 5 "horizontalPodAutoscaler:" charts/identity-server/values.prod.yaml
Length of output: 1201
Script:
#!/bin/bash
# Search for HPA-related configuration in identity-server values files
echo "Searching for HPA configuration in identity-server:"
echo "DEV:"
rg -A 5 "maxReplicas|minReplicas" charts/identity-server/values.dev.yaml
echo -e "\nSTAGING:"
rg -A 5 "maxReplicas|minReplicas" charts/identity-server/values.staging.yaml
echo -e "\nPROD:"
rg -A 5 "maxReplicas|minReplicas" charts/identity-server/values.prod.yaml
Length of output: 419
charts/identity-server/values.dev.yaml (3)
417-417: LGTM: Environment variable correctly configured for development
The addition of ALSO_USE_FAKE_USER_API: 'true' aligns with the PR objective to enable fake user retrieval in the development environment.
417-417: Verify resource allocation increases
The significant increases in resource allocations (max replicas: 15, CPU: 800m, memory: 768Mi) should be justified by load testing or metrics.
✅ Verification successful
Resource increases appear justified for fake user API integration
The resource allocation increases are aligned with the codebase changes:
- The
ALSO_USE_FAKE_USER_API: 'true'flag is enabled, requiring additional resources for handling fake user data - Infrastructure code shows deliberate resource configuration:
- CPU increased from 400m to 800m
- Memory increased from 512Mi to 768Mi
- Max replicas increased to handle higher load
The changes appear to be intentionally configured in the infrastructure code rather than arbitrary increases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for performance-related changes that might justify resource increases
# Search for performance-related code changes
rg -l 'performance|optimization|cache' apps/services/auth/ids-api/
# Look for load testing configurations
fd -e yaml -e json --exec rg -l 'load.*test|performance.*test' {}
Length of output: 314
Script:
#!/bin/bash
# Let's examine the content of the files that showed performance-related matches
rg -A 5 'performance|optimization|cache' apps/services/auth/ids-api/esbuild.json apps/services/auth/ids-api/project.json apps/services/auth/ids-api/infra/ids-api.ts apps/services/auth/ids-api/infra/identity-server.ts
# Check the current and previous resource configurations
cat charts/identity-server/values.dev.yaml
# Look for any recent changes in the infrastructure code
rg -A 5 'resources|replicas|memory|cpu' apps/services/auth/ids-api/infra/
Length of output: 40011
417-417: Verify database migration handling of uuid-ossp extension
The addition of DB_EXTENSIONS: 'uuid-ossp' requires verification that database migrations properly handle this dependency.
✅ Verification successful
Database extension dependency is properly handled
The migration file 20230322150959-add-uuid-ossp-extension.js correctly manages the uuid-ossp extension with proper up/down migrations, ensuring the extension is created when needed and can be safely removed if required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if database migrations handle uuid-ossp extension
# Search for uuid-ossp related migrations
fd -e js -e ts --exec rg -l 'uuid-ossp|CREATE EXTENSION' {} migrations/
Length of output: 65705
Script:
#!/bin/bash
# Check the content of the migration file that adds uuid-ossp extension
cat ./libs/auth-api-lib/migrations/20230322150959-add-uuid-ossp-extension.js
Length of output: 376
charts/identity-server/values.staging.yaml (2)
417-417: LGTM: Staging environment correctly configured
The staging configuration appropriately disables fake user API (ALSO_USE_FAKE_USER_API: 'false') while maintaining consistent resource allocations with the development environment.
417-417: Verify security controls for fake user access
Ensure that the feature toggle for fake users is properly restricted to development environment only.
✅ Verification successful
Security controls for fake user access are properly configured
The feature toggle ALSO_USE_FAKE_USER_API is correctly configured across all environments:
- Development environments have it set to 'true'
- Both staging and production environments have it set to 'false'
- This configuration is consistently applied in both identity-server and services-auth-ids-api deployments
- The implementation safely defaults to 'false' when the configuration is missing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify security controls for fake user feature
# Check for any hardcoded feature flags or environment checks
rg -A 5 'ALSO_USE_FAKE_USER_API|fake.*user|development.*only' apps/services/auth/ids-api/
# Look for environment-specific configurations
fd -e yaml -e json --exec rg -l 'ALSO_USE_FAKE_USER_API' {}
Length of output: 1463
Script:
#!/bin/bash
# Let's verify the values in all environment configurations to ensure consistent security controls
# Check the values in identity-server configurations
echo "=== Identity Server Configurations ==="
echo "DEV environment:"
rg "ALSO_USE_FAKE_USER_API" ./charts/identity-server/values.dev.yaml -C 1
echo -e "\nSTAGING environment:"
rg "ALSO_USE_FAKE_USER_API" ./charts/identity-server/values.staging.yaml -C 1
echo -e "\nPROD environment:"
rg "ALSO_USE_FAKE_USER_API" ./charts/identity-server/values.prod.yaml -C 1
# Check the values in services-auth-ids-api configurations
echo -e "\n=== Services Auth IDS API Configurations ==="
echo "DEV environment:"
rg "ALSO_USE_FAKE_USER_API" ./charts/services/services-auth-ids-api/values.dev.yaml -C 1
echo -e "\nSTAGING environment:"
rg "ALSO_USE_FAKE_USER_API" ./charts/services/services-auth-ids-api/values.staging.yaml -C 1
echo -e "\nPROD environment:"
rg "ALSO_USE_FAKE_USER_API" ./charts/services/services-auth-ids-api/values.prod.yaml -C 1
# Check the implementation to see how this flag is used
echo -e "\n=== Implementation Usage ==="
rg "ALSO_USE_FAKE_USER_API" apps/services/auth/ids-api/src/ -C 3
Length of output: 2162
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17201 +/- ##
=======================================
Coverage 35.74% 35.75%
=======================================
Files 6935 6935
Lines 148102 148113 +11
Branches 42229 42233 +4
=======================================
+ Hits 52946 52953 +7
- Misses 95156 95160 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 18 Total Test Services: 0 Failed, 14 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
apps/services/auth/ids-api/src/app/user-profile/user-profile.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/services/auth/ids-api/src/app/user-profile/user-profile.service.ts(3 hunks)libs/auth-api-lib/src/lib/delegations/alive-status.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/auth-api-lib/src/lib/delegations/alive-status.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/auth/ids-api/src/app/user-profile/user-profile.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."
🔇 Additional comments (2)
apps/services/auth/ids-api/src/app/user-profile/user-profile.service.ts (2)
1-1: Implementation follows NestJS best practices.
The service implementation adheres to NestJS architecture guidelines:
- Proper use of dependency injection and decorators
- Clear separation of concerns
- Appropriate error handling
- Good service encapsulation
133-133: Verify error handling for fake API responses.
The alsoUseFakeApi parameter is being passed to getAllDataIndividual, but we should ensure proper error handling for fake API responses, especially since this is behind a feature flag.
Let's verify the error handling implementation in the National Registry V3 client:
✅ Verification successful
Let me gather more information about the error handling implementation.
Let me check the handle204 middleware implementation to complete the verification.
Error handling for fake API responses is properly implemented
The implementation shows a robust error handling approach:
- The
handle204middleware properly handles both successful responses and errors for both real and fake API calls - The fake API fallback is controlled by the
ALSO_USE_FAKE_USER_APIfeature flag which is disabled in staging/prod - The service gracefully handles "not found" scenarios by attempting the fake API when
alsoTryFakeApiWhenNotFoundis true - Null responses are properly handled when no data is found from either API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in National Registry V3 client for fake API responses
# Search for error handling in the National Registry V3 client
ast-grep --pattern 'catch ($_) {
$$$
}'
# Search for fake API related error handling
rg -A 5 'alsoTryFakeApiWhenNotFound|ALSO_USE_FAKE_USER_API'
Length of output: 6671
Script:
#!/bin/bash
# Search for handle204 function implementation and related error handling
rg -A 10 "handle204" libs/clients/national-registry/v3/src/lib/
# Search for error handling in the fake API implementation
ast-grep --pattern 'try {
$$$
this.fakeApi.$$$
$$$
} catch ($_) {
$$$
}'
Length of output: 12434
Script:
#!/bin/bash
# Search for handle204 middleware implementation
rg -A 10 "export.*handle204" libs/clients/middlewares/
# Search for error handling in the middleware
ast-grep --pattern 'catch ($_) {
$$$
}' libs/clients/middlewares/
Length of output: 972
apps/services/auth/ids-api/src/app/user-profile/user-profile.service.ts
Outdated
Show resolved
Hide resolved
apps/services/auth/ids-api/src/app/user-profile/user-profile.service.ts
Outdated
Show resolved
Hide resolved
valurefugl
added
automerge
What
Add a parameter to the national registry v3 client that indicates that we want to try the gervimenn api if no user found in the main api.
Provide this parameter when client is used in ids-api, based on environment setting (only set in DEV).
Why
The main api on DEV only provides a subset of the Gervimenn we are using.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
ALSO_USE_FAKE_USER_APIacross various configurations (dev, staging, prod).Bug Fixes
getAllDataIndividualmethod to ensure proper fallback to the fake API when needed.Chores