feat: move expires_in field after scope in OAuth2 datasource config#41623
feat: move expires_in field after scope in OAuth2 datasource config#41623shanliuling wants to merge 3 commits intoappsmithorg:releasefrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx`:
- Around line 686-695: The expiresIn field is rendered inside
renderOauth2Common() but per RestAPIForm.ts it belongs only to
AuthorizationCode; update the rendering so the FormInputContainer with
configProperty "authentication.expiresIn" (the
renderInputTextControlViaFormControl call) is shown only when the OAuth grant
type equals Authorization Code (e.g., check the grantType/auth.grantType value
or use the renderAuthorizationCode() path) instead of unconditionally in
renderOauth2Common(); either move the field into the Authorization Code render
helper or wrap it with a conditional like grantType === 'AUTHORIZATION_CODE' so
Client Credentials no longer sees the expiresIn input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7c93df2-9736-4cae-b6dd-f252b7f818c2
📒 Files selected for processing (1)
app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx
| <FormInputContainer data-location-id={btoa("authentication.expiresIn")}> | ||
| {this.renderInputTextControlViaFormControl({ | ||
| configProperty: "authentication.expiresIn", | ||
| label: "Authorization expires in (seconds)", | ||
| placeholderText: "3600", | ||
| dataType: "NUMBER", | ||
| encrypted: false, | ||
| isRequired: false, | ||
| })} | ||
| </FormInputContainer> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify expiresIn is only defined on AuthorizationCode interface
echo "=== Checking Oauth2Common interface ==="
rg -nA 15 'interface Oauth2Common' --type=ts
echo ""
echo "=== Checking ClientCredentials interface ==="
rg -nA 8 'interface ClientCredentials' --type=ts
echo ""
echo "=== Checking AuthorizationCode interface ==="
rg -nA 10 'interface AuthorizationCode' --type=tsRepository: appsmithorg/appsmith
Length of output: 3132
🏁 Script executed:
#!/bin/bash
# Search for the render method definitions and their calls to renderOauth2Common
echo "=== Finding renderOauth2Common calls ==="
rg -n 'renderOauth2Common' app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx
echo ""
echo "=== Checking renderOauth2ClientCredentials and renderOauth2AuthorizationCode method signatures ==="
rg -n -A 3 'renderOauth2ClientCredentials|renderOauth2AuthorizationCode' app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsxRepository: appsmithorg/appsmith
Length of output: 918
🏁 Script executed:
#!/bin/bash
# Check GrantType import and verify formData accessibility
echo "=== Checking GrantType import ==="
rg -n 'import.*GrantType|from.*GrantType' app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx | head -5
echo ""
echo "=== Checking how grantType is accessed elsewhere ==="
rg -n 'grantType' app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx | head -10
echo ""
echo "=== Checking formData usage pattern in render methods ==="
rg -n 'formData\.authentication' app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx | head -5Repository: appsmithorg/appsmith
Length of output: 1255
🏁 Script executed:
#!/bin/bash
# Check top of file for GrantType import
echo "=== Checking file imports ==="
head -50 app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx | rg -n 'import|GrantType'
echo ""
echo "=== Verifying GrantType is available ==="
rg -n 'enum GrantType|type GrantType' --type=tsRepository: appsmithorg/appsmith
Length of output: 1732
expiresIn field now incorrectly appears for Client Credentials grant type.
Per the TypeScript interfaces in RestAPIForm.ts, expiresIn is defined only on AuthorizationCode, not on ClientCredentials or Oauth2Common. By placing this field in renderOauth2Common(), it renders for both grant types, violating the type contract and exposing an irrelevant field for Client Credentials flow.
Conditionally render only for Authorization Code grant type:
Suggested fix
+ {_.get(formData.authentication, "grantType") ===
+ GrantType.AuthorizationCode && (
<FormInputContainer data-location-id={btoa("authentication.expiresIn")}>
{this.renderInputTextControlViaFormControl({
configProperty: "authentication.expiresIn",
label: "Authorization expires in (seconds)",
placeholderText: "3600",
dataType: "NUMBER",
encrypted: false,
isRequired: false,
})}
</FormInputContainer>
+ )}📝 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.
| <FormInputContainer data-location-id={btoa("authentication.expiresIn")}> | |
| {this.renderInputTextControlViaFormControl({ | |
| configProperty: "authentication.expiresIn", | |
| label: "Authorization expires in (seconds)", | |
| placeholderText: "3600", | |
| dataType: "NUMBER", | |
| encrypted: false, | |
| isRequired: false, | |
| })} | |
| </FormInputContainer> | |
| {_.get(formData.authentication, "grantType") === | |
| GrantType.AuthorizationCode && ( | |
| <FormInputContainer data-location-id={btoa("authentication.expiresIn")}> | |
| {this.renderInputTextControlViaFormControl({ | |
| configProperty: "authentication.expiresIn", | |
| label: "Authorization expires in (seconds)", | |
| placeholderText: "3600", | |
| dataType: "NUMBER", | |
| encrypted: false, | |
| isRequired: false, | |
| })} | |
| </FormInputContainer> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx`
around lines 686 - 695, The expiresIn field is rendered inside
renderOauth2Common() but per RestAPIForm.ts it belongs only to
AuthorizationCode; update the rendering so the FormInputContainer with
configProperty "authentication.expiresIn" (the
renderInputTextControlViaFormControl call) is shown only when the OAuth grant
type equals Authorization Code (e.g., check the grantType/auth.grantType value
or use the renderAuthorizationCode() path) instead of unconditionally in
renderOauth2Common(); either move the field into the Authorization Code render
helper or wrap it with a conditional like grantType === 'AUTHORIZATION_CODE' so
Client Credentials no longer sees the expiresIn input.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
1 participant
Description
This PR moves the
expires_infield to appear after thescopefield in the OAuth2 datasource configuration, keeping all authentication-related fields together for better UX.Fixes #31059
Changes
expiresIninput field fromrenderOauth2AuthorizationCodetorenderOauth2Common(afterscopeString)Testing
Type of change
Summary by CodeRabbit