-
Notifications
You must be signed in to change notification settings - Fork 61
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
chore(regulations-admin): Update national registry for regulations #15995
Conversation
WalkthroughThe changes involve updates to the environment variable configurations for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RegulationsAdminBackend
participant NationalRegistryB2C
Client->>RegulationsAdminBackend: Request data
RegulationsAdminBackend->>NationalRegistryB2C: Authenticate and retrieve data
NationalRegistryB2C-->>RegulationsAdminBackend: Return data
RegulationsAdminBackend-->>Client: Provide data
Possibly related PRs
Tip OpenAI O1 model for chat
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15995 +/- ##
=======================================
Coverage 36.84% 36.84%
=======================================
Files 6718 6718
Lines 137732 137732
Branches 39159 39159
=======================================
Hits 50753 50753
Misses 86979 86979 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 ✅ 23 Total Test Services: 0 Failed, 23 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕺🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
apps/services/regulations-admin-backend/infra/regulations-admin-backend.ts (1)
32-33
: Ensure secure storage and access of the client secret.The addition of
NATIONAL_REGISTRY_B2C_CLIENT_SECRET
to the API endpoints looks good. It indicates that the service will be authenticating with the national registry using a client secret.Please ensure that the client secret is securely stored and accessed. Consider the following best practices:
- Store the client secret in a secure vault or secrets manager.
- Rotate the client secret regularly.
- Limit access to the client secret to only the necessary services and personnel.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- apps/services/regulations-admin-backend/infra/regulations-admin-backend.ts (2 hunks)
- apps/services/regulations-admin-backend/src/app/modules/draft_regulation/draft_regulation.service.ts (3 hunks)
- charts/islandis/values.dev.yaml (2 hunks)
- charts/islandis/values.prod.yaml (2 hunks)
- charts/islandis/values.staging.yaml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/services/regulations-admin-backend/src/app/modules/draft_regulation/draft_regulation.service.ts
Additional context used
Path-based instructions (1)
apps/services/regulations-admin-backend/infra/regulations-admin-backend.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."
Gitleaks
charts/islandis/values.staging.yaml
1609-1609: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
charts/islandis/values.prod.yaml
1735-1735: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
charts/islandis/values.dev.yaml
1867-1867: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (6)
charts/islandis/values.staging.yaml (2)
1609-1612
: LGTM!The additions for the National Registry B2C integration look good. Note that the static analysis hint about
NATIONAL_REGISTRY_B2C_CLIENT_ID
being a generic API key is likely a false positive, as client IDs are typically not considered sensitive.Tools
Gitleaks
1609-1609: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1685-1685
: LGTM!Storing the
NATIONAL_REGISTRY_B2C_CLIENT_SECRET
as a Kubernetes secret is the correct way to handle this sensitive value.charts/islandis/values.prod.yaml (4)
1735-1735
: The static analysis hint for a generic API key is a false positive. This is a client ID for a specific service which is fine to be present in the prod config file.Tools
Gitleaks
1735-1735: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1736-1738
: The new National Registry B2C config values look good!The added environment variables specify the correct public endpoints, path and scope needed for the app to integrate with the National Registry B2C API in production. No secrets are exposed.
1811-1811
: Good practice of providing the client secret via a Kubernetes secret!Instead of putting the sensitive client secret value directly in the config file, it is correctly referenced from a pre-configured Kubernetes secret at path
/k8s/api/NATIONAL_REGISTRY_B2C_CLIENT_SECRET
. This follows the best practice for keeping secrets out of source control.
1735-1738
: Code in lines 1736-1738 was already reviewed and approved in a previous comment.Tools
Gitleaks
1735-1735: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
apps/services/regulations-admin-backend/infra/regulations-admin-backend.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- charts/islandis/values.dev.yaml (2 hunks)
- charts/islandis/values.prod.yaml (2 hunks)
- charts/islandis/values.staging.yaml (2 hunks)
Additional context used
Gitleaks
charts/islandis/values.staging.yaml
1610-1610: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
charts/islandis/values.prod.yaml
1736-1736: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
charts/islandis/values.dev.yaml
1868-1868: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (5)
charts/islandis/values.staging.yaml (1)
1686-1686
: LGTM! Using a Kubernetes secret for the sensitive client secret is the way to go.Referencing the secret via the Kubernetes secret path ensures the sensitive value is not exposed in the config file. This is a secure approach. Good job!
charts/islandis/values.prod.yaml (2)
1736-1739
: LGTM!The added environment variables for National Registry B2C integration look good. The
NATIONAL_REGISTRY_B2C_CLIENT_ID
has the correct UUID format for a client ID.Tools
Gitleaks
1736-1736: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1812-1812
: Good security practice!Securely storing the
NATIONAL_REGISTRY_B2C_CLIENT_SECRET
in Kubernetes secrets is the right approach.charts/islandis/values.dev.yaml (2)
1869-1871
: LGTM!The addition of the
NATIONAL_REGISTRY_B2C_ENDPOINT
,NATIONAL_REGISTRY_B2C_PATH
andNATIONAL_REGISTRY_B2C_SCOPE
environment variables for integrating with the National Registry B2C services looks good.
1944-1944
: Excellent!Storing the
NATIONAL_REGISTRY_B2C_CLIENT_SECRET
in Kubernetes secrets and referencing it via an environment variable is the secure way to handle secrets.Please also make sure to update the
NATIONAL_REGISTRY_B2C_CLIENT_ID
to follow this same pattern as suggested in the previous comment.
What
For regulations:
Remove national registry v2.
Add national registry v3.
Why
Updates.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation