feat: add dex auth #2793
feat: add dex auth #2793
Conversation
|
Warning Rate limit exceeded@nmrgt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis update modifies continuous integration and end-to-end testing configurations. In the GitHub Actions workflow, additional environment variables are introduced for the e2e integration tests. The docker-compose file now removes the Keycloak service and adds a new Dex service with updated environment variables, port mapping, commands, volume mappings, and Traefik labels. A new Dex configuration file is added to define OIDC settings, static clients, PostgreSQL storage details, and OAuth2 parameters. Additionally, new classes and methods for handling authentication and user data management are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Test Runner
participant TestSystem as Integration Test System
GH->>Runner: Trigger e2e Job
Runner->>Runner: Set env vars (PRO_CONNECT_CLIENT_ID, etc.)
Runner->>TestSystem: Execute integration tests
TestSystem-->>Runner: Return test results
Runner-->>GH: Report outcomes
sequenceDiagram
participant Client
participant Traefik
participant Dex
Client->>Traefik: Send authentication request
Traefik->>Dex: Forward request with proper labels
Dex->>Dex: Load configuration from /etc/dex/config.yaml
Dex-->>Traefik: Processed authentication response
Traefik-->>Client: Return auth details/redirect
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docker/dex/config.yaml (1)
51-51: YAML Formatting: Remove Trailing Spaces and Correct Indentation.
Static analysis has flagged trailing spaces on several lines (specifically on lines 51, 66, 93, 97, and 133). Please remove these trailing spaces and check that all indentation follows the YAML standards to avoid any potential parsing issues.Also applies to: 66-66, 93-93, 97-97, 133-133
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 51-51: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml (2)
66-67: YAML Indentation: Adjust "depends_on" Section.
Thedepends_onblock for the Dex service appears to have an indentation issue (static analysis expects 6 spaces, but 8 are found). Please adjust the indentation to comply with YAML standards and avoid potential parsing errors.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 67-67: wrong indentation: expected 6 but found 8
(indentation)
52-52: YAML Formatting: Remove Trailing Spaces.
Line 52 has been flagged for trailing spaces. Please remove these spaces so that the file conforms to YAML best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 52-52: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/analysis.yml(1 hunks)docker-compose.e2e.yml(1 hunks)docker/dex/config.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
docker/dex/config.yaml
[error] 51-51: trailing spaces
(trailing-spaces)
[warning] 56-56: wrong indentation: expected 4 but found 5
(indentation)
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 93-93: trailing spaces
(trailing-spaces)
[error] 97-97: trailing spaces
(trailing-spaces)
[error] 133-133: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml
[error] 52-52: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 6 but found 8
(indentation)
🔇 Additional comments (12)
.github/workflows/analysis.yml (1)
170-173: Environment Variables: Validate Secrets vs. Hardcoded Values.
The new environment variables for integration tests are added here. Note that whilePRO_CONNECT_CLIENT_IDandPRO_CONNECT_CLIENT_SECRETcorrectly pull from GitHub secrets, the values forAPP_AUTH_CLIENT_IDandAPP_AUTH_CLIENT_SECRETare hardcoded as"secret". Please verify if these hardcoded values are intentional (e.g. for test purposes) or if they should also be secured via secrets.docker/dex/config.yaml (6)
4-9: Static Clients Configuration: Verify Environment Variables and Sensitive Data.
The first static client uses environment variables ($PDC_CLIENT_ID,$PDC_CLIENT_SECRET, and$PDC_URL). Ensure that these are correctly set in your environment and that no sensitive information is exposed inadvertently.
10-14: Test Client Configuration: Validate Hardcoded Credentials.
The second static client (withid: test) uses a fixed redirect URI and the secret is set to"secret". Confirm that this client is intended only for testing. If it might run in production, consider securing these credentials or documenting clearly that it is a dummy/test client.
15-25: Storage Configuration: Ensure Security Considerations.
The PostgreSQL storage settings disable SSL (mode: disable). If this configuration is for a non‑production or test environment, it may be acceptable; however, for production it is recommended to enable SSL (and configure a CA file if needed) in order to secure data transfers.
27-40: OIDC Connector Configuration: Validate Environment-Driven Settings.
The connector configuration relies on several environment variables ($OIDC_ISSUER_URL,$OIDC_CLIENT_ID,$OIDC_CLIENT_SECRET, and$OIDC_REDIRECT_URL). Please ensure that these are correctly set and that the settings (including the issuer and redirect URI) match your OIDC provider’s expected values.
62-66: Email Verification Setting: Assess Security Implications.
The configuration setsinsecureSkipEmailVerified: true, which bypasses email verification. Ensure that this is acceptable in the current environment—if used in production, this setting could reduce security.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 66-66: trailing spaces
(trailing-spaces)
135-138: OAuth2 Settings: Confirm Behavior Alignment.
The OAuth2 block specifies a response type of"code"and minimal approval settings. Verify that these settings align with your authentication flow and that any needed user consent or additional response types have been considered based on your requirements.docker-compose.e2e.yml (5)
36-38: Dex Service Image: Verify Version and Compatibility.
The Dex service uses the imagebitnami/dex:2. Please confirm that this version offers the features and security fixes you require.
39-50: Dex Service Environment Variables: Validate Configuration.
The Dex service is configured with several environment variables (e.g.,PG_DB,PG_USERNAME,PG_PASSWORD,PG_HOST,PG_PORT,PDC_URL, and OIDC-related variables). Note thatPDC_CLIENT_IDandPDC_CLIENT_SECRETare hardcoded as"secret"while the OIDC client variables pull from the environment. Verify that these values are appropriate for your integration environment and consider using secrets where needed for security.
51-53: Port Mapping and Network Configuration: Confirm Exposure Settings.
The Dex service maps container port 8080 to host port 8090 and connects to thebacknetwork. Please ensure that this port mapping aligns with your intended access patterns and that there are no conflicts with other services.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 52-52: trailing spaces
(trailing-spaces)
56-62: Traefik Labels: Confirm Routing and TLS Settings.
The Traefik labels define both HTTP and HTTPS routing rules for the Dex service using the hostauth.covoiturage.test. Verify that these labels and the associated entrypoints (webandwebsecure) reflect your intended edge routing configuration and that TLS is correctly enforced on the secure route.
63-65: Dex Service Launch Configuration: Verify Command and Volume Mapping.
Thecommandparameter and volume mapping correctly point to the Dex configuration file (./docker/dex/config.yaml) with a read-only flag. Ensure that the configuration file path is accurate and that the command parameters meet your deployment requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docker/dex/config.yaml (2)
4-10: Static Clients Configuration – Validate Client Identifiers and URIs
The static clients are defined with environment variables for the first client ($PDC_CLIENT_ID,$PDC_CLIENT_SECRET) and literal values for the second client. Confirm that using environment variables for the production client and static credentials for testing aligns with your security and testing policies.
51-51: YAML Formatting Issues – Trailing Spaces and Indentation Fixes
Static analysis flagged trailing spaces on lines 51, 66, 93, 97, and 133, and an indentation issue on line 56. Please remove extra spaces and adjust the indentation as demonstrated below to comply with YAML linting rules.- insecureSkipEmailVerified: true + insecureSkipEmailVerified: trueEnsure similar trimming of trailing spaces on lines 51, 93, 97, and 133.
Also applies to: 66-66, 93-93, 97-97, 133-133
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 51-51: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml (1)
52-52: YAML Formatting: Trailing Spaces and Indentation Correction
Static analysis indicates that line 52 has trailing spaces and line 67 has an indentation error (expected 6 spaces but found 8). Please correct these as follows:
- Line 52 Diff Suggestion:
- - 8090:8080 + - 8090:8080
- Line 67 Diff Suggestion:
- depends_on: - - postgres + depends_on: + - postgresThis will improve the YAML quality and avoid potential parsing issues.
Also applies to: 67-67
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 52-52: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/analysis.yml(1 hunks)docker-compose.e2e.yml(1 hunks)docker/dex/config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/analysis.yml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
docker/dex/config.yaml
[error] 51-51: trailing spaces
(trailing-spaces)
[warning] 56-56: wrong indentation: expected 4 but found 5
(indentation)
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 93-93: trailing spaces
(trailing-spaces)
[error] 97-97: trailing spaces
(trailing-spaces)
[error] 133-133: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml
[error] 52-52: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 6 but found 8
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (8)
docker/dex/config.yaml (7)
1-3: Issuer and Web Server Configuration Setup
The configuration correctly sets the issuer URL and binds the web server to listen on all interfaces at port 8080. Verify that the issuer URL (http://auth.covoiturage.test) aligns with your test environment.
15-25: PostgreSQL Storage Settings
The storage section is well defined with typepostgresand configuration parameters sourced from environment variables. Double-check that the environment variables (e.g.,$PG_HOST,$PG_DB) are correctly set in your deployment environment.
26-51: OIDC Connector Configuration (ProConnect)
The OIDC connector block for “ProConnect” is comprehensive with appropriate comments and settings. It reads most secrets from environment variables (e.g.,$OIDC_ISSUER_URL,$OIDC_CLIENT_ID,$OIDC_CLIENT_SECRET, and$OIDC_REDIRECT_URL). Ensure these environment variables are correctly provided and that the scopes and claim mappings meet your integration needs.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 51-51: trailing spaces
(trailing-spaces)
62-74: Connector Advanced Settings and Claim Mapping
The comments and options for handling additional settings (likeinsecureSkipEmailVerifiedand claim mapping) are useful. Ensure the settinginsecureSkipEmailVerified: true(line 66) is acceptable for your security posture.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 66-66: trailing spaces
(trailing-spaces)
80-100: OIDC Connector: UserInfo and Prompt Configuration
The configuration enablinggetUserInfo, setting theuserNameKey, and adjusting the prompt type is clearly structured. Confirm thatpromptType: nonemeets the requirements of your identity provider.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 93-93: trailing spaces
(trailing-spaces)
[error] 97-97: trailing spaces
(trailing-spaces)
106-124: Claim Mapping and Provider Discovery Overrides
The claimMapping section and the override flag are clearly documented. Verify that mapping thepreferred_usernametosiretandfamily_nametousual_nameis intentional.
125-139: OAuth2 Settings Configuration
The OAuth2 block is minimal and correct, specifying response types and screen approval settings. Ensure that these settings are compatible with your OAuth2 clients.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 133-133: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml (1)
36-67: Dex Service Integration in Docker Compose
The newdexservice configuration is well integrated into the compose file. Key points include:
- Image & Command: The service uses
bitnami/dex:2with the command to serve Dex using the provided configuration file.- Environment Variables: Variables for PostgreSQL, PDC URL, and OIDC settings are appropriately set. Ensure that sensitive credentials (e.g.,
OIDC_CLIENT_IDandOIDC_CLIENT_SECRET) are managed securely.- Port Mapping: The mapping
8090:8080ensures Dex is externally available on port 8090.- Traefik Labels: The router and service labels are correctly configured for routing via Traefik.
Overall, the changes align with your Dex authentication requirements.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 52-52: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 6 but found 8
(indentation)
| scopes: | ||
| - openid | ||
| - siret | ||
| - given_name | ||
| - usual_name | ||
| # - groups |
There was a problem hiding this comment.
🛠️ Refactor suggestion
OIDC Scopes Definition
The scopes are defined as a list; however, YAMLlint reports an indentation issue on line 56. The scopes should be indented by 4 spaces relative to the scopes: key.
Apply the following diff to fix the indentation:
- scopes:
- - openid
- - email
- - siret
- - given_name
- - usual_name
+ scopes:
+ - openid
+ - email
+ - siret
+ - given_name
+ - usual_name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scopes: | |
| - openid | |
| - siret | |
| - given_name | |
| - usual_name | |
| # - groups | |
| scopes: | |
| - openid | |
| - siret | |
| - given_name | |
| - usual_name | |
| # - groups |
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 56-56: wrong indentation: expected 4 but found 5
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
docker/dex/config.yaml (1)
50-57: YAML Indentation Issue in Scopes List
The scopes list under the OIDC connector is currently indented with 5 spaces instead of the expected 4. YAMLlint has flagged this indentation error. Please adjust the indentation to ensure proper YAML parsing. For example, you can apply the following diff:- scopes: - - openid - - email - - siret - - given_name - - usual_name + scopes: + - openid + - email + - siret + - given_name + - usual_name[refactor_suggestion_essential]
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 51-51: wrong indentation: expected 4 but found 5
(indentation)
🧹 Nitpick comments (12)
api/src/pdc/services/auth/actions/OidcCallbackAction.ts (2)
7-13: Consider consistent naming of array types.The type for permissions is defined as
Array<string>here, while in some parts of the codebase you might usestring[]. Maintaining a consistent style across the project improves readability.
44-46: Gracefully handle unknown or missing roles.Currently, an unknown role defaults to an empty permissions array. If that scenario is invalid, you might consider throwing an error or logging a warning to catch role configuration issues early.
api/src/pdc/services/auth/dto/OidcCallback.ts (1)
3-5: Validate additional OIDC parameters if applicable.Most OIDC flows utilize both a
codeand astateparameter. If thestateor other parameters are relevant, consider adding them for better security.api/src/pdc/services/auth/config/oidc.ts (1)
2-5: Provide fallback or sample values to ease local development.Using
env_or_failensures required environment variables are set, but for local setups, consider providing a.env.exampleor development fallback to reduce friction.api/src/pdc/services/auth/AuthRouter.ts (1)
23-23: Remove debug console.log statement.Debug statements should not be present in production code.
- console.log(req.query);api/src/pdc/services/auth/providers/UserRepository.ts (1)
26-26: Fix typo in JOIN statement.There's a typo in the JOIN statement: "JOIn" should be "JOIN".
- LEFT JOIn ${raw(this.operatorTable)} o + LEFT JOIN ${raw(this.operatorTable)} oapi/justfile (1)
163-174: Consider providing a default port value.The watch command uses
$PORTenvironment variable without a default value. Consider providing a fallback value to prevent startup failures whenPORTis not set.- src/main.ts http \$PORT + src/main.ts http \${PORT:-8080}api/src/pdc/services/auth/config/permissions.ts (1)
168-170: Consider enhancing type safety of permission functions.The helper functions could benefit from stricter typing:
-function scopeToGroup(permissionName: string, group: string) { +type PermissionName = keyof typeof permissions; +type GroupName = keyof typeof permissionsByGroup; + +function scopeToGroup(permissionName: PermissionName, group: GroupName) { return `${group}.${permissionName}`; } function dispatchPermissionsFromMatrix( - permissionsObject: Record<string, string[]>, + permissionsObject: Record<PermissionName, string[]>, ) {Also applies to: 172-199
docker-compose.dev.yml (1)
47-50: Addition of Dex Service in Dev Compose
A newdexservice is introduced with an environment variablePG_DB: local. While the environment setting is in place, it appears that there is no explicit image or build attribute specified for thedexservice. Please verify if this service definition is intentionally left minimal or if further configuration (such as specifying an image) is required for local development.docker/dex/config.yaml (1)
46-46: Trailing Whitespace Detected
Static analysis has detected trailing spaces on several lines (e.g. lines 46, 61, 88, 92, and 128). Removing these will improve readability and prevent potential formatting issues with YAML parsers.Also applies to: 61-61, 88-88, 92-92, 128-128
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml (2)
52-52: Trailing Space in Port Mapping
There is a trailing space at the end of the port mapping line for Dex (- 8090:8080). Please remove the extra whitespace to maintain consistency across the YAML file.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 52-52: trailing spaces
(trailing-spaces)
67-67: Indentation Correction independs_onBlock
Thedepends_onblock for the Dex service appears to be indented with 8 spaces instead of the expected 6. Adjusting the indentation improves YAML clarity and prevents potential parsing issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 67-67: wrong indentation: expected 6 but found 8
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
api/justfile(1 hunks)api/src/db/migrations/20250210000000-alter-users.sql(1 hunks)api/src/ilos/common/Decorators.ts(1 hunks)api/src/ilos/connection-redis/RedisConnection.ts(1 hunks)api/src/ilos/core/foundation/ServiceContainer.ts(2 hunks)api/src/pdc/proxy/HttpTransport.ts(3 hunks)api/src/pdc/proxy/Kernel.ts(2 hunks)api/src/pdc/services/auth/AuthRouter.ts(1 hunks)api/src/pdc/services/auth/AuthServiceProvider.ts(1 hunks)api/src/pdc/services/auth/actions/OidcCallbackAction.ts(1 hunks)api/src/pdc/services/auth/config/index.ts(1 hunks)api/src/pdc/services/auth/config/oidc.ts(1 hunks)api/src/pdc/services/auth/config/permissions.ts(1 hunks)api/src/pdc/services/auth/dto/OidcCallback.ts(1 hunks)api/src/pdc/services/auth/providers/OidcProvider.ts(1 hunks)api/src/pdc/services/auth/providers/UserRepository.ts(1 hunks)docker-compose.dev.yml(2 hunks)docker-compose.e2e.yml(3 hunks)docker/dex/config.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/src/ilos/common/Decorators.ts
- api/src/pdc/services/auth/config/index.ts
🧰 Additional context used
🪛 YAMLlint (1.35.1)
docker/dex/config.yaml
[error] 46-46: trailing spaces
(trailing-spaces)
[warning] 51-51: wrong indentation: expected 4 but found 5
(indentation)
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 88-88: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
[error] 128-128: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml
[error] 52-52: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 6 but found 8
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (17)
api/src/ilos/core/foundation/ServiceContainer.ts (2)
11-11: LGTM! Using symbol for dependency injection.Using a symbol instead of a string for dependency injection is a good practice as it prevents naming collisions.
111-111: LGTM! Using symbol-based binding.The change from string-based binding to symbol-based binding enhances type safety and prevents naming collisions.
api/src/pdc/proxy/HttpTransport.ts (1)
15-24: LGTM! New imports for nested route registration.The imports are well-organized and support the new nested route registration functionality.
Also applies to: 29-29
api/src/pdc/services/auth/actions/OidcCallbackAction.ts (2)
22-29: Constructor dependency injection is well-structured.All dependencies (
oidcProvider,userRepository, andconfig) are cleanly injected. This promotes modularity and testability.
31-37: Ensure no sensitive fields are returned.Spreading
...userin the response can lead to unintentionally leaking sensitive data (e.g., hashed passwords). Verify that the user object doesn't contain sensitive fields. If it does, omit them before returning.api/src/pdc/services/auth/AuthServiceProvider.ts (1)
1-23: LGTM! Well-structured authentication service provider.The implementation follows best practices with proper dependency injection, middleware configuration, and separation of concerns.
api/src/ilos/connection-redis/RedisConnection.ts (1)
14-16: LGTM! Improved type safety and environment compatibility.The conditional check for
windowexistence prevents potential runtime errors in environments where thewindowobject is not available.api/src/pdc/proxy/Kernel.ts (1)
7-7: Verify service initialization order.The AuthServiceProvider has been added as the first service in the children array. While the implementation looks correct, please verify that this initialization order doesn't affect other services that might depend on authentication.
Also applies to: 30-30
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any direct dependencies on AuthServiceProvider in other services rg -l "AuthServiceProvider" api/src/pdc/services/Length of output: 98
Verified initialization order for AuthServiceProvider.
The shell script search shows that the only file referencing AuthServiceProvider is the file defining it, with no other service directly depending on it. This indicates that placing AuthServiceProvider first in the children array should not affect any other services relying on authentication.
api/src/pdc/services/auth/config/permissions.ts (1)
19-19: Review permission assignments for security.The
certificate.findpermission is assigned to "common" role, while related operations likecertificate.listandcertificate.createare more restricted. Verify if this broad access is intentional.Also applies to: 26-28
api/src/db/migrations/20250210000000-alter-users.sql (2)
1-3: User Table Modifications: Dropping NOT NULL Constraints
The migration correctly drops the NOT NULL constraints on thefirstnameandlastnamecolumns. This change offers more flexibility with user data—possibly to accommodate scenarios where these fields might not be provided (for example, during third‐party authentication flows).
5-6: Territory Table Update: New SIRET Column
A new columnsiret(of type VARCHAR) is added to theterritory.territoriestable. Ensure that any application logic consuming this field (perhaps for business or authorization rules) is updated accordingly.docker-compose.dev.yml (1)
60-62: API Service Command Modification
The API service now uses a custom command (just watch) likely to invoke a watch mode for automatic restarts. This seems aligned with the new development workflow, so please ensure that the correspondingjustfileand watch scripts are correctly set up.docker/dex/config.yaml (4)
1-3: Dex Issuer and Web Server Configuration
The issuer is set tohttp://auth.covoiturage.test, and the web server is configured to listen on0.0.0.0:8080. Double-check that using HTTP (instead of HTTPS) is intentional for your environment (likely acceptable for a development or testing setup).
4-9: Static Clients Config – Removal of Client ID?
The static client configuration now omits the previously presentid: secretline. Verify that the client’s identity is correctly managed by the remaining fields (redirectURIs,name, andsecret) and that this omission is intentional, ensuring compatibility with client registration requirements.
10-20: PostgreSQL Storage Configuration
The storage section is clearly set up for PostgreSQL with parameters read from environment variables. This setup will make deployments more flexible—just ensure that all required environment variables are defined in your deployments.
21-32: OIDC Connector Setup
The OIDC connector (named “ProConnect”) is configured with dynamic values (via environment variables) for issuer, client credentials, and redirect URI. The comments provide useful links for configuration discovery. This segment is clear and matches expected patterns for Dex connectors.docker-compose.e2e.yml (1)
36-67: Dex Service Configuration in E2E Environment
A newdexservice is defined using the Bitnami Dex image with detailed environment variables for PostgreSQL connectivity and OIDC configuration. The port mapping (8090:8080), labels for Traefik routing, and command to serve Dex with the configuration file are all appropriately set.
Please verify that all environment variable values (likeOIDC_ISSUER_URL,OIDC_CLIENT_ID, andOIDC_CLIENT_SECRET) match those used in your Dex configuration and integration tests.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 52-52: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 6 but found 8
(indentation)
| private registerNestedRoutes() { | ||
| this.kernel.getContainer().bind(proxy).toConstantValue(this.app); | ||
| const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children); | ||
| for (const serviceProvider of serviceProviders) { | ||
| const container = serviceProvider.getContainer(); | ||
| if (container.isBound(router)) { | ||
| const routerInstance = container.resolve<RegisterHookInterface>(container.get(router)); | ||
| routerInstance.register(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for route registration.
The registerNestedRoutes method should handle potential errors during route registration to prevent silent failures.
Apply this diff to add error handling:
private registerNestedRoutes() {
this.kernel.getContainer().bind(proxy).toConstantValue(this.app);
const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children);
for (const serviceProvider of serviceProviders) {
const container = serviceProvider.getContainer();
if (container.isBound(router)) {
- const routerInstance = container.resolve<RegisterHookInterface>(container.get(router));
- routerInstance.register();
+ try {
+ const routerInstance = container.resolve<RegisterHookInterface>(container.get(router));
+ routerInstance.register();
+ } catch (error) {
+ logger.error(`Failed to register routes for service provider: ${error.message}`);
+ }
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private registerNestedRoutes() { | |
| this.kernel.getContainer().bind(proxy).toConstantValue(this.app); | |
| const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children); | |
| for (const serviceProvider of serviceProviders) { | |
| const container = serviceProvider.getContainer(); | |
| if (container.isBound(router)) { | |
| const routerInstance = container.resolve<RegisterHookInterface>(container.get(router)); | |
| routerInstance.register(); | |
| } | |
| } | |
| } | |
| private registerNestedRoutes() { | |
| this.kernel.getContainer().bind(proxy).toConstantValue(this.app); | |
| const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children); | |
| for (const serviceProvider of serviceProviders) { | |
| const container = serviceProvider.getContainer(); | |
| if (container.isBound(router)) { | |
| try { | |
| const routerInstance = container.resolve<RegisterHookInterface>(container.get(router)); | |
| routerInstance.register(); | |
| } catch (error) { | |
| logger.error(`Failed to register routes for service provider: ${error.message}`); | |
| } | |
| } | |
| } | |
| } |
| this.app.get( | ||
| "/auth/callback", | ||
| asyncHandler(async (req: express.Request, res: express.Response) => { | ||
| console.log(req.query); | ||
| const { code } = req.query; | ||
| if (typeof code === "string") { | ||
| const user = await this.oidcCallbackAction.handle({ code }); | ||
| req.session.user = user; | ||
| } | ||
| return res.end(); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Add error handling and CSRF protection.
The callback route needs:
- Error handling for authentication failures
- CSRF protection to prevent request forgery
- Session validation
this.app.get(
"/auth/callback",
+ csrfProtection,
asyncHandler(async (req: express.Request, res: express.Response) => {
- console.log(req.query);
const { code } = req.query;
if (typeof code === "string") {
- const user = await this.oidcCallbackAction.handle({ code });
- req.session.user = user;
+ try {
+ const user = await this.oidcCallbackAction.handle({ code });
+ if (!user) {
+ return res.status(401).json({ error: 'Authentication failed' });
+ }
+ req.session.user = user;
+ return res.redirect('/dashboard');
+ } catch (error) {
+ console.error('Authentication error:', error);
+ return res.status(500).json({ error: 'Authentication failed' });
+ }
}
- return res.end();
+ return res.status(400).json({ error: 'Invalid authorization code' });
}),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.app.get( | |
| "/auth/callback", | |
| asyncHandler(async (req: express.Request, res: express.Response) => { | |
| console.log(req.query); | |
| const { code } = req.query; | |
| if (typeof code === "string") { | |
| const user = await this.oidcCallbackAction.handle({ code }); | |
| req.session.user = user; | |
| } | |
| return res.end(); | |
| }), | |
| ); | |
| this.app.get( | |
| "/auth/callback", | |
| csrfProtection, | |
| asyncHandler(async (req: express.Request, res: express.Response) => { | |
| const { code } = req.query; | |
| if (typeof code === "string") { | |
| try { | |
| const user = await this.oidcCallbackAction.handle({ code }); | |
| if (!user) { | |
| return res.status(401).json({ error: 'Authentication failed' }); | |
| } | |
| req.session.user = user; | |
| return res.redirect('/dashboard'); | |
| } catch (error) { | |
| console.error('Authentication error:', error); | |
| return res.status(500).json({ error: 'Authentication failed' }); | |
| } | |
| } | |
| return res.status(400).json({ error: 'Invalid authorization code' }); | |
| }), | |
| ); |
| async findUserByEmail(email: string) { | ||
| const query = sql` | ||
| SELECT | ||
| u.email, | ||
| u.role, | ||
| u.operator_id, | ||
| u.territory_id, | ||
| COALESCE(o.siret, t.siret) as siret | ||
| FROM ${raw(this.table)} u | ||
| LEFT JOIN ${raw(this.territoryTable)} t | ||
| ON t._id = u.territory_id | ||
| LEFT JOIn ${raw(this.operatorTable)} o | ||
| ON o._id = u.operator_id | ||
| WHERE u.email = ${email} | ||
| LIMIT 1 | ||
| `; | ||
| const results = await this.connection.getClient().query(query); | ||
| return results.rows[0]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The findUserByEmail method needs:
- Email validation
- Error handling for database operations
async findUserByEmail(email: string) {
+ if (!email || !email.includes('@')) {
+ throw new Error('Invalid email format');
+ }
+
const query = sql`
SELECT
u.email,
u.role,
u.operator_id,
u.territory_id,
COALESCE(o.siret, t.siret) as siret
FROM ${raw(this.table)} u
LEFT JOIN ${raw(this.territoryTable)} t
ON t._id = u.territory_id
- LEFT JOIn ${raw(this.operatorTable)} o
+ LEFT JOIN ${raw(this.operatorTable)} o
ON o._id = u.operator_id
WHERE u.email = ${email}
LIMIT 1
`;
- const results = await this.connection.getClient().query(query);
- return results.rows[0];
+ try {
+ const results = await this.connection.getClient().query(query);
+ return results.rows[0];
+ } catch (error) {
+ console.error('Database error:', error);
+ throw new Error('Failed to fetch user data');
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async findUserByEmail(email: string) { | |
| const query = sql` | |
| SELECT | |
| u.email, | |
| u.role, | |
| u.operator_id, | |
| u.territory_id, | |
| COALESCE(o.siret, t.siret) as siret | |
| FROM ${raw(this.table)} u | |
| LEFT JOIN ${raw(this.territoryTable)} t | |
| ON t._id = u.territory_id | |
| LEFT JOIn ${raw(this.operatorTable)} o | |
| ON o._id = u.operator_id | |
| WHERE u.email = ${email} | |
| LIMIT 1 | |
| `; | |
| const results = await this.connection.getClient().query(query); | |
| return results.rows[0]; | |
| } | |
| async findUserByEmail(email: string) { | |
| if (!email || !email.includes('@')) { | |
| throw new Error('Invalid email format'); | |
| } | |
| const query = sql` | |
| SELECT | |
| u.email, | |
| u.role, | |
| u.operator_id, | |
| u.territory_id, | |
| COALESCE(o.siret, t.siret) as siret | |
| FROM ${raw(this.table)} u | |
| LEFT JOIN ${raw(this.territoryTable)} t | |
| ON t._id = u.territory_id | |
| LEFT JOIN ${raw(this.operatorTable)} o | |
| ON o._id = u.operator_id | |
| WHERE u.email = ${email} | |
| LIMIT 1 | |
| `; | |
| try { | |
| const results = await this.connection.getClient().query(query); | |
| return results.rows[0]; | |
| } catch (error) { | |
| console.error('Database error:', error); | |
| throw new Error('Failed to fetch user data'); | |
| } | |
| } |
| getLoginUrl() { | ||
| const clientId = this.config.get("oidc.client_id"); | ||
| const authBaseUrl = this.config.get("oidc.base_url"); | ||
| const redirectUrl = this.config.get("oidc.redirect_url"); | ||
| const url = new URL(authBaseUrl); | ||
| url.pathname = "/auth"; | ||
| url.searchParams.set("client_id", clientId); | ||
| url.searchParams.set("scope", "openid profile email"); | ||
| url.searchParams.set("redirect_uri", redirectUrl); | ||
| url.searchParams.set("response_type", "code"); | ||
| return url.toString(); | ||
| } |
There was a problem hiding this comment.
Add state parameter for CSRF protection.
The login URL generation should include a state parameter to prevent CSRF attacks.
getLoginUrl() {
const clientId = this.config.get("oidc.client_id");
const authBaseUrl = this.config.get("oidc.base_url");
const redirectUrl = this.config.get("oidc.redirect_url");
+ const state = crypto.randomUUID();
const url = new URL(authBaseUrl);
url.pathname = "/auth";
url.searchParams.set("client_id", clientId);
url.searchParams.set("scope", "openid profile email");
url.searchParams.set("redirect_uri", redirectUrl);
url.searchParams.set("response_type", "code");
+ url.searchParams.set("state", state);
+ // Store state in session for validation during callback
+ return { url: url.toString(), state };
- return url.toString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getLoginUrl() { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const redirectUrl = this.config.get("oidc.redirect_url"); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/auth"; | |
| url.searchParams.set("client_id", clientId); | |
| url.searchParams.set("scope", "openid profile email"); | |
| url.searchParams.set("redirect_uri", redirectUrl); | |
| url.searchParams.set("response_type", "code"); | |
| return url.toString(); | |
| } | |
| getLoginUrl() { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const redirectUrl = this.config.get("oidc.redirect_url"); | |
| const state = crypto.randomUUID(); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/auth"; | |
| url.searchParams.set("client_id", clientId); | |
| url.searchParams.set("scope", "openid profile email"); | |
| url.searchParams.set("redirect_uri", redirectUrl); | |
| url.searchParams.set("response_type", "code"); | |
| url.searchParams.set("state", state); | |
| // Store state in session for validation during callback | |
| return { url: url.toString(), state }; | |
| } |
| async getTokenFromCode(code: string) { | ||
| const clientId = this.config.get("oidc.client_id"); | ||
| const clientSecret = this.config.get("oidc.client_secret"); | ||
| const authBaseUrl = this.config.get("oidc.base_url"); | ||
| const redirectUrl = this.config.get("oidc.redirect_url"); | ||
| const url = new URL(authBaseUrl); | ||
| url.pathname = "/token"; | ||
| const form = new FormData(); | ||
| form.set("grant_type", "authorization_code"); | ||
| form.set("client_id", clientId); | ||
| form.set("client_secret", clientSecret); | ||
| form.set("code", code); | ||
| form.set("redirect_uri", redirectUrl); | ||
| const response = await fetch(url, { body: form, method: "POST" }); | ||
| const json = await response.json(); | ||
| return json.access_token; | ||
| } | ||
|
|
||
| async getUserInfoFromToken(token: string) { | ||
| const authBaseUrl = this.config.get("oidc.base_url"); | ||
| const url = new URL(authBaseUrl); | ||
| url.pathname = "/userinfo"; | ||
| const response = await fetch(url, { | ||
| headers: { | ||
| "Authorization": `Bearer ${token}`, | ||
| }, | ||
| }); | ||
| const json = await response.json(); | ||
| return { | ||
| email: json.email, | ||
| siret: json.preferred_username, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add error handling and request timeouts.
The token and userinfo endpoints need:
- Error handling for failed requests
- Request timeouts
- Token validation
async getTokenFromCode(code: string) {
const clientId = this.config.get("oidc.client_id");
const clientSecret = this.config.get("oidc.client_secret");
const authBaseUrl = this.config.get("oidc.base_url");
const redirectUrl = this.config.get("oidc.redirect_url");
const url = new URL(authBaseUrl);
url.pathname = "/token";
const form = new FormData();
form.set("grant_type", "authorization_code");
form.set("client_id", clientId);
form.set("client_secret", clientSecret);
form.set("code", code);
form.set("redirect_uri", redirectUrl);
- const response = await fetch(url, { body: form, method: "POST" });
- const json = await response.json();
- return json.access_token;
+ try {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 5000);
+ const response = await fetch(url, {
+ body: form,
+ method: "POST",
+ signal: controller.signal
+ });
+ clearTimeout(timeout);
+ if (!response.ok) {
+ throw new Error(`Token request failed: ${response.statusText}`);
+ }
+ const json = await response.json();
+ if (!json.access_token) {
+ throw new Error('No access token in response');
+ }
+ return json.access_token;
+ } catch (error) {
+ if (error.name === 'AbortError') {
+ throw new Error('Token request timeout');
+ }
+ throw error;
+ }
}
async getUserInfoFromToken(token: string) {
const authBaseUrl = this.config.get("oidc.base_url");
const url = new URL(authBaseUrl);
url.pathname = "/userinfo";
- const response = await fetch(url, {
- headers: {
- "Authorization": `Bearer ${token}`,
- },
- });
- const json = await response.json();
- return {
- email: json.email,
- siret: json.preferred_username,
- };
+ try {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 5000);
+ const response = await fetch(url, {
+ headers: {
+ "Authorization": `Bearer ${token}`,
+ },
+ signal: controller.signal
+ });
+ clearTimeout(timeout);
+ if (!response.ok) {
+ throw new Error(`Userinfo request failed: ${response.statusText}`);
+ }
+ const json = await response.json();
+ if (!json.email) {
+ throw new Error('No email in userinfo response');
+ }
+ return {
+ email: json.email,
+ siret: json.preferred_username,
+ };
+ } catch (error) {
+ if (error.name === 'AbortError') {
+ throw new Error('Userinfo request timeout');
+ }
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getTokenFromCode(code: string) { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const clientSecret = this.config.get("oidc.client_secret"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const redirectUrl = this.config.get("oidc.redirect_url"); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/token"; | |
| const form = new FormData(); | |
| form.set("grant_type", "authorization_code"); | |
| form.set("client_id", clientId); | |
| form.set("client_secret", clientSecret); | |
| form.set("code", code); | |
| form.set("redirect_uri", redirectUrl); | |
| const response = await fetch(url, { body: form, method: "POST" }); | |
| const json = await response.json(); | |
| return json.access_token; | |
| } | |
| async getUserInfoFromToken(token: string) { | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/userinfo"; | |
| const response = await fetch(url, { | |
| headers: { | |
| "Authorization": `Bearer ${token}`, | |
| }, | |
| }); | |
| const json = await response.json(); | |
| return { | |
| email: json.email, | |
| siret: json.preferred_username, | |
| }; | |
| } | |
| async getTokenFromCode(code: string) { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const clientSecret = this.config.get("oidc.client_secret"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const redirectUrl = this.config.get("oidc.redirect_url"); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/token"; | |
| const form = new FormData(); | |
| form.set("grant_type", "authorization_code"); | |
| form.set("client_id", clientId); | |
| form.set("client_secret", clientSecret); | |
| form.set("code", code); | |
| form.set("redirect_uri", redirectUrl); | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 5000); | |
| const response = await fetch(url, { | |
| body: form, | |
| method: "POST", | |
| signal: controller.signal, | |
| }); | |
| clearTimeout(timeout); | |
| if (!response.ok) { | |
| throw new Error(`Token request failed: ${response.statusText}`); | |
| } | |
| const json = await response.json(); | |
| if (!json.access_token) { | |
| throw new Error("No access token in response"); | |
| } | |
| return json.access_token; | |
| } catch (error) { | |
| if (error.name === "AbortError") { | |
| throw new Error("Token request timeout"); | |
| } | |
| throw error; | |
| } | |
| } | |
| async getUserInfoFromToken(token: string) { | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/userinfo"; | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 5000); | |
| const response = await fetch(url, { | |
| headers: { | |
| Authorization: `Bearer ${token}`, | |
| }, | |
| signal: controller.signal, | |
| }); | |
| clearTimeout(timeout); | |
| if (!response.ok) { | |
| throw new Error(`Userinfo request failed: ${response.statusText}`); | |
| } | |
| const json = await response.json(); | |
| if (!json.email) { | |
| throw new Error("No email in userinfo response"); | |
| } | |
| return { | |
| email: json.email, | |
| siret: json.preferred_username, | |
| }; | |
| } catch (error) { | |
| if (error.name === "AbortError") { | |
| throw new Error("Userinfo request timeout"); | |
| } | |
| throw error; | |
| } | |
| } |
| for (const permissionName of Reflect.ownKeys(permissionsObject) as string[]) { | ||
| const permissionRoles = permissionsObject[permissionName]; | ||
|
|
||
| for (const permissionRole of permissionRoles) { | ||
| const group = permissionRole.split(".")[0]; | ||
| permissionsByGroup[permissionRole].push( | ||
| scopeToGroup(permissionName, group), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for permission roles.
The permission role splitting assumes a valid format (group.role). Consider adding validation to prevent runtime errors.
for (const permissionName of Reflect.ownKeys(permissionsObject) as string[]) {
const permissionRoles = permissionsObject[permissionName];
for (const permissionRole of permissionRoles) {
- const group = permissionRole.split(".")[0];
+ const [group, role] = permissionRole.split(".");
+ if (!group || !role) {
+ throw new Error(`Invalid permission role format: ${permissionRole}`);
+ }
+ if (!permissionsByGroup[permissionRole]) {
+ throw new Error(`Unknown permission role: ${permissionRole}`);
+ }
permissionsByGroup[permissionRole].push(
scopeToGroup(permissionName, group),
);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const permissionName of Reflect.ownKeys(permissionsObject) as string[]) { | |
| const permissionRoles = permissionsObject[permissionName]; | |
| for (const permissionRole of permissionRoles) { | |
| const group = permissionRole.split(".")[0]; | |
| permissionsByGroup[permissionRole].push( | |
| scopeToGroup(permissionName, group), | |
| ); | |
| } | |
| } | |
| for (const permissionName of Reflect.ownKeys(permissionsObject) as string[]) { | |
| const permissionRoles = permissionsObject[permissionName]; | |
| for (const permissionRole of permissionRoles) { | |
| const [group, role] = permissionRole.split("."); | |
| if (!group || !role) { | |
| throw new Error(`Invalid permission role format: ${permissionRole}`); | |
| } | |
| if (!permissionsByGroup[permissionRole]) { | |
| throw new Error(`Unknown permission role: ${permissionRole}`); | |
| } | |
| permissionsByGroup[permissionRole].push( | |
| scopeToGroup(permissionName, group), | |
| ); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/justfile (2)
163-167: Consider security and stability improvements for the debug command.
- Security: Replace the
-Aflag with explicit permissions to follow the principle of least privilege.- Stability: The
--unstable-fsflag indicates reliance on unstable APIs that might break in future Deno versions.Apply this diff to improve security:
- deno repl \ - -A \ - --unstable-fs \ - --eval 'import { getKernel } from "./src/debug.ts"; const kernel = await getKernel();' + deno repl \ + --allow-net \ + --allow-env \ + --allow-read \ + --allow-write \ + --unstable-fs \ + --eval 'import { getKernel } from "./src/debug.ts"; const kernel = await getKernel();'
169-180: Fix environment variable usage and note stability concerns.
- Environment: The
$PORTenvironment variable should be properly escaped in shell script.- Stability: The
--unstable-fsflag indicates reliance on unstable APIs that might break in future Deno versions.Apply this diff to fix environment variable usage:
- src/main.ts http \$PORT + src/main.ts http "$PORT"api/src/pdc/services/auth/config/oidc.ts (1)
1-6: Add type information and rename redundant environment variable.
- Add type information for better type safety:
-export const client_id = env_or_fail("OIDC_CLIENT_ID"); +export const client_id: string = env_or_fail("OIDC_CLIENT_ID"); -export const client_secret = env_or_fail("OIDC_CLIENT_SECRET"); +export const client_secret: string = env_or_fail("OIDC_CLIENT_SECRET"); -export const base_url = env_or_fail("OIDC_BASE_URL"); +export const base_url: string = env_or_fail("OIDC_BASE_URL"); -export const redirect_url = env_or_fail("OIDC_REDIRECT_URL"); +export const redirect_url: string = env_or_fail("OIDC_REDIRECT_URL"); -export const app_url = env_or_fail("APP_APP_URL"); +export const app_url: string = env_or_fail("APP_URL");
- The environment variable
APP_APP_URLhas a redundant "APP" prefix. Consider renaming it to justAPP_URL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/justfile(1 hunks)api/src/pdc/services/auth/AuthRouter.ts(1 hunks)api/src/pdc/services/auth/config/oidc.ts(1 hunks)api/src/pdc/services/auth/providers/OidcProvider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/pdc/services/auth/providers/OidcProvider.ts
🔇 Additional comments (1)
api/src/pdc/services/auth/AuthRouter.ts (1)
21-31: Add error handling and CSRF protection.The callback route needs:
- Error handling for authentication failures
- CSRF protection to prevent request forgery
- Session validation
this.app.get( "/auth/callback", + csrfProtection, asyncHandler(async (req: express.Request, res: express.Response) => { const { code } = req.query; if (typeof code === "string") { - const user = await this.oidcCallbackAction.handle({ code }); - req.session.user = user; + try { + const user = await this.oidcCallbackAction.handle({ code }); + if (!user) { + return res.status(401).json({ error: 'Authentication failed' }); + } + req.session.user = user; + return res.redirect(this.config.get("oidc.app_url")); + } catch (error) { + console.error('Authentication error:', error); + return res.status(500).json({ error: 'Authentication failed' }); + } } - return res.redirect(this.config.get("oidc.app_url")); + return res.status(400).json({ error: 'Invalid authorization code' }); }), );
| this.app.get("/auth/login", (req, res, next) => { | ||
| return res.redirect(this.oidcProvider.getLoginUrl()); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and CSRF protection to the login route.
- Add error handling to catch potential exceptions from
getLoginUrl. - Remove the unused
nextparameter. - Add CSRF protection to prevent request forgery.
- this.app.get("/auth/login", (req, res, next) => {
+ this.app.get("/auth/login", csrfProtection, (req, res) => {
+ try {
return res.redirect(this.oidcProvider.getLoginUrl());
+ } catch (error) {
+ console.error('Login error:', error);
+ return res.status(500).json({ error: 'Login failed' });
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.app.get("/auth/login", (req, res, next) => { | |
| return res.redirect(this.oidcProvider.getLoginUrl()); | |
| }); | |
| this.app.get("/auth/login", csrfProtection, (req, res) => { | |
| try { | |
| return res.redirect(this.oidcProvider.getLoginUrl()); | |
| } catch (error) { | |
| console.error('Login error:', error); | |
| return res.status(500).json({ error: 'Login failed' }); | |
| } | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/analysis.yml(1 hunks)api/justfile(2 hunks)docker/keycloak/realm.json(0 hunks)
💤 Files with no reviewable changes (1)
- docker/keycloak/realm.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/analysis.yml
🔇 Additional comments (1)
api/justfile (1)
29-29: LGTM: Added dex service to e2e services.The addition of the
dexservice to the default services list aligns with the PR objective of adding dex auth.
| watch: | ||
| deno run \ | ||
| --allow-net \ | ||
| --allow-env \ | ||
| --allow-read \ | ||
| --allow-write \ | ||
| --allow-ffi \ | ||
| --allow-sys \ | ||
| --allow-run \ | ||
| --watch \ | ||
| --unstable-fs \ | ||
| src/main.ts http \$PORT | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add PORT validation and review unstable-fs usage.
The watch command has a few potential issues:
- The
$PORTenvironment variable is used without validation. - The
--unstable-fsflag is experimental.
Consider adding PORT validation before starting the server:
watch:
+ #!/usr/bin/env bash
+ if [[ -z "${PORT}" ]]; then
+ echo "Error: PORT environment variable is not set"
+ exit 1
+ fi
deno run \
--allow-net \
--allow-env \
--allow-read \
--allow-write \
--allow-ffi \
--allow-sys \
--allow-run \
--watch \
--unstable-fs \
src/main.ts http \$PORT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| watch: | |
| deno run \ | |
| --allow-net \ | |
| --allow-env \ | |
| --allow-read \ | |
| --allow-write \ | |
| --allow-ffi \ | |
| --allow-sys \ | |
| --allow-run \ | |
| --watch \ | |
| --unstable-fs \ | |
| src/main.ts http \$PORT | |
| watch: | |
| #!/usr/bin/env bash | |
| if [[ -z "${PORT}" ]]; then | |
| echo "Error: PORT environment variable is not set" | |
| exit 1 | |
| fi | |
| deno run \ | |
| --allow-net \ | |
| --allow-env \ | |
| --allow-read \ | |
| --allow-write \ | |
| --allow-ffi \ | |
| --allow-sys \ | |
| --allow-run \ | |
| --watch \ | |
| --unstable-fs \ | |
| src/main.ts http \$PORT |
| debug: | ||
| deno repl \ | ||
| -A \ | ||
| --unstable-fs \ | ||
| --eval 'import { getKernel } from "./src/debug.ts"; const kernel = await getKernel();' | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the use of unstable-fs flag and add error handling.
The debug command uses the --unstable-fs flag which is experimental and may change in future Deno releases. Additionally, consider adding error handling for failed imports.
🌐 Web query:
What is the current status of Deno's unstable-fs flag and are there any stable alternatives?
💡 Result:
The --unstable-fs flag has been deprecated as of Deno 1.44 (May 2024) and is no longer required for core file system operations[1][4]. Key file system APIs previously behind this flag have been stabilized:
Stabilized replacements include:
Deno.FsFile.syncData()/syncDataSync()for explicit file data synchronization[1]Deno.FsFile.sync()/syncSync()for metadata synchronization[1]Deno.FsFile.lock()/lockSync()andunlock()/unlockSync()for file locking[1]
These changes eliminate previous compatibility issues with frameworks like Next.js while maintaining Deno's security model[1]. The stabilization means developers can now use these file system features without special flags:
// Example using stabilized APIs
const file = await Deno.open("data.txt", { read: true, write: true });
await file.lock(); // Now stable
await Deno.write(file.rid, new TextEncoder().encode("updated"));
await file.syncData(); // Now stable
file.close();For advanced filesystem access patterns beyond core operations, Deno recommends:
- Web File System Access API: For browser-like file handling (specification)[3]
- Third-party libraries: Such as
deno.land/x/fsfor extended utilities - Node.js compatibility layer: Via
--compatflag for npm package interoperability[2]
While most filesystem features are now stable, some edge-case operations remain behind newer granular unstable flags like --unstable-write for experimental write optimizations[9]. The broader --unstable flag itself is being phased out in favor of these targeted feature flags[9].
Citations:
- 1: https://deno.com/blog/v1.44
- 2: https://betterstack.com/community/guides/scaling-nodejs/nodejs-vs-deno-vs-bun/
- 3: https://github.com/use-strict/file-system-access
- 4: https://docs.deno.com/runtime/fundamentals/stability_and_releases/
- 5: https://answers.netlify.com/t/enabling-the-unstable-flag-for-deno-in-edge-functions/56182
- 6: https://stackshare.io/deno/alternatives
- 7: https://docs.deno.com/api/deno/file-system
- 8: https://mvolkmann.github.io/blog/deno/?v=1.1.1
- 9: https://deno-docs.dev.org.tw/runtime/reference/cli/unstable_flags/
- 10: https://github.com/denolib/awesome-deno
Action Required: Update debug command in api/justfile
- Remove the deprecated
--unstable-fsflag as it is no longer required (core file system operations are now stable since Deno 1.44). - Update the inline evaluation to include error handling—for example, wrap the dynamic import in a try-catch block to log or manage failures.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (4)
api/justfile (1)
163-167: 🛠️ Refactor suggestionRemove deprecated flag and use granular permissions.
- The
--unstable-fsflag has been deprecated since Deno 1.44.- Using
-Agrants all permissions, which violates the principle of least privilege.Apply this diff to use stable APIs and granular permissions:
debug: deno repl \ - -A \ - --unstable-fs \ + --allow-net \ + --allow-env \ + --allow-read \ + --allow-write \ --eval 'import { getKernel } from "./src/debug.ts"; const kernel = await getKernel();'api/src/pdc/services/auth/providers/OidcProvider.ts (3)
22-33:⚠️ Potential issueAdd state parameter for CSRF protection.
The login URL generation should include a state parameter to prevent CSRF attacks.
getLoginUrl() { const clientId = this.config.get("oidc.client_id"); const authBaseUrl = this.config.get("oidc.base_url"); const redirectUrl = this.config.get("oidc.redirect_url"); + const state = crypto.randomUUID(); const url = new URL(authBaseUrl); url.pathname = "/auth"; url.searchParams.set("client_id", clientId); url.searchParams.set("scope", "openid profile email"); url.searchParams.set("redirect_uri", redirectUrl); url.searchParams.set("response_type", "code"); + url.searchParams.set("state", state); + // Store state in session for validation during callback + return { url: url.toString(), state }; - return url.toString(); }
35-57:⚠️ Potential issueAdd error handling and request timeouts.
The token endpoint needs proper error handling, request timeouts, and token validation.
async getTokenFromCode(code: string) { const clientId = this.config.get("oidc.client_id"); const clientSecret = this.config.get("oidc.client_secret"); const authBaseUrl = this.config.get("oidc.base_url"); const redirectUrl = this.config.get("oidc.redirect_url"); const url = new URL(authBaseUrl); url.pathname = "/token"; const form = new URLSearchParams(); form.set("grant_type", "authorization_code"); form.set("client_id", clientId); form.set("client_secret", clientSecret); form.set("code", code); form.set("redirect_uri", redirectUrl); - const response = await fetch(url, { - body: form.toString(), - method: "POST", - headers: { - "Content-type": "application/x-www-form-urlencoded", - }, - }); - const json = await response.json(); - return json.access_token; + try { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 5000); + const response = await fetch(url, { + body: form.toString(), + method: "POST", + headers: { + "Content-type": "application/x-www-form-urlencoded", + }, + signal: controller.signal + }); + clearTimeout(timeout); + if (!response.ok) { + throw new Error(`Token request failed: ${response.statusText}`); + } + const json = await response.json(); + if (!json.access_token) { + throw new Error('No access token in response'); + } + return json.access_token; + } catch (error) { + if (error.name === 'AbortError') { + throw new Error('Token request timeout'); + } + throw error; + } }
59-73:⚠️ Potential issueAdd error handling and response validation.
The userinfo endpoint needs proper error handling, request timeouts, and response validation.
async getUserInfoFromToken(token: string) { const authBaseUrl = this.config.get("oidc.base_url"); const url = new URL(authBaseUrl); url.pathname = "/userinfo"; - const response = await fetch(url, { - headers: { - "Authorization": `Bearer ${token}`, - }, - }); - const json = await response.json(); - return { - email: json.email, - siret: json.preferred_username, - }; + try { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 5000); + const response = await fetch(url, { + headers: { + "Authorization": `Bearer ${token}`, + }, + signal: controller.signal + }); + clearTimeout(timeout); + if (!response.ok) { + throw new Error(`Userinfo request failed: ${response.statusText}`); + } + const json = await response.json(); + if (!json.email || !json.preferred_username) { + throw new Error('Missing required fields in userinfo response'); + } + return { + email: json.email, + siret: json.preferred_username, + }; + } catch (error) { + if (error.name === 'AbortError') { + throw new Error('Userinfo request timeout'); + } + throw error; + } }
🧹 Nitpick comments (10)
api/src/pdc/services/auth/providers/DexClient.ts (4)
1-7: Consider adding error handling for gRPC imports or missing modules.While the imports look correct, if the proto or
grpcdeps become unavailable or erroneous, consider a try-catch strategy or validation to fail gracefully rather than throwing unhandled exceptions.
10-16: Initialize the gRPC client upon construction to detect potential config issues earlier.Currently,
init()only callsawait this.getClient();to lazily initialize the client. If the environment or gRPC definitions are invalid, an error emerges at runtime after this class is constructed (but not necessarily immediately). Consider eagerly initializing or validating the environment so problems surface as soon as possible in the application startup flow.
31-44: Optimize or document client-side filtering of passwords.
listByOperatorfetches all passwords regardless of operator, then filters them locally. This approach might lead to performance degradation if the server has many records. If feasible, adjust server logic to accept an operator filter, or document that a smaller data set is expected, preventing performance surprises.
64-73: Implement error handling for gRPC deletion.
deleteByOperatordeletes a password and only handles aNotFoundExceptionif the token isn’t found locally. If the gRPC service fails or the password no longer exists in the server, it may surface unhandled errors. Consider gracefully catching and interpreting gRPC-related exceptions to maintain consistent error messaging.docker/dex/config.yaml (2)
7-12: Review static client configuration.The static client uses hardcoded secrets in development configuration. While this is acceptable for local development, ensure that production deployments use secure secrets.
Consider using environment variables for the client secret:
- secret: secret + secret: $APP_AUTH_CLIENT_SECRET
139-148: Review token expiry configuration.The token expiry configuration looks reasonable but verify:
refreshTokens.validIfNotUsedFor: "2160h"(90 days) aligns with security requirementsrefreshTokens.absoluteLifetime: "3960h"(165 days) aligns with security requirementsdocker-compose.e2e.yml (2)
39-50: Consider using secrets for sensitive information.Environment variables contain sensitive information like database credentials and client secrets. Consider using Docker secrets or environment files for better security.
67-68: Fix indentation.The
depends_onsection has incorrect indentation.- - postgres + - postgres🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 68-68: wrong indentation: expected 6 but found 8
(indentation)
api/src/pdc/services/auth/dex/protofiles/api.proto (1)
66-66: Address TODO comment.The comment "TODO(ericchiang): expand this." needs to be addressed.
Would you like me to help expand this section or create an issue to track this task?
api/src/pdc/services/auth/providers/OidcProvider.ts (1)
90-111: Consider replacing password grant type with more secure alternatives.The resource owner password credentials flow is not recommended by OAuth 2.0 security best practices. Consider using authorization code flow with PKCE for better security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
api/deno.lockis excluded by!**/*.lockapi/src/pdc/services/auth/dex/generated/api.tsis excluded by!**/generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
api/justfile(3 hunks)api/src/deps.ts(1 hunks)api/src/pdc/providers/keycloak/IBotCredentials.ts(0 hunks)api/src/pdc/providers/keycloak/KeycloakBotAuthenticator.integration.spec.ts(0 hunks)api/src/pdc/providers/keycloak/KeycloakBotAuthenticator.ts(0 hunks)api/src/pdc/providers/keycloak/KeycloakManager.integration.spec.ts(0 hunks)api/src/pdc/providers/keycloak/KeycloakManager.ts(0 hunks)api/src/pdc/services/auth/config/dex.ts(1 hunks)api/src/pdc/services/auth/dex/getDexClient.ts(1 hunks)api/src/pdc/services/auth/dex/protofiles/api.proto(1 hunks)api/src/pdc/services/auth/providers/DexClient.ts(1 hunks)api/src/pdc/services/auth/providers/OidcProvider.ts(1 hunks)docker-compose.dev.yml(2 hunks)docker-compose.e2e.yml(3 hunks)docker/dex/config.yaml(1 hunks)package.json(0 hunks)
💤 Files with no reviewable changes (6)
- api/src/pdc/providers/keycloak/IBotCredentials.ts
- package.json
- api/src/pdc/providers/keycloak/KeycloakBotAuthenticator.ts
- api/src/pdc/providers/keycloak/KeycloakManager.integration.spec.ts
- api/src/pdc/providers/keycloak/KeycloakBotAuthenticator.integration.spec.ts
- api/src/pdc/providers/keycloak/KeycloakManager.ts
🧰 Additional context used
🪛 YAMLlint (1.35.1)
docker/dex/config.yaml
[error] 49-49: trailing spaces
(trailing-spaces)
[warning] 54-54: wrong indentation: expected 4 but found 5
(indentation)
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 91-91: trailing spaces
(trailing-spaces)
[error] 95-95: trailing spaces
(trailing-spaces)
[error] 131-131: trailing spaces
(trailing-spaces)
docker-compose.e2e.yml
[error] 53-53: trailing spaces
(trailing-spaces)
[warning] 68-68: wrong indentation: expected 6 but found 8
(indentation)
🪛 Buf (1.47.2)
api/src/pdc/services/auth/dex/protofiles/api.proto
3-3: Files with package "api" must be within a directory "api" relative to root but were in directory "api/src/pdc/services/auth/dex/protofiles".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (15)
api/src/pdc/services/auth/providers/DexClient.ts (3)
8-9: Annotation is properly applied.Using
@provider()annotation to inject the class as a service is appropriate for maintaining consistency with the IoC container approach.
17-29: Verify environment variables and handle missing or invalid gRPC endpoints more gracefully.The
getClient()method’s lazy initialization approach is fine, though it does not handle cases whereconfig.get("dex.grpc_host")orconfig.get("dex.grpc_port")is absent or improperly set (e.g., an empty string or invalid number). Consider adding fallback or improved error handling to produce helpful logs or exceptions.
31-44: Validate split username format to avoid runtime errors.
p.username?.split(":")might yield insufficient array items ifusernameis missing or incorrectly formatted. Consider validating the array length (e.g.,roleandoperator_idexist) or handle fallback scenarios gracefully.api/src/pdc/services/auth/config/dex.ts (2)
1-2: Ensure dependency injection or environment readiness testing for Dex.The environment variable usage is correct, but consider testing that your CI/CD or container environment provides these variables. This avoids runtime failures when
DEX_GRPC_HOSTis missing, and ensures the fallback forDEX_GRPC_PORTis valid.
3-4: Hard-coded default for gRPC port is acceptable.Using
5557as a default forgrpc_portis reasonable. Document or confirm with your ops team that this port doesn’t conflict in production.api/src/pdc/services/auth/dex/getDexClient.ts (1)
1-3: Code is straightforward and well-organized.Imports are clearly structured, and a dedicated file for building the client fosters modularity.
api/justfile (3)
29-29: LGTM!The addition of
dexto the default services list aligns with the PR objective of adding Dex authentication.
169-180: Remove deprecated flag and add PORT validation.The watch command has the following issues:
- The
--unstable-fsflag has been deprecated since Deno 1.44.- The
$PORTenvironment variable is used without validation.Apply this diff to fix both issues:
watch: + #!/usr/bin/env bash + if [[ -z "${PORT}" ]]; then + echo "Error: PORT environment variable is not set" + exit 1 + fi deno run \ --allow-net \ --allow-env \ --allow-read \ --allow-write \ --allow-ffi \ --allow-sys \ --allow-run \ --watch \ - --unstable-fs \ src/main.ts http \$PORT
196-196: Verify the implications of disabling frozen lockfile.Using
--frozen=falseallows updating dependencies in the lock file, which could lead to inconsistent builds across different environments.Please ensure that:
- This change is only used in development environments
- CI/CD pipelines use
--frozen=trueto ensure reproducible builds- The lock file is committed after updates
api/src/deps.ts (1)
245-245:✅ Verification successful
Verify the gRPC client version.
The gRPC client is pinned to version 0.4.7. Let's verify if this is the latest stable version and check for any known issues.
🌐 Web query:
What is the latest version of deno.land/x/grpc_basic and are there any known issues?💡 Result:
The latest version of
deno.land/x/grpc_basicis 0.4.7, released 2 years ago[5][9]. This library provides basic gRPC implementation for Deno but comes with several caveats:Known Issues
Server Stream Errors on Windows
Users encounterTypeError: Cannot read properties of null (reading 'write')when using server streaming methods likeShoutHello[1]. A temporary workaround involves modifying the client code to skip problematic frames:// Workaround from Issue #10 if (f.type === "WINDOW_UPDATE" && f.window_size % 24 === 0) { break; }Unmaintained Status
The project is no longer actively maintained[1][5], with the author recommending users track Deno's native gRPC implementation progress instead[1][10].Feature Limitations
- No TLS support
- Missing bidirectional/client streaming[5]
- Uses a JavaScript HTTP/2 implementation rather than Deno's native bindings[5]
Compatibility Notes
While Deno 1.44 added native gRPC support[4], users report compatibility issues when integrating with existing Node.js gRPC libraries like
@grpc/grpc-js[2][8]. The native implementation still has unresolved bugs affecting error handling and stream cancellation[4][8].For new projects, consider monitoring Deno’s official gRPC developments rather than relying on this deprecated library[1][10].
Citations:
- 1: conn is null prohazko2/deno-grpc#10
- 2: v1.44.0 gRPC client support doesn't work denoland/deno#24058
- 3: https://deno.land/x/[email protected]/README.md
- 4: https://deno.com/blog/v1.44
- 5: https://deno.land/x/grpc_basic
- 6: https://questions.deno.com/c/1022163295895027722?page=1
- 7: https://docs.deno.com
- 8: gRPC / node:http2 denoland/deno#23246
- 9: https://github.com/prohazko2/deno-grpc
- 10: Add support for gRPC denoland/deno#3326
gRPC Client Version Verified
The version used in
api/src/deps.ts(line 245) is correctly pinned to 0.4.7, which is currently the latest stable release ofdeno.land/x/grpc_basic. Note, however, that this library has known caveats:
- Server Stream Errors on Windows: Users have reported errors during server streaming.
- Unmaintained: The library is no longer actively maintained.
- Feature Limitations: Missing TLS support and bidirectional/client streaming, among other issues.
For projects that rely heavily on robust gRPC features, consider monitoring Deno's native gRPC developments for a future migration.
docker-compose.dev.yml (1)
60-62: LGTM!The api service command change aligns with the new watch command in the justfile, enabling hot reload during development.
docker/dex/config.yaml (2)
1-5: LGTM!The server configuration correctly defines:
- Issuer URL for local development
- HTTP server on port 8080
- gRPC server on port 5557
53-59: Fix indentation of scopes.The scopes are indented with 5 spaces instead of 4 spaces.
Apply this diff to fix the indentation:
scopes: - - openid - - email - - siret - - given_name - - usual_name + - openid + - email + - siret + - given_name + - usual_name🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 54-54: wrong indentation: expected 4 but found 5
(indentation)
docker-compose.e2e.yml (1)
51-53: Port mapping inconsistency.Port 8090 is mapped to internal port 8080, but Traefik labels reference port 8080. This could lead to confusion.
Consider standardizing the port mapping:
- - 8090:8080 + - 8080:8080🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 53-53: trailing spaces
(trailing-spaces)
api/src/pdc/services/auth/dex/protofiles/api.proto (1)
246-284: LGTM! Well-structured gRPC service definition.The Dex service definition is comprehensive and well-documented, covering all necessary operations for OAuth2 client management.
| async createForOperator(operator_id: number, role?: "application") { | ||
| const uuid = v4(); | ||
| const password = randomString(); | ||
| const hash = new TextEncoder().encode(await bcrypt_hash(password)); | ||
| await (await this.getClient()).CreatePassword({ | ||
| password: { | ||
| email: uuid, | ||
| username: `${role}:${operator_id}`, | ||
| hash, | ||
| userId: uuid, | ||
| }, | ||
| }); | ||
| return { | ||
| uuid, | ||
| password, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exposing the plaintext password might be a security risk.
createForOperator returns the plaintext password. While often needed to communicate credentials to a caller, it’s good practice to ensure it’s not logged or stored unencrypted beyond initial usage. Evaluate your logging configuration or add a comment explaining why returning plaintext is acceptable.
| export async function getDexClient( | ||
| config: { | ||
| host: string; | ||
| port: number; | ||
| } & Partial<grpc.ClientInitOptions>, | ||
| ) { | ||
| const protoPath = new URL("./protofiles/api.proto", import.meta.url); | ||
| const protoFile = await Deno.readTextFile(protoPath); | ||
| // Create a gRPC client | ||
| const client = grpc.getClient<Dex>({ | ||
| root: protoFile, | ||
| serviceName: "Dex", | ||
| ...config, | ||
| }); | ||
| return client; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential file read errors or missing proto file.
Deno.readTextFile(protoPath) might throw if the file is missing or inaccessible. Consider wrapping this in a try-catch with a clear error message to diagnose misconfigurations quickly.
| dex: | ||
| environment: | ||
| PG_DB: dex |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify completeness of dex configuration.
The dex service configuration only includes the PG_DB environment variable. Consider adding other required variables like PG_USERNAME, PG_PASSWORD, and PG_HOST for consistency with the e2e configuration.
| OIDC_CLIENT_ID: secret | ||
| OIDC_CLIENT_SECRET: secret | ||
| OIDC_BASE_URL: http://auth.covoiturage.test | ||
| OIDC_REDIRECT_URL: http://api.covoiturage.test/auth/callback | ||
| DEX_GRPC_HOST: localhost | ||
| DEX_GRPC_PORT: 5557 |
There was a problem hiding this comment.
Fix DEX_GRPC_HOST configuration.
The DEX_GRPC_HOST is set to localhost which won't work correctly in Docker. It should reference the service name.
- DEX_GRPC_HOST: localhost
+ DEX_GRPC_HOST: dex📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| OIDC_CLIENT_ID: secret | |
| OIDC_CLIENT_SECRET: secret | |
| OIDC_BASE_URL: http://auth.covoiturage.test | |
| OIDC_REDIRECT_URL: http://api.covoiturage.test/auth/callback | |
| DEX_GRPC_HOST: localhost | |
| DEX_GRPC_PORT: 5557 | |
| OIDC_CLIENT_ID: secret | |
| OIDC_CLIENT_SECRET: secret | |
| OIDC_BASE_URL: http://auth.covoiturage.test | |
| OIDC_REDIRECT_URL: http://api.covoiturage.test/auth/callback | |
| - DEX_GRPC_HOST: localhost | |
| + DEX_GRPC_HOST: dex | |
| DEX_GRPC_PORT: 5557 |
| kc: | ||
| image: keycloak/keycloak:24.0 | ||
| dex: | ||
| image: bitnami/dex:2 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the Dex image version.
The image version is pinned to version 2, which may not be the latest stable version.
🌐 Web query:
What is the latest stable version of Bitnami's Dex image?
💡 Result:
The latest stable version of Bitnami's Dex container image is 2.41.1-debian-12-r8, based on upstream Dex v2.41.1[5][6]. This image includes security updates and compatibility improvements from both Bitnami's packaging and the upstream Dex project[3][5].
For deployment, you can pull it using:
docker pull bitnami/dex:2.41.1-debian-12-r8Bitnami maintains this image with their standard optimizations for container environments, including regular updates for security patches and dependency management[1][8]. The upstream Dex v2.41.1 release contains important bug fixes for LDAP connections and dependency updates[3].
Citations:
- 1: https://bitnami.com/stack/dex/containers
- 2: https://bitnami.com/stacks/containers
- 3: https://github.com/dexidp/dex/releases
- 4: https://github.com/bitnami/charts/blob/main/bitnami/argo-cd/README.md
- 5: https://hub.docker.com/r/bitnami/dex/tags
- 6: https://artifacthub.io/packages/helm/dex/dex
- 7: https://github.com/bitnami/containers
- 8: https://docker.laomai.org/r/bitnami/dex
- 9: https://artifacthub.io/packages/helm/bitnami/argo-cd/7.0.15
- 10: https://argo-cd.readthedocs.io/en/stable/user-guide/helm/
Update Dex Image Version to Latest Stable Release
The current configuration in docker-compose.e2e.yml pins the Dex image to the generic tag "2", which is outdated. Bitnami’s latest stable Dex image is now "2.41.1-debian-12-r8", offering important security patches and bug fixes. Update the image reference to ensure enhanced security and reliability.
- File:
docker-compose.e2e.yml - Line: 37 (and surrounding service configuration)
- Recommendation: Replace
with
image: bitnami/dex:2
image: bitnami/dex:2.41.1-debian-12-r8
| @@ -0,0 +1,284 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package api; | |||
There was a problem hiding this comment.
Fix package directory structure.
The file with package "api" should be within a directory "api" relative to root but is currently in "api/src/pdc/services/auth/dex/protofiles".
Consider either:
- Moving the file to the correct directory structure, or
- Updating the package name to match the current directory structure.
🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "api" must be within a directory "api" relative to root but were in directory "api/src/pdc/services/auth/dex/protofiles".
(PACKAGE_DIRECTORY_MATCH)
| async verifyToken(token: string) { | ||
| const clientId = this.config.get("oidc.client_id"); | ||
| const authBaseUrl = this.config.get("oidc.base_url"); | ||
| const { payload } = await jose.jwtVerify<{ name: string; email: string }>(token, this.getJWKS(), { | ||
| issuer: authBaseUrl, | ||
| audience: clientId, | ||
| }); | ||
| const [role, operator_id] = (payload.name || "").split(":"); | ||
| return { | ||
| operator_id: parseInt(operator_id), | ||
| role, | ||
| token_id: payload.email, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add validation for name claim and operator_id.
The token verification needs proper validation for the name claim format and operator_id parsing.
async verifyToken(token: string) {
const clientId = this.config.get("oidc.client_id");
const authBaseUrl = this.config.get("oidc.base_url");
const { payload } = await jose.jwtVerify<{ name: string; email: string }>(token, this.getJWKS(), {
issuer: authBaseUrl,
audience: clientId,
});
+ if (!payload.name || !payload.email) {
+ throw new Error('Missing required claims in token');
+ }
const [role, operator_id] = (payload.name || "").split(":");
+ if (!role || !operator_id) {
+ throw new Error('Invalid name claim format');
+ }
+ const parsedOperatorId = parseInt(operator_id);
+ if (isNaN(parsedOperatorId)) {
+ throw new Error('Invalid operator_id');
+ }
return {
- operator_id: parseInt(operator_id),
+ operator_id: parsedOperatorId,
role,
token_id: payload.email,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async verifyToken(token: string) { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const { payload } = await jose.jwtVerify<{ name: string; email: string }>(token, this.getJWKS(), { | |
| issuer: authBaseUrl, | |
| audience: clientId, | |
| }); | |
| const [role, operator_id] = (payload.name || "").split(":"); | |
| return { | |
| operator_id: parseInt(operator_id), | |
| role, | |
| token_id: payload.email, | |
| }; | |
| } | |
| async verifyToken(token: string) { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const { payload } = await jose.jwtVerify<{ name: string; email: string }>(token, this.getJWKS(), { | |
| issuer: authBaseUrl, | |
| audience: clientId, | |
| }); | |
| if (!payload.name || !payload.email) { | |
| throw new Error('Missing required claims in token'); | |
| } | |
| const [role, operator_id] = payload.name.split(":"); | |
| if (!role || !operator_id) { | |
| throw new Error('Invalid name claim format'); | |
| } | |
| const parsedOperatorId = parseInt(operator_id); | |
| if (isNaN(parsedOperatorId)) { | |
| throw new Error('Invalid operator_id'); | |
| } | |
| return { | |
| operator_id: parsedOperatorId, | |
| role, | |
| token_id: payload.email, | |
| }; | |
| } |
| async init(): Promise<void> { | ||
| this.getJWKS(); | ||
| } | ||
|
|
||
| protected getJWKS() { | ||
| if (this.JWKS) { | ||
| return this.JWKS; | ||
| } | ||
| const authBaseUrl = this.config.get("oidc.base_url"); | ||
| this.JWKS = jose.createRemoteJWKSet(new URL(`${authBaseUrl}/keys`)); | ||
| return this.JWKS; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JWKS retrieval.
The JWKS retrieval could fail due to network issues or invalid responses. Add error handling to gracefully handle these scenarios.
protected getJWKS() {
if (this.JWKS) {
return this.JWKS;
}
const authBaseUrl = this.config.get("oidc.base_url");
- this.JWKS = jose.createRemoteJWKSet(new URL(`${authBaseUrl}/keys`));
- return this.JWKS;
+ try {
+ this.JWKS = jose.createRemoteJWKSet(new URL(`${authBaseUrl}/keys`));
+ return this.JWKS;
+ } catch (error) {
+ throw new Error(`Failed to retrieve JWKS: ${error.message}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async init(): Promise<void> { | |
| this.getJWKS(); | |
| } | |
| protected getJWKS() { | |
| if (this.JWKS) { | |
| return this.JWKS; | |
| } | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| this.JWKS = jose.createRemoteJWKSet(new URL(`${authBaseUrl}/keys`)); | |
| return this.JWKS; | |
| } | |
| async init(): Promise<void> { | |
| this.getJWKS(); | |
| } | |
| protected getJWKS() { | |
| if (this.JWKS) { | |
| return this.JWKS; | |
| } | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| try { | |
| this.JWKS = jose.createRemoteJWKSet(new URL(`${authBaseUrl}/keys`)); | |
| return this.JWKS; | |
| } catch (error) { | |
| throw new Error(`Failed to retrieve JWKS: ${error.message}`); | |
| } | |
| } |
| async getToken(access_key: string, secret_key: string): Promise<string> { | ||
| const clientId = this.config.get("oidc.client_id"); | ||
| const clientSecret = this.config.get("oidc.client_secret"); | ||
| const authBaseUrl = this.config.get("oidc.base_url"); | ||
| const url = new URL(authBaseUrl); | ||
| url.pathname = "/token"; | ||
| const form = new URLSearchParams(); | ||
| form.set("grant_type", "password"); | ||
| form.set("username", access_key); | ||
| form.set("password", secret_key); | ||
| form.set("scope", "openid email profile"); | ||
| const response = await fetch(url, { | ||
| body: form.toString(), | ||
| method: "POST", | ||
| headers: { | ||
| "Content-type": "application/x-www-form-urlencoded", | ||
| "Authorization": `Basic ${encodeBase64(`${clientId}:${clientSecret}`)}`, | ||
| }, | ||
| }); | ||
| const json = await response.json(); | ||
| return json.access_token; | ||
| } |
There was a problem hiding this comment.
Add error handling and request timeouts.
The token endpoint needs proper error handling, request timeouts, and token validation.
async getToken(access_key: string, secret_key: string): Promise<string> {
const clientId = this.config.get("oidc.client_id");
const clientSecret = this.config.get("oidc.client_secret");
const authBaseUrl = this.config.get("oidc.base_url");
const url = new URL(authBaseUrl);
url.pathname = "/token";
const form = new URLSearchParams();
form.set("grant_type", "password");
form.set("username", access_key);
form.set("password", secret_key);
form.set("scope", "openid email profile");
- const response = await fetch(url, {
- body: form.toString(),
- method: "POST",
- headers: {
- "Content-type": "application/x-www-form-urlencoded",
- "Authorization": `Basic ${encodeBase64(`${clientId}:${clientSecret}`)}`,
- },
- });
- const json = await response.json();
- return json.access_token;
+ try {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 5000);
+ const response = await fetch(url, {
+ body: form.toString(),
+ method: "POST",
+ headers: {
+ "Content-type": "application/x-www-form-urlencoded",
+ "Authorization": `Basic ${encodeBase64(`${clientId}:${clientSecret}`)}`,
+ },
+ signal: controller.signal
+ });
+ clearTimeout(timeout);
+ if (!response.ok) {
+ throw new Error(`Token request failed: ${response.statusText}`);
+ }
+ const json = await response.json();
+ if (!json.access_token) {
+ throw new Error('No access token in response');
+ }
+ return json.access_token;
+ } catch (error) {
+ if (error.name === 'AbortError') {
+ throw new Error('Token request timeout');
+ }
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getToken(access_key: string, secret_key: string): Promise<string> { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const clientSecret = this.config.get("oidc.client_secret"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/token"; | |
| const form = new URLSearchParams(); | |
| form.set("grant_type", "password"); | |
| form.set("username", access_key); | |
| form.set("password", secret_key); | |
| form.set("scope", "openid email profile"); | |
| const response = await fetch(url, { | |
| body: form.toString(), | |
| method: "POST", | |
| headers: { | |
| "Content-type": "application/x-www-form-urlencoded", | |
| "Authorization": `Basic ${encodeBase64(`${clientId}:${clientSecret}`)}`, | |
| }, | |
| }); | |
| const json = await response.json(); | |
| return json.access_token; | |
| } | |
| async getToken(access_key: string, secret_key: string): Promise<string> { | |
| const clientId = this.config.get("oidc.client_id"); | |
| const clientSecret = this.config.get("oidc.client_secret"); | |
| const authBaseUrl = this.config.get("oidc.base_url"); | |
| const url = new URL(authBaseUrl); | |
| url.pathname = "/token"; | |
| const form = new URLSearchParams(); | |
| form.set("grant_type", "password"); | |
| form.set("username", access_key); | |
| form.set("password", secret_key); | |
| form.set("scope", "openid email profile"); | |
| + try { | |
| + const controller = new AbortController(); | |
| + const timeout = setTimeout(() => controller.abort(), 5000); | |
| + const response = await fetch(url, { | |
| + body: form.toString(), | |
| + method: "POST", | |
| + headers: { | |
| + "Content-type": "application/x-www-form-urlencoded", | |
| + "Authorization": `Basic ${encodeBase64(`${clientId}:${clientSecret}`)}`, | |
| + }, | |
| + signal: controller.signal | |
| + }); | |
| + clearTimeout(timeout); | |
| + if (!response.ok) { | |
| + throw new Error(`Token request failed: ${response.statusText}`); | |
| + } | |
| + const json = await response.json(); | |
| + if (!json.access_token) { | |
| + throw new Error('No access token in response'); | |
| + } | |
| + return json.access_token; | |
| + } catch (error) { | |
| + if (error.name === 'AbortError') { | |
| + throw new Error('Token request timeout'); | |
| + } | |
| + throw error; | |
| + } | |
| } |
Summary by CodeRabbit
New Features
AuthRouterclass to manage authentication routes, including login and callback handling.OidcProviderclass for handling OpenID Connect authentication processes.AuthServiceProviderclass to integrate authentication services within the application.OidcCallbackActionclass to handle OIDC callback actions with improved error management.UserRepositoryclass for managing user data interactions with the database.DexClientclass for managing password operations associated with operators.Bug Fixes