-
Notifications
You must be signed in to change notification settings - Fork 7.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
refactor: v2 origin check error message #15143
Conversation
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Ignored Deployments
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (05/21/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add platform team as reviewer" took an action on this PR • (05/21/24)1 reviewer was added to this PR based on Keith Williams's automation. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
Current Playwright Test Results Summary✅ 6 Passing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 05/21/2024 03:06:03pm UTC) Run DetailsRunning Workflow PR Update on Github Actions Commit: a055787 Started: 05/21/2024 03:05:00pm UTC
|
|
4 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Namespacing Inline Embed Double install Embed Snippet with inline embed using a namespace
Retry 1 • Initial Attempt |
0.43% (1)1 / 232 runfailed over last 7 days |
61.64% (143)143 / 232 runsflaked over last 7 days |
Namespacing Inline Embed Add inline embed using a namespace without reload
Retry 1 • Initial Attempt |
0% (0)0 / 232 runsfailed over last 7 days |
63.79% (148)148 / 232 runsflaked over last 7 days |
Namespacing Inline Embed Double install Embed Snippet with inline embed without a namespace(i.e. default namespace)
Retry 1 • Initial Attempt |
0% (0)0 / 232 runsfailed over last 7 days |
64.66% (150)150 / 232 runsflaked over last 7 days |
Namespacing Different namespaces can have different init configs
Retry 1 • Initial Attempt |
0% (0)0 / 231 runsfailed over last 7 days |
61.47% (142)142 / 231 runsflaked over last 7 days |
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.
This is definitely an improvement, nice!
I think it's fine to be helpful to the user with a very descriptive and information-rich error message, given that this part of the code is behind an auth check, so we've already validated the user's access rights (& therefore we trust the user and want to be as helpful as possible).
Right? @supalarry & @ThyMinimalDev
--
For posterity, this could be improved even further with e.g.:
- add a section
https://cal.com/docs/platform/oauthclient
to our docs with a breadcrumb sublink to/platform/oauthclient#redirecturis
to explain (1) this required to make platform work in a browser environment (or certain requests will fail) & (2) why we're doing this check - Add a
/platform/errors
section to our docs (possibly auto-generated from the nextjs app/swagger?) - Update the error message to point to the docs
Example app fails because of incorrect origin error. The browser appends example app URL as the "origin" header, but it is not set as redirect uri in the OAuth client, so the requests fail.
Purpose of the PR is to improve error message in such case to be more helpful.