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

603 better oauth datasources #611

Draft
wants to merge 35 commits into
base: develop
Choose a base branch
from
Draft

Conversation

NaderRNA
Copy link
Collaborator

@NaderRNA NaderRNA commented Oct 15, 2024

Ability to connect to datasources (airbyte) using OAuth login page

Uses passport.js to authenticate the OAuth, gets the refreshToken (which is the only thing airbyte needs to establish a connection to a source with OAuth).

Aims to keep current datasource flow as unchanged as possible.

We only have client ID and client Secrets for the following which will be the only ones supported:
(crossed out oauth providers are implemented)

  • Hubspot
  • Salesforce (forcedotcom)
  • Airtable
  • Google Ads (not sure if we have an approved clientId and secret)
  • Xero
  • Slack

@NaderRNA NaderRNA changed the base branch from master to develop October 15, 2024 22:39
Copy link

github-actions bot commented Oct 15, 2024

File Coverage
All files 96%
src/lib/utils/validationutils.ts 95%

Minimum allowed coverage is 80%

Generated by 🐒 cobertura-action against 0e14b90

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

ESLint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

…handling of new route to get the redirect url for oauth to be completed
server.get('/register', unauthedMiddlewareChain, checkSessionWelcome, renderStaticPage(app, '/register'));
server.get(
'/login',
unauthedMiddlewareChain,

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
webapp/src/router.ts Dismissed Show dismissed Hide dismissed
webapp/src/controllers/airbyte.ts Fixed Show fixed Hide fixed
webapp/src/controllers/airbyte.ts Fixed Show fixed Hide fixed
@NaderRNA NaderRNA linked an issue Oct 21, 2024 that may be closed by this pull request
@NaderRNA NaderRNA self-assigned this Oct 21, 2024
…ill need to patch bugs, fix issues, make dynamic
webapp/src/router.ts Fixed Show fixed Hide fixed
@NaderRNA
Copy link
Collaborator Author

NaderRNA commented Oct 22, 2024

Current status of the code:

  1. Successfully authenticates the user with specifically HubSpot currently.
  2. Passes the refreshToken back through the callback function (OAuth controller)
  3. Callback redirects back to datasource/add page with information required to set up datasource
  4. CreateDatasourceForm makes a POST with the sourceConfig set to the required config for the given oauth connector
  5. That then creates the TestDatasource object using the OAuth credentials
  6. Continue datasource creation as standard using the existing flow

The next few commits will likely be refactoring the implementation to do the following in order:

  1. pass the datasource name and datasource description through the flow
  2. Provide the user with better feedback indicating what's happening (adding a new loader for OAuth to say that authentication was successful and Agent Cloud is testing the datasource)
  3. Make the entire flow more compatible for other oauth providers, not just hubspot

@NaderRNA
Copy link
Collaborator Author

I merged with #622 since locally rabbitmq was broken on my ubuntu environment, those changes fixed the issue with airbyte being unable to connect to rabbitmq as a destination locally for development. Consider removing that merge if it's unnecessary to push to dev/prod

webapp/src/router.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/struct/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/controllers/oauth.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/lib/oauthsecret/index.ts Fixed Show fixed Hide fixed
webapp/src/router.ts Fixed Show fixed Hide fixed
webapp/src/router.ts Fixed Show fixed Hide fixed
oauthRouter.get(
'/hubspot/callback',
useSession,
useJWT,

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix AI 15 days ago

To fix the problem, we will introduce a rate-limiting middleware using the express-rate-limit package. This middleware will limit the number of requests a client can make to the server within a specified time window. We will apply this middleware to the routes that use the useJWT middleware to ensure that authorization operations are rate-limited.

  1. Install the express-rate-limit package if it is not already installed.
  2. Import the express-rate-limit package in the webapp/src/router.ts file.
  3. Set up a rate limiter with appropriate configuration (e.g., maximum 100 requests per 15 minutes).
  4. Apply the rate limiter to the routes that use the useJWT middleware.
Suggested changeset 2
webapp/src/router.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/src/router.ts b/webapp/src/router.ts
--- a/webapp/src/router.ts
+++ b/webapp/src/router.ts
@@ -32,2 +32,3 @@
 import Permissions from 'permissions/permissions';
+import rateLimit from 'express-rate-limit';
 import { PlanLimitsKeys, pricingMatrix, SubscriptionPlan } from 'struct/billing';
@@ -37,2 +38,7 @@
 
+const limiter = rateLimit({
+    windowMs: 15 * 60 * 1000, // 15 minutes
+    max: 100, // limit each IP to 100 requests per windowMs
+});
+
 import { restrictToFirstScrollableAncestor } from '@dnd-kit/modifiers';
@@ -130,2 +136,4 @@
 		useSession,
+		limiter,
+		limiter,
 		useJWT,
EOF
@@ -32,2 +32,3 @@
import Permissions from 'permissions/permissions';
import rateLimit from 'express-rate-limit';
import { PlanLimitsKeys, pricingMatrix, SubscriptionPlan } from 'struct/billing';
@@ -37,2 +38,7 @@

const limiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // limit each IP to 100 requests per windowMs
});

import { restrictToFirstScrollableAncestor } from '@dnd-kit/modifiers';
@@ -130,2 +136,4 @@
useSession,
limiter,
limiter,
useJWT,
webapp/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/package.json b/webapp/package.json
--- a/webapp/package.json
+++ b/webapp/package.json
@@ -123,3 +123,4 @@
     "tsconfig-paths": "^4.2.0",
-    "uuid": "^9.0.1"
+    "uuid": "^9.0.1",
+    "express-rate-limit": "^7.4.1"
   },
EOF
@@ -123,3 +123,4 @@
"tsconfig-paths": "^4.2.0",
"uuid": "^9.0.1"
"uuid": "^9.0.1",
"express-rate-limit": "^7.4.1"
},
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 7.4.1 None
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
oauthRouter.get(
'/salesforce/callback',
useSession,
useJWT,

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
oauthRouter.get(
'/xero/callback',
useSession,
useJWT,

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix AI 4 days ago

To fix the problem, we need to introduce rate limiting to the route handlers that perform authorization. The best way to achieve this is by using the express-rate-limit package, which allows us to easily set up and apply rate limiting middleware to the relevant routes.

  1. Install the express-rate-limit package.
  2. Import the express-rate-limit package in the webapp/src/router.ts file.
  3. Set up a rate limiter with appropriate configuration (e.g., maximum of 100 requests per 15 minutes).
  4. Apply the rate limiter to the relevant routes.
Suggested changeset 2
webapp/src/router.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/src/router.ts b/webapp/src/router.ts
--- a/webapp/src/router.ts
+++ b/webapp/src/router.ts
@@ -33,2 +33,8 @@
 import { PlanLimitsKeys, pricingMatrix, SubscriptionPlan } from 'struct/billing';
+import RateLimit from 'express-rate-limit';
+
+const limiter = RateLimit({
+    windowMs: 15 * 60 * 1000, // 15 minutes
+    max: 100, // max 100 requests per windowMs
+});
 
@@ -150,2 +156,3 @@
 		'/salesforce/callback',
+		limiter,
 		useSession,
@@ -171,2 +178,4 @@
 		'/xero/callback',
+		limiter,
+		limiter,
 		useSession,
EOF
@@ -33,2 +33,8 @@
import { PlanLimitsKeys, pricingMatrix, SubscriptionPlan } from 'struct/billing';
import RateLimit from 'express-rate-limit';

const limiter = RateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // max 100 requests per windowMs
});

@@ -150,2 +156,3 @@
'/salesforce/callback',
limiter,
useSession,
@@ -171,2 +178,4 @@
'/xero/callback',
limiter,
limiter,
useSession,
webapp/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/package.json b/webapp/package.json
--- a/webapp/package.json
+++ b/webapp/package.json
@@ -123,3 +123,4 @@
     "tsconfig-paths": "^4.2.0",
-    "uuid": "^9.0.1"
+    "uuid": "^9.0.1",
+    "express-rate-limit": "^7.4.1"
   },
EOF
@@ -123,3 +123,4 @@
"tsconfig-paths": "^4.2.0",
"uuid": "^9.0.1"
"uuid": "^9.0.1",
"express-rate-limit": "^7.4.1"
},
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 7.4.1 None
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
oauthRouter.get(
'/airtable/callback',
useSession,
useJWT,

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix AI 15 days ago

To fix the problem, we need to introduce rate limiting to the routes that use the useJWT middleware. We can use the express-rate-limit package to achieve this. The rate limiter should be configured to allow a reasonable number of requests per window of time to prevent abuse while not hindering legitimate users.

  1. Install the express-rate-limit package.
  2. Import the express-rate-limit package in the webapp/src/router.ts file.
  3. Configure the rate limiter with appropriate settings.
  4. Apply the rate limiter to the routes that use the useJWT middleware.
Suggested changeset 2
webapp/src/router.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/src/router.ts b/webapp/src/router.ts
--- a/webapp/src/router.ts
+++ b/webapp/src/router.ts
@@ -33,2 +33,8 @@
 import { PlanLimitsKeys, pricingMatrix, SubscriptionPlan } from 'struct/billing';
+import rateLimit from 'express-rate-limit';
+
+const limiter = rateLimit({
+    windowMs: 15 * 60 * 1000, // 15 minutes
+    max: 100, // limit each IP to 100 requests per windowMs
+});
 
@@ -207,2 +213,3 @@
 		'/airtable/callback',
+		limiter,
 		useSession,
@@ -221,3 +228,3 @@
 
-	server.use('/auth', useSession, passportInstance.session(), oauthRouter);
+	server.use('/auth', limiter, useSession, passportInstance.session(), oauthRouter);
 
EOF
@@ -33,2 +33,8 @@
import { PlanLimitsKeys, pricingMatrix, SubscriptionPlan } from 'struct/billing';
import rateLimit from 'express-rate-limit';

const limiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // limit each IP to 100 requests per windowMs
});

@@ -207,2 +213,3 @@
'/airtable/callback',
limiter,
useSession,
@@ -221,3 +228,3 @@

server.use('/auth', useSession, passportInstance.session(), oauthRouter);
server.use('/auth', limiter, useSession, passportInstance.session(), oauthRouter);

webapp/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/package.json b/webapp/package.json
--- a/webapp/package.json
+++ b/webapp/package.json
@@ -123,3 +123,4 @@
     "tsconfig-paths": "^4.2.0",
-    "uuid": "^9.0.1"
+    "uuid": "^9.0.1",
+    "express-rate-limit": "^7.4.1"
   },
EOF
@@ -123,3 +123,4 @@
"tsconfig-paths": "^4.2.0",
"uuid": "^9.0.1"
"uuid": "^9.0.1",
"express-rate-limit": "^7.4.1"
},
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 7.4.1 None
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@NaderRNA
Copy link
Collaborator Author

NaderRNA commented Nov 5, 2024

Somewhere along the line here a bug was introduced that replaced the file upload/drop area with the connectors dropdown

@ragyabraham
Copy link
Contributor

@NaderRNA what's happening with this PR?? where are we with this??

@NaderRNA
Copy link
Collaborator Author

NaderRNA commented Nov 12, 2024

Currently the branch supports custom OAuth implementation for any OAuth provider where we have a Client ID and Client Secret for the oauth. Currently HubSpot is completley functional using OAuth but there are some issues with out Airtable, Salesforce and Xero app credentials so I'm just trying to iron those out with Andrew.
If you'd like a version that can go to prod alongside BYO VectorDBs I can modify this code so it won't break prod and will work for HubSpot.
But currently if you know the OAuth provider endpoint and required scopes you can create any custom OAuth implementation using this implementation of the code, nothing is hardcoded so introduction of new OAuth implementation is a lot easier for both account creation and datasource auth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
3 participants