-
Notifications
You must be signed in to change notification settings - Fork 52
Require authorize field in email provider config #265
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
base: main
Are you sure you want to change the base?
Conversation
authorize can still be undefined, it just has to be explicitly set as unintentionally skipping it can lead to vulnerabilities
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughChanges across configuration and type definitions: .gitignore now excludes tgz files, package.json restructures release scripts and renames npm-run-all to npm-run-all2, and EmailConfig.authorize field signature changes from optional method to union type with undefined. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
40-45: Release/test scripts are coherent; consider pinning prettier for theversionstepThe new
preversion,alpha, andreleasescripts give you a clear, test-gated release flow, and simplifyingprepublishOnlyto justbuildis reasonable if you standardize on those commands.One small portability nit: the
versionscript assumes aprettierbinary is on PATH, but it isn’t declared in this package’s devDependencies. To avoid surprises on fresh clones, consider adding Prettier here or calling it vianpx prettier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsontest-nextjs/package-lock.jsonis excluded by!**/package-lock.jsontest-router/package-lock.jsonis excluded by!**/package-lock.jsontest/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.gitignore(1 hunks)package.json(2 hunks)src/server/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/server/types.ts (1)
src/server/convex_types.ts (1)
GenericDoc(15-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (3)
.gitignore (1)
43-45: Ignoring test artifacts and package tarballs looks goodKeeping
test-results/ignored and adding*.tgzwill prevent local test output andnpm packartifacts from being committed; no issues from a tooling or DX standpoint.src/server/types.ts (1)
253-284: EmailConfig.authorize typing and docs align with the stated goalMaking
authorizerequired onEmailConfigwhile allowingundefinedmatches the intent of always having an explicit choice (OTP vs magic link), and the inline example does a nice job of showing how to bind the token to the original email. No functional or typing issues stand out here.package.json (1)
109-109: npm-run-all2 devDependency matches the new preversion usageAdding
npm-run-all2and usingrun-pinpreversionis consistent; just ensure this is the intended fork and that its CLI behavior matches the oldnpm-run-allexpectations (especially around parallel failure handling).
|
This change is at odds with Convex Auth's general support for Auth.js providers, as they don't include an authorize function. Putting this in draft for now. |
The
authorizefield in a custom Email provider config can be skipped unintentionally as it isn't required in types. Not providing an authorize function must be allowed to support "magic link" auth. This change:Unrelated maintenance:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.