Feature/add connection token #2671
Conversation
|
Warning Rate limit exceeded@komyg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 28 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes involve updates to the database schema, authentication processes, and connection management across multiple components. Key modifications include the addition of a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant OAuthController
participant ConnectionService
User->>Frontend: Initiate Connection Creation
Frontend->>OAuthController: Request Authentication
OAuthController->>ConnectionService: Check for connection_id
alt connection_id exists
ConnectionService-->>OAuthController: Use existing connection_id
else connection_id missing
ConnectionService-->>OAuthController: Generate connectionToken
end
OAuthController-->>Frontend: Return Authentication Result
Frontend-->>User: Display Success/Error Message
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: 1
Outside diff range, codebase verification and nitpick comments (1)
packages/shared/lib/services/connection.service.ts (1)
93-118: Potential Issue withconnectionTokenHandlingThe current implementation correctly handles the absence of
connectionTokenby checking for existing connections and updating them. However, there is no evidence of logic to handle the case whenconnectionTokenis provided, which suggests that the feature might not be fully implemented. Please review the code to ensure that new connections are inserted withconnectionTokenwhen it is provided.
- Ensure that there is logic outside the
if (!connectionToken)block to handle the insertion of new connections withconnectionToken.Analysis chain
Verify the handling of
connectionTokenin the new conditional block.Ensure that the new conditional block correctly handles the presence and absence of
connectionToken:
- If
connectionTokenis not provided, the method should proceed with the existing logic to check if a connection exists and update or insert it accordingly.- If
connectionTokenis provided, the method should skip the existing logic and directly insert a new connection with theconnectionTokenincluded in the encrypted connection object.Run the following script to verify the handling of
connectionToken:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of `connectionToken` in the `upsertConnection` method. # Test 1: Search for the conditional block that checks the presence of `connectionToken`. # Expect: The conditional block should be present. ast-grep --lang typescript --pattern $'if (!connectionToken) { $$$ }' # Test 2: Search for the existing logic to check if a connection exists and update or insert it accordingly within the conditional block. # Expect: The existing logic should be present within the conditional block. ast-grep --lang typescript --pattern $'if (!connectionToken) { $$$ const storedConnection = await this.checkIfConnectionExists($$$); $$$ if (storedConnection) { $$$ const encryptedConnection = encryptionManager.encryptConnection({ $$$ }); $$$ const connection = await db.knex .from<StoredConnection>(\`_nango_connections\`) .where({ id: storedConnection.id!, deleted: false }) .update(encryptedConnection) .returning(\'*\'); $$$ } }' # Test 3: Search for the logic to insert a new connection with `connectionToken` outside the conditional block. # Expect: The logic to insert a new connection with `connectionToken` should be present outside the conditional block. ast-grep --lang typescript --pattern $'const connection = await db.knex .from<StoredConnection>(\`_nango_connections\`) .insert( encryptionManager.encryptConnection({ $$$ connection_token: connectionToken, $$$ }) ) .returning(\'*\');'Length of output: 3614
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- packages/database/lib/migrations/20240903153801_create_connection_token.cjs (1 hunks)
- packages/frontend/lib/index.ts (5 hunks)
- packages/server/lib/controllers/environment.controller.ts (1 hunks)
- packages/server/lib/controllers/oauth.controller.ts (8 hunks)
- packages/shared/lib/models/Auth.ts (1 hunks)
- packages/shared/lib/models/Connection.ts (3 hunks)
- packages/shared/lib/services/connection.service.ts (3 hunks)
- packages/webapp/src/pages/Connection/Create.tsx (3 hunks)
- packages/webapp/src/utils/api.tsx (1 hunks)
Additional comments not posted (24)
packages/database/lib/migrations/20240903153801_create_connection_token.cjs (1)
7-12: Verify the impact of making theconnection_idcolumn nullable and consider using Knex's schema builder methods.Please consider the following:
- Verify the impact of making the
connection_idcolumn nullable:
- Ensure that the existing code handles nullable
connection_idvalues correctly.- If the
connection_idcolumn is used as a foreign key in other tables, ensure that those tables allow null values for this column.Run the following script to verify the impact:
- Consider using Knex's schema builder methods for better portability across different databases. For example:
await knex.schema.alterTable('_nango_connections', (table) => { table.uuid(CONNECTION_TOKEN_COLUMN).unique(); table.uuid(CONNECTION_ID_COLUMN).nullable().alter(); });packages/shared/lib/models/Connection.ts (3)
12-12: LGTM!The addition of the optional
connection_tokenproperty to theBaseConnectioninterface looks good. It enhances the flexibility of the interface without breaking existing code.
68-68: LGTM!The addition of the optional
connection_tokenproperty to theNangoConnectioninterface looks good. It enhances the flexibility of the interface without breaking existing code.
83-83: LGTM!The addition of the optional
connection_tokenproperty to theConnectionListinterface looks good. It enhances the flexibility of the interface without breaking existing code.packages/shared/lib/models/Auth.ts (1)
24-24: LGTM!The addition of the optional
connectionTokenproperty to theOAuthSessioninterface is a valid change. The property is correctly marked as optional and its type is appropriately defined asstring | undefined.packages/server/lib/controllers/environment.controller.ts (1)
Line range hint
80-95: Verify the handling ofundefinedconnectionIdin thehmacService.digestmethod.The conditional check for
connectionIdhas been removed. Ensure that thehmacService.digestmethod handles the case whenconnectionIdisundefinedto avoid any potential issues.Run the following script to verify the handling of
undefinedconnectionId:packages/webapp/src/utils/api.tsx (1)
665-669: LGTM!The changes to the
useGetHmacAPIfunction look good:
- Making the
connectionIdparameter optional allows for more flexible usage of the function.- The updated URL construction logic correctly handles the presence or absence of
connectionId, improving maintainability and reducing potential errors.These modifications align with the alterations mentioned in the summary.
packages/frontend/lib/index.ts (6)
29-33: LGTM!The new interface
AuthenticationResultis well-defined and follows the existing coding conventions.
39-47: LGTM!The new type
AuthConfigOptionsis correctly defined as a union type and includes all the relevant types.
128-149: LGTM!The new method
authenticationis well-implemented and follows the existing coding conventions. The validation checks and URL construction are handled appropriately.
159-159: LGTM!The changes to the
authmethod are minimal and do not affect its functionality. The deprecation notice is appropriate.
185-209: LGTM!The changes to the
callAuthmethod correctly handle bothAuthResultandAuthenticationResult. The updatedsuccessHandlerfunction and promise resolution logic are implemented appropriately.
484-488: LGTM!Making the
connectionIdparameter optional in thetoQueryStringmethod is consistent with the changes made to theauthmethod and provides flexibility.packages/server/lib/controllers/oauth.controller.ts (4)
56-60: LGTM!The changes ensure that a valid connection identifier (
connectionId) is always available by using theconnection_idfrom the request query if it exists, otherwise falling back to a newly generatedconnectionToken.
109-112: LGTM!The HMAC verification process is updated to conditionally include
connectionIdonly if it was received from the request, allowing for a more flexible verification.
Line range hint
831-931: LGTM!The
connectionTokenis now included in various logging statements, ensuring that it is consistently tracked throughout the OAuth flow. This may facilitate better debugging and monitoring of connection-related activities.
Line range hint
956-1065: LGTM!The
connectionTokenis included in the logging statement after receiving the token response and passed to theconnectionService.upsertConnectionmethod, ensuring that it is consistently tracked and associated with the connection.packages/webapp/src/pages/Connection/Create.tsx (4)
33-33: LGTM!The changes to the
connectionIdstate variable look good:
- Changing the type to
string | nullallows for cases where the connection ID may not be available immediately.- Initializing it as
nullis a best practice when the initial value is unknown.These changes enhance the component's flexibility in handling different connection states.
202-220: LGTM!The added code block to handle missing
connection_idlooks good:
- It enhances the component's functionality by initiating the authentication process when
connection_idis not provided.- The
nango.authenticationmethod is called with the necessary parameters to handle the authentication flow.- Showing success and error messages improves the user experience by providing appropriate feedback.
- Navigating to the connections page on successful authentication ensures a smooth user flow.
These changes make the component more robust in handling different scenarios during the connection creation process.
204-210: LGTM!The parameters passed to the
nango.authenticationmethod look good:
- The
integration_unique_keyis correctly obtained from the form data.- The
user_scopeis obtained from theselectedScopesstate variable.- The
paramsare obtained from theconnectionConfigParamsstate variable or an empty object.- The
authorization_paramsare obtained from theauthorizationParamsstate variable or an empty object.- The
hmacis obtained from thehmacDigeststate variable or an empty string.- The
credentialsare obtained based on theauthModeand the corresponding state variables.These parameters ensure that the necessary data is passed to the authentication method for proper handling of the connection creation process.
211-219: LGTM!The handling of success and error scenarios after the authentication process looks good:
- On successful authentication:
- A success toast message is shown to provide visual feedback to the user.
- An analytics event is tracked to monitor and analyze user actions.
- The data is mutated to ensure the UI reflects the updated state.
- The user is navigated to the connections page, providing a logical flow after the successful connection creation.
- If an error occurs during authentication:
- An error message is set based on the error type to provide meaningful feedback to the user.
These changes ensure proper handling of the authentication outcome and enhance the user experience by providing appropriate feedback and navigation.
packages/shared/lib/services/connection.service.ts (3)
73-73: LGTM!The new optional
connectionTokenparameter in the method signature looks good.
83-83: LGTM!The new optional
connectionTokenparameter in the method signature looks good.
126-126: LGTM!The inclusion of
connectionTokenin the encrypted connection object when inserting a new connection looks good.
| exports.down = async function (knex) { | ||
| await knex.schema.raw(`ALTER TABLE "_nango_connections" DROP COLUMN "${CONNECTION_TOKEN_COLUMN}"`); | ||
| await knex.schema.raw(`ALTER TABLE "_nango_connections" ALTER COLUMN "${CONNECTION_ID_COLUMN}" SET NOT NULL;`); | ||
| await knex.schema.raw(`ALTER TABLE "_nango_oauth_sessions" DROP COLUMN "${CONNECTION_TOKEN_COLUMN}"`); | ||
| await knex.schema.raw(`ALTER TABLE "_nango_oauth_sessions" ALTER COLUMN "${CONNECTION_ID_COLUMN}" SET NOT NULL;`); | ||
| }; |
There was a problem hiding this comment.
Consider using Knex's schema builder methods for better portability.
Similar to the up function, consider using Knex's schema builder methods for better portability across different databases. For example:
await knex.schema.alterTable('_nango_connections', (table) => {
table.dropColumn(CONNECTION_TOKEN_COLUMN);
table.uuid(CONNECTION_ID_COLUMN).notNullable().alter();
});
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/frontend/lib/index.ts (8 hunks)
- packages/server/lib/clients/publisher.client.ts (1 hunks)
- packages/server/lib/controllers/oauth.controller.ts (9 hunks)
- packages/webapp/src/pages/Connection/Create.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/frontend/lib/index.ts
- packages/server/lib/controllers/oauth.controller.ts
- packages/webapp/src/pages/Connection/Create.tsx
Additional comments not posted (1)
packages/server/lib/clients/publisher.client.ts (1)
208-215: LGTM!The code changes to the
notifySuccessmethod are approved:
- The method signature has been correctly updated to include an optional
connectionTokenparameter.- The
connectionTokenis correctly included in the JSON payload sent to the client.- The changes are consistent with the AI-generated summary and do not introduce any issues or inconsistencies.
Also applies to: 221-221
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .vscode/launch.json (1 hunks)
- docker-compose.yaml (1 hunks)
- packages/shared/lib/services/connection.service.ts (4 hunks)
Files skipped from review due to trivial changes (2)
- .vscode/launch.json
- docker-compose.yaml
Files skipped from review as they are similar to previous changes (1)
- packages/shared/lib/services/connection.service.ts
komyg
force-pushed
the
feature/add-connection-token
branch
from
445840b
to
69ec8f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- .vscode/launch.json (1 hunks)
- docker-compose.yaml (1 hunks)
- docs-v2/integrate/guides/advanced/secure-frontend-sdk.mdx (2 hunks)
- docs-v2/integrate/guides/authorize-an-api.mdx (1 hunks)
- docs-v2/integrate/overview.mdx (1 hunks)
- docs-v2/introduction.mdx (2 hunks)
- docs-v2/reference/sdks/frontend.mdx (4 hunks)
- packages/database/lib/migrations/20240903153801_create_connection_token.cjs (1 hunks)
- packages/frontend/lib/index.ts (8 hunks)
- packages/server/lib/clients/publisher.client.ts (1 hunks)
- packages/server/lib/controllers/environment.controller.ts (1 hunks)
- packages/server/lib/controllers/oauth.controller.ts (9 hunks)
- packages/shared/lib/models/Auth.ts (1 hunks)
- packages/shared/lib/models/Connection.ts (3 hunks)
- packages/shared/lib/services/connection.service.ts (4 hunks)
- packages/webapp/src/pages/Connection/Create.tsx (5 hunks)
- packages/webapp/src/utils/api.tsx (1 hunks)
Files skipped from review due to trivial changes (4)
- .vscode/launch.json
- docker-compose.yaml
- docs-v2/introduction.mdx
- packages/database/lib/migrations/20240903153801_create_connection_token.cjs
Files skipped from review as they are similar to previous changes (7)
- packages/server/lib/controllers/environment.controller.ts
- packages/server/lib/controllers/oauth.controller.ts
- packages/shared/lib/models/Auth.ts
- packages/shared/lib/models/Connection.ts
- packages/shared/lib/services/connection.service.ts
- packages/webapp/src/pages/Connection/Create.tsx
- packages/webapp/src/utils/api.tsx
Additional comments not posted (22)
docs-v2/integrate/overview.mdx (2)
23-23: LGTM!The code change is consistent with the AI-generated summary. The
connectionIdvariable is no longer being passed to thenango.authfunction.
27-27: LGTM!The code change is consistent with the AI-generated summary. The
connectionTokenobtained from theresultof the authentication process is now being passed to thesaveToDatabasefunction.docs-v2/integrate/guides/advanced/secure-frontend-sdk.mdx (2)
11-11: LGTM!The code changes are approved.
151-154: LGTM!The code changes are approved.
docs-v2/integrate/guides/authorize-an-api.mdx (1)
82-82: LGTM!The change in the method name from
nango.auth()tonango.authentication()improves clarity and aligns with the alterations list.packages/server/lib/clients/publisher.client.ts (1)
208-215: LGTM!The code changes are approved for the following reasons:
- The
notifySuccessmethod signature has been correctly updated to include an optionalconnectionTokenparameter.- The
connectionTokenis correctly included in the JSON payload.- The control flow remains intact.
- The method now supports a broader range of use cases by accommodating the new parameter.
Also applies to: 221-221
docs-v2/reference/sdks/frontend.mdx (9)
27-27: LGTM!The code segment is approved.
52-54: LGTM!The code segment is approved.
59-63: LGTM!The code segment is approved.
64-117: LGTM!The code segment is approved.
119-148: LGTM!The code segment is approved.
150-151: LGTM!The code segment is approved.
208-208: LGTM!The code segment is approved.
212-212: LGTM!The code segment is approved.
271-271: LGTM!The code segment is approved.
packages/frontend/lib/index.ts (7)
29-33: LGTM!The new
AuthenticationResultinterface is well-defined and serves the purpose of encapsulating the results of the authentication process.
39-47: LGTM!The new
AuthConfigOptionstype alias consolidates various credential types into a single type union, which improves code readability and maintainability.
128-149: LGTM!The new
authenticationmethod is well-structured and handles the authentication process effectively. The validation checks ensure that only standard OAuth2 flows are supported, and the error handling is appropriate.
Line range hint
159-183: LGTM!The changes to the
authmethod are consistent with the introduction of the newauthenticationmethod. The deprecation notice is helpful for guiding users towards the preferred authentication approach.
185-207: LGTM!The changes to
callAuthreflect the updates in the authentication flow and ensure compatibility with bothAuthResultandAuthenticationResult. The modified success handler handles the presence ofconnectionTokenappropriately.
Line range hint
486-537: LGTM!Making the
connectionIdparameter optional intoQueryStringallows for greater flexibility in generating query strings. The changes are consistent with the rest of the codebase.
Line range hint
666-697: LGTM!The changes to the
handleMessagemethod in theAuthorizationModalclass are consistent with the updates in the authentication flow. The modified success handler signature aligns with the changes made in theNangoclass.
komyg
force-pushed
the
feature/add-connection-token
branch
from
69ec8f3
to
5f009f1
Compare
| * @param options - Optional. Additional options for authorization | ||
| * @returns A promise that resolves with the authorization result | ||
| */ | ||
| public async authentication(providerConfigKey: string, options?: AuthConfigOptions): Promise<AuthenticationResult> { |
There was a problem hiding this comment.
I think I would prefer a different nameauthenticate perhaps?
Describe your changes
connection_tokencolumn to the_nango_connectionstable.connection_tokencolumn to the_nango_oauth_sessionstable.authenticationmethod in the frontend SDK that does not require theconnectionIdand that returns the newconnectionTokencreated by the backend.authmethod in the frontend SDK.createcomponent to call the newauthenticationmethod, that does not require aconnectionIdif noconnectionIdis set by the user.connectionTokenand save it.connectionTokenas the default value for theconnectionId.ConnectionServiceto save theconnectionTokenon the database.connectionId.Issue ticket number and link
Checklist before requesting a review (skip if just adding/editing APIs & templates)
Summary by CodeRabbit
New Features
connection_tokenfeature in the database schema for enhanced connection management.Bug Fixes
Documentation
connectionTokenproperties for better flexibility in OAuth sessions.