-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(wren-ui): Mask OIDC web identity token field #2063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds Athena OIDC handling: changes the web identity token input from a multi-line TextArea to a single-line Password input (autoComplete off) in the UI, marks Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-ui/src/components/pages/setup/dataSources/AthenaProperties.tsx (1)
60-63: LGTM! Security improvement aligns with existing patterns.The change from
Input.TextAreatoInput.Passwordappropriately masks the web identity token, matching the existing pattern for the AWS Secret Key field (line 38). This prevents accidental exposure when pasting or displaying tokens.Optional: Consider more specific autoComplete value.
For better browser compliance, consider using
autoComplete="one-time-code"instead of"off", as browsers often ignore"off"for password fields:<Input.Password placeholder="OAuth 2.0 access token or OpenID Connect ID token" - autoComplete="off" + autoComplete="one-time-code" />This hint better communicates to browsers that this field shouldn't be saved or auto-filled.
📜 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(1 hunks)
⏰ 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)
fredalai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Freda Lai <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-ui/src/hooks/useSetupConnectionDataSource.tsx (1)
197-213: Consider adding ATHENA-specific handling intransformPropertiesToFormfor consistency.The
getAthenaAuthenticationfunction correctly handles placeholder filtering and follows the established pattern (as suggested in past review). However, there's no corresponding ATHENA case intransformPropertiesToForm(lines 111-160) to addPASSWORD_PLACEHOLDERforwebIdentityTokenwhen loading existing connections.This means when editing an existing Athena connection that uses
webIdentityToken:
- The UI won't display a placeholder (unlike REDSHIFT's
awsSecretKeyat lines 129-139 or DATABRICKS's sensitive fields at lines 140-151)- Users must re-enter the token every time, even when not changing it
Add ATHENA handling to
transformPropertiesToForm:} else if (dataSourceType === DataSourceName.ATHENA) { return { ...properties, webIdentityToken: properties?.webIdentityToken || PASSWORD_PLACEHOLDER, awsSecretKey: properties?.awsSecretKey || PASSWORD_PLACEHOLDER, };This would align with how REDSHIFT and DATABRICKS handle their sensitive authentication fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/hooks/useSetupConnectionDataSource.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/hooks/useSetupConnectionDataSource.tsx
⏰ 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 (1)
wren-ui/src/hooks/useSetupConnectionDataSource.tsx (1)
88-92: LGTM! Consistent integration with existing patterns.The ATHENA data source branch follows the same authentication handling pattern as SNOWFLAKE and DATABRICKS, maintaining consistency across the codebase.
Description
Use
<Input.Password />to mask the token, matching the behavior of the AWS Secret Key field and preventing accidental exposure when pasting tokens.Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.