Add login module with client and server authentication capabilities #7106
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a comprehensive login/authentication system to the Changes
Sequence Diagram(s)sequenceDiagram
participant ClientApp
participant LoginModule
participant ServerAPI
participant WalletProvider
ClientApp->>LoginModule: login({type: "email"/"phone"/"wallet"/"jwt"/"social", ...})
alt JWT Login
LoginModule->>WalletProvider: connectWithJwt(jwt)
LoginModule-->>ClientApp: AuthenticatedAccount
else Email/Phone Login
LoginModule->>WalletProvider: preAuth(email/phone)
LoginModule-->>ClientApp: { verifyOtp(otp) }
ClientApp->>LoginModule: verifyOtp(otp)
LoginModule->>WalletProvider: verifyOtp(otp)
LoginModule-->>ClientApp: AuthenticatedAccount
else Wallet Login
LoginModule->>WalletProvider: connect(wallet, chain)
LoginModule-->>ClientApp: AuthenticatedAccount
else Social Login
LoginModule->>WalletProvider: connectWithSocial(option)
LoginModule-->>ClientApp: AuthenticatedAccount
end
ClientApp->>AuthenticatedAccount: getJWT()
AuthenticatedAccount->>ServerAPI: GET /payload (address, chainId)
ServerAPI-->>AuthenticatedAccount: payload
AuthenticatedAccount->>WalletProvider: signPayload(payload)
AuthenticatedAccount->>ServerAPI: POST /login (signedPayload)
ServerAPI-->>AuthenticatedAccount: JWT, expiration
ClientApp->>AuthenticatedAccount: logout()
AuthenticatedAccount->>ServerAPI: POST /logout
ServerAPI-->>AuthenticatedAccount: Session cleared
sequenceDiagram
participant ClientApp
participant ServerAPI
ClientApp->>ServerAPI: GET /is-logged-in (with JWT cookie or header)
alt JWT valid
ServerAPI-->>ClientApp: { address, jwt, expiration }
else JWT invalid/expired
ServerAPI-->>ClientApp: 401 Unauthorized
end
sequenceDiagram
participant Developer
participant AuthHandler
participant IntegrationUtil
Developer->>AuthHandler: createAuthHandler({ client, wallet, ... })
AuthHandler-->>Developer: AuthRouter
Developer->>IntegrationUtil: toNodeHandler(AuthRouter)
IntegrationUtil-->>Developer: Express-compatible handler
Developer->>IntegrationUtil: toNextJsHandler(AuthRouter)
IntegrationUtil-->>Developer: Next.js API route handlers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
jnsdls
force-pushed
the
Add_login_functionality_with_multiple_authentication_methods
branch
2 times, most recently
from
ed82b58 to
c0d0f67
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
jnsdls
changed the title
jnsdls
added
the
DO NOT MERGE
Graphite App
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/thirdweb/src/wallets/types.ts (1)
38-40: Type guard implementation is correctThe
isSocialAuthOptiontype guard uses TypeScript's type predicate pattern properly. For a potentially more efficient implementation with larger sets of options, consider using a Set instead of array lookup.-export function isSocialAuthOption(option: string): option is SocialAuthOption { - return socialAuthOptions.includes(option as SocialAuthOption); +export function isSocialAuthOption(option: string): option is SocialAuthOption { + const socialOptionSet = new Set(socialAuthOptions); + return socialOptionSet.has(option as SocialAuthOption); }However, given the small number of options, the current implementation is likely fast enough.
packages/thirdweb/package.json (1)
162-162: Consider adding wildcard mapping for deeper paths.
Currently only"login"is mapped intypesVersions. If consumers need typings for subpaths (e.g.,"thirdweb/login/client"), you may want to include:"login/*": ["./dist/types/exports/login/*.d.ts"]packages/thirdweb/src/login/README.md (1)
63-72: Minor style: hyphenate compound adjective “Rate-limiting”For consistency with bullet 160 (
Rate limiting support) use a hyphen when the phrase modifies a noun.- - Rate limiting support + - Rate-limiting support
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
packages/thirdweb/package.json(6 hunks)packages/thirdweb/src/exports/login.ts(1 hunks)packages/thirdweb/src/exports/thirdweb.ts(1 hunks)packages/thirdweb/src/login/README.md(1 hunks)packages/thirdweb/src/login/client/index.ts(1 hunks)packages/thirdweb/src/login/client/login.ts(1 hunks)packages/thirdweb/src/login/index.ts(1 hunks)packages/thirdweb/src/login/server/auth-handler.ts(1 hunks)packages/thirdweb/src/login/server/index.ts(1 hunks)packages/thirdweb/src/login/server/integrations/next-js.ts(1 hunks)packages/thirdweb/src/login/server/integrations/node.ts(1 hunks)packages/thirdweb/src/wallets/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/thirdweb/src/login/client/login.ts (6)
packages/thirdweb/src/login/client/index.ts (2)
LoginOptions(1-1)login(1-1)packages/thirdweb/src/wallets/types.ts (2)
AuthOption(53-53)isSocialAuthOption(38-40)packages/thirdweb/src/exports/thirdweb.ts (5)
Chain(33-33)ThirdwebClient(26-26)PreparedTransaction(109-109)sendTransaction(142-142)sendBatchTransaction(147-147)packages/thirdweb/src/exports/storage.ts (1)
AsyncStorage(13-13)packages/thirdweb/src/exports/wallets.ts (1)
Account(22-22)packages/thirdweb/src/login/server/auth-handler.ts (1)
AuthRouter(222-222)
packages/thirdweb/src/login/server/auth-handler.ts (5)
packages/thirdweb/src/login/index.ts (3)
CreateAuthHandlerOptions(10-10)createAuthHandler(9-9)login(5-5)packages/thirdweb/src/login/server/index.ts (2)
CreateAuthHandlerOptions(3-3)createAuthHandler(2-2)packages/thirdweb/src/exports/thirdweb.ts (2)
ThirdwebClient(26-26)isAddress(313-313)packages/thirdweb/src/login/client/login.ts (1)
login(42-153)packages/thirdweb/src/exports/utils.ts (1)
decodeJWT(175-175)
🪛 LanguageTool
packages/thirdweb/src/login/README.md
[uncategorized] ~160-~160: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e verification - Secure token storage - Rate limiting support - Framework integrations ## Se...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~194-~194: Loose punctuation mark.
Context: ...xports - login(options: LoginOptions): Initiates the login process - `LoginPar...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~199-~199: Loose punctuation mark.
Context: ...dler(options: CreateAuthHandlerOptions): Creates an authentication handler - to...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~201-~201: Loose punctuation mark.
Context: ...tify, etc.) - toNextJsHandler(handler): Converts auth handler to Next.js API ro...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit Tests
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Build Packages
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
packages/thirdweb/src/exports/thirdweb.ts (1)
89-93: Correctly exports login functionality under a namespaceThe new
Loginnamespace export follows the established pattern used for other major feature exports likeBridge,Insight, andEngine. This approach maintains consistent architecture while avoiding potential naming collisions.packages/thirdweb/src/exports/login.ts (1)
1-1: LGTM: Clean re-export patternThis file follows the standard pattern of re-exporting from a central module, enabling users to import directly from
thirdweb/loginrather than from the internal file structure. This approach simplifies imports for library consumers.packages/thirdweb/src/login/client/index.ts (1)
1-1: Good API design with clear namingThe re-export provides a clean public API for the login functionality. Renaming
LoginOptionstoLoginParamsfor the public export improves clarity for consumers of the library, indicating these are parameters for the login function.packages/thirdweb/src/login/server/index.ts (1)
1-6: Exports are correct and concise.
This central re-export module neatly surfacescreateAuthHandler, its options type, and the integration adapters (toNodeHandler,toNextJsHandler). No issues detected.packages/thirdweb/src/login/server/integrations/next-js.ts (1)
33-47: Adapter logic looks sound.
ThetoNextJsHandlerfunction properly detects whetherauthis an object with a.handlermethod or a raw function, and correctly binds it to bothGETandPOST. No functional issues found.packages/thirdweb/src/login/server/integrations/node.ts (1)
39-47: Node.js adapter is implemented correctly.
The conditional unwrap of.handlerand forwarding tobetter-call/node’stoNodeconverter is concise and correct. Ready for use in Express or similar frameworks.packages/thirdweb/package.json (3)
63-67: New login export entry looks correct.
Consumers can now import login APIs directly via"thirdweb/login". Ensure these paths align with your publisheddist/esm/exports/login.jsanddist/cjs/exports/login.js.
210-211: Verify newly added dependencies.
better-callandzodwere introduced to support server-side handlers and schema validation. Confirm these versions are compatible and that your lockfile (e.g.,pnpm-lock.yamlorpackage-lock.json) is updated to avoid installation or CI failures.Also applies to: 222-223
295-296: Lint and build scripts updated fornodenext.
Switching thelintandbuild:typesscripts to use--module nodenext --moduleResolution nodenextaligns with your ESM setup. Double-check thattsconfig.build.jsonmatches these options to prevent type-checking mismatches.Also applies to: 306-306
packages/thirdweb/src/login/index.ts (1)
1-6: Centralized login entrypoint is clear.
Re-exporting under theClientandServernamespaces plus direct exports forloginandcreateAuthHandlerdelivers an intuitive API surface. Everything aligns with the package exports.
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (5.83%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7106 +/- ##
==========================================
- Coverage 55.59% 55.27% -0.33%
==========================================
Files 901 909 +8
Lines 58121 58510 +389
Branches 4068 4069 +1
==========================================
+ Hits 32315 32342 +27
- Misses 25701 26063 +362
Partials 105 105
🚀 New features to boost your workflow:
|
jnsdls
force-pushed
the
Add_login_functionality_with_multiple_authentication_methods
branch
from
01b4576 to
f72823c
Compare
jnsdls
force-pushed
the
Add_login_functionality_with_multiple_authentication_methods
branch
from
f72823c to
791d39b
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/thirdweb/src/login/README.md (1)
192-202:⚠️ Potential issueInconsistent type naming in API Reference
Under Client Exports you listlogin(options: LoginOptions)thenLoginParams, but earlier the doc definesLoginOptions(notLoginParams). Please unify the type name throughout the README.🧰 Tools
🪛 LanguageTool
[uncategorized] ~194-~194: Loose punctuation mark.
Context: ...xports -login(options: LoginOptions): Initiates the login process - `LoginPar...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~199-~199: Loose punctuation mark.
Context: ...dler(options: CreateAuthHandlerOptions): Creates an authentication handler -to...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~201-~201: Loose punctuation mark.
Context: ...tify, etc.) -toNextJsHandler(handler): Converts auth handler to Next.js API ro...(UNLIKELY_OPENING_PUNCTUATION)
🧹 Nitpick comments (3)
packages/thirdweb/src/login/README.md (3)
33-37: Clarify OTP verification return type
AfterLogin.Client.loginreturns arequires_otpstatus, callingverifyOtplikely returns an authenticated account. Consider adding a note thatverifyOtpyields the same account interface (with JWT methods, transaction capabilities, etc.).
122-141: Consider adding Next.js Pages Router guidance
You cover App Router integration. For completeness, you may also provide an example for Next.js Pages Router (e.g., inpages/api/auth/[...all].ts) so users on older versions have a reference.
156-189: Standardize list formatting
Several sections use unpunctuated list items (e.g., Server-Side Features, Security Considerations), whereas the API Reference items include colons and full sentences. For consistency and readability, consider adopting a uniform list style across all sections (either punctuated or unpunctuated).🧰 Tools
🪛 LanguageTool
[uncategorized] ~160-~160: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e verification - Secure token storage - Rate limiting support - Framework integrations ## Se...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🛑 Comments failed to post (1)
packages/thirdweb/src/login/README.md (1)
61-74:
⚠️ Potential issueDocument all
logininput parameters
TheLoginOptionstype only coversclient,options, andbaseURL, but examples pass atypediscriminator plus credentials (phoneNumber,wallet,jwt). The documentation needs a discriminated-union definition reflecting all required fields for each authentication method.🤖 Prompt for AI Agents
In packages/thirdweb/src/login/README.md around lines 61 to 74, the LoginOptions type is incomplete and does not document all input parameters used in login, such as the type discriminator and credentials like email, phoneNumber, wallet, and jwt. Update the LoginOptions definition to a discriminated union that clearly defines each authentication method with its required fields, including the type property and corresponding credential fields, to accurately reflect all valid login inputs.
jnsdls
force-pushed
the
Add_login_functionality_with_multiple_authentication_methods
branch
from
791d39b to
49c465c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/thirdweb/src/login/client/login.ts (3)
209-212: Address comparison looks correct.The address comparison now correctly uses
getAddressto normalize both addresses before comparison, which prevents issues with checksum mismatches.
214-217: JWT cache expiration handling looks good.The code properly converts the string expiration time to a Date object, addressing the previous issue where string values were compared with Date objects.
270-273: JWT cache expiration handling looks good here too.Similar to the previous comment, the code properly converts the string expiration time to a Date object here as well.
packages/thirdweb/src/login/server/auth-handler.ts (3)
82-83: Type validation for chainId looks good.Using
z.coerce.number()instead ofz.number()is the correct approach for validating URL query parameters, which come as strings. This ensures that string values like "1" are properly converted to numbers.
129-161: JWT expiration handling and response serialization look correct.The implementation now properly:
- Handles different formats of JWT expiration claims (string or number)
- Validates the expiration time and throws an appropriate error
- Sets a reasonable cookie maxAge calculated from the JWT expiration time
- Returns the expiration date as an ISO string for proper JSON serialization
This fixes the previously identified issues with JWT expiration handling.
202-202: JWT expiration handling looks good here too.The code correctly multiplies the Unix timestamp by 1000 to get milliseconds for the Date constructor and converts the result to an ISO string.
🧹 Nitpick comments (3)
packages/thirdweb/src/login/client/login.ts (2)
125-125: Remove unnecessary commented code.The "// google login" comment appears to be a leftover placeholder that doesn't provide any value or context. Consider removing this commented line to improve code clarity.
- // google login
139-153: Improve error handling in default case.The default case only handles social login options using the
isSocialAuthOptioncheck, but does not explicitly throw an error within the default block when the check fails. Instead, execution falls through to the error throw outside the switch statement, which makes the flow less clear.default: { // all social login options are handled here (google, apple, etc) const { isSocialAuthOption } = await import("../../wallets/types.js"); if (isSocialAuthOption(loginOptions.type)) { const account = await IAW.connect({ client: loginOptions.client, strategy: loginOptions.type, }); return mapAccount(account, IAW, loginOptions.baseURL); } + throw new Error(`Invalid login type: ${loginOptions.type}`); } } -throw new Error("Invalid login type");packages/thirdweb/src/login/server/auth-handler.ts (1)
172-179: Improve token type validation logic.The current code checks if
token && type !== "Bearer"which assumestypeis defined whentokenis defined. While this usually works, it's more robust to first check iftypeexists before comparing it.- // if the token is set but the type is not Bearer, return a 401 error - if (token && type !== "Bearer") { + // if the token is set but the type is not Bearer or missing, return a 401 error + if (token && (!type || type !== "Bearer")) { throw ctx.error(401, { message: "Invalid authorization header", }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/thirdweb/src/login/client/login.ts(1 hunks)packages/thirdweb/src/login/server/auth-handler.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/thirdweb/src/login/client/login.ts (4)
packages/thirdweb/src/wallets/types.ts (2)
AuthOption(53-53)isSocialAuthOption(38-40)packages/thirdweb/src/exports/thirdweb.ts (6)
Chain(33-33)ThirdwebClient(26-26)getAddress(312-312)PreparedTransaction(109-109)sendTransaction(142-142)sendBatchTransaction(147-147)packages/thirdweb/src/exports/storage.ts (1)
AsyncStorage(13-13)packages/thirdweb/src/login/server/auth-handler.ts (1)
AuthRouter(236-236)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
jnsdls
force-pushed
the
Add_login_functionality_with_multiple_authentication_methods
branch
2 times, most recently
from
3e96dad to
0cba3cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/thirdweb/src/login/server/auth-handler.ts (5)
73-74: LGTM! The comment typo has been fixed.The issue with duplicated "to" in the comment has been fixed, correctly stating "re-map the server wallet to the admin account option".
81-84: LGTM! Fixed the chainId validation using z.coerce.number()The code now properly uses
z.coerce.number()instead ofz.number()for the chainId parameter, ensuring that string values from URL query parameters (like "1") are correctly converted to numbers instead of causing validation failures.
129-146: LGTM! JWT expiration handling has been properly fixedThe code now correctly:
- Handles both string and number expiration times
- Validates the expiration time and checks for NaN
- Calculates maxAge as the minimum of 30 days and the actual time until expiration
This ensures cookies won't live longer than their corresponding JWT tokens.
158-161: LGTM! Fixed the Date serialization issueThe code now correctly returns the expiration date as an ISO string using
toISOString()instead of returning a Date object that would be improperly serialized to JSON.
199-203: LGTM! Fixed the Date serialization in the isLoggedIn endpointThe expiresAt value is now properly returned as an ISO string, ensuring consistent serialization across all endpoints that return JWT expiration times.
🧹 Nitpick comments (3)
packages/thirdweb/src/login/server/auth-handler.ts (3)
172-184: Consider adding more specific error messages for authorization failuresWhile the error handling is functionally correct, the error messages could be more specific to help clients understand exactly what went wrong. For example, instead of just "Invalid authorization header", consider specifying what is expected (e.g., "Authorization header must use Bearer scheme").
- throw ctx.error(401, { - message: "Invalid authorization header", - }); + throw ctx.error(401, { + message: "Invalid authorization header format. Expected 'Bearer {token}'", + });
148-155: Consider adding domain/path options for cookie configurationThe current cookie configuration doesn't specify domain or path attributes. In multi-domain or complex routing scenarios, this could cause issues with cookie availability.
ctx.setCookie("tw:jwt", jwt, { httpOnly: true, secure: true, sameSite: "lax", maxAge: maxAgeInSeconds, expires: expiresAt, + path: "/", // Or make configurable via options + domain: options.cookieDomain, // Make configurable via options });You would need to add the
cookieDomainoption to theCreateAuthHandlerOptionstype.
10-14: Consider adding cookie configuration options to CreateAuthHandlerOptionsAdding cookie configuration options would make the auth handler more flexible for different deployment scenarios.
export type CreateAuthHandlerOptions = Omit<AuthOptions, "adminAccount"> & { client: ThirdwebClient; serverWallet: ServerWallet; basePath?: string; + cookieOptions?: { + domain?: string; + path?: string; + sameSite?: "strict" | "lax" | "none"; + }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/thirdweb/.size-limit.json(1 hunks)packages/thirdweb/src/login/client/login.ts(1 hunks)packages/thirdweb/src/login/server/auth-handler.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/thirdweb/.size-limit.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/thirdweb/src/login/client/login.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/thirdweb/src/login/server/auth-handler.ts (5)
packages/thirdweb/src/login/server/index.ts (2)
CreateAuthHandlerOptions(3-3)createAuthHandler(2-2)packages/thirdweb/src/login/index.ts (3)
CreateAuthHandlerOptions(10-10)createAuthHandler(9-9)login(5-5)packages/thirdweb/src/exports/thirdweb.ts (2)
ThirdwebClient(26-26)isAddress(313-313)packages/thirdweb/src/login/client/login.ts (1)
login(43-152)packages/thirdweb/src/exports/utils.ts (1)
decodeJWT(175-175)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
jnsdls
force-pushed
the
Add_login_functionality_with_multiple_authentication_methods
branch
from
0cba3cb to
c8a9d02
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/thirdweb/src/login/README.md (3)
156-162: Consider adding hyphens for compound adjectivesFor better readability, consider adding hyphens for compound adjectives that modify nouns.
- JWT validation and generation - Session management - Authentication state verification - Secure token storage - Rate limiting support - Framework integrations + JWT validation and generation + Session management + Authentication-state verification + Secure-token storage + Rate-limiting support + Framework integrations🧰 Tools
🪛 LanguageTool
[uncategorized] ~160-~160: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e verification - Secure token storage - Rate limiting support - Framework integrations ## Se...(EN_COMPOUND_ADJECTIVE_INTERNAL)
194-195: Improve API reference formattingThe formatting here could be improved by using consistent formatting for the code elements.
-`login(options: LoginOptions)`: Initiates the login process -`LoginParams`: Type definition for login options +`login(options: LoginOptions)`: Initiates the login process +`LoginParams`: Type definition for login options🧰 Tools
🪛 LanguageTool
[uncategorized] ~194-~194: Loose punctuation mark.
Context: ...xports -login(options: LoginOptions): Initiates the login process - `LoginPar...(UNLIKELY_OPENING_PUNCTUATION)
199-201: Improve API reference formattingThe formatting here could also be improved with consistent styling.
-`createAuthHandler(options: CreateAuthHandlerOptions)`: Creates an authentication handler -`toNodeHandler(handler)`: Converts auth handler to be used in Node.js servers (e.g. Express, Fastify, etc.) -`toNextJsHandler(handler)`: Converts auth handler to Next.js API route handler +`createAuthHandler(options: CreateAuthHandlerOptions)`: Creates an authentication handler +`toNodeHandler(handler)`: Converts auth handler to be used in Node.js servers (e.g. Express, Fastify, etc.) +`toNextJsHandler(handler)`: Converts auth handler to Next.js API route handler🧰 Tools
🪛 LanguageTool
[uncategorized] ~199-~199: Loose punctuation mark.
Context: ...dler(options: CreateAuthHandlerOptions): Creates an authentication handler -to...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~201-~201: Loose punctuation mark.
Context: ...tify, etc.) -toNextJsHandler(handler): Converts auth handler to Next.js API ro...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/thirdweb/.size-limit.json(1 hunks)packages/thirdweb/package.json(6 hunks)packages/thirdweb/src/exports/login.ts(1 hunks)packages/thirdweb/src/exports/thirdweb.ts(1 hunks)packages/thirdweb/src/login/README.md(1 hunks)packages/thirdweb/src/login/client/index.ts(1 hunks)packages/thirdweb/src/login/client/login.ts(1 hunks)packages/thirdweb/src/login/index.ts(1 hunks)packages/thirdweb/src/login/server/auth-handler.ts(1 hunks)packages/thirdweb/src/login/server/index.ts(1 hunks)packages/thirdweb/src/login/server/integrations/next-js.ts(1 hunks)packages/thirdweb/src/login/server/integrations/node.ts(1 hunks)packages/thirdweb/src/wallets/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/thirdweb/.size-limit.json
- packages/thirdweb/src/login/client/index.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/thirdweb/src/exports/login.ts
- packages/thirdweb/src/exports/thirdweb.ts
- packages/thirdweb/src/wallets/types.ts
- packages/thirdweb/src/login/server/index.ts
- packages/thirdweb/src/login/server/integrations/next-js.ts
- packages/thirdweb/src/login/server/integrations/node.ts
- packages/thirdweb/package.json
- packages/thirdweb/src/login/index.ts
- packages/thirdweb/src/login/client/login.ts
🧰 Additional context used
🪛 LanguageTool
packages/thirdweb/src/login/README.md
[uncategorized] ~160-~160: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e verification - Secure token storage - Rate limiting support - Framework integrations ## Se...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~194-~194: Loose punctuation mark.
Context: ...xports - login(options: LoginOptions): Initiates the login process - `LoginPar...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~199-~199: Loose punctuation mark.
Context: ...dler(options: CreateAuthHandlerOptions): Creates an authentication handler - to...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~201-~201: Loose punctuation mark.
Context: ...tify, etc.) - toNextJsHandler(handler): Converts auth handler to Next.js API ro...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/thirdweb/src/login/server/auth-handler.ts (5)
74-74: Code looks good!The server wallet is correctly mapped to the admin account, and the comment no longer contains the typo that was previously fixed.
81-84: Validation is properly implementedThe
chainIdparameter is correctly validated usingz.coerce.number()instead ofz.number(), which will properly handle string inputs from query parameters.
129-139: JWT expiration handling is robustThe JWT expiration parsing is well-implemented with proper type checking and error handling. This correctly addresses the previous concerns about handling string expiration times and NaN values.
158-161: Correct serialization for expiresAtThe
expiresAtis properly returned as an ISO string, which will ensure it's correctly serialized in the JSON response.
202-202: Consistent date serializationThe
expiresAtis correctly converted to ISO string format, ensuring consistent handling across endpoints.
| const maxAgeInSeconds = Math.min( | ||
| thirtyDaysInSeconds, | ||
| Math.floor((expiresAt.getTime() - Date.now()) / 1000), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure maxAgeInSeconds is not negative
The maxAgeInSeconds calculation doesn't handle the case where the JWT might already be expired (when expiresAt < Date.now()), which would result in a negative value.
Consider adding a check to ensure the value is never negative:
const maxAgeInSeconds = Math.max(
0,
Math.min(
thirtyDaysInSeconds,
Math.floor((expiresAt.getTime() - Date.now()) / 1000),
),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const maxAgeInSeconds = Math.min( | |
| thirtyDaysInSeconds, | |
| Math.floor((expiresAt.getTime() - Date.now()) / 1000), | |
| ); | |
| const maxAgeInSeconds = Math.max( | |
| 0, | |
| Math.min( | |
| thirtyDaysInSeconds, | |
| Math.floor((expiresAt.getTime() - Date.now()) / 1000), | |
| ), | |
| ); |
🤖 Prompt for AI Agents
In packages/thirdweb/src/login/server/auth-handler.ts around lines 143 to 146,
the calculation of maxAgeInSeconds can result in a negative value if expiresAt
is earlier than the current time. To fix this, add a check to ensure
maxAgeInSeconds is never negative by using Math.max with 0 as the lower bound,
so the value is always zero or positive.
| ### Basic Setup | ||
|
|
||
| ```typescript | ||
| import { Login } from "thirdweb/login"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent import path
The import path here is also inconsistent with the basic usage example.
-import { Login } from "thirdweb/login";
+import { Login } from "thirdweb";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Login } from "thirdweb/login"; | |
| -import { Login } from "thirdweb/login"; | |
| +import { Login } from "thirdweb"; |
🤖 Prompt for AI Agents
In packages/thirdweb/src/login/README.md at line 109, the import path for the
Login module is inconsistent with the basic usage example. Update the import
statement to match the correct and consistent path used in the rest of the
documentation, ensuring uniformity across all examples.
| The package provides built-in integrations for popular frameworks: | ||
|
|
||
| ```typescript | ||
| import { Login } from "thirdweb/login"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent import path
The import path here is inconsistent with the basic usage example.
-import { Login } from "thirdweb/login";
+import { Login } from "thirdweb";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Login } from "thirdweb/login"; | |
| -import { Login } from "thirdweb/login"; | |
| +import { Login } from "thirdweb"; |
🤖 Prompt for AI Agents
In packages/thirdweb/src/login/README.md at line 122, the import path for the
Login module is inconsistent with the basic usage example. Update the import
statement to match the correct and consistent path used elsewhere in the
documentation, ensuring uniformity across all examples.
| ### Basic Usage | ||
|
|
||
| ```typescript | ||
| import { Login } from "thirdweb/login"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent import path
The import path here doesn't match the example shown in the basic usage section (line 11).
-import { Login } from "thirdweb/login";
+import { Login } from "thirdweb";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Login } from "thirdweb/login"; | |
| import { Login } from "thirdweb"; |
🤖 Prompt for AI Agents
In packages/thirdweb/src/login/README.md at line 24, the import path for the
Login module is inconsistent with the example shown at line 11. Update the
import statement to match the path used in the basic usage section to maintain
consistency and avoid confusion.
PR-Codex overview
This PR focuses on enhancing the
loginfunctionality within thethirdwebpackage by introducing new authentication methods and improving existing ones. It also updates type definitions and expands server-side capabilities.Detailed summary
export * from "../login/index.js"tologin.ts.Loginnamespace andcreateAuthHandlerinserver/index.ts.loginfunction with various authentication methods (email, phone, wallet, JWT).Loginmodule..size-limit.json.types.tsand added new utility functions for handling social authentication options.Summary by CodeRabbit
New Features
Documentation
Style