Evaluate after middleware#96
Evaluate after middleware#96andresilvagomez wants to merge 1 commit intostephenjude:3.xfrom andresilvagomez:3.x
Conversation
WalkthroughThis change introduces lazy evaluation support to the TwoFactorAuthenticationPlugin by adding Closure type unions to three configuration properties. Properties now store closures directly and evaluation is deferred to getters, enabling runtime evaluation rather than setup-time evaluation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.33)Invalid configuration: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TwoFactorAuthenticationPlugin.php (1)
80-80: Return type mismatch: getTwoFactorChallengeMiddleware() declares string but can return bool.The method at line 165 declares return type
string, but the property$twoFactorChallengeMiddlewarecan bestring | bool | Closure(line 42), and theevaluate()method preserves non-Closure values as-is. The deprecatedsetChallengeTwoFactorMiddleware()explicitly acceptsbool, creating a path where false can be returned.This causes a problem at line 80:
->middleware($this->getTwoFactorChallengeMiddleware())passes the result directly to Filament'smiddleware()method without filtering. If the middleware is false (via the deprecated setter), this violates the method's expectations. Line 98 correctly handles this by wrapping the same method call inarray_filter(), which filters out false values.Update the return type to
string | booland either wrap line 80 in conditional handling or usearray_filter()like line 98 does.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/TwoFactorAuthenticationPlugin.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/TwoFactorAuthenticationPlugin.php (2)
src/Middleware/TwoFactorChallenge.php (1)
TwoFactorChallenge(8-32)src/TwoFactorAuthenticatable.php (2)
hasEnabledTwoFactorAuthentication(22-26)hasEnabledPasskeyAuthentication(28-31)
🔇 Additional comments (4)
src/TwoFactorAuthenticationPlugin.php (4)
29-29: LGTM! Property types updated for lazy evaluation.The addition of
Closureto these property types enables runtime evaluation while maintaining backward compatibility with existingboolandstringvalues.Also applies to: 31-31, 42-42
210-212: LGTM! Lazy evaluation correctly implemented.The method stores closures directly and relies on
hasEnabledTwoFactorAuthentication()(line 219) andgetTwoFactorChallengeMiddleware()(line 167) to evaluate them at runtime.
219-219: LGTM! Evaluation logic is correct.The getter properly evaluates the property to return a
bool, maintaining type safety.
224-224: LGTM! Passkey authentication follows the same lazy evaluation pattern.The setter stores the condition directly (line 229) and the getter evaluates it at runtime (line 224), consistent with the refactoring approach.
Also applies to: 229-229
| public function requirePasswordWhenEnabling(bool | Closure $condition = true): static | ||
| { | ||
| $this->isPasswordRequiredForEnable = $this->evaluate($condition); | ||
| $this->isPasswordRequiredForEnable = $condition; |
There was a problem hiding this comment.
Critical: Deprecated setters now store closures but getters don't evaluate them.
These deprecated setters now assign closures directly without evaluation (Lines 107, 115, 123), but their corresponding getters (isPasswordRequiredForEnable(), isPasswordRequiredForDisable(), isPasswordRequiredForRegenerateRecoveryCodes() at lines 131-143) return the raw property value without calling $this->evaluate().
If a user passes a Closure to these deprecated setters, the getters will return a Closure instead of a bool, violating their return type and breaking existing code.
Apply this diff to fix the deprecated getters:
#[Deprecated('Use twoFactorSetupRequiresPassword instead')]
public function isPasswordRequiredForRegenerateRecoveryCodes(): bool
{
- return $this->isPasswordRequiredForRegenerateRecoveryCodes;
+ return $this->evaluate($this->isPasswordRequiredForRegenerateRecoveryCodes);
}
#[Deprecated('Use twoFactorSetupRequiresPassword instead')]
public function isPasswordRequiredForEnable(): bool
{
- return $this->isPasswordRequiredForEnable;
+ return $this->evaluate($this->isPasswordRequiredForEnable);
}
#[Deprecated('Use twoFactorSetupRequiresPassword instead')]
public function isPasswordRequiredForDisable(): bool
{
- return $this->isPasswordRequiredForDisable;
+ return $this->evaluate($this->isPasswordRequiredForDisable);
}Also applies to: 115-115, 123-123
🤖 Prompt for AI Agents
In src/TwoFactorAuthenticationPlugin.php lines 107, 115, 123 and getters at
131-143, the deprecated setters store closures into properties but the getters
return the raw property values; update each getter (isPasswordRequiredForEnable,
isPasswordRequiredForDisable, isPasswordRequiredForRegenerateRecoveryCodes) to
evaluate the stored value via $this->evaluate($this->propertyName) and return
the evaluated result coerced to bool so a Closure yields a boolean instead of
being returned directly.
| return $this->evaluate($this->twoFactorChallengeMiddleware); | ||
| } |
There was a problem hiding this comment.
Return type mismatch: methods can return bool but declare string.
Both getter methods have a return type of string, but since twoFactorChallengeMiddleware is typed as string | bool | Closure (line 42), evaluating it can return false. This violates the type contract and may cause issues at line 80 where the result is passed directly to ->middleware().
Consider one of these solutions:
Solution 1: Change return type to nullable string and filter false values
-public function getChallengeTwoFactorMiddleware(): string
+public function getChallengeTwoFactorMiddleware(): ?string
{
- return $this->evaluate($this->twoFactorChallengeMiddleware);
+ $middleware = $this->evaluate($this->twoFactorChallengeMiddleware);
+ return $middleware === false ? null : $middleware;
}
-public function getTwoFactorChallengeMiddleware(): string
+public function getTwoFactorChallengeMiddleware(): ?string
{
- return $this->evaluate($this->twoFactorChallengeMiddleware);
+ $middleware = $this->evaluate($this->twoFactorChallengeMiddleware);
+ return $middleware === false ? null : $middleware;
}Solution 2: Change return type to string | bool to match the property
-public function getChallengeTwoFactorMiddleware(): string
+public function getChallengeTwoFactorMiddleware(): string | bool
{
return $this->evaluate($this->twoFactorChallengeMiddleware);
}
-public function getTwoFactorChallengeMiddleware(): string
+public function getTwoFactorChallengeMiddleware(): string | bool
{
return $this->evaluate($this->twoFactorChallengeMiddleware);
}Also applies to: 167-168
🤖 Prompt for AI Agents
In src/TwoFactorAuthenticationPlugin.php around lines 162-163 (and likewise
167-168), the getter methods declare a return type of string but may return
false because $twoFactorChallengeMiddleware is typed as string|bool|Closure;
update the method signatures to return string|bool to match the property OR
change them to ?string and ensure they convert/filter out false before
returning; additionally, update the call site (around line 80) so ->middleware()
is only passed a string (e.g., check if the getter returned a string before
calling ->middleware(), or cast/throw if false) to preserve type-safety and
avoid passing false into middleware.
There was a problem hiding this comment.
Pull request overview
This PR modifies the two-factor authentication plugin to defer closure evaluation until after middleware execution. Previously, closures were evaluated immediately when configuration methods were called; now they are stored and evaluated only when the values are accessed through getter methods.
- Changed property type hints to accept
Closurein addition to their original types - Modified setter methods to store closures directly instead of evaluating them immediately
- Updated getter methods to evaluate closures when the values are accessed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.