Easier integration with 3rd parties #44
Conversation
WalkthroughThe pull request includes modifications to several files related to the Kinde SDK. Key changes involve updating the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (15)
lib/Sdk/Storage/StorageInterface.php (2)
5-6: Add PHPDoc documentation for the interface.Since this is a public interface that will be implemented by third parties, it should be well-documented.
Add this documentation block:
+/** + * Interface for storage implementations in the Kinde SDK. + * + * This interface defines the contract for storing and retrieving data + * in the Kinde SDK. Implementations can use different storage backends + * such as cookies, sessions, or other persistence mechanisms. + * + * @since 1.0.0 + */ interface StorageInterface
1-16: Consider enhancing storage abstraction for better third-party integration.While this interface provides basic storage operations, consider these architectural improvements for better third-party integration:
- Add a factory interface for creating storage implementations
- Consider supporting async operations for future-proofing
- Add support for bulk operations
- Consider adding TTL (Time To Live) support separate from expires
This would make the SDK more flexible for various framework integrations and use cases.
lib/Sdk/Storage/SessionStorage.php (4)
1-6: Add class-level documentation.Consider adding a PHPDoc block to describe the class purpose, implementation details, and usage examples.
<?php namespace Kinde\KindeSDK\Sdk\Storage; +/** + * Session-based implementation of StorageInterface. + * + * This class provides session storage capabilities for the Kinde SDK, + * managing data persistence across requests while handling expiration. + */ class SessionStorage implements StorageInterface
37-41: Add key validation in removeItem method.While the implementation is functional, consider adding key validation for consistency with other methods.
public function removeItem(string $key): void { $this->startSession(); + if (empty($key)) { + throw new \InvalidArgumentException('Key cannot be empty'); + } unset($_SESSION[$key]); }
43-48: Consider adding session security configurations.While the session initialization works correctly, consider adding security-related configurations.
private function startSession(): void { if (session_status() === PHP_SESSION_NONE) { + // Set secure session configurations + ini_set('session.cookie_httponly', '1'); + ini_set('session.cookie_secure', '1'); + ini_set('session.use_strict_mode', '1'); session_start(); } }
1-49: Consider environment-specific session handling configurations.The current implementation might not be suitable for all environments (e.g., CLI, workers). Consider:
- Adding a configuration mechanism for session options
- Implementing environment checks
- Providing a fallback mechanism for environments where sessions aren't available
Would you like assistance in implementing these architectural improvements?
lib/Sdk/Storage/CookieStorage.php (2)
13-16: Add input validation and documentation for getItem method.The method lacks input validation and documentation.
Consider these improvements:
+ /** + * Retrieve a value from cookie storage + * @param string $key Cookie key + * @return string Cookie value or empty string if not found + * @throws \InvalidArgumentException If key is empty + */ public function getItem(string $key): string { + if (empty($key)) { + throw new \InvalidArgumentException('Cookie key cannot be empty'); + } return $_COOKIE[self::getKey($key)] ?? ""; }
42-45: Improve type safety and add validation for key generation.The utility method lacks type hints and input validation.
Consider these improvements:
+ /** + * Generate prefixed cookie key + * @param string $key Base key + * @return string Prefixed key + */ - private function getKey($key) + private function getKey(string $key): string { + $key = trim($key); + $key = preg_replace('/[^a-zA-Z0-9_-]/', '', $key); return self::$prefix . '_' . $key; }lib/Sdk/OAuth2/ClientCredentials.php (1)
46-51: Consider enhancing error handling for HTTP responsesWhile JSON decoding errors are handled explicitly, HTTP errors from the Guzzle client could provide more context. Consider wrapping the request in a try-catch block with specific error messages.
- $response = - $client->request('POST', $clientSDK->tokenEndpoint, [ - 'form_params' => $formData - ]); - $token = $response->getBody()->getContents(); - return json_decode($token, false, 512, JSON_THROW_ON_ERROR); + try { + $response = $client->request('POST', $clientSDK->tokenEndpoint, [ + 'form_params' => $formData + ]); + $token = $response->getBody()->getContents(); + return json_decode($token, false, 512, JSON_THROW_ON_ERROR); + } catch (\GuzzleHttp\Exception\ClientException $e) { + throw new \RuntimeException('Authentication failed: ' . $e->getResponse()->getBody()->getContents(), $e->getCode(), $e); + } catch (\GuzzleHttp\Exception\ServerException $e) { + throw new \RuntimeException('Authentication server error: ' . $e->getResponse()->getBody()->getContents(), $e->getCode(), $e); + }lib/Sdk/Storage/Storage.php (3)
12-14: Add type declaration for tokenTimeToLive property.For better type safety and consistency with
jwksUrl, consider adding a type declaration for thetokenTimeToLiveproperty.- private $tokenTimeToLive; + private int $tokenTimeToLive;
Line range hint
100-111: Add error handling for JWT parsing.The
getUserProfilemethod (and similar JWT parsing methods) should include error handling for invalid JWT tokens. Consider wrapping the JWT parsing in a try-catch block to handle potential parsing errors gracefully.public function getUserProfile() { $token = $this->getToken(); - $payload = Utils::parseJWT($token['id_token'] ?? ''); - return [ - 'id' => $payload['sub'] ?? '', - 'given_name' => $payload['given_name'] ?? '', - 'family_name' => $payload['family_name'] ?? '', - 'email' => $payload['email'] ?? '', - 'picture' => $payload['picture'] ?? '', - ]; + try { + $payload = Utils::parseJWT($token['id_token'] ?? ''); + return [ + 'id' => $payload['sub'] ?? '', + 'given_name' => $payload['given_name'] ?? '', + 'family_name' => $payload['family_name'] ?? '', + 'email' => $payload['email'] ?? '', + 'picture' => $payload['picture'] ?? '', + ]; + } catch (\Exception $e) { + return [ + 'id' => '', + 'given_name' => '', + 'family_name' => '', + 'email' => '', + 'picture' => '', + ]; + } }
24-34: Well-structured transition to instance methods.The refactoring from static to instance methods while maintaining the singleton pattern through
getInstanceis well-implemented. This change:
- Improves testability by allowing dependency injection
- Maintains backward compatibility
- Aligns with the PR objective of easier third-party integration
Consider documenting the singleton usage pattern in the class documentation to guide implementers on proper instantiation.
lib/Sdk/Storage/BaseStorage.php (1)
10-10: Consider restricting the visibility of$innerStorage.The
$innerStorageproperty is declared aspublic, which allows external modification and can lead to unintended side effects. It's recommended to change its visibility toprotectedorprivateto encapsulate the storage mechanism and prevent external interference.lib/Sdk/OAuth2/AuthorizationCode.php (1)
37-44: Consider refactoring method parameters forbuildAuthorizationURIThe
buildAuthorizationURImethod accepts multiple parameters. It might improve readability and maintainability to refactor these parameters into an associative array or an object. This approach can make the code more scalable and easier to manage, especially if more parameters are added in the future.lib/KindeClientSDK.php (1)
Line range hint
586-588: Avoid suppressing exceptions without handling them inisAuthenticatedIn the
isAuthenticatedmethod, the catch block is empty, which suppresses all exceptions without any handling. This can make debugging difficult and may hide underlying issues.Consider logging the exception or handling it appropriately to ensure that any errors during the refresh token process are not silently ignored.
Apply this diff to log the exception:
} catch (\Throwable $th) { + error_log('Exception during token refresh: ' . $th->getMessage()); }Alternatively, rethrow the exception or handle it as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (9)
.gitignore(1 hunks)lib/KindeClientSDK.php(17 hunks)lib/Sdk/OAuth2/AuthorizationCode.php(1 hunks)lib/Sdk/OAuth2/ClientCredentials.php(2 hunks)lib/Sdk/Storage/BaseStorage.php(2 hunks)lib/Sdk/Storage/CookieStorage.php(1 hunks)lib/Sdk/Storage/SessionStorage.php(1 hunks)lib/Sdk/Storage/Storage.php(4 hunks)lib/Sdk/Storage/StorageInterface.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (4)
lib/Sdk/Storage/StorageInterface.php (1)
1-4: LGTM! Proper namespace and file structure.
The namespace follows PSR-4 autoloading standard and matches the directory structure.
lib/Sdk/Storage/CookieStorage.php (1)
1-46: Verify cookie handling across the codebase.
Let's ensure there are no conflicting cookie implementations or security issues.
✅ Verification successful
Cookie implementation is secure and properly isolated
The cookie implementation in CookieStorage.php is secure and well-implemented:
- Security flags are properly set with secure defaults (
httpOnly=true,secure=true,samesite=Lax) - No conflicting cookie implementations found in the codebase
- Cookie storage is properly isolated through the
StorageInterface - No direct cookie value echoing or XSS vulnerabilities detected
- Test implementations use mock cookies (
COOKIE_FAKE) instead of real ones
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other cookie implementations and security-sensitive patterns
echo "Checking for other cookie implementations..."
rg -l "setcookie|$_COOKIE" --type php
echo "Checking for security-sensitive patterns in cookie handling..."
rg "cookie.*secure.*=.*false|httponly.*=.*false" --type php
echo "Checking for potential XSS vulnerabilities in cookie handling..."
rg "echo.*\$_COOKIE" --type php
Length of output: 4010
Script:
#!/bin/bash
# Let's check for actual cookie implementations and storage classes
# Check for cookie-related implementations
echo "=== Checking cookie storage implementations ==="
rg -A 10 "class.*Storage.*implements StorageInterface" --type php
# Check for cookie-related configurations
echo "=== Checking cookie configurations ==="
rg "cookie.*=|COOKIE" --type php -A 2 -B 2
# Check session storage implementation for comparison
echo "=== Checking session storage implementation ==="
cat lib/Sdk/Storage/SessionStorage.php
Length of output: 7271
lib/Sdk/OAuth2/ClientCredentials.php (1)
17-17: LGTM: Type declaration enhances type safety
The addition of the Storage type declaration aligns with modern PHP practices and improves code maintainability.
lib/Sdk/Storage/Storage.php (1)
9-9: Verify BaseStorage implementation.
Since this class extends BaseStorage, we should verify that all required storage interface methods are properly implemented.
✅ Verification successful
Let me gather more information about the implementation details.
Let me check one more thing to see the actual Storage class implementation since we're reviewing Storage.php.
Storage implementation is properly structured
The verification shows a well-implemented storage hierarchy:
StorageInterfacedefines the required contract withgetItem,setItem, andremoveItemmethodsCookieStorageimplementsStorageInterfacewith proper cookie handlingBaseStorageprovides a base implementation withCookieStorageas the default inner storageStorageextendsBaseStorageand adds token-specific functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check BaseStorage implementation and interface
# Look for BaseStorage class definition and any interfaces it implements
rg -A 5 "class BaseStorage"
# Look for interface definitions that might be related to storage
rg -A 5 "interface.*Storage"
Length of output: 1052
Script:
#!/bin/bash
# Check complete StorageInterface definition
rg -A 15 "interface StorageInterface"
# Check BaseStorage implementation details
rg -A 20 "class BaseStorage" lib/Sdk/Storage/BaseStorage.php
# Look for getItem and setItem implementations
ast-grep --pattern 'function getItem($_) { $$$ }'
ast-grep --pattern 'function setItem($_) { $$$ }'
Length of output: 1412
Script:
#!/bin/bash
# Check Storage class implementation
rg -A 20 "class Storage" lib/Sdk/Storage/Storage.php
# Also check CookieStorage implementation since it's used as innerStorage
rg -A 20 "class CookieStorage"
Length of output: 2018
| public function getItem(string $key): string; | ||
|
|
||
| public function setItem( | ||
| string $key, | ||
| string $value, | ||
| int $expires = 0, | ||
| ): void; | ||
|
|
||
| public function removeItem(string $key): void; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Essential improvements needed for robust storage interface.
Several improvements are needed to make this interface more robust and developer-friendly:
-
The
getItemmethod doesn't handle non-existent keys. It should either:- Return
?stringto handle missing items - Or throw a specific exception
- Return
-
The
setItemmethod needs parameter validation for the expiration time. -
Method documentation is missing.
Here's the suggested implementation:
+/**
+ * Retrieves an item from storage by key.
+ *
+ * @param string $key The key of the item to retrieve
+ * @return string|null The value of the item, or null if not found
+ * @throws StorageException If retrieval fails
+ */
-public function getItem(string $key): string;
+public function getItem(string $key): ?string;
+/**
+ * Stores an item in storage.
+ *
+ * @param string $key The key under which to store the value
+ * @param string $value The value to store
+ * @param int $expires The expiration time in seconds (0 for no expiration)
+ * @throws StorageException If storage fails
+ * @throws \InvalidArgumentException If expires is negative
+ */
public function setItem(
string $key,
string $value,
int $expires = 0,
): void;
+/**
+ * Removes an item from storage.
+ *
+ * @param string $key The key of the item to remove
+ * @throws StorageException If removal fails
+ */
public function removeItem(string $key): void;Additionally, consider adding:
- A custom
StorageExceptionclass for storage-specific errors - A method to check if an item exists:
hasItem(string $key): bool - A method to clear all items:
clear(): void
📝 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.
| public function getItem(string $key): string; | |
| public function setItem( | |
| string $key, | |
| string $value, | |
| int $expires = 0, | |
| ): void; | |
| public function removeItem(string $key): void; | |
| } | |
| /** | |
| * Retrieves an item from storage by key. | |
| * | |
| * @param string $key The key of the item to retrieve | |
| * @return string|null The value of the item, or null if not found | |
| * @throws StorageException If retrieval fails | |
| */ | |
| public function getItem(string $key): ?string; | |
| /** | |
| * Stores an item in storage. | |
| * | |
| * @param string $key The key under which to store the value | |
| * @param string $value The value to store | |
| * @param int $expires The expiration time in seconds (0 for no expiration) | |
| * @throws StorageException If storage fails | |
| * @throws \InvalidArgumentException If expires is negative | |
| */ | |
| public function setItem( | |
| string $key, | |
| string $value, | |
| int $expires = 0, | |
| ): void; | |
| /** | |
| * Removes an item from storage. | |
| * | |
| * @param string $key The key of the item to remove | |
| * @throws StorageException If removal fails | |
| */ | |
| public function removeItem(string $key): void; | |
| } |
| public function setItem(string $key, string $value, int $expires = 0): void | ||
| { | ||
| $this->startSession(); | ||
| $_SESSION[$key] = | ||
| json_encode([ | ||
| 'key' => $key, | ||
| 'value' => $value, | ||
| 'exp' => $expires, | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix potential JSON encoding issues and validate expiration time.
The current implementation has several potential issues:
- Missing error handling for JSON encoding
- No validation of the expiration timestamp
- The string concatenation syntax appears incorrect
Apply this fix:
public function setItem(string $key, string $value, int $expires = 0): void
{
$this->startSession();
- $_SESSION[$key] =
- json_encode([
+ if ($expires < 0) {
+ throw new \InvalidArgumentException('Expiration time cannot be negative');
+ }
+
+ try {
+ $_SESSION[$key] = json_encode([
'key' => $key,
'value' => $value,
'exp' => $expires,
- ]
- );
+ ], JSON_THROW_ON_ERROR);
+ } catch (\JsonException $e) {
+ throw new \RuntimeException('Failed to encode storage data', 0, $e);
+ }
}📝 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.
| public function setItem(string $key, string $value, int $expires = 0): void | |
| { | |
| $this->startSession(); | |
| $_SESSION[$key] = | |
| json_encode([ | |
| 'key' => $key, | |
| 'value' => $value, | |
| 'exp' => $expires, | |
| ] | |
| ); | |
| } | |
| public function setItem(string $key, string $value, int $expires = 0): void | |
| { | |
| $this->startSession(); | |
| if ($expires < 0) { | |
| throw new \InvalidArgumentException('Expiration time cannot be negative'); | |
| } | |
| try { | |
| $_SESSION[$key] = json_encode([ | |
| 'key' => $key, | |
| 'value' => $value, | |
| 'exp' => $expires, | |
| ], JSON_THROW_ON_ERROR); | |
| } catch (\JsonException $e) { | |
| throw new \RuntimeException('Failed to encode storage data', 0, $e); | |
| } | |
| } |
| public function getItem(string $key): string | ||
| { | ||
| $this->startSession(); | ||
| if (!isset($_SESSION[$key])) { | ||
| return ''; | ||
| } | ||
| $rawData = $_SESSION[$key]; | ||
| /** | ||
| * @var array{'exp': int, 'key': string, 'value': string} $data | ||
| */ | ||
| $data = json_decode($rawData, true, 512, JSON_THROW_ON_ERROR); | ||
| if ($data['exp'] > 0 && $data['exp'] < time()) { | ||
| $this->removeItem($key); | ||
| return ''; | ||
| } | ||
| return $data['value']; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type safety in getItem method.
Several improvements could enhance the robustness of this method:
- The JSON decoding could throw an exception that isn't caught
- The decoded data structure isn't validated before accessing its properties
- There's a potential race condition between the isset check and data usage
Consider this safer implementation:
public function getItem(string $key): string
{
$this->startSession();
if (!isset($_SESSION[$key])) {
return '';
}
$rawData = $_SESSION[$key];
+ try {
/**
- * @var array{'exp': int, 'key': string, 'value': string} $data
+ * @var mixed $decoded
*/
- $data = json_decode($rawData, true, 512, JSON_THROW_ON_ERROR);
+ $decoded = json_decode($rawData, true, 512, JSON_THROW_ON_ERROR);
+
+ if (!is_array($decoded) ||
+ !isset($decoded['exp']) ||
+ !isset($decoded['key']) ||
+ !isset($decoded['value']) ||
+ !is_int($decoded['exp']) ||
+ !is_string($decoded['key']) ||
+ !is_string($decoded['value'])
+ ) {
+ $this->removeItem($key);
+ return '';
+ }
+ $data = $decoded;
+
if ($data['exp'] > 0 && $data['exp'] < time()) {
$this->removeItem($key);
return '';
}
return $data['value'];
+ } catch (\JsonException $e) {
+ $this->removeItem($key);
+ return '';
+ }
}📝 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.
| public function getItem(string $key): string | |
| { | |
| $this->startSession(); | |
| if (!isset($_SESSION[$key])) { | |
| return ''; | |
| } | |
| $rawData = $_SESSION[$key]; | |
| /** | |
| * @var array{'exp': int, 'key': string, 'value': string} $data | |
| */ | |
| $data = json_decode($rawData, true, 512, JSON_THROW_ON_ERROR); | |
| if ($data['exp'] > 0 && $data['exp'] < time()) { | |
| $this->removeItem($key); | |
| return ''; | |
| } | |
| return $data['value']; | |
| } | |
| public function getItem(string $key): string | |
| { | |
| $this->startSession(); | |
| if (!isset($_SESSION[$key])) { | |
| return ''; | |
| } | |
| $rawData = $_SESSION[$key]; | |
| try { | |
| /** | |
| * @var mixed $decoded | |
| */ | |
| $decoded = json_decode($rawData, true, 512, JSON_THROW_ON_ERROR); | |
| if (!is_array($decoded) || | |
| !isset($decoded['exp']) || | |
| !isset($decoded['key']) || | |
| !isset($decoded['value']) || | |
| !is_int($decoded['exp']) || | |
| !is_string($decoded['key']) || | |
| !is_string($decoded['value']) | |
| ) { | |
| $this->removeItem($key); | |
| return ''; | |
| } | |
| $data = $decoded; | |
| if ($data['exp'] > 0 && $data['exp'] < time()) { | |
| $this->removeItem($key); | |
| return ''; | |
| } | |
| return $data['value']; | |
| } catch (\JsonException $e) { | |
| $this->removeItem($key); | |
| return ''; | |
| } | |
| } |
| public function removeItem(string $key): void | ||
| { | ||
| $newKey = self::getKey($key); | ||
| if (isset($_COOKIE[$newKey])) { | ||
| unset($_COOKIE[$newKey]); | ||
| self::setItem($key, "", -1); | ||
| } | ||
| self::setItem($key, "", -1); | ||
| } |
There was a problem hiding this comment.
Fix redundant cookie removal and add validation.
The method has a redundant call to setItem and lacks input validation.
Apply these fixes:
+ /**
+ * Remove a cookie from storage
+ * @param string $key Cookie key
+ * @throws \InvalidArgumentException If key is empty
+ */
public function removeItem(string $key): void
{
+ if (empty($key)) {
+ throw new \InvalidArgumentException('Cookie key cannot be empty');
+ }
$newKey = self::getKey($key);
if (isset($_COOKIE[$newKey])) {
unset($_COOKIE[$newKey]);
self::setItem($key, "", -1);
}
- self::setItem($key, "", -1);
}📝 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.
| public function removeItem(string $key): void | |
| { | |
| $newKey = self::getKey($key); | |
| if (isset($_COOKIE[$newKey])) { | |
| unset($_COOKIE[$newKey]); | |
| self::setItem($key, "", -1); | |
| } | |
| self::setItem($key, "", -1); | |
| } | |
| /** | |
| * Remove a cookie from storage | |
| * @param string $key Cookie key | |
| * @throws \InvalidArgumentException If key is empty | |
| */ | |
| public function removeItem(string $key): void | |
| { | |
| if (empty($key)) { | |
| throw new \InvalidArgumentException('Cookie key cannot be empty'); | |
| } | |
| $newKey = self::getKey($key); | |
| if (isset($_COOKIE[$newKey])) { | |
| unset($_COOKIE[$newKey]); | |
| self::setItem($key, "", -1); | |
| } | |
| } |
| public function setItem(string $key, string $value, int $expires = 0): void | ||
| { | ||
| $newKey = self::getKey($key); | ||
| $_COOKIE[$newKey] = $value; | ||
| setcookie($newKey, $value, [ | ||
| 'expires' => $expires, | ||
| 'path' => self::$cookiePath, | ||
| 'domain' => self::$cookieDomain, | ||
| 'samesite' => 'Lax', | ||
| 'secure' => self::$cookieSecure, | ||
| 'httponly' => self::$cookieHttpOnly | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Add validation and improve security for setItem method.
The method needs input validation and has potential security concerns.
Consider these security improvements:
+ /**
+ * Store a value in cookie storage
+ * @param string $key Cookie key
+ * @param string $value Cookie value
+ * @param int $expires Expiration timestamp
+ * @throws \InvalidArgumentException If key is empty
+ */
public function setItem(string $key, string $value, int $expires = 0): void
{
+ if (empty($key)) {
+ throw new \InvalidArgumentException('Cookie key cannot be empty');
+ }
+ if ($value === null) {
+ $value = '';
+ }
$newKey = self::getKey($key);
- $_COOKIE[$newKey] = $value;
+ $options = [
+ 'expires' => $expires,
+ 'path' => self::$cookiePath,
+ 'domain' => self::$cookieDomain,
+ 'samesite' => 'Lax',
+ 'secure' => self::$cookieSecure,
+ 'httponly' => self::$cookieHttpOnly
+ ];
- setcookie($newKey, $value, [
- 'expires' => $expires,
- 'path' => self::$cookiePath,
- 'domain' => self::$cookieDomain,
- 'samesite' => 'Lax',
- 'secure' => self::$cookieSecure,
- 'httponly' => self::$cookieHttpOnly
- ]);
+ if (setcookie($newKey, $value, $options)) {
+ $_COOKIE[$newKey] = $value;
+ }
}📝 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.
| public function setItem(string $key, string $value, int $expires = 0): void | |
| { | |
| $newKey = self::getKey($key); | |
| $_COOKIE[$newKey] = $value; | |
| setcookie($newKey, $value, [ | |
| 'expires' => $expires, | |
| 'path' => self::$cookiePath, | |
| 'domain' => self::$cookieDomain, | |
| 'samesite' => 'Lax', | |
| 'secure' => self::$cookieSecure, | |
| 'httponly' => self::$cookieHttpOnly | |
| ]); | |
| } | |
| /** | |
| * Store a value in cookie storage | |
| * @param string $key Cookie key | |
| * @param string $value Cookie value | |
| * @param int $expires Expiration timestamp | |
| * @throws \InvalidArgumentException If key is empty | |
| */ | |
| public function setItem(string $key, string $value, int $expires = 0): void | |
| { | |
| if (empty($key)) { | |
| throw new \InvalidArgumentException('Cookie key cannot be empty'); | |
| } | |
| if ($value === null) { | |
| $value = ''; | |
| } | |
| $newKey = self::getKey($key); | |
| $options = [ | |
| 'expires' => $expires, | |
| 'path' => self::$cookiePath, | |
| 'domain' => self::$cookieDomain, | |
| 'samesite' => 'Lax', | |
| 'secure' => self::$cookieSecure, | |
| 'httponly' => self::$cookieHttpOnly | |
| ]; | |
| if (setcookie($newKey, $value, $options)) { | |
| $_COOKIE[$newKey] = $value; | |
| } | |
| } |
| 'client_id' => $clientId, | ||
| 'grant_type' => GrantType::authorizationCode, | ||
| 'redirect_uri' => $clientSDK->redirectUri, | ||
| 'redirect_uri' => $redirectUri, |
There was a problem hiding this comment.
Remove 'grant_type' from the authorization URL parameters
In the OAuth 2.0 Authorization Code flow, the 'grant_type' parameter is not included in the authorization request URL. It is used when exchanging the authorization code for an access token at the token endpoint. Including it in the authorization URL might cause unexpected behavior or errors.
Apply this diff to remove the 'grant_type' parameter:
$searchParams = [
'client_id' => $clientId,
- 'grant_type' => GrantType::authorizationCode,
'redirect_uri' => $redirectUri,
'response_type' => 'code',Committable suggestion skipped: line range outside the PR's diff.
| if (!headers_sent()) { | ||
| exit(header('Location: ' . $this->buildAuthorizationURI($clientSDK->authorizationEndpoint, $clientSDK->clientId, $clientSDK->redirectUri, $clientSDK->scopes, $additionalParameters, $clientSDK->additionalParameters))); | ||
| } |
There was a problem hiding this comment.
Correct the use of header() and handle cases where headers have been sent
The use of exit(header('Location: ...')) is incorrect because header() returns void, so passing it to exit() is ineffective. Instead, call header() and then exit(); separately.
Additionally, if headers_sent() returns true, the redirect will not occur, and no feedback is provided. Consider handling the else case to inform the user or handle the error appropriately.
Apply this diff to fix the issue:
if (!headers_sent()) {
- exit(header('Location: ' . $this->buildAuthorizationURI($clientSDK->authorizationEndpoint, $clientSDK->clientId, $clientSDK->redirectUri, $clientSDK->scopes, $additionalParameters, $clientSDK->additionalParameters)));
+ header('Location: ' . $this->buildAuthorizationURI($clientSDK->authorizationEndpoint, $clientSDK->clientId, $clientSDK->redirectUri, $clientSDK->scopes, $additionalParameters, $clientSDK->additionalParameters));
+ exit();
} else {
+ // Handle the case when headers have already been sent
+ // For example, throw an exception or log an error
+ throw new \RuntimeException('Cannot redirect, headers already sent.');
}📝 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.
| if (!headers_sent()) { | |
| exit(header('Location: ' . $this->buildAuthorizationURI($clientSDK->authorizationEndpoint, $clientSDK->clientId, $clientSDK->redirectUri, $clientSDK->scopes, $additionalParameters, $clientSDK->additionalParameters))); | |
| } | |
| if (!headers_sent()) { | |
| header('Location: ' . $this->buildAuthorizationURI($clientSDK->authorizationEndpoint, $clientSDK->clientId, $clientSDK->redirectUri, $clientSDK->scopes, $additionalParameters, $clientSDK->additionalParameters)); | |
| exit(); | |
| } else { | |
| // Handle the case when headers have already been sent | |
| // For example, throw an exception or log an error | |
| throw new \RuntimeException('Cannot redirect, headers already sent.'); | |
| } |
| * @return object{'access_token': string, 'expires_in': int, 'id_token': string, 'refresh_token': string, 'scope': string, 'token_type': string}|void The authentication result. | ||
| * | ||
| * @throws InvalidArgumentException When an incorrect grant_type is provided. | ||
| * @throws Throwable When an error occurs during authentication. | ||
| * @throws \Throwable When an error occurs during authentication. |
There was a problem hiding this comment.
Inconsistent return types in login method
The login method has inconsistent return types depending on the grant type. For clientCredentials, it returns an object, but for other grant types, it returns void. This inconsistency may lead to confusion and potential errors when using the method.
Consider updating the method to have a consistent return type or properly document the expected return types for each grant type.
| header('Location: ' . $this->getLogoutUrl()); | ||
| exit(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid performing redirects inside the logout method
The logout method currently performs an HTTP redirect by calling header('Location: ' . $this->getLogoutUrl()); and then exit();. This practice can make unit testing difficult and may not align with best practices for handling HTTP responses within a library.
Consider refactoring the logout method to return the logout URL, allowing the caller to handle the redirection. Alternatively, provide a separate method for generating the logout URL and leave the responsibility of redirecting to the application layer.
Apply this diff to refactor the method:
public function logout(): void
{
$this->cleanStorage();
- header('Location: ' . $this->getLogoutUrl());
- exit();
+ // Caller can use getLogoutUrl() to redirect the user
}📝 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.
| header('Location: ' . $this->getLogoutUrl()); | |
| exit(); | |
| $this->cleanStorage(); | |
| // Caller can use getLogoutUrl() to redirect the user |
| @@ -508,7 +520,7 @@ private function fetchToken($formParams) | |||
|
|
|||
| $token = $response->getBody()->getContents(); | |||
| $this->storage->setToken($token); | |||
| $tokenDecode = json_decode($token, false); | |||
| $tokenDecode = json_decode($token, false, 512, JSON_THROW_ON_ERROR); | |||
There was a problem hiding this comment.
Handle potential exceptions from json_decode
The use of JSON_THROW_ON_ERROR in json_decode will cause a JsonException to be thrown on error, but this exception is not currently caught or handled.
Wrap the json_decode call in a try-catch block to handle potential JsonException and provide appropriate error handling.
Apply this diff to handle the exception:
$this->storage->setToken($token);
- $tokenDecode = json_decode($token, false, 512, JSON_THROW_ON_ERROR);
+ try {
+ $tokenDecode = json_decode($token, false, 512, JSON_THROW_ON_ERROR);
+ } catch (\JsonException $e) {
+ throw new \Exception('Failed to decode token JSON: ' . $e->getMessage(), 0, $e);
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
lib/KindeClientSDK.php (1)
Line range hint
77-152: Consider using StorageInterface for better dependency injection.While the addition of the Storage parameter is good, using the concrete Storage class and getInstance() fallback could make testing and extending the SDK harder.
Consider this change:
- public Storage $storage; + use Kinde\KindeSDK\Sdk\Storage\StorageInterface; + + public StorageInterface $storage; - public function __construct( - string $domain, - ?string $redirectUri, - string $clientId, - string $clientSecret, - string $grantType, - ?string $logoutRedirectUri, - string $scopes = 'openid profile email offline', - array $additionalParameters = [], - string $protocol = "", - ?Storage $storage = null + public function __construct( + string $domain, + ?string $redirectUri, + string $clientId, + string $clientSecret, + string $grantType, + ?string $logoutRedirectUri, + string $scopes = 'openid profile email offline', + array $additionalParameters = [], + string $protocol = "", + ?StorageInterface $storage = null ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lib/KindeClientSDK.php(18 hunks)
🔇 Additional comments (2)
lib/KindeClientSDK.php (2)
Line range hint 27-75: LGTM! Property changes enhance type safety and flexibility.
The changes to property types, particularly making URIs nullable and adding proper type hints, improve the SDK's flexibility and type safety.
159-182: LGTM! Improved login method with better type safety.
The switch statement is cleaner than the previous try-catch, and the return type documentation is more precise.
| public function getLogoutUrl(): string | ||
| { | ||
| $searchParams = [ | ||
| 'redirect' => $this->logoutRedirectUri | ||
| ]; | ||
| header('Location: ' . $this->logoutEndpoint . '?' . http_build_query($searchParams)); | ||
| exit(); | ||
| return $this->logoutEndpoint . '?' . http_build_query($searchParams); |
There was a problem hiding this comment.
Ensure proper URL encoding of the redirect parameter.
The logout URL generation should encode the redirect parameter to prevent potential security issues.
Apply this change:
public function getLogoutUrl(): string
{
$searchParams = [
- 'redirect' => $this->logoutRedirectUri
+ 'redirect' => urlencode($this->logoutRedirectUri)
];
return $this->logoutEndpoint . '?' . http_build_query($searchParams);
}📝 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.
| public function getLogoutUrl(): string | |
| { | |
| $searchParams = [ | |
| 'redirect' => $this->logoutRedirectUri | |
| ]; | |
| header('Location: ' . $this->logoutEndpoint . '?' . http_build_query($searchParams)); | |
| exit(); | |
| return $this->logoutEndpoint . '?' . http_build_query($searchParams); | |
| public function getLogoutUrl(): string | |
| { | |
| $searchParams = [ | |
| 'redirect' => urlencode($this->logoutRedirectUri) | |
| ]; | |
| return $this->logoutEndpoint . '?' . http_build_query($searchParams); |
| public function refreshWithToken(string $refreshToken) | ||
| { | ||
| $formParams = [ | ||
| 'client_id' => $this->clientId, | ||
| 'client_secret' => $this->clientSecret, | ||
| 'grant_type' => 'refresh_token', | ||
| 'refresh_token' => $refreshToken | ||
| ]; | ||
|
|
||
| $token = $this->fetchToken($formParams); | ||
| return $token; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type hint and error handling to refreshWithToken method.
The new method needs proper type hinting and should handle potential errors during token refresh.
Apply these improvements:
- public function refreshWithToken(string $refreshToken)
+ /**
+ * Refreshes the access token using a refresh token.
+ *
+ * @param string $refreshToken The refresh token to use
+ * @return object{'access_token': string, 'expires_in': int, 'id_token': string, 'refresh_token': string, 'scope': string, 'token_type': string}
+ * @throws GuzzleException When the HTTP request fails
+ * @throws JsonException When token decoding fails
+ */
+ public function refreshWithToken(string $refreshToken): object
{
+ if (empty($refreshToken)) {
+ throw new InvalidArgumentException('Refresh token cannot be empty');
+ }
$formParams = [
'client_id' => $this->clientId,
'client_secret' => $this->clientSecret,
'grant_type' => 'refresh_token',
'refresh_token' => $refreshToken
];
$token = $this->fetchToken($formParams);
return $token;
}📝 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.
| public function refreshWithToken(string $refreshToken) | |
| { | |
| $formParams = [ | |
| 'client_id' => $this->clientId, | |
| 'client_secret' => $this->clientSecret, | |
| 'grant_type' => 'refresh_token', | |
| 'refresh_token' => $refreshToken | |
| ]; | |
| $token = $this->fetchToken($formParams); | |
| return $token; | |
| } | |
| /** | |
| * Refreshes the access token using a refresh token. | |
| * | |
| * @param string $refreshToken The refresh token to use | |
| * @return object{'access_token': string, 'expires_in': int, 'id_token': string, 'refresh_token': string, 'scope': string, 'token_type': string} | |
| * @throws GuzzleException When the HTTP request fails | |
| * @throws JsonException When token decoding fails | |
| */ | |
| public function refreshWithToken(string $refreshToken): object | |
| { | |
| if (empty($refreshToken)) { | |
| throw new InvalidArgumentException('Refresh token cannot be empty'); | |
| } | |
| $formParams = [ | |
| 'client_id' => $this->clientId, | |
| 'client_secret' => $this->clientSecret, | |
| 'grant_type' => 'refresh_token', | |
| 'refresh_token' => $refreshToken | |
| ]; | |
| $token = $this->fetchToken($formParams); | |
| return $token; | |
| } |
The main changes are: