-
Notifications
You must be signed in to change notification settings - Fork 9k
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(core): Fix oauth2 callback and add integration tests (no-changelog) #10272
Conversation
43aafe0
to
f583b16
Compare
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.
LGTM, nice tests 👏 Couple questions but can also be merged as is
import { setupTestServer } from '@test-integration/utils'; | ||
import type { SuperAgentTest } from '@test-integration/types'; | ||
|
||
describe('OAuth2 API', () => { |
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.
Do we need tests for Oauth1 as well? Or is the support for that deprecated?
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.
TBH, everytime I've asked about oauth1, I've only heard that some folks might be using it for Twitter, but I haven't actually seen anyone use it.
In the past we've had bugs in oauth1 flow, and no one reported them either.
In the last few months, we've refactored this code to make oauth1 flow use a lot more code shared with oauth2 flow.
So technically this already covers quite a bit of the oauth1 flow as well.
But, I don't think it's worth the effort to write dedicated integration tests for oauth1.
We can add these tests when someone reports a bug in that code.
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
✅ All Cypress E2E specs passed |
✅ All Cypress E2E specs passed |
* master: refactor(core): Decouple workflow created, saved, deleted events from internal hooks (no-changelog) (#10264) fix(core): Fix oauth2 callback and add integration tests (no-changelog) (#10272) fix(n8n Form Trigger Node): Improve copy and a few tweaks (no-changelog) (#10243) fix(core): Fix expressions in webhook nodes(Form, Webhook) to access previous node's data (#10247) refactor(core): Port endpoints config (no-changelog) (#10268) feat(AWS Lambda Node): Add support for paired items (no-changelog) (#10270) fix: Loop node no input data shown (#10224) fix(core): Make OAuth1/OAuth2 callback not require auth (#10263) 🚀 Release 1.53.0 (#10266) fix(core): Upgrade @n8n/vm2 to address CVE‑2023‑37466 (#10265) feat(Calendly Trigger Node): Add OAuth Credentials Support (#10251) test(core): Improve and expand log streaming tests (no-changelog) (#10261) refactor(editor): Migrate `WorkflowExecutionsListView.vue` to composition API (no-changelog) (#10198) fix(core): Restore log event `n8n.workflow.failed` (#10253) test(core): Stop showing JWT warning during test runs (no-changelog) (#10255) fix(Postgres Node): Expressions in query parameters for Postgres executeQuery operation (#10217) fix(editor): Fix workflow execution list scrolling after filter change (#10226)
Got released with |
Summary
Accidentally broke oauth callback in #10263 .
To prevent that from happening again, this PR also adds integration tests.
Review / Merge checklist