Enhance authentication strategies with improved type safety and additional options - #387
Enhance authentication strategies with improved type safety and additional options#387sergiodxa wants to merge 28 commits into
Conversation
…support to extra authenticate options
There was a problem hiding this comment.
Pull Request Overview
This PR enhances authentication strategies with improved type safety and additional options, making the authentication system more flexible and secure. The changes introduce a more strongly-typed approach to strategy configuration and callback handling.
- Refactored Strategy class to use
CallbackFunctioninstead ofVerifyFunctionwith better parameter naming - Redesigned Authenticator to accept a record of strategies at construction time with improved type inference
- Added support for strategies that accept additional authenticate method parameters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/strategy.ts | Updated Strategy class with improved generics, callback function terminology, and support for extra authenticate parameters |
| src/strategy.test.ts | Removed existing test file (complete deletion) |
| src/index.ts | Completely refactored Authenticator class with constructor-based strategy registration and enhanced type safety |
| src/index.test.ts | Added new comprehensive tests demonstrating the enhanced API with form and login strategies |
| README.md | Updated documentation to reflect the new API design and improved examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| get strategies(): Readonly<{ | ||
| [K in keyof StrategyRecord]: Omit<StrategyRecord[K], "authenticate">; | ||
| }> { | ||
| return this.#strategies; |
There was a problem hiding this comment.
The strategies getter returns a type that omits the authenticate method from each strategy, but the actual implementation returns this.#strategies which still contains the authenticate method. This creates a type mismatch where the runtime object has more methods than the type indicates. Either the return type should not omit authenticate, or the getter should return a mapped object that actually omits it.
| return this.#strategies; | |
| // Return a mapped object omitting 'authenticate' from each strategy | |
| return Object.freeze( | |
| Object.fromEntries( | |
| Object.entries(this.#strategies).map(([key, strategy]) => { | |
| // Omit 'authenticate' using destructuring | |
| const { authenticate, ...rest } = strategy as any; | |
| return [key, rest]; | |
| }), | |
| ), | |
| ) as Readonly<{ | |
| [K in keyof StrategyRecord]: Omit<StrategyRecord[K], "authenticate">; | |
| }>; |
| } | ||
|
|
||
| /** | ||
| * Options that can be defined at athentication time to override the default |
There was a problem hiding this comment.
The word "athentication" is misspelled. It should be "authentication".
| * Options that can be defined at athentication time to override the default | |
| * Options that can be defined at authentication time to override the default |
|
|
||
| private get cookieName() { | ||
| if (typeof this.options.cookie === "string") { | ||
| return this.options.cookie || "oauth2"; |
There was a problem hiding this comment.
The logic in this getter has a bug. When this.options.cookie is an empty string "", the expression this.options.cookie || "oauth2" will return "oauth2" instead of using the empty string. If you intend to allow empty strings as cookie names, use nullish coalescing instead. If empty strings should fall back to the default, this is correct but potentially confusing. Consider using this.options.cookie || "oauth2" only when the value is explicitly falsy (not just empty string), or document this behavior clearly.
| return this.options.cookie || "oauth2"; | |
| return this.options.cookie ?? "oauth2"; |
| } | ||
|
|
||
| private get cookieOptions() { | ||
| if (typeof this.options.cookie !== "object") return {}; |
There was a problem hiding this comment.
The condition typeof this.options.cookie !== "object" will be true when this.options.cookie is null, since typeof null === "object" in JavaScript. This means when cookie is null, you'll return an empty object instead of checking the nullish case properly. Consider using if (typeof this.options.cookie !== "object" || this.options.cookie === null) or restructure the logic to handle null explicitly.
| if (typeof this.options.cookie !== "object") return {}; | |
| if (typeof this.options.cookie !== "object" || this.options.cookie === null) return {}; |
Improve type safety in authentication strategies, allowing for more flexible and secure user verification.
Also adds support for extra authenticate options defined by strategies.
This PR refactors the authentication system to provide better type safety and a more intuitive API:
Usertype with a more accurateSessionDatatypeAuthenticator.infer<typeof authenticator>verifytocallbackfor clarity and consistency