-
Notifications
You must be signed in to change notification settings - Fork 10
Add support for api key connectors + initial migration of greenhouse … #616
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
Conversation
Bug Report
Comments? Email us. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
1 similar comment
|
✨ No issues found! Your code is sparkling clean! ✨ 🗒️ View all ignored comments in this repo
|
Stably Runner - Test Suite - 'Pre Merge CI Checks'Test Suite Run Result: 🟢 Success (2/2 tests passed) [dashboard] This comment was generated from stably-runner-action |
Bug ReportName: API Key Header Injection Vulnerability Comments? Email us. |
0f603d6
to
fa81c4c
Compare
Bug Report
Comments? Email us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok. Do you have videos of this working? Added non-blocking comments Would be good to add a test that this works in a similar way to OAuth. Using something like an Acme API Key similar to Acme (Oauth2) by adding a simple hard-coded api key server in our route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 22e5d51 in 1 minute and 38 seconds. Click for details.
- Reviewed
1249
lines of code in19
files - Skipped
3
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/api-v1/trpc/routers/connector.models.ts:80
- Draft comment:
Good use of optional chaining for auth properties. Ensure non-OAUTH2 connectors (eg API_KEY) intentionally yield undefined scopes. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. packages/ui-v1/__stories__/ConnectionSettingsForm.stories.tsx:12
- Draft comment:
Story wrapper correctly checks existence of 'connection_settings'. Confirm that new connectors like 'acme-apikey' and 'greenhouse2' provide this schema. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. packages/ui-v1/__stories__/ConnectorConfigForm.stories.tsx:12
- Draft comment:
ConnectorConfig form story uses a check for 'connector_config' in schemas. Verify that added connectors include this schema section as expected. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. packages/ui-v1/__stories__/ConnectorDisplay.stories.tsx:11
- Draft comment:
ConnectorDisplay story is consistent with other stories. No issues detected. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. packages/api-v1/__generated__/openapi.types.d.ts:8543
- Draft comment:
It appears that the connector name ‘greenhouse2’ is used in this list, whereas later in the file we have ‘connector.greenhouse’. Please verify if ‘greenhouse2’ is intentional or a typographical error. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. packages/api-v1/__generated__/openapi.types.d.ts:9335
- Draft comment:
Typographical note: The connector name 'connector.greenhouse2.discriminated_connection_settings' stands out compared to the other naming conventions. Please confirm if 'greenhouse2' is intentional or if it should be 'greenhouse'. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_7n2kb8xOA9CqyHSQ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Implemented comprehensive API key authentication support with both Acme API Key test connector and a new Greenhouse2 connector migration.
- Added
connectors/cnext/auth-apikey/createApiKeyConnectorServer.spec.ts
with test coverage for Basic Auth header verification - Added
connectors/cnext/auth-apikey/createAPIKeyConnectorServer.ts
with a factory function supporting both Basic Auth and custom header methods - Marked new
greenhouse2
connector as 'alpha' inconnectors/cnext/json-defs/greenhouse2.ts
until fully validated - Added test connector
acme-apikey
for API key verification using/acme-apikey/authorize
endpoint - Integrated
@opensdks/sdk-greenhouse@^0.0.8
incnext/package.json
for the new Greenhouse implementation
18 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
greenhouse2: { | ||
hasClient: false, | ||
hasServer: false, | ||
metadata: { | ||
displayName: 'Greenhouse 2', | ||
stage: 'alpha', | ||
verticals: ['ats'], | ||
logoUrl: '/_assets/logo-greenhouse2.svg', | ||
authType: 'API_KEY', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider suffixing the displayName with '(API Key)' to better differentiate from the original Greenhouse connector, following the pattern used for other auth variants like 'Zoho (OAuth)'
displayName: 'Acme API Key', | ||
stage: 'ga', | ||
verticals: ['social-media'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider whether 'social-media' is the appropriate vertical for an API key test connector. 'developer-tools' might be more suitable
hasServer: false, | ||
metadata: { | ||
displayName: 'Acme API Key', | ||
stage: 'ga', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Setting stage to 'ga' for a test connector could be misleading. Consider using 'alpha' to match the experimental nature
stage: 'ga', | |
stage: 'alpha', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 47e5ecf in 1 minute and 19 seconds. Click for details.
- Reviewed
54
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. connectors/cnext/auth-apikey/def.ts:18
- Draft comment:
Removed the .default({}) for connector_config. Confirm this change is intentional since it alters the default value behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. connectors/cnext/json-defs/acme-apikey.ts:10
- Draft comment:
Redundant connector_config and connection_settings have been removed. Verify that consumers don’t depend on explicit defaults here. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that consumers don't depend on explicit defaults, which violates the rule against asking the author to confirm or verify things. It doesn't provide a specific suggestion or point out a specific issue.
3. connectors/cnext/json-defs/greenhouse2.ts:10
- Draft comment:
Removed redundant connector_config and connection_settings entries. Ensure that this change aligns with the intended behavior of the auth schema. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is asking the PR author to ensure that the change aligns with the intended behavior, which violates the rule against asking for confirmation of intention. However, it does point out a specific change (removal of entries) which could be useful if it were rephrased to focus on the change itself rather than asking for confirmation.
Workflow ID: wflow_NpfMSkWNudezaB7L
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed ecc4e5d in 1 minute and 15 seconds. Click for details.
- Reviewed
60
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. connectors/cnext/auth-apikey/createApiKeyConnectorDef.ts:18
- Draft comment:
Return merged base schema instead of constructing a new object with just def shapes. Ensure this meets the intended behavior when no extra config is provided. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
The comment suggests a change in the code logic by returning a merged base schema instead of constructing a new object. However, it also asks the author to ensure that this meets the intended behavior, which violates the rule against asking for confirmation of intended behavior. The suggestion part is valid, but the request for confirmation is not.
2. connectors/cnext/auth-apikey/schemas.ts:28
- Draft comment:
Updated description text to correctly refer to 'API Key-based providers'. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. connectors/cnext/connectors.def.ts:17
- Draft comment:
Improved error message to include the unsupported auth type for clearer debugging. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. connectors/cnext/connectors.server.ts:29
- Draft comment:
Enhanced error message by JSON-stringifying the auth object for more detailed debugging output. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_PazpmjOEHA0m36dK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 6699417 in 1 minute and 1 seconds. Click for details.
- Reviewed
57
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/api-v1/__generated__/openapi.types.d.ts:318
- Draft comment:
Auto-generated file: ensure changes originate from the API spec and avoid manual modifications. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_KPNQT2hYrbBk0FPd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
…as greenhouse2
Important
Add support for API key connectors and migrate Greenhouse to Greenhouse 2, including schema updates and UI stories.
createApiKeyConnectorDef
increateApiKeyConnectorDef.ts
for defining API key connectors.createAPIKeyConnectorServer
increateApiKeyConnectorServer.ts
for server-side handling of API key connectors.apiKeySchemas
inschemas.ts
for API key schema definitions.greenhouse2
connector definition inconnectors.meta.ts
andgreenhouse2.ts
.openapi.json
andopenapi.types.d.ts
to includegreenhouse2
.acme-apikey
andgreenhouse2
inConnectionSettingsForm.stories.tsx
,ConnectorConfigForm.stories.tsx
, andConnectorDisplay.stories.tsx
.connectors.def.ts
andconnectors.server.ts
to handle API key connectors.getConnectorModel
inconnector.models.ts
to support new connector types.This description was created by
for 6699417. You can customize this summary. It will automatically update as commits are pushed.