Move S3 config from region to onyxia-web - #1082
Open
garronej wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughChangesS3 configuration centralization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Environment as env.S3
participant App as App
participant Bootstrap as bootstrap
participant Profiles as S3 profile management
participant UI as LeftBar and home Page
Environment->>App: provide parsed S3 configuration
App->>Bootstrap: pass s3Config
Bootstrap->>Profiles: expose bootstrap S3 entries
Profiles->>Profiles: resolve bookmarks and STS roles
UI->>Profiles: query S3 Explorer availability
Profiles-->>UI: return enabled state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web/src/core/ports/OnyxiaApi/S3Config.ts (1)
48-69: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
.strict()onzClaimFilterandzOidcConfigurationShapeto catch typos.
zClaimFilter's two object branches andzOidcConfigurationShapeare all-optional or loosely matched, andz.object()strips unknown keys by default instead of rejecting them. A typo such as a wrong field name is accepted silently and the mistyped value disappears without any validation error.web/scripts/unyamlify-env-local.tsdemonstrates this exact failure mode withissuerURI/clientIDinstead ofissuerUri/clientId.Add
.strict()to these object schemas so unexpected or misspelled keys throw during environment parsing instead of being dropped silently.♻️ Proposed fix
const zClaimFilter = z.union([ - z.object({ - claimName: z.undefined().optional() - }), - z.object({ + z.object({ + claimName: z.undefined().optional() + }).strict(), + z.object({ claimName: z.string(), includedClaimPattern: z.string().optional(), excludedClaimPattern: z.string().optional() - }) + }).strict() ]);const zOidcConfigurationShape = z.object({ issuerUri: z.string().optional(), clientId: z.string().optional(), extraQueryParams_raw: z.string().optional(), scope_spaceSeparated: z.string().optional(), idleSessionLifetimeInSeconds: z.number().optional() - }); + }).strict();🤖 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 `@web/src/core/ports/OnyxiaApi/S3Config.ts` around lines 48 - 69, Apply .strict() to both object branches within zClaimFilter and to zOidcConfigurationShape so unknown or misspelled keys are rejected during validation rather than stripped. Leave the surrounding zOidcConfiguration custom validation and defined fields unchanged.
🤖 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 `@web/scripts/unyamlify-env-local.ts`:
- Around line 48-53: Update the OIDC template fields in unyamlify-env-local.ts
to use the S3Config schema names issuerUri and clientId instead of issuerURI and
clientID, preserving the existing issuer and client values.
In `@web/src/core/ports/OnyxiaApi/S3Config.ts`:
- Around line 87-100: Resolve the S3 monitoring configuration mismatch by either
adding the monitoring field consistently to S3Config_UserProvided and
S3Config_Parsed.Entry and propagating it through s3Config_userProvidedToParsed,
or removing S3.monitoring.URLPattern from unyamlify-env-local.ts if unsupported.
Ensure the chosen behavior prevents the field from being silently stripped.
In `@web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts`:
- Around line 188-193: Ensure the S3 profile aggregation flow skips entries
whose resolved STS configuration contains no roles before parsing or asserting
on it. Update the logic around resolvedTemplatedStsRoles and
aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet to require at least one
valid stsRoles entry, removing or excluding unresolved entries while preserving
valid profile aggregation.
---
Nitpick comments:
In `@web/src/core/ports/OnyxiaApi/S3Config.ts`:
- Around line 48-69: Apply .strict() to both object branches within zClaimFilter
and to zOidcConfigurationShape so unknown or misspelled keys are rejected during
validation rather than stripped. Leave the surrounding zOidcConfiguration custom
validation and defined fields unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 74665204-c19a-40dc-96e1-ebe72af5b75f
📒 Files selected for processing (21)
web/.envweb/scripts/unyamlify-env-local.tsweb/src/core/adapters/onyxiaApi/ApiTypes.tsweb/src/core/adapters/onyxiaApi/onyxiaApi.tsweb/src/core/bootstrap.tsweb/src/core/ports/OnyxiaApi/DeploymentRegion.tsweb/src/core/ports/OnyxiaApi/S3Config.tsweb/src/core/rootContext.tsweb/src/core/usecases/s3ExplorerUiController/thunks.tsweb/src/core/usecases/s3ProfilesCreationUiController/thunks.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedBookmark.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedStsRole.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.tsweb/src/core/usecases/s3ProfilesManagement/selectors.tsweb/src/core/usecases/s3ProfilesManagement/state.tsweb/src/core/usecases/s3ProfilesManagement/thunks.tsweb/src/env.tsweb/src/ui/App/App.tsxweb/src/ui/App/LeftBar.tsxweb/src/ui/pages/home/Page.tsxweb/src/vite-env.d.ts
💤 Files with no reviewable changes (3)
- web/src/core/adapters/onyxiaApi/ApiTypes.ts
- web/src/core/adapters/onyxiaApi/onyxiaApi.ts
- web/src/core/ports/OnyxiaApi/DeploymentRegion.ts
Comment on lines
+48
to
+53
| ` sts: {`, | ||
| ` durationSeconds: 604800,`, | ||
| ` oidcConfiguration: {`, | ||
| ` issuerURI: "https://auth.lab.sspcloud.fr/auth/realms/sspcloud",`, | ||
| ` clientID: "onyxia"`, | ||
| ` },`, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix OIDC field name casing to match the S3Config schema.
This template sets issuerURI and clientID, but S3Config.ts's zOidcConfigurationShape expects issuerUri and clientId (see web/src/core/ports/OnyxiaApi/S3Config.ts lines 60-61), and s3Config_userProvidedToParsed reads exactly those field names (lines 207-208). Because the OIDC shape is all-optional, this casing mismatch passes validation silently and both values resolve to undefined in the generated local config, breaking OIDC-based STS role assumption for local development.
🐛 Proposed fix
` oidcConfiguration: {`,
- ` issuerURI: "https://auth.lab.sspcloud.fr/auth/realms/sspcloud",`,
- ` clientID: "onyxia"`,
+ ` issuerUri: "https://auth.lab.sspcloud.fr/auth/realms/sspcloud",`,
+ ` clientId: "onyxia"`,
` },`,📝 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.
Suggested change
| ` sts: {`, | |
| ` durationSeconds: 604800,`, | |
| ` oidcConfiguration: {`, | |
| ` issuerURI: "https://auth.lab.sspcloud.fr/auth/realms/sspcloud",`, | |
| ` clientID: "onyxia"`, | |
| ` },`, | |
| ` sts: {`, | |
| ` durationSeconds: 604800,`, | |
| ` oidcConfiguration: {`, | |
| ` issuerUri: "https://auth.lab.sspcloud.fr/auth/realms/sspcloud",`, | |
| ` clientId: "onyxia"`, | |
| ` },`, |
🤖 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 `@web/scripts/unyamlify-env-local.ts` around lines 48 - 53, Update the OIDC
template fields in unyamlify-env-local.ts to use the S3Config schema names
issuerUri and clientId instead of issuerURI and clientID, preserving the
existing issuer and client values.
Comment on lines
+87
to
+100
| const zS3Config = z.object({ | ||
| URL: z.string(), | ||
| pathStyleAccess: z.literal(true).optional(), | ||
| region: z.string().optional(), | ||
| sts: z | ||
| .object({ | ||
| URL: z.string().optional(), | ||
| durationSeconds: z.number().optional(), | ||
| role: z.union([zRole, z.array(zRole)]), | ||
| oidcConfiguration: zOidcConfiguration.optional() | ||
| }) | ||
| .optional(), | ||
| bookmarks: z.array(zBookmark).optional() | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether "monitoring" / "URLPattern" S3 fields are consumed anywhere in the codebase.
rg -n -C3 'monitoring|URLPattern' --type=ts .Repository: InseeFrLab/onyxia
Length of output: 18779
🏁 Script executed:
#!/bin/bash
#!/bin/bash
# Inspect the S3 config schema/parser and the local template around the reported fields.
set -euo pipefail
printf '\n--- S3Config.ts relevant sections ---\n'
sed -n '1,280p' web/src/core/ports/OnyxiaApi/S3Config.ts | nl -ba | sed -n '70,115p;170,265p'
printf '\n--- unyamlify-env-local.ts relevant sections ---\n'
sed -n '1,90p' web/scripts/unyamlify-env-local.ts | nl -ba | sed -n '1,90p'
printf '\n--- S3Config symbol usages ---\n'
rg -n 'S3Config_UserProvided|S3Config_Parsed|s3Config_userProvidedToParsed|parseS3Config|monitoring' web/src/core/ports web/src/core usecases src/core | rg -v i18n | head -200Repository: InseeFrLab/onyxia
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Inspect the S3 config schema/parser, local template, and monitoring URL usage.
set -euo pipefail
printf '\n--- S3Config.ts relevant sections ---\n'
sed -n '70,115p;170,265p' web/src/core/ports/OnyxiaApi/S3Config.ts
printf '\n--- unyamlify-env-local.ts relevant sections ---\n'
sed -n '1,90p' web/scripts/unyamlify-env-local.ts
printf '\n--- S3 config validation / monitoring usage ---\n'
rg -n 'S3Config_UserProvided|S3Config_Parsed|s3Config_userProvidedToParsed|parseS3Config|monitoring|monitoringUrl|URLPattern' web/src/core web/src/ui --glob '*.ts' --glob '*.tsx'Repository: InseeFrLab/onyxia
Length of output: 15035
Handle the S3 monitoring field or remove it from the local template.
web/scripts/unyamlify-env-local.ts sets S3.monitoring.URLPattern, but S3Config_UserProvided does not declare it. Zod strips this unknown key from the validated input. If S3-level monitoring links are part of the supported config, add monitoring to S3Config_UserProvided/S3Config_Parsed.Entry and pass it through s3Config_userProvidedToParsed. If the feature is not supported, remove it from the local template.
🤖 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 `@web/src/core/ports/OnyxiaApi/S3Config.ts` around lines 87 - 100, Resolve the
S3 monitoring configuration mismatch by either adding the monitoring field
consistently to S3Config_UserProvided and S3Config_Parsed.Entry and propagating
it through s3Config_userProvidedToParsed, or removing S3.monitoring.URLPattern
from unyamlify-env-local.ts if unsupported. Ensure the chosen behavior prevents
the field from being silently stripped.
Comment on lines
+188
to
+193
| if (fromAdminConfig.resolvedTemplatedStsRoles === undefined) { | ||
| return []; | ||
| } | ||
|
|
||
| const entry = fromRegion.resolvedTemplatedStsRoles.find( | ||
| e => e.correspondingS3ConfigIndexInRegion === index | ||
| const entry = fromAdminConfig.resolvedTemplatedStsRoles.find( | ||
| e => e.correspondingS3ConfigEntryIndex === index |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline web/src/core/ports/OnyxiaApi/S3Config.ts --items all
rg -n -C 5 'roles\s*:|\.min\(|nonempty\(' \
web/src/core/ports/OnyxiaApi/S3Config.ts
rg -n -C 4 'roles:\s*\[\s*\]|sts:.*roles' web \
-g '*.{ts,tsx,json,yaml,yml}'Repository: InseeFrLab/onyxia
Length of output: 1687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' web/src/core/ports/OnyxiaApi/S3Config.ts
printf '\n--- s3Profiles aggregate section ---\n'
sed -n '150,220p' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts
printf '\n--- schema occurrences ---\n'
rg -n -C 4 'S3Config_UserProvided|s3Config_userProvidedToParsed|zS3Config_Entry|roles|entry\.roles|sts: ' web/src/core/ports/OnyxiaApi/S3Config.ts web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.tsRepository: InseeFrLab/onyxia
Length of output: 17003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- s3Profiles relevant whole function ---'
rg -n -A 220 -B 40 'resolvedTemplatedStsRoles\.map|resolvedTemplatedStsRoles_forThisProfile' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts
printf '\n%s\n' '--- imports and surrounding definitions of useS3ProfilesProfileManager ---'
rg -n -C 80 'useS3ProfilesProfileManager|profileName =|resolvedTemplatedStsRoles_forThisProfile|profileName.*fromAdminConfig|correspondingS3ConfigEntryIndex' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts
printf '\n%s\n' '--- occurrences of resolvedTemplatedStsRoles in repo ---'
rg -n -C 8 'resolvedTemplatedStsRoles|correspondingS3ConfigEntryIndex' web/src/core/usecases/s3ProfilesManagement -g '*.ts'Repository: InseeFrLab/onyxia
Length of output: 36565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
async function main() {
for await (const module of [
"core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedStsRole",
"core/tools/resolveTemplate",
"core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplates",
]) {
try {
const path = `web/src/core/${module}.ts`;
const { createRequire } = await import("module");
const fs = await import("fs");
if (!fs.existsSync(path)) {
console.log(`${module}: missing ${path}`);
continue;
}
console.log(`\n--- ${module} outline ---`);
const outline = await import("ast-grep");
// Skip external dependency inspection when ast-grep Node API is unavailable.
} catch (error) {
console.log(`${module}: unavailable`);
}
const { readFileSync } = await import("fs");
console.log(readFileSync(`web/src/core/${module}.ts`, "utf8"));
}
}
main();
JSRepository: InseeFrLab/onyxia
Length of output: 566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- resolveTemplatedStsRole.ts ---'
sed -n '1,260p' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedStsRole.ts
printf '%s\n' '--- resolveTemplate.ts candidates ---'
fd -a 'resolveTemplate.*\.ts$' web/src/core | sed 's#^\./##' | head -50
printf '%s\n' '--- resolveTemplates.ts candidates ---'
fd -a 'resolveTemplates.*\.ts$' web/src/core | sed 's#^\./##' | head -50
printf '%s\n' '--- imports of resolveTemplatedStsRole ---'
rg -n -C 3 'resolveTemplatedStsRole|resolveTemplate|resolveTemplates' web/src/core -g '*.ts'Repository: InseeFrLab/onyxia
Length of output: 10139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'OnyxiaApi.*\.ts$' web/src/core/ports | sed 's#^\./##' | xargs -r rg -n -C 6 'S3Config_UserProvided|zS3Config_UserProvided|s3Config_userProvidedToParsed|role:'
printf '%s\n' '--- OnyxiaApi exports ---'
fd -a 'OnyxiaApi.*\.ts$' web/src/core/ports | sed 's#^\./##' | xargs -r rg -n -C 5 'export .*S3|S3Config|s3Config'Repository: InseeFrLab/onyxia
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- OnyxiaApi files ---'
git ls-files web/src/core/ports/OnyxiaApi
printf '%s\n' '--- S3 config validation occurrences ---'
rg -n -C 5 'S3Config_UserProvided|zS3Config_UserProvided|s3Config_userProvidedToParsed|role:|min\(|nonempty|atLeast' web/src/core/ports web/src/core/usecases/web -g '*.ts' || true
printf '%s\n' '--- schema field docs around S3 config ---'
rg -n -C 4 'S3Config_UserProvided|zS3Config_UserProvided|role\s*:|sts\.role' web -g '*.ts' -g '*.tsx' || trueRepository: InseeFrLab/onyxia
Length of output: 18220
Handle STS entries that resolve no roles.
resolveTemplatedStsRole can return [] when a claim value is absent, empty, rejected by filters, or no substitutions match. That empty result makes fromAdminConfig.entries[index].resolvedTemplatedStsRoles.stsRoles empty, and aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet asserts before creating a profile. Require at least one valid STS role before parsing, or remove entries that resolve no roles.
🤖 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 `@web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts`
around lines 188 - 193, Ensure the S3 profile aggregation flow skips entries
whose resolved STS configuration contains no roles before parsing or asserting
on it. Update the logic around resolvedTemplatedStsRoles and
aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet to require at least one
valid stsRoles entry, removing or excluding unresolved entries while preserving
valid profile aggregation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit