Skip to content
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

fix: fix refresh token flow in REST API OAuth #10875

Merged
merged 8 commits into from
Feb 9, 2022

Conversation

sumitsum
Copy link
Contributor

@sumitsum sumitsum commented Feb 4, 2022

Description

  • This PR adds the following config to REST API datasource when OAuth2.0 authentication is selected:
    • config to add client credentials on either header or body during refresh token flow.
    • if scope needs to be sent on refresh token flow.
  • The configurations above are required because various OAuth servers behave differently and prefer different configurations.
  • This PR also includes minor refactor to improve code debuggability (no change in code logic)
  • Client side changes to add config on the REST API datasource page.

Fixes #10493

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Manual

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Test coverage results 🧪

🔴 Total coverage has decreased
// Code coverage diff between base branch:release and head branch: fix/fix_refresh_token_10493 
Status File % Stmts % Branch % Funcs % Lines
🔴 total 55.02 (-0.01) 36.35 (-0.02) 34.71 (0) 55.4 (-0.01)
🔴 app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx 25 (-0.98) 14.08 (-1.3) 2.7 (-0.08) 27.66 (-1.23)

1. where to add client creds on refresh token flow.
2. if scope needs to be sent on refresh token flow.
@vercel
Copy link

vercel bot commented Feb 4, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/get-appsmith/appsmith/CXTeAFCcz2m2H4jjw41cVG86xAqZ
✅ Preview: https://appsmith-git-fix-fixrefreshtoken10493-get-appsmith.vercel.app

@nidhi-nair
Copy link
Contributor

LGTM, would recommend that the client adds these into a separate "Advanced" section since these settings won't usually be helpful for providers that follow the spec.

@github-actions github-actions bot added the Bug Something isn't working label Feb 4, 2022
- add dropdown components to capture advanced settings
add handling for client credentials based authorization
update form.json
Copy link
Contributor

@nidhi-nair nidhi-nair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a few UI changes

@sumitsum
Copy link
Contributor Author

sumitsum commented Feb 8, 2022

/ok-to-test sha=b023cbe

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1812674860.
Workflow: Appsmith External Integration Test Workflow.
Commit: b023cbe.
PR: 10875.

@sumitsum
Copy link
Contributor Author

sumitsum commented Feb 8, 2022

/ok-to-test sha=b023cbe

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1812919964.
Workflow: Appsmith External Integration Test Workflow.
Commit: b023cbe.
PR: 10875.

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/1812919964.
Commit: b023cbe.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
Edit input
scripting 439.58 404.47 293.09 331.24 295.43 331.24 352.76 18.76 16.78
painting 41.54 12.87 15.16 19.98 46.18 19.98 27.15 57.31 51.27
rendering 21.58 25.31 14.01 18.62 17.12 18.62 19.33 22.35 19.97
Clear input
scripting 782.67 726.6 729.57 722.94 746.32 729.57 741.62 3.32 2.97
painting 6.58 7.07 14.02 14.51 5.67 7.07 9.57 45.14 40.33
rendering 215.55 215.66 179.81 192.06 187.43 192.06 198.1 8.36 7.48
Edit input again
scripting 277.81 646.32 289.31 635.28 316.99 316.99 433.14 43.90 39.26
painting 5.65 14.97 22.7 5.2 18.98 14.97 13.5 58.22 52.07
rendering 21.73 20.01 22.26 24.08 25.45 22.26 22.71 9.29 8.32

@mohanarpit mohanarpit merged commit aa22904 into release Feb 9, 2022
@mohanarpit mohanarpit deleted the fix/fix_refresh_token_10493 branch February 9, 2022 04:08
sumitsum added a commit that referenced this pull request Feb 9, 2022
Co-authored-by: Segun Daniel Oluwadare <dodanieloluwadare@gmail.com>
(cherry picked from commit aa22904)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Refresh token is not being processed correctly
5 participants