feat(wren-ui): Athena OIDC auth #2047
Conversation
WalkthroughAdds OIDC and instance-profile authentication options for Athena: makes AWS access key/secret optional in server types, extends server conversion to produce OIDC or key-based Ibis configs, updates sensitive props, and adds a UI auth selector with conditional fields and new validation messages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as AthenaProperties UI
participant Form as Form State
participant Server as toIbisConnectionInfo
User->>UI: Select auth method
UI->>Form: set athenaAuthType
alt classic
User->>UI: Enter awsAccessKey & awsSecretKey
UI->>Form: store keys
Form->>Server: request Ibis config
Server-->>Form: return aws_access_key_id + aws_secret_access_key + base fields
else oidc
User->>UI: Enter webIdentityToken & roleArn (optional roleSessionName)
UI->>Form: store token & role info
Form->>Server: request Ibis config
Server-->>Form: return web_identity_token + role_arn (+ role_session_name) + base fields
else instance_profile
UI-->>User: show informational note (no credentials)
Form->>Server: request Ibis config
Server-->>Form: return base fields only (rely on instance profile)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
71-84: Athena auth fields are now all optional; consider a discriminated union for stronger typing.This shape is functionally fine, but TypeScript no longer enforces that a valid auth combination is present (either access keys or OIDC fields). If you want the type system to guard callers, consider a union:
-export interface IbisAthenaConnectionInfo { - // AWS access key auth (optional if using OIDC) - aws_access_key_id?: string; - aws_secret_access_key?: string; - - // OIDC auth (optional if using access keys) - web_identity_token?: string; - role_arn?: string; - role_session_name?: string; - - region_name: string; - s3_staging_dir: string; - schema_name: string; -} +export type IbisAthenaConnectionInfo = + | { + // AWS access key auth + aws_access_key_id: string; + aws_secret_access_key: string; + region_name: string; + s3_staging_dir: string; + schema_name: string; + } + | { + // OIDC auth + web_identity_token: string; + role_arn: string; + role_session_name?: string; + region_name: string; + s3_staging_dir: string; + schema_name: string; + };Only adopt this if it doesn’t ripple too much through existing call sites; current interface is acceptable if you rely on runtime validation.
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (2)
6-9: Auth-method split between classic keys and OIDC looks good; enum reuse could reduce drift.The separation into
AthenaClassicFieldsandAthenaOIDCFieldswith appropriate validation + error texts is clean, and the Radio-drivenathenaAuthTypeswitch matches the new backend contract.One minor improvement: there is now both this local
ATHENA_AUTH_METHODand a generatedATHENA_AUTH_METHODenum inwren-ui/src/apollo/client/graphql/__types__.ts. If practical, consider centralizing this (e.g., re-exporting from a shared module or aliasing) so future additions/renames can’t diverge.Also applies to: 15-47, 49-97, 202-223
103-133: Edit-mode tracking viainitialTypeRefis reasonable; please verifyathenaAuthTypeis always set in edit mode.The pattern of:
- watching
athenaAuthTypeviaForm.useWatch,- capturing the first non-null value in
initialTypeRef, and- using
getIsEditModeForComponentto decide when to lock the OIDC role ARNis sound.
The only fragile spot is that both conditional blocks:
{authType === ATHENA_AUTH_METHOD.classic && <AthenaClassicFields />} {authType === ATHENA_AUTH_METHOD.oidc && <AthenaOIDCFields ... />}won’t render anything if
authTypeis everundefinedin edit mode (e.g., for legacy connections saved before this field existed). That depends on how the parentFormsets initial values.If there’s any chance of
athenaAuthTypebeing missing on existing rows, consider a defensive default such as:- useEffect(() => { - if (!isEditMode) { - form.setFieldsValue({ - athenaAuthType: ATHENA_AUTH_METHOD.classic, - }); - } - }, [isEditMode, form]); + useEffect(() => { + if (!authType) { + form.setFieldsValue({ + athenaAuthType: ATHENA_AUTH_METHOD.classic, + }); + } + }, [authType, form]);or deriving the default from which credential fields are present, so auth fields always render in edit mode as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wren-ui/src/apollo/client/graphql/__types__.ts(1 hunks)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(1 hunks)wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx(6 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-18T10:28:10.593Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:10.593Z
Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.
Applied to files:
wren-ui/src/apollo/client/graphql/__types__.ts
📚 Learning: 2025-03-18T10:28:26.608Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:26.608Z
Learning: The enum names `IDENTIED_AS_GENERAL` and `IDENTIED_AS_MISLEADING_QUERY` in GeneralErrorCodes enum in wren-ui/src/apollo/server/utils/error.ts have typos (missing "FI" in "IDENTIFIED") but fixing them is not in scope for the current PR.
Applied to files:
wren-ui/src/apollo/client/graphql/__types__.ts
🧬 Code graph analysis (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-ui/src/utils/error/dictionary.ts (1)
67-72: New AWS_ROLE_ARN / WEB_IDENTITY_TOKEN error texts match the new fields.The keys and messages align with the Athena OIDC form labels and existing wording style; nothing else to change here.
wren-ui/src/apollo/client/graphql/__types__.ts (1)
1249-1252: ATHENA_AUTH_METHOD enum addition looks consistent.Values (
classic,oidc) follow the same pattern as other connection enums (e.g., RedshiftConnectionType) and match the UI’s auth-method options.wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
166-177: Updated S3 staging hint is clear and helpful.The inline guidance (“Settings → Query result location”) tightly matches how users find this in the Athena console, and the validation wiring is unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wren-ui/src/apollo/server/dataSource.ts (1)
77-106: Consider adding explicit handling when both auth methods are provided.The current logic gives implicit priority to OIDC authentication when both sets of credentials are present. While this may be intentional, it could lead to confusion if users accidentally provide both authentication methods.
Consider adding an explicit check and logging:
toIbisConnectionInfo(connectionInfo) { const decryptedConnectionInfo = decryptConnectionInfo( DataSourceName.ATHENA, connectionInfo, ); const { awsAccessKey, awsRegion, awsSecretKey, s3StagingDir, schema, webIdentityToken, roleArn, roleSessionName } = decryptedConnectionInfo as ATHENA_CONNECTION_INFO; + // Check if both auth methods are provided + const hasOIDC = !!(webIdentityToken && roleArn); + const hasAWSKeys = !!(awsAccessKey && awsSecretKey); + + if (hasOIDC && hasAWSKeys) { + // Log warning or throw error based on desired behavior + console.warn('Both OIDC and AWS credentials provided for Athena. Using OIDC authentication.'); + } + // Base fields shared by both authentication methods const base = { region_name: awsRegion, s3_staging_dir: s3StagingDir, schema_name: schema, }; // If OIDC fields are provided → send OIDC config - if (webIdentityToken && roleArn) { + if (hasOIDC) { return {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/apollo/server/dataSource.ts(1 hunks)wren-ui/src/apollo/server/repositories/projectRepository.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-28T20:09:06.106Z
Learnt from: narsik123
Repo: Canner/WrenAI PR: 1606
File: wren-ui/src/apollo/server/dataSource.ts:135-150
Timestamp: 2025-04-28T20:09:06.106Z
Learning: The Oracle data source in WrenAI includes an SSL toggle in the UI (OracleProperties component) and should pass this setting to the connection info using the pattern `...(ssl && { kwargs: { ssl: true } })` for consistency with other host-based connections like MS SQL.
Applied to files:
wren-ui/src/apollo/server/dataSource.ts
📚 Learning: 2025-03-18T10:28:10.593Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:10.593Z
Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.
Applied to files:
wren-ui/src/apollo/server/repositories/projectRepository.ts
🧬 Code graph analysis (1)
wren-ui/src/apollo/server/dataSource.ts (2)
wren-ui/src/apollo/server/repositories/projectRepository.ts (1)
ATHENA_CONNECTION_INFO(89-99)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
IbisAthenaConnectionInfo(71-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
wren-ui/src/apollo/server/dataSource.ts (3)
76-76: LGTM!Correctly adds
webIdentityTokento sensitive properties for encryption alongsideawsSecretKey. The other OIDC fields (roleArn, roleSessionName) are appropriately not marked as sensitive since they're identifiers rather than credentials.
82-90: LGTM!Good refactoring to extract common fields into a base object that both authentication paths can reuse. The destructuring correctly captures all needed fields from the decrypted connection info.
92-99: LGTM!The OIDC authentication branch correctly verifies that both required fields (webIdentityToken and roleArn) are present before constructing the OIDC configuration. The optional roleSessionName is properly handled with conditional spreading.
wren-ui/src/apollo/server/repositories/projectRepository.ts (1)
94-98: <!-- [scratchpad_start]
[task review_file_1/1]
Verify authentication validation for ATHENA_CONNECTION_INFO interface changes[observations]
- Interface changes at projectRepository.ts lines 94-98 make auth fields optional
- AthenaProperties.tsx form:
- Conditional rendering via radio button (classic vs oidc)
- AthenaClassicFields: awsAccessKey and awsSecretKey both marked required
- AthenaOIDCFields: webIdentityToken and roleArn both marked required, roleSessionName optional
- Only ONE set of fields renders/validates based on auth method selection
- Backend dataSource.ts conversion (lines 91-105):
- Checks
if (webIdentityToken && roleArn)→ routes to OIDC config- Otherwise → falls back to AWS credentials
- Form validation prevents submission with missing required fields
[analysis]
- Interface optionality is intentional design: allows both auth methods as alternatives
- Form validation enforces mutual exclusivity via conditional rendering:
- User selects auth method via radio button
- Only selected method's fields appear and are marked required
- Form validates only visible/selected fields
- Prevents submission if selected method incomplete
- Backend routing correctly handles both auth methods
- Design is sound: flexible interface + strict form validation = correct behavior
[pending]
- None - verification complete
[actions]
- Generate final rewritten comment confirming verification
[done]
Authentication validation is properly implemented through:
- Form-level conditional field rendering and validation
- Backend routing logic in dataSource.ts
- Interface optionality supports design requirements
</scratchpad_start] -->Authentication validation is properly implemented through form conditionals and backend routing.
The interface changes correctly support both authentication methods. Form validation properly enforces that exactly one complete authentication method is provided:
- User selects auth method via radio button (classic or OIDC)
- Only the selected method's fields render and validate as required
- Backend routing in
dataSource.ts(lines 92-105) correctly handles both methods: OIDC ifwebIdentityToken && roleArn, otherwise AWS credentialsThe design is sound—flexible interface constraints combined with strict form validation prevents invalid connection configurations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
6-98: Auth enum and field subcomponents look solid; minor copy nit for OIDC placeholderThe
ATHENA_AUTH_METHODenum and the split intoAthenaClassicFields/AthenaOIDCFieldsmake the form structure clearer, and the validation rules correctly referenceERROR_TEXTS.CONNECTION.AWS_ACCESS_KEY,.AWS_SECRET_KEY,.WEB_IDENTITY_TOKEN, and.AWS_ROLE_ARNfromwren-ui/src/utils/error/dictionary.ts.One small UX suggestion: the textarea placeholder
"Paste Google OIDC token here"(Line 68) is quite IdP‑specific; if other OIDC providers are expected, consider a more generic wording like “Paste your OIDC token here” to avoid confusing non‑Google setups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (3)
1-2: Imports are consistent with usageThe added React and antd imports match the hooks/components used below; no issues spotted here.
151-177: Common field copy and behavior are appropriateThe “Database (schema)” label + extra text and the expanded S3 staging directory guidance (“Settings → Query result location”) improve clarity without affecting behavior. Keeping
schemaandawsRegiondisabled in edit mode is consistent with treating these as immutable connection attributes.
218-234: Instance Profile messaging is clear; ensure server uses it as a true no‑credential pathThe conditional rendering for Instance Profile (Lines 229‑233) is a nice, minimal UX: no extra fields, just an explanatory note that credentials are auto‑detected from the runtime role. Just double‑check that the backend treats
athenaAuthType === 'instance_profile'as “don’t expect access key / secret / web identity fields” and correctly falls back to the environment / instance metadata rather than failing validation if those fields are empty.
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
Outdated
Show resolved
Hide resolved
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-18T10:28:10.593Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:10.593Z
Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
📚 Learning: 2025-03-18T10:28:26.608Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:26.608Z
Learning: The enum names `IDENTIED_AS_GENERAL` and `IDENTIED_AS_MISLEADING_QUERY` in GeneralErrorCodes enum in wren-ui/src/apollo/server/utils/error.ts have typos (missing "FI" in "IDENTIFIED") but fixing them is not in scope for the current PR.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
🧬 Code graph analysis (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-180)
🪛 Biome (2.1.2)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
[error] 231-231: expected , but instead found will
Remove will
(parse)
[error] 231-231: expected , but instead found automatically
Remove automatically
(parse)
[error] 231-231: expected , but instead found detect
Remove detect
(parse)
[error] 231-231: expected , but instead found AWS
Remove AWS
(parse)
[error] 231-231: expected , but instead found credentials
Remove credentials
(parse)
[error] 231-231: expected , but instead found from
Remove from
(parse)
[error] 231-231: expected , but instead found the
Remove the
(parse)
[error] 231-231: expected , but instead found Instance
Remove Instance
(parse)
[error] 231-231: expected , but instead found Profile
Remove Profile
(parse)
[error] 231-232: expected , but instead found role
Remove role
(parse)
[error] 232-232: expected , but instead found assigned
Remove assigned
(parse)
[error] 232-232: expected , but instead found to
Remove to
(parse)
[error] 232-232: expected , but instead found this
Remove this
(parse)
[error] 232-232: expected : but instead found compute
Remove compute
(parse)
[error] 232-232: expected , but instead found environment
Remove environment
(parse)
[error] 232-233: Expected a function body but instead found '.'.
Expected a function body here.
(parse)
[error] 233-233: expected , but instead found <
Remove <
(parse)
[error] 233-233: Expected a type parameter but instead found '/'.
Expected a type parameter here.
(parse)
[error] 233-233: expected , but instead found div
Remove div
(parse)
[error] 234-234: Expected a parenthesis '(' but instead found ')'.
Expected a parenthesis '(' here.
(parse)
🔇 Additional comments (2)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (2)
16-48: LGTM! Clean extraction of AWS credential fields.The component properly encapsulates the AWS access key and secret access key fields with appropriate validation rules and error messages.
50-98: LGTM! OIDC fields implementation is sound.The component correctly handles the OIDC authentication fields with proper validation. The
roleArnfield is appropriately disabled in edit mode, which is a good security practice.
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
206-208: Instance Profile auth method lacks backend support.The
instance_profileoption is rendered in the UI but was previously identified as missing from the GraphQL schema (__types__.tsonly containsclassicandoidc). Users selecting this option may encounter submission failures.Either remove this option until backend support is complete, or ensure the GraphQL schema and backend handlers are updated to support
instance_profile.Also applies to: 223-228
🧹 Nitpick comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
102-105: Consider explicit type handling foruseWatchreturn value.
Form.useWatchcan returnundefinedbefore the form is initialized. While the current conditional checks (authType === ATHENA_AUTH_METHOD.classic) naturally handle this by evaluating tofalse, the type cast obscures this possibility.- const authType = Form.useWatch( - 'athenaAuthType', - form, - ) as ATHENA_AUTH_METHOD; + const authType = Form.useWatch('athenaAuthType', form) as + | ATHENA_AUTH_METHOD + | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-18T10:28:10.593Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:10.593Z
Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
📚 Learning: 2025-03-18T10:28:26.608Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:26.608Z
Learning: The enum names `IDENTIED_AS_GENERAL` and `IDENTIED_AS_MISLEADING_QUERY` in GeneralErrorCodes enum in wren-ui/src/apollo/server/utils/error.ts have typos (missing "FI" in "IDENTIFIED") but fixing them is not in scope for the current PR.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
🧬 Code graph analysis (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-180)
🔇 Additional comments (4)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (4)
10-42: LGTM!The
AthenaClassicFieldscomponent properly encapsulates AWS credential fields with appropriate validation rules and usesInput.Passwordfor the secret key.
44-92: LGTM!The
AthenaOIDCFieldscomponent is well-structured with appropriate validation, helpful placeholders, and correct edit-mode handling for the role ARN field.
129-195: LGTM!Common form fields are well-structured with appropriate validation, disabled states for edit mode, and helpful extra text. The formatting updates (e.g., bold "Settings → Query result location") improve clarity.
213-221: LGTM!The conditional rendering for
classicandoidcauthentication methods correctly uses thegetIsEditModeForComponenthelper to determine field behavior in edit mode.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
197-210: Critical:instance_profileauthentication option remains unsupported by the backend.Previous review identified that
instance_profileis absent from the GraphQL generated types (which only containsclassicandoidc). This creates a breaking mismatch where users can select "Instance Profile" but the backend cannot process it.As noted in the previous review, either:
- Remove the incomplete feature: Delete
instance_profilefrom lines 206-208 and line 223-228, or- Complete the backend implementation: Add
instance_profileto the GraphQL schema, implement backend handling, and regenerate__types__.ts
🧹 Nitpick comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
223-228: Syntax error fixed; consider using design system color.The missing quote syntax error from the previous review has been resolved.
Optionally, replace the hardcoded color value
'#666'with a design system variable or use a className like"gray-8"for consistency with the project's styling approach.Apply this diff to use a className instead:
- {authType === ATHENA_AUTH_METHOD.instance_profile && ( - <div style={{ fontStyle: 'italic', color: '#666' }}> - We will automatically detect AWS credentials from the Instance Profile role - assigned to this compute environment (EC2, ECS, EKS). - </div> - )} + {authType === ATHENA_AUTH_METHOD.instance_profile && ( + <div className="gray-8" style={{ fontStyle: 'italic' }}> + We will automatically detect AWS credentials from the Instance Profile role + assigned to this compute environment (EC2, ECS, EKS). + </div> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-18T10:28:10.593Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:10.593Z
Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
📚 Learning: 2025-03-18T10:28:26.608Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:26.608Z
Learning: The enum names `IDENTIED_AS_GENERAL` and `IDENTIED_AS_MISLEADING_QUERY` in GeneralErrorCodes enum in wren-ui/src/apollo/server/utils/error.ts have typos (missing "FI" in "IDENTIFIED") but fixing them is not in scope for the current PR.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
🧬 Code graph analysis (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (4)
1-92: Well-structured modular components and proper error message usage.The refactoring into
AthenaClassicFieldsandAthenaOIDCFieldsimproves readability and maintainability. The ATHENA_AUTH_METHOD enum import from@/utils/enumaddresses previous feedback. All referenced error messages (AWS_ACCESS_KEY.REQUIRED,AWS_SECRET_KEY.REQUIRED,WEB_IDENTITY_TOKEN.REQUIRED,AWS_ROLE_ARN.REQUIRED) exist in the error dictionary.
98-127: State management logic correctly handles create and edit modes.The combination of
Form.useFormInstance,Form.useWatch,initialTypeRef, andgetIsEditModeForComponentproperly:
- Defaults new connections to
classicauth- Preserves the original auth type during edits
- Conditionally disables the OIDC role ARN only when editing an existing OIDC connection
145-195: Improved field descriptions enhance user guidance.The updated descriptions for the database field and S3 staging directory provide clearer context for users configuring Athena connections.
10-92: Backend toIbisConnectionInfo does not handle instance_profile authentication type; requests for this auth method will fail.The frontend exposes
instance_profileas an authentication option in the radio buttons (line 206 of AthenaProperties.tsx), but the backend'stoIbisConnectionInfofunction inwren-ui/src/apollo/server/dataSource.ts(lines 77–106) only handles OIDC (ifwebIdentityToken && roleArnare provided) or falls back to classic AWS credentials. When a user selectsinstance_profile, no credentials are collected from the form, yet the backend attempts classic authentication with undefinedawsAccessKeyandawsSecretKey, causing connection failures.Add explicit handling for
instance_profilein thetoIbisConnectionInfofunction—it should return only the base configuration fields (region_name,s3_staging_dir,schema_name) without credentials, or alternatively, removeinstance_profilefrom the frontend enum and UI options if not yet supported by the backend.⛔ Skipped due to learnings
Learnt from: andreashimin Repo: Canner/WrenAI PR: 1414 File: wren-ui/src/apollo/server/utils/error.ts:0-0 Timestamp: 2025-03-18T10:28:10.593Z Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.Learnt from: andreashimin Repo: Canner/WrenAI PR: 1414 File: wren-ui/src/apollo/server/utils/error.ts:0-0 Timestamp: 2025-03-18T10:28:26.608Z Learning: The enum names `IDENTIED_AS_GENERAL` and `IDENTIED_AS_MISLEADING_QUERY` in GeneralErrorCodes enum in wren-ui/src/apollo/server/utils/error.ts have typos (missing "FI" in "IDENTIFIED") but fixing them is not in scope for the current PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
198-228: Theinstance_profileoption lacks backend support.This concern was raised in a previous review: the frontend includes
instance_profilein the Radio.Group (lines 206-208) and conditional rendering (lines 223-228), but the GraphQL generated types only containclassicandoidc. Users can select "Instance Profile" but submission will fail due to the schema mismatch.Either remove
instance_profilefrom this component until backend support is added, or complete the backend implementation first.
🧹 Nitpick comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
108-127: Consider handling legacy connections withoutathenaAuthType.The current logic correctly defaults new connections to
classicand preserves the initial type in edit mode. However, for legacy Athena connections that were created before this feature and don't haveathenaAuthTypestored,authTypewill beundefined, leaving no radio option selected and no auth fields rendered.Consider inferring a default based on existing credential fields:
// Set default auth type when creating useEffect(() => { if (!isEditMode) { form.setFieldsValue({ athenaAuthType: ATHENA_AUTH_METHOD.classic, }); + } else if (isEditMode && !authType) { + // Legacy connections: infer classic if AWS credentials exist + const awsAccessKey = form.getFieldValue('awsAccessKey'); + if (awsAccessKey) { + form.setFieldsValue({ + athenaAuthType: ATHENA_AUTH_METHOD.classic, + }); + } } - }, [isEditMode, form]); + }, [isEditMode, form, authType]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-18T10:28:10.593Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:10.593Z
Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
📚 Learning: 2025-03-18T10:28:26.608Z
Learnt from: andreashimin
Repo: Canner/WrenAI PR: 1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:26.608Z
Learning: The enum names `IDENTIED_AS_GENERAL` and `IDENTIED_AS_MISLEADING_QUERY` in GeneralErrorCodes enum in wren-ui/src/apollo/server/utils/error.ts have typos (missing "FI" in "IDENTIFIED") but fixing them is not in scope for the current PR.
Applied to files:
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx
🧬 Code graph analysis (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (4)
1-8: LGTM!Imports are appropriate and the
ATHENA_AUTH_METHODenum is now properly imported from@/utils/enumas previously suggested.
10-42: LGTM!The
AthenaClassicFieldscomponent is well-structured with proper validation rules and appropriate use ofInput.Passwordfor the secret access key.
44-92: LGTM!The
AthenaOIDCFieldscomponent properly handles the edit mode behavior by disabling the role ARN field, and the optionalroleSessionNamefield has helpful explanatory text.
144-195: LGTM!The common fields (database, S3 staging directory, AWS region) are well-structured with clear validation rules, helpful extra descriptions, and appropriate
disabledstates in edit mode.
Description
This pull request introduces
OIDC (Web Identity Token)authentication for theAWS Athenadata source.Athenanow supports two login methods:AWS Classic Credentials
OIDC Web Identity Authentication
UI
AWS credentials
OIDC
Instance profile
Summary by CodeRabbit
New Features
Documentation / Validation
✏️ Tip: You can customize this high-level summary in your review settings.