[SDK-3722] Fix: Stateless strategies should not invoke stateful session classes #662
Conversation
evansims
changed the title
Codecov ReportBase: 94.44% // Head: 94.35% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #662 +/- ##
============================================
- Coverage 94.44% 94.35% -0.10%
- Complexity 1254 1275 +21
============================================
Files 65 65
Lines 4211 4231 +20
============================================
+ Hits 3977 3992 +15
- Misses 234 239 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -93,11 +94,16 @@ public function login( | |||
| ?string $redirectUrl = null, | |||
| ?array $params = null | |||
| ): string { | |||
| if (! $this->configuration()->usingStatefulness()) { | |||
There was a problem hiding this comment.
I've added a usingStatefulness() helper method on the Auth0\SDK\Configuration\SdkConfiguration class, which simply returns true if the host application has configured a stateless application strategy.
These throw a ConfigurationException if a method dependant on sessions/statefulness is invoked. (This is not a breaking change as this would have naturally thrown an exception from being inaccessible — just a cleaner exception type is thrown now.)
| $params = $params ?? []; | ||
| $state = $params['state'] ?? $this->getTransientStore()->issue('state'); | ||
| $params['nonce'] = $params['nonce'] ?? $this->getTransientStore()->issue('nonce'); | ||
| $state = $params['state'] ?? $store?->issue('state') ?? uniqid(); |
There was a problem hiding this comment.
Null-safe operators are a new language feature in PHP 8.0+ which offers cleaner syntax than null coalescing, as that does not work on method calls. I'll be transitioning the SDK to use these as time goes on, now that the SDK requires PHP 8+.
…mory state cleanup.
| @@ -601,9 +632,9 @@ public function getInvitationParameters(): ?array | |||
| /** | |||
| * Create a transient storage handler using the configured transientStorage medium. | |||
| */ | |||
| private function getTransientStore(): TransientStoreHandler | |||
| private function getTransientStore(bool $reset = false): ?TransientStoreHandler | |||
There was a problem hiding this comment.
Enables the recreation of TransientStore class instances for memory cleanup. getTransientStore() now correctly returns a potential null value when a store isn't configured (in stateless cases.) Previously calling this method in a stateless strategy would have thrown an exception.
| @@ -141,7 +141,7 @@ public function __construct( | |||
| $this->setupStateCookies(); | |||
| $this->setupStateFactories(); | |||
|
|
|||
| if (in_array($this->getStrategy(), self::STRATEGIES_USING_SESSIONS, true)) { | |||
| if ($this->usingStatefulness()) { | |||
There was a problem hiding this comment.
This logic has been moved into the new usingStatefuless() method for reusability.
| @@ -48,6 +49,13 @@ public static function requiresStrategy( | |||
| return new self(self::MSG_STRATEGY_REQUIRED, 0, $previous); | |||
| } | |||
|
|
|||
| public static function requiresStatefulness( | |||
There was a problem hiding this comment.
A new message variant for the ConfigurationException type, when a stateful method is incorrectly invoked in a stateless application type.
…ke stateful session classes (#662) * [SDK-3722] Fix: Stateless strategies should not invoke stateful session checks * Fix: getState($reset: true) should still be run on clear(), for in-memory state cleanup. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Changes
This PR fixes a bug (manifested downstream in the Laravel SDK) in which the SDK invokes stateful session classes and checks in applications using stateless strategies, in some cases throwing errors as those session classes are not (and should not) be accessible.
References
Resolves auth0/laravel-auth0#325
Testing
Run
vendor/bin/pestfrom a local clone of the repo, or review of the GitHub status checks below.Contributor Checklist