-
Notifications
You must be signed in to change notification settings - Fork 52.8k
fix(GitHub Node): Update auth urls for enterprise server #15533
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(GitHub Node): Update auth urls for enterprise server #15533
Conversation
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.
cubic found 1 issue across 3 files. Review it in cubic.dev
| type: 'hidden', | ||
| default: | ||
| '={{$self["server"] === "https://api.github.com" ? "https://github.com" : $self["server"]}}/login/oauth/authorize', | ||
| '={{$self["server"] === "https://api.github.com" ? "https://github.com" : "https://" + $self["server"].replace("https://", "").split("/")[0]}}/login/oauth/authorize', |
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.
URL parsing doesn't handle HTTP protocol or URLs without protocol prefix
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.
@michael-radency should this handle the case when it's HTTP, or do we just assume it's always HTTPS?
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.
Thanks! It's probably safe to assume, but not guaranteed - I'll update the expression
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
…community-issue-github-oauth-not-working-for-enterprise
| import { NodeTestHarness } from '@nodes-testing/node-test-harness'; | ||
|
|
||
| describe('Test Github Oauth2 Credentials Expression', () => { | ||
| new NodeTestHarness().setupTests(); |
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.
The tests fail locally: Your test suite must contain at least one test., I think you missed { workflowFiles: ['test.workflow.json'] } here
Workflow Test Results 📊
|
| Workflow ID | Workflow Name | Reason |
|---|---|---|
| 237 | BasicLLMChain:AzureChat | Workflow contains new data that previously did not exist. |
| 35 | Slack:User:getPresence info:UserProfile:get update... | Workflow contains new data that previously did not exist. |
| 257 | Agent:auto-fix:anthropic | Workflow contains new data that previously did not exist. |
| 53 | ConvertKit:CustomField:create getAll update delete... | Workflow contains new data that previously did not exist. |
|
|
…community-issue-github-oauth-not-working-for-enterprise
Workflow Test Results 📊 🔴 2 Failed,
|
| Workflow ID | Workflow Name | Reason |
|---|---|---|
| 237 | BasicLLMChain:AzureChat | Workflow contains 1 deleted data. |
| 258 | Agent:auto-fix:openai | Workflow contains 2 deleted data. |
⚠️ Warnings (2)
| Workflow ID | Workflow Name | Reason |
|---|---|---|
| 35 | Slack:User:getPresence info:UserProfile:get update... | Workflow contains new data that previously did not exist. |
| 257 | Agent:auto-fix:anthropic | Workflow contains new data that previously did not exist. |
|
✅ All Cypress E2E specs passed |
|
Got released with |
Summary
extract
hostnameforauthUrlandaccessTokenUrlfrom server urlRelated Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/NODE-2963/community-issue-github-oauth-not-working-for-enterprise-server
closes #15326
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)