Enhance configuration documentation and secrets management for AI Workspace - #2625
Enhance configuration documentation and secrets management for AI Workspace#2625Thushani-Jayasekera wants to merge 13 commits into
Conversation
Port the shared config-interpolation mechanism drafted for the gateway (PR wso2#2596) to the platform-api config loader. Any config field may now pull its value from an environment variable ({{ env "NAME" }}) or an allowlisted file ({{ file "/path" }}), resolved after the env+file merge and before unmarshal. String leaves without a "{{" token pass through unchanged, so existing token-free configs are unaffected. - Vendor common/configinterpolate from wso2#2596 (branch conf-template). This is a temporary copy to be reconciled when wso2#2596 merges to main. - Wire an interpolate() step into config.LoadConfig, mirroring the gateway-controller. File reads are restricted to a per-component allowlist (/etc/platform-api, /secrets/platform-api), overridable via the shared APIP_CONFIG_FILE_SOURCE_ALLOWLIST env var. Fails closed on a missing required env var or a disallowed/missing/oversize file; resolved values are never logged (only reference counts at info level). - Add github.com/knadh/koanf/providers/confmap for the reload step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Updated the configuration reference to clarify the use of environment variables and interpolation tokens for secrets. - Introduced a section on managing sensitive information, emphasizing the use of mounted secret files and environment variables for the OIDC client secret. - Adjusted the examples in the authentication setup to reflect the new configuration practices. - Improved consistency in naming conventions for environment variables across documentation and code. - Added tests to validate the loading of configuration values from environment variables and files, ensuring proper error handling for missing or invalid configurations.
📝 WalkthroughWalkthroughThis change standardizes Platform API and AI Workspace configuration prefixes, adds startup environment/file interpolation with fail-closed secret handling, updates BFF runtime configuration and packaging, adds portal identity/permission discovery, revises Compose wiring, and refreshes authentication and deployment documentation. ChangesConfiguration and deployment overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
portals/ai-workspace/.env.example (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the OIDC client secret comment to avoid implying a token is required.
Line 30 says
config.toml reads it with oidc_client_secret = '{{ env "APIP_AIW_OIDC_CLIENT_SECRET" }}', but the README (lines 61-63) states theAPIP_AIW_OIDC_CLIENT_SECRETenv var overridesoidc_client_secretdirectly — no{{ env }}token is needed in config.toml. The comment could mislead users into adding an unnecessary token. Consider rewording to match the README's simpler guidance.Based on learnings: In this repository's Markdown documentation, illustrative configuration snippets are allowed to omit language identifiers.
♻️ Proposed clarification
# The AI Workspace confidential-client secret, issued by your IDP. It stays # server-side and is never sent to the browser. Set this env var and the BFF -# reads it with -# oidc_client_secret = '{{ env "APIP_AIW_OIDC_CLIENT_SECRET" }}' -# In production, prefer mounting a secret file and swap that token for +# reads it directly — no {{ env }} token needed in config.toml. +# In production, prefer mounting a secret file and referencing it with # oidc_client_secret = '{{ file "/secrets/ai-workspace/oidc_client_secret" }}' # APIP_AIW_OIDC_CLIENT_SECRET=🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/.env.example` around lines 27 - 33, Update the OIDC client secret comments near APIP_AIW_OIDC_CLIENT_SECRET to state that the environment variable directly overrides oidc_client_secret, without instructing users to add an {{ env }} token in config.toml. Retain the production mounted-secret-file guidance and clarify that the variable is optional.Source: Learnings
portals/ai-workspace/README.md (1)
151-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify that
make bff-runforwardsPLATFORM_API_URLtoAPIP_AIW_PLATFORM_API_URL. The README uses both names for the same setting; a short note would avoid confusion between the Makefile flag and the BFF env var.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/README.md` around lines 151 - 152, Update the README guidance around PLATFORM_API_URL and make bff-run to explicitly state that make bff-run forwards PLATFORM_API_URL to the BFF environment variable APIP_AIW_PLATFORM_API_URL, clarifying that both names represent the same setting.docs/ai-workspace/configuration.md (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the "not prefixed" wording for
APIP_AIW_CONFIG_FILE.The sentence says three variables are "deliberately not prefixed," but
APIP_AIW_CONFIG_FILEvisibly carries theAPIP_AIW_prefix. The intended meaning is that these variables are not derived from config keys via the prefix-stripping rule, not that they lack a prefix entirely. Consider rewording to avoid confusion, e.g., "are not subject to the config-key prefix rule" instead of "are deliberately not prefixed."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ai-workspace/configuration.md` at line 14, Clarify the sentence describing APIP_DEMO_MODE, APIP_AIW_CONFIG_FILE, and APIP_CONFIG_FILE_SOURCE_ALLOWLIST by replacing “deliberately not prefixed” with wording that they are not subject to the config-key prefix/derivation rule, while preserving the explanation of their distinct roles and unprefixed {{ env "NAME" }} tokens.portals/ai-workspace/Makefile (1)
55-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuecheckmake:
bff-runrecipe exceeds the 5-line body-length lint threshold (7 lines).Purely a lint nit; the extra lines are just per-variable env overrides for readability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/Makefile` around lines 55 - 61, Reduce the bff-run recipe body below checkmake’s five-line limit by consolidating the per-variable environment overrides while preserving the existing directory, environment values, and go run command.Source: Linters/SAST tools
portals/ai-workspace/Dockerfile (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop
-mod=modfromGOFLAGS.bff/go.sumalready covers the external deps, andcommon/httpkitare brought in via localreplacedirectives, so the build can use the default readonly mode without mutating module files during image builds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/Dockerfile` at line 34, Update the Dockerfile’s Go environment declaration by removing the -mod=mod setting from GOFLAGS, while preserving CGO_ENABLED=0 and GOWORK=off so image builds use the default readonly module behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@distribution/all-in-one/.env.example`:
- Around line 9-13: Add an env_file configuration for the platform-api service
in the Docker Compose setup, pointing to the environment file that supplies
AUTH_JWT_SECRET_KEY and ENCRYPTION_KEY. Keep the existing token-resolution
behavior and avoid hardcoding secret values in docker-compose.yaml.
In `@distribution/all-in-one/docker-compose.yaml`:
- Around line 65-77: Add the missing unprefixed AUTH_JWT_SECRET_KEY and
ENCRYPTION_KEY values to the platform-api service in the Docker Compose
configuration, using the documented .env injection approach via env_file or
compose-level environment substitution. Ensure both variables are available
inside the container for the existing config validation to succeed, while
preserving the current APIP_CP_* environment entries.
In `@docs/ai-workspace/configuration.md`:
- Line 44: Update the configuration documentation and related quick-start or
cross-document references to use the platform_api_url key consistently,
replacing platform_api_base_url where it refers to the BFF’s config.toml
setting. Preserve platform_api_base_url only for runtime/browser-facing usage.
In `@docs/ai-workspace/features/secrets-management.md`:
- Line 337: Restructure the sentence spanning the preceding line and the bold
fragment so “required and never auto-generated” has a clear subject and the
Platform API failure condition reads as a complete sentence. Remove the dangling
colon or reposition punctuation as needed while preserving the existing meaning.
In `@portals/ai-workspace/bff/internal/config/config.go`:
- Line 218: Validate authMode immediately after it is loaded in the
configuration flow, allowing only "basic" and "oidc"; return a clear
invalid-value error for any other value before demo-mode handling or
buildRuntimeConfig exposes it. Apply the same validation consistently to the
related auth-mode handling at the other referenced locations, while preserving
the existing behavior for valid values.
- Around line 293-297: Validate oidc_post_logout_redirect_url during startup
configuration loading, alongside the existing platform_api_url validation, and
reject any non-empty value that is not an absolute URL. Preserve the empty
default behavior while ensuring invalid relative values fail before runtime
logout handling.
In `@portals/ai-workspace/bff/internal/config/settings.go`:
- Around line 107-138: Update parseFlatTOML to remove trailing inline comments
from each value before unquoting and storing it, while preserving # characters
inside quoted values such as URLs where applicable. Ensure platform_api_url and
other string settings receive only the configured value, without the comment
suffix.
In `@portals/ai-workspace/configs/config-template.toml`:
- Line 87: Update the oidc_username_claim default in the configuration template
to match the frontend’s existing OIDC_USERNAME_CLAIM default of given_name,
keeping the template’s environment-variable interpolation and commented
configuration behavior unchanged.
In `@portals/ai-workspace/src/config.env.ts`:
- Around line 192-196: Update OIDC_USERNAME_CLAIM to use the template’s default
value, username, and correct the AUTH_MODE comment to state that basic is the
default. Keep the existing environment variable names and auth-mode type
unchanged.
---
Nitpick comments:
In `@docs/ai-workspace/configuration.md`:
- Line 14: Clarify the sentence describing APIP_DEMO_MODE, APIP_AIW_CONFIG_FILE,
and APIP_CONFIG_FILE_SOURCE_ALLOWLIST by replacing “deliberately not prefixed”
with wording that they are not subject to the config-key prefix/derivation rule,
while preserving the explanation of their distinct roles and unprefixed {{ env
"NAME" }} tokens.
In `@portals/ai-workspace/.env.example`:
- Around line 27-33: Update the OIDC client secret comments near
APIP_AIW_OIDC_CLIENT_SECRET to state that the environment variable directly
overrides oidc_client_secret, without instructing users to add an {{ env }}
token in config.toml. Retain the production mounted-secret-file guidance and
clarify that the variable is optional.
In `@portals/ai-workspace/Dockerfile`:
- Line 34: Update the Dockerfile’s Go environment declaration by removing the
-mod=mod setting from GOFLAGS, while preserving CGO_ENABLED=0 and GOWORK=off so
image builds use the default readonly module behavior.
In `@portals/ai-workspace/Makefile`:
- Around line 55-61: Reduce the bff-run recipe body below checkmake’s five-line
limit by consolidating the per-variable environment overrides while preserving
the existing directory, environment values, and go run command.
In `@portals/ai-workspace/README.md`:
- Around line 151-152: Update the README guidance around PLATFORM_API_URL and
make bff-run to explicitly state that make bff-run forwards PLATFORM_API_URL to
the BFF environment variable APIP_AIW_PLATFORM_API_URL, clarifying that both
names represent the same setting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7264d68-5fd3-41a8-a233-be5bbf221fb1
⛔ Files ignored due to path filters (2)
platform-api/go.sumis excluded by!**/*.sumportals/ai-workspace/bff/go.sumis excluded by!**/*.sum
📒 Files selected for processing (50)
distribution/all-in-one/.env.exampledistribution/all-in-one/docker-compose.yamldocs/ai-workspace/authentication/asgardeo-setup.mddocs/ai-workspace/authentication/oidc-auth.mddocs/ai-workspace/configuration.mddocs/ai-workspace/features/secrets-management.mdplatform-api/README.mdplatform-api/config/config.goplatform-api/config/config.tomlplatform-api/config/config_test.goplatform-api/go.modplatform-api/internal/server/server.goportals/ai-workspace/.env.exampleportals/ai-workspace/Dockerfileportals/ai-workspace/Makefileportals/ai-workspace/README.mdportals/ai-workspace/bff/go.modportals/ai-workspace/bff/internal/config/config.goportals/ai-workspace/bff/internal/config/config_test.goportals/ai-workspace/bff/internal/config/runtime_config.goportals/ai-workspace/bff/internal/config/settings.goportals/ai-workspace/bff/internal/config/toml_mapping.goportals/ai-workspace/bff/internal/proxy/reverse_proxy_test.goportals/ai-workspace/bff/internal/proxy/transport.goportals/ai-workspace/bff/internal/server/composite_handlers_test.goportals/ai-workspace/bff/main.goportals/ai-workspace/configs/config-platform-api-template.tomlportals/ai-workspace/configs/config-platform-api.tomlportals/ai-workspace/configs/config-template.tomlportals/ai-workspace/configs/config.tomlportals/ai-workspace/docker-compose.yamlportals/ai-workspace/production/README.mdportals/ai-workspace/src/apis/platformApis.tsportals/ai-workspace/src/auth/logout.tsportals/ai-workspace/src/clients/choreoApiClient.tsportals/ai-workspace/src/config.env.tsportals/ai-workspace/src/contexts/ChoreoUserContext.tsxportals/ai-workspace/src/utils/logger.tsportals/ai-workspace/vite.config.tsportals/developer-portal/.env.exampleportals/developer-portal/configs/config-platform-api.toml.exampleportals/developer-portal/distribution/.env.exampleportals/developer-portal/distribution/docker-compose.yamlportals/developer-portal/docker-compose.platform-api.yamlportals/developer-portal/docker-compose.yamlportals/developer-portal/it/docker-compose.test.postgres.yamlportals/developer-portal/it/docker-compose.test.yamltests/integration-e2e/docker-compose.sqlite.yamltests/integration-e2e/docker-compose.sqlserver.yamltests/integration-e2e/docker-compose.yaml
💤 Files with no reviewable changes (4)
- portals/developer-portal/docker-compose.yaml
- portals/ai-workspace/bff/internal/config/toml_mapping.go
- portals/developer-portal/docker-compose.platform-api.yaml
- portals/developer-portal/distribution/docker-compose.yaml
| # AUTH_JWT_SECRET_KEY and ENCRYPTION_KEY are injected into the Platform API | ||
| # container (env_file) and resolved by its config via {{ env "..." }} tokens. | ||
| # AUTH_JWT_SECRET_KEY is also shared with the devportal service so it can | ||
| # verify the Platform API's login JWTs. Never hardcode secret values in | ||
| # docker-compose.yaml. For production, prefer secret files and {{ file "..." }}. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for env_file directive in the all-in-one docker-compose.yaml
rg -n 'env_file' distribution/all-in-one/docker-compose.yaml
# Also check the full platform-api service block
sed -n '/^ platform-api:/,/^ [a-z]/p' distribution/all-in-one/docker-compose.yamlRepository: wso2/api-platform
Length of output: 1308
Add env_file for platform-api
AUTH_JWT_SECRET_KEY and ENCRYPTION_KEY are documented as env-file inputs, but platform-api has no env_file here. Without it, the {{ env "..." }} lookups fail and the service cannot start.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@distribution/all-in-one/.env.example` around lines 9 - 13, Add an env_file
configuration for the platform-api service in the Docker Compose setup, pointing
to the environment file that supplies AUTH_JWT_SECRET_KEY and ENCRYPTION_KEY.
Keep the existing token-resolution behavior and avoid hardcoding secret values
in docker-compose.yaml.
| environment: | ||
| - DEFAULT_DEVPORTAL_API_URL=http://devportal:3001 | ||
| - DATABASE_DRIVER=postgres | ||
| - DATABASE_HOST=postgres | ||
| - DATABASE_PORT=5432 | ||
| - DATABASE_NAME=platform_api | ||
| - DATABASE_USER=postgres | ||
| - DATABASE_PASSWORD=postgres | ||
| - DATABASE_SSL_MODE=disable | ||
| - DATABASE_MAX_OPEN_CONNS=25 | ||
| - DATABASE_MAX_IDLE_CONNS=10 | ||
| - DATABASE_CONN_MAX_LIFETIME=300 | ||
| - DATABASE_EXECUTE_SCHEMA_DDL=true | ||
| - AUTH_JWT_SECRET_KEY=${AUTH_JWT_SECRET_KEY:-} | ||
| - ENCRYPTION_KEY=${ENCRYPTION_KEY:-} | ||
| - APIP_CP_DEFAULT_DEVPORTAL_API_URL=http://devportal:3001 | ||
| - APIP_CP_DATABASE_DRIVER=postgres | ||
| - APIP_CP_DATABASE_HOST=postgres | ||
| - APIP_CP_DATABASE_PORT=5432 | ||
| - APIP_CP_DATABASE_NAME=platform_api | ||
| - APIP_CP_DATABASE_USER=postgres | ||
| - APIP_CP_DATABASE_PASSWORD=postgres | ||
| - APIP_CP_DATABASE_SSL_MODE=disable | ||
| - APIP_CP_DATABASE_MAX_OPEN_CONNS=25 | ||
| - APIP_CP_DATABASE_MAX_IDLE_CONNS=10 | ||
| - APIP_CP_DATABASE_CONN_MAX_LIFETIME=300 | ||
| - APIP_CP_DATABASE_EXECUTE_SCHEMA_DDL=true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Missing AUTH_JWT_SECRET_KEY and ENCRYPTION_KEY will cause platform-api startup failure.
The AUTH_JWT_SECRET_KEY and ENCRYPTION_KEY entries were removed from the platform-api environment: list, but no env_file: directive is present to inject them from .env. The platform-api's default config uses {{ env "ENCRYPTION_KEY" }} and {{ env "AUTH_JWT_SECRET_KEY" }} interpolation tokens, which require these unprefixed env vars to be available inside the container. Without them, validateEncryptionKey and validateJWTConfig in config.go will fail startup.
The .env.example documents that these are injected via env_file, so either an env_file: .env directive needs to be added to the platform-api service, or the variables need to be passed via environment: using compose-level substitution:
🔧 Proposed fix — add env_file or environment entries
Option A: Add env_file to platform-api service (as documented in .env.example):
volumes:
- platform-api-data:/api-platform/data
+ env_file:
+ - .env
environment:
- APIP_CP_DEFAULT_DEVPORTAL_API_URL=http://devportal:3001Option B: Pass via environment with compose substitution:
environment:
+ - AUTH_JWT_SECRET_KEY=${AUTH_JWT_SECRET_KEY:-}
+ - ENCRYPTION_KEY=${ENCRYPTION_KEY:-}
- APIP_CP_DEFAULT_DEVPORTAL_API_URL=http://devportal:3001🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@distribution/all-in-one/docker-compose.yaml` around lines 65 - 77, Add the
missing unprefixed AUTH_JWT_SECRET_KEY and ENCRYPTION_KEY values to the
platform-api service in the Docker Compose configuration, using the documented
.env injection approach via env_file or compose-level environment substitution.
Ensure both variables are available inside the container for the existing config
validation to succeed, while preserving the current APIP_CP_* environment
entries.
| # JWT claim name mappings — which token claim carries each user/org field. Override | ||
| # only if your IDP uses different claim names. The org_* names must match the | ||
| # [auth.idp] claim names in config-platform-api.toml. | ||
| # oidc_username_claim = '{{ env "APIP_AIW_OIDC_USERNAME_CLAIM" "username" }}' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
oidc_username_claim default does not match the frontend default.
The template defaults oidc_username_claim to "username", but config.env.ts line 192 defaults OIDC_USERNAME_CLAIM to 'given_name'. Since oidc_username_claim is in the BFF's browserSafeKeys, the BFF will pass its configured value to the SPA via window.__RUNTIME_CONFIG__. However, when the key is left commented out (not in the BFF config), the SPA falls back to given_name — a different claim name than the template suggests. Align the defaults to avoid silent claim-name mismatches.
🔧 Proposed fix — align the template default with the frontend default
-# oidc_username_claim = '{{ env "APIP_AIW_OIDC_USERNAME_CLAIM" "username" }}'
+# oidc_username_claim = '{{ env "APIP_AIW_OIDC_USERNAME_CLAIM" "given_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.
| # oidc_username_claim = '{{ env "APIP_AIW_OIDC_USERNAME_CLAIM" "username" }}' | |
| # oidc_username_claim = '{{ env "APIP_AIW_OIDC_USERNAME_CLAIM" "given_name" }}' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/ai-workspace/configs/config-template.toml` at line 87, Update the
oidc_username_claim default in the configuration template to match the
frontend’s existing OIDC_USERNAME_CLAIM default of given_name, keeping the
template’s environment-variable interpolation and commented configuration
behavior unchanged.
- Restructured the configuration in `config.toml` to group keys into TOML tables for better organization and clarity. - Updated the documentation to reflect the new configuration format, emphasizing the use of interpolation tokens for environment variables and secrets. - Clarified the handling of OIDC settings, including client ID and secret, ensuring they are referenced correctly in the configuration. - Improved consistency in naming conventions for environment variables across documentation and code. - Added tests to validate the loading of configuration values from environment variables and files, ensuring proper error handling for missing or invalid configurations.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
portals/ai-workspace/bff/internal/config/settings.go (1)
127-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
flattensilently drops any value that isn't a string/bool/int64/float64.TOML datetime literals (decoded as
time.Time) and arrays fall through theswitchand vanish fromsettings, so the key silently reverts to its default instead of surfacing a config error. Only arrays are documented as intentionally skipped. Consider handlingtime.Time(or logging/erroring on unexpected non-scalar types) if any such key is ever added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/bff/internal/config/settings.go` around lines 127 - 134, Update flatten to explicitly handle time.Time values and preserve them in settings using the expected string representation; keep arrays intentionally skipped, but ensure other unsupported value types do not disappear silently by surfacing a configuration error or diagnostic instead of falling through unnoticed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@portals/ai-workspace/bff/internal/config/settings.go`:
- Around line 127-134: Update flatten to explicitly handle time.Time values and
preserve them in settings using the expected string representation; keep arrays
intentionally skipped, but ensure other unsupported value types do not disappear
silently by surfacing a configuration error or diagnostic instead of falling
through unnoticed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 509313dc-2392-4ec6-8f0d-6bf662c98044
⛔ Files ignored due to path filters (1)
portals/ai-workspace/bff/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
docs/ai-workspace/authentication/asgardeo-setup.mddocs/ai-workspace/authentication/oidc-auth.mddocs/ai-workspace/configuration.mdportals/ai-workspace/.env.exampleportals/ai-workspace/Dockerfileportals/ai-workspace/Makefileportals/ai-workspace/README.mdportals/ai-workspace/bff/go.modportals/ai-workspace/bff/internal/config/config.goportals/ai-workspace/bff/internal/config/config_test.goportals/ai-workspace/bff/internal/config/runtime_config.goportals/ai-workspace/bff/internal/config/settings.goportals/ai-workspace/bff/main.goportals/ai-workspace/configs/config-template.tomlportals/ai-workspace/configs/config.tomlportals/ai-workspace/docker-compose.yamlportals/ai-workspace/production/README.mdportals/ai-workspace/src/config.env.tsportals/ai-workspace/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- docs/ai-workspace/authentication/asgardeo-setup.md
- portals/ai-workspace/bff/go.mod
- portals/ai-workspace/bff/internal/config/runtime_config.go
- portals/ai-workspace/.env.example
- portals/ai-workspace/vite.config.ts
- portals/ai-workspace/Dockerfile
- portals/ai-workspace/docker-compose.yaml
- docs/ai-workspace/configuration.md
- portals/ai-workspace/configs/config-template.toml
- portals/ai-workspace/README.md
- portals/ai-workspace/src/config.env.ts
- Updated the secrets management documentation to clarify the required environment variable for the Platform API encryption. - Modified the Dockerfile to remove unnecessary GOFLAGS, streamlining the build process. - Improved error handling in the configuration loading logic to ensure invalid authentication modes are properly reported. - Added tests for the shipped configuration to validate loading behavior with and without environment variables, ensuring correct defaults and overrides. - Adjusted the OIDC claim mappings in the configuration to align with the BFF defaults, enhancing consistency across components.
Dependency Validation ResultsDependency name: github.com/knadh/koanf/providers/confmap Dependency name: github.com/pelletier/go-toml/v2 Next Steps
|
- Implemented the `/me` endpoint to return the signed-in user's identity, roles, and effective permissions. - Created `MeHandler` to manage the retrieval of user information and associated roles/scopes. - Added tests for the `GetMe` functionality to ensure correct reporting of roles and scopes based on the user's token. - Updated the OpenAPI documentation to include the new endpoint and its expected behavior. - Enhanced middleware to support role and scope claims extraction for improved authorization handling.
Dependency Validation ResultsDependency name: github.com/knadh/koanf/providers/confmap Dependency name: github.com/pelletier/go-toml/v2 Next Steps
|
1 similar comment
Dependency Validation ResultsDependency name: github.com/knadh/koanf/providers/confmap Dependency name: github.com/pelletier/go-toml/v2 Next Steps
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
portals/ai-workspace/bff/internal/server/permissions.go (1)
36-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHardcoded response-size ceiling should be configurable.
maxMeResponseBytesis a fixed constant used to bound the/meresponse read. As per coding guidelines, byte limits on network-originated readers should be sourced from configuration with a safe default rather than hardcoded.As per coding guidelines:
Wrap every user- or network-originated io.Reader in io.LimitReader before reading it into memory. Source the byte limit from configuration (environment variable or config file), provide a safe default, and do not hardcode the ceiling.♻️ Proposed fix: source the limit from Server config with a safe default
-// maxMeResponseBytes caps how much of the /me response we will read. The payload -// is a small identity record; anything larger is an upstream fault, not something -// to pull into memory. -const maxMeResponseBytes = 1 << 20 // 1 MiB +// defaultMaxMeResponseBytes caps how much of the /me response we will read when +// no override is configured. The payload is a small identity record; anything +// larger is an upstream fault, not something to pull into memory. +const defaultMaxMeResponseBytes = 1 << 20 // 1 MiB- if err := json.NewDecoder(io.LimitReader(resp.Body, maxMeResponseBytes)).Decode(&me); err != nil { + if err := json.NewDecoder(io.LimitReader(resp.Body, s.maxMeResponseBytes())).Decode(&me); err != nil {Also applies to: 61-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/bff/internal/server/permissions.go` around lines 36 - 39, Replace the fixed maxMeResponseBytes constant with a Server configuration value for the /me response limit, including a safe default when configuration is unset or invalid. Update the /me network-reader path to pass that configured limit to io.LimitReader before reading the response into memory, including the additional usage around the referenced location.Source: Coding guidelines
portals/ai-workspace/bff/internal/config/shipped_config_test.go (1)
72-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TLSSkipVerifyoverride is set but never asserted.
APIP_AIW_PLATFORM_API_TLS_SKIP_VERIFY=trueis set (Line 75) but the assertion table never checkscfg.PlatformAPI.TLSSkipVerify. Since the quickstart default is alreadytrue, this also wouldn't catch a broken override even if asserted — flip the env value tofalseand add the check to actually prove the{{ env }}token works for this field.✅ Proposed fix
t.Setenv("APIP_AIW_PLATFORM_API_URL", "https://localhost:9243") - t.Setenv("APIP_AIW_PLATFORM_API_TLS_SKIP_VERIFY", "true") + t.Setenv("APIP_AIW_PLATFORM_API_TLS_SKIP_VERIFY", "false") t.Setenv("APIP_AIW_LISTEN_ADDR", ":8081") t.Setenv("APIP_AIW_STATIC_DIR", "../dist") t.Setenv("APIP_AIW_LOG_LEVEL", "debug") cfg, err := Load(quickstartConfig) if err != nil { t.Fatalf("Load(configs/config.toml) error = %v", err) } + if cfg.PlatformAPI.TLSSkipVerify { + t.Error("PlatformAPI.TLSSkipVerify = true, want false — the env override did not take effect") + } for _, tc := range []struct{ name, got, want string }{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/bff/internal/config/shipped_config_test.go` around lines 72 - 96, Update TestShippedConfig_MakeBffRunOverrides to set APIP_AIW_PLATFORM_API_TLS_SKIP_VERIFY to false instead of true, then add an assertion for cfg.PlatformAPI.TLSSkipVerify expecting false so the environment override is verified rather than masked by the default.platform-api/internal/middleware/auth.go (1)
627-639: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMove these test-only auth helpers out of
platform-api/internal/middleware/auth.go
WithUserID,WithOrganization,WithScope, andWithRolesare only used from*_test.go, but they still ship in the non-test middleware package and are callable from any code inplatform-api/internal/middleware. A dedicated test-only helper package orexport_test.go-style file would keep them out of the production auth surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/middleware/auth.go` around lines 627 - 639, Move the test-only helpers WithUserID, WithOrganization, WithScope, and WithRoles out of the production auth.go implementation and into an export_test.go-style file or dedicated test helper package. Preserve their existing context injection behavior and update test references so production middleware no longer exposes these helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/handler/me.go`:
- Around line 74-88: Update the user ID extraction in the me handler to retain
and validate its ok result from GetUserIDFromRequest; if extraction fails, deny
access and stop before constructing or returning the meResponse. Keep the
existing username, email, roles, and scopes handling unchanged.
---
Nitpick comments:
In `@platform-api/internal/middleware/auth.go`:
- Around line 627-639: Move the test-only helpers WithUserID, WithOrganization,
WithScope, and WithRoles out of the production auth.go implementation and into
an export_test.go-style file or dedicated test helper package. Preserve their
existing context injection behavior and update test references so production
middleware no longer exposes these helpers.
In `@portals/ai-workspace/bff/internal/config/shipped_config_test.go`:
- Around line 72-96: Update TestShippedConfig_MakeBffRunOverrides to set
APIP_AIW_PLATFORM_API_TLS_SKIP_VERIFY to false instead of true, then add an
assertion for cfg.PlatformAPI.TLSSkipVerify expecting false so the environment
override is verified rather than masked by the default.
In `@portals/ai-workspace/bff/internal/server/permissions.go`:
- Around line 36-39: Replace the fixed maxMeResponseBytes constant with a Server
configuration value for the /me response limit, including a safe default when
configuration is unset or invalid. Update the /me network-reader path to pass
that configured limit to io.LimitReader before reading the response into memory,
including the additional usage around the referenced location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d484797c-00e7-4178-beed-35b8dc8e1a3e
📒 Files selected for processing (15)
docs/ai-workspace/features/secrets-management.mdplatform-api/internal/handler/me.goplatform-api/internal/handler/me_test.goplatform-api/internal/middleware/auth.goplatform-api/internal/middleware/authorization.goplatform-api/internal/server/server.goplatform-api/resources/portal-api.yamlportals/ai-workspace/Dockerfileportals/ai-workspace/README.mdportals/ai-workspace/bff/internal/config/config.goportals/ai-workspace/bff/internal/config/shipped_config_test.goportals/ai-workspace/bff/internal/server/handlers.goportals/ai-workspace/bff/internal/server/permissions.goportals/ai-workspace/bff/internal/server/permissions_test.goportals/ai-workspace/src/config.env.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/ai-workspace/features/secrets-management.md
- platform-api/internal/server/server.go
- portals/ai-workspace/Dockerfile
- portals/ai-workspace/bff/internal/config/config.go
- portals/ai-workspace/README.md
- portals/ai-workspace/src/config.env.ts
- Removed the `github.com/pelletier/go-toml/v2` dependency from the project as it is no longer needed. - Introduced a custom `decodeTOML` function to handle TOML parsing directly, supporting a subset of TOML grammar for configuration files. - Updated the `settings.go` file to utilize the new TOML decoding logic, enhancing configuration loading and error handling. - Added comprehensive tests for the new TOML decoding functionality to ensure correctness and robustness.
Dependency Validation ResultsDependency name: github.com/knadh/koanf/providers/confmap |
- Updated the GetMe handler to fail closed when no user ID is found in the token, improving security. - Introduced MaxMeResponseBytes configuration to limit the size of the /me response read into memory, with a fallback to a safe default. - Added a new utility function for parsing int64 configuration values, ensuring robust handling of malformed or unset values. - Updated tests to reflect changes in the handling of user identity and configuration settings.
- Introduced tests to verify that the MaxMeResponseBytes configuration is correctly loaded from the config file. - Added checks to ensure that defaults are applied when the configuration is absent or invalid. - Enhanced the permissions enrichment test to respect the configured response limit, ensuring proper handling of oversized responses.
Dependency Validation ResultsDependency name: github.com/knadh/koanf/providers/confmap |
2 similar comments
Dependency Validation ResultsDependency name: github.com/knadh/koanf/providers/confmap |
Dependency Validation ResultsDependency name: github.com/knadh/koanf/providers/confmap |
2 participants
AI Workspace BFF — Configuration Convention & the
/meEndpointThis note captures two related pieces of the AI Workspace BFF: the restructured
configuration model, and the
GET /mepermissions hop the BFF makes to thePlatform API. They are documented together because the
/mecall exists toresolve a gap that the configuration model deliberately refuses to paper over.
1. Configuration restructuring
1.1 One source of truth:
config.tomlThe BFF loads all configuration from a single
config.toml(
internal/config/settings.go,config.go). There is no implicit environmentoverlay on top of the file — a key takes its value from the environment or a
mounted secret only when its own interpolation token says so:
Consequences of this design:
than implied by a naming rule or a precedence ladder. If a key has no token, it
simply cannot be set from the environment.
secret is resolved the same way as
log_level— via a{{ env }}/{{ file }}token — so there is no separate secret-loading path.
{{ env }}variable with no default,or an unreadable/disallowed
{{ file }}path, aborts startup rather thansilently yielding an empty credential.
Tokens are resolved by the shared
common/configinterpolatelibrary.{{ file }}reads are constrained to an allowlist (
/etc/ai-workspace,/secrets/ai-workspaceby default, overridable via
APIP_CONFIG_FILE_SOURCE_ALLOWLIST).1.2 Naming convention:
APIP_AIW_The AI Workspace namespaces its environment variables with the
APIP_AIW_prefix — mirroring the Platform API's
APIP_CP_and the Developer Portal'sAPIP_DP_. The convention is: a key's variable is its dotted config path,uppercased, dots→underscores, prefixed.
Crucially this is a convention, not a binding — the prefix documents the
expected variable name, but the environment only reaches config through the
explicit
{{ env "NAME" }}token. A token may name any variable.1.3 Flattened, typed access
The decoded TOML tree is flattened to dotted keys (
[platform_api] url→platform_api.url). Values are read through small typed accessors that decidewhat a bad value should do:
get/getbool/getdur— a malformed boolean or duration failsstartup rather than silently reverting to the default (a typo shouldn't
quietly change behaviour).
getint64— used for defensive size ceilings (e.g.MaxMeResponseBytes); anunset, malformed, or non-positive value degrades to the safe default
instead of blocking startup, because a size bound is a guard rail, not a
correctness knob.
auth_modeis validated explicitly: anything other thanbasicoroidcfailsstartup, so a typo can't silently degrade to basic auth.
1.4 Browser-safe runtime config (allowlist, not filter)
The SPA reads a subset of config from
window.__RUNTIME_CONFIG__. The set ofkeys that reach the browser is an allowlist (
browserSafeKeysinruntime_config.go), not a filter:an upstream URL, a cookie setting — cannot leak into the browser merely by
being added to
config.toml.[oidc] client_secret/client_id/authorityare deliberately absent: theBFF performs the entire OIDC handshake, so the SPA needs no client identity.
the browser only ever talks to the BFF, never the Platform API directly.
The runtime key spelling (
runtimeKey) matches the{{ env }}token spelling, soa value has one name across
config.toml, the environment, Vite'simport.meta.env, andwindow.__RUNTIME_CONFIG__.1.5 Claim-mapping mirror
[oidc.claim_mappings]deliberately mirrors the Platform API's[auth.idp.claim_mappings]key for key. Both sides describe the same IDPtoken, so they must agree — naming them identically makes drift obvious. The same
config entry drives both the BFF's session mapping and the SPA's identity display,
keeping the two layers in sync from one source.
2. The
GET /meendpoint2.1 What it is
internal/server/permissions.gocalls the Platform API endpoint:It returns the signed-in user's effective identity and permissions — the
roles they hold and the scopes those roles grant:
The BFF issues this call on the user's behalf (forwarding their JWT) and folds the
result into the server-side session.
2.2 Why it matters — the problem it solves
The SPA gates every UI control on
hasPermission(scope). So the session mustcarry a scope list. But there is a mode where the token does not provide one:
The Platform API already knows how to expand roles into scopes — it does this via
roles.yamlfor its own authorization decisions.GET /mereturns that sameexpansion. This is the important part:
second copy of the role→scope mapping shipped to the browser.
Platform API, the workspace UI follows automatically — nothing to re-ship.
2.3 How it behaves (
enrichPermissions)/mecall. The enrichment only runs to filla gap the token left.
/melookup fails, the user is left withno scopes — the app hides privileged controls rather than offering actions
the API would then reject. It logs a warning and continues; it does not block
login.
platform_roleclaim, so thedisplayed role label falls back to the first IDP role from
/me.2.4 Defensive bound:
MaxMeResponseBytesThe
/mepayload is a small identity record, so the response read is capped(
platform_api.max_me_response_bytes, default 1 MiB) viaio.LimitReader.Anything larger is treated as an upstream fault, not something to buffer into
memory. Per §1.3, a non-positive or malformed configured limit falls back to the
safe default rather than failing startup.
3. How the two connect
/medoesgetint64degrades to safe defaultMaxMeResponseBytesThe through-line: a single authoritative source, resolved at the layer that
enforces it, never re-implemented in the browser. The configuration model keeps
authorization data off the client;
/mesupplies it on demand from the servicethat owns it.