Add OAuth2 support to IbisTrinoConnection #1298
Conversation
Add support for OAuth2 authentication in IbisTrinoConnection. * Add `OAuth2Authentication` interface to handle OAuth2 authentication in `wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts`. * Update `IbisTrinoConnectionInfo` interface to include optional `auth` field for OAuth2 authentication. * Add fields for OAuth2 client ID, client secret, and token URL in `TrinoProperties` component in `wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx`. * Update form submission logic to include OAuth2 fields in `TrinoProperties` component.
WalkthroughThe pull request introduces OAuth2 authentication support. A new Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
67-71: Consider adding URL validation and type constraints for OAuth2 fields.The OAuth2Authentication interface looks good, but could benefit from additional type safety:
export interface OAuth2Authentication { clientId: string; clientSecret: string; - tokenUrl: string; + tokenUrl: string & { __brand: 'URL' }; // Type brand for URL validation }wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx (1)
92-127: Consider UX improvements for OAuth2 configuration.The OAuth2 form fields implementation is good, but could benefit from these improvements:
- Add conditional rendering to show OAuth2 fields only when OAuth2 authentication is selected
- Add URL format validation for the token URL field
- Add helper text to explain the OAuth2 configuration process
+// Add a new form item for auth type selection +<Form.Item + label="Authentication Type" + name="authType" + initialValue="basic" +> + <Select> + <Select.Option value="basic">Basic Auth</Select.Option> + <Select.Option value="oauth2">OAuth2</Select.Option> + </Select> +</Form.Item> +// Wrap OAuth2 fields in a conditional +<Form.Item + noStyle + shouldUpdate={(prevValues, currentValues) => + prevValues.authType !== currentValues.authType + } +> + {({ getFieldValue }) => + getFieldValue('authType') === 'oauth2' && ( <Form.Item label="OAuth2 Client ID" name="clientId" rules={[ { required: true, // Make required when OAuth2 is selected message: ERROR_TEXTS.CONNECTION.CLIENT_ID.REQUIRED, }, ]} + tooltip="The client ID provided by your OAuth2 provider" > <Input placeholder="OAuth2 Client ID" /> </Form.Item> // ... other OAuth2 fields + ) + } +</Form.Item> // Add URL validation for token URL <Form.Item label="OAuth2 Token URL" name="tokenUrl" rules={[ { required: false, message: ERROR_TEXTS.CONNECTION.TOKEN_URL.REQUIRED, }, + { + type: 'url', + message: 'Please enter a valid URL', + }, ]} + tooltip="The token endpoint URL of your OAuth2 provider" > <Input placeholder="OAuth2 Token URL" /> </Form.Item>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(2 hunks)wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx(1 hunks)
🔇 Additional comments (3)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
56-56: LGTM! Good use of optional auth field.The optional auth field maintains backward compatibility while adding OAuth2 support.
wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx (2)
114-114: Good use of Input.Password for sensitive data.Properly using Input.Password component for the client secret ensures the sensitive value is masked.
92-127:❓ Verification inconclusive
Verify OAuth2 field validation messages.
Ensure that the error message constants referenced in the OAuth2 form fields are defined in the ERROR_TEXTS object.
🏁 Script executed:
#!/bin/bash # Check if OAuth2-related error messages are defined rg -A 3 "CONNECTION\." src/utils/error.ts | grep -E "CLIENT_ID|CLIENT_SECRET|TOKEN_URL"Length of output: 148
Verify Existing OAuth2 Error Message Constants
The script couldn’t locatesrc/utils/error.ts, so it’s unclear whether theERROR_TEXTS.CONNECTIONkeys (for CLIENT_ID, CLIENT_SECRET, and TOKEN_URL) are defined anywhere in the codebase. Please manually verify that these error message constants exist—either in an alternative file or under a different structure—and update the reference if necessary.
- Check the repository for the definition of the
ERROR_TEXTSobject (e.g., using a command likerg -n "export const ERROR_TEXTS" .).- If the OAuth2-specific keys are missing, add them accordingly.
…no data source connection * **Unit tests** - Add mock OAuth2 Trino connection info - Add test for getting Trino constraints with OAuth2 * **Integration tests** - Add test for connecting Trino data source with OAuth2 - Fill in OAuth2 client ID, client secret, and token URL fields - Verify successful connection and navigation to models setup page
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wren-ui/e2e/specs/connectTrino.spec.ts (1)
37-63: Enhance OAuth2 test coverage.While the happy path is covered, consider adding the following test cases for more robust coverage:
- Verify that OAuth2 fields are properly displayed/hidden based on authentication type selection
- Add negative test cases for invalid OAuth2 configurations
- Verify form validation rules for OAuth2 fields
test('OAuth2 fields validation', async ({ page }) => { await page.goto('/setup/connection'); await page.locator('button').filter({ hasText: 'Trino' }).click(); // Test required fields await page.getByRole('button', { name: 'Next' }).click(); await expect(page.getByText('Client ID is required')).toBeVisible(); // Test invalid token URL await page.getByLabel('OAuth2 Token URL').fill('invalid-url'); await expect(page.getByText('Invalid URL format')).toBeVisible(); });wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (1)
289-315: Enhance OAuth2 test coverage.Consider adding the following test cases for more comprehensive coverage:
- Error scenarios for OAuth2 authentication (e.g., invalid token URL)
- Multiple schema configurations
- SSL configuration with OAuth2
it('should handle OAuth2 authentication errors', async () => { const mockError = { response: { data: 'Invalid token URL' } }; mockedAxios.post.mockRejectedValue(mockError); await expect( ibisAdaptor.getConstraints( DataSourceName.TRINO, mockOAuth2TrinoConnectionInfo ) ).rejects.toThrow('Invalid token URL'); }); it('should handle multiple schemas with OAuth2', async () => { const multiSchemaOAuth2Info = { ...mockOAuth2TrinoConnectionInfo, schemas: 'catalog1.schema1,catalog2.schema2' }; // Test implementation });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/e2e/specs/connectTrino.spec.ts(1 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(2 hunks)
🔇 Additional comments (1)
wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (1)
93-103: LGTM! Well-structured mock object.The mock OAuth2 connection info object is well-structured and includes all necessary OAuth2 fields.
Add support for OAuth2 authentication in IbisTrinoConnection.
OAuth2Authenticationinterface to handle OAuth2 authentication inwren-ui/src/apollo/server/adaptors/ibisAdaptor.ts.IbisTrinoConnectionInfointerface to include optionalauthfield for OAuth2 authentication.TrinoPropertiescomponent inwren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx.TrinoPropertiescomponent.Summary by CodeRabbit
New Features
Tests