Feature v 12 0 3 #9
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 19515427 | Triggered | Generic High Entropy Secret | eef1a6d | tests/Fixtures/Saloon/get-vaults.json | View secret |
| 19515428 | Triggered | Generic High Entropy Secret | f3e6198 | tests/Fixtures/Saloon/get-vaults.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Pull Request Overview
This PR implements the v12.0.3 feature update for the M-Files package, focusing on significant refactoring of authentication, caching mechanisms, and API integration patterns. The changes move from fixture-based testing to using real API responses through Saloon's MockResponse system and introduce a new caching layer for authentication tokens.
Key changes include:
- Complete refactoring of authentication and caching mechanisms with new CacheKeyManager and updated authentication flow
- Migration from custom fixtures to Saloon's MockResponse system with real API response data
- Restructuring of DTOs and removal of deprecated classes like Documents, User, and DocumentProperties
Reviewed Changes
Copilot reviewed 72 out of 74 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Helpers/CacheKeyManagerTest.php | New unit tests for the cache key management functionality |
| tests/Unit/DTO/PropertyValueTest.php | Comprehensive unit tests for PropertyValue DTO with all data types |
| tests/Fixtures/Saloon/*.json | Updated test fixtures with real API responses replacing redacted test data |
| tests/Feature/Requests/*.php | Refactored feature tests to use MockResponse instead of custom fixtures |
| src/Helpers/CacheKeyManager.php | New cache management utility for authentication tokens |
| src/DTO/PropertyValue.php | Simplified PropertyValue implementation with improved data type handling |
| src/DTO/ConfigWithCredentials.php | Moved from Config subdirectory and simplified structure |
| src/Requests/*.php | Updated request classes with simplified authentication and endpoint handling |
| src/Connectors/MFilesConnector.php | Updated to use new authentication pattern with HeaderAuthenticator |
Comments suppressed due to low confidence (1)
tests/Unit/DTO/PropertyValueTest.php:350
- This test only verifies that properties can be accessed but doesn't actually test the readonly nature. Consider adding assertions that attempt to modify the properties and expect exceptions or use reflection to verify the readonly attribute.
public function test_properties_are_readonly(): void
| public static function fromArray(int $propertyDef, MFDataTypeEnum $dataType, mixed $value): self | ||
| { | ||
| $typedValue = Arr::get($data, 'TypedValue', []); | ||
| $dataTypeId = Arr::get($typedValue, 'DataType', 0); | ||
| $dataType = MFDataTypeEnum::tryFrom($dataTypeId) ?? MFDataTypeEnum::UNINITIALIZED; | ||
|
|
||
| return new self( | ||
| propertyDef: Arr::get($data, 'PropertyDef'), | ||
| propertyDef: $propertyDef, | ||
| dataType: $dataType, | ||
| value: Arr::get($typedValue, 'Value'), | ||
| lookup: Arr::get($typedValue, 'Lookup'), | ||
| lookups: Arr::get($typedValue, 'Lookups'), | ||
| value: $value, |
There was a problem hiding this comment.
The method signature suggests it creates from an array but actually takes individual parameters. Consider renaming to 'create' or 'make' to better reflect its purpose, or change the implementation to actually accept an array parameter.
|
|
||
| $cacheManager = new CacheKeyManager($config); | ||
|
|
||
| return $cacheManager->rememberAuthToken(3600, function () use ($config) { |
There was a problem hiding this comment.
The TTL value of 3600 seconds is hardcoded. Consider making this configurable through the config parameter or a class constant to improve maintainability.
| return $cacheManager->rememberAuthToken(3600, function () use ($config) { | |
| return $cacheManager->rememberAuthToken(self::DEFAULT_TTL, function () use ($config) { |
| /** | ||
| * Set authentication token in cache | ||
| */ | ||
| public function setAuthToken(mixed $token, int $ttl = 3600): bool |
There was a problem hiding this comment.
The default TTL of 3600 seconds is hardcoded. Consider using a class constant or configuration value to make this more maintainable and consistent across the application.
| public function setAuthToken(mixed $token, int $ttl = 3600): bool | |
| public function setAuthToken(mixed $token, int $ttl = self::DEFAULT_TTL): bool |
| 'Expiration' => $this->expiration, | ||
| ]; | ||
| $value = Arr::get($responseData, 'Value'); | ||
| if (empty($value)) { |
There was a problem hiding this comment.
Using empty() check may not be appropriate here as it would return true for '0' string or 0 integer, which could be valid token values. Consider using a more specific check like $value === null || $value === ''.
| if (empty($value)) { | |
| if ($value === null || $value === '') { |
| 'username' => env('M_FILES_USERNAME'), | ||
| 'password' => env('M_FILES_PASSWORD'), | ||
| 'expiration' => env('M_FILES_EXPIRATION_SECONDS', '3600'), // Default to 1 hour | ||
| 'expiration' => env('M_FILES_EXPIRATION_SECONDS', 1), |
There was a problem hiding this comment.
The default expiration is set to 1 second, which is extremely short and likely will cause authentication failures. This should probably be a reasonable default like 3600 seconds (1 hour).
| 'expiration' => env('M_FILES_EXPIRATION_SECONDS', 1), | |
| 'expiration' => env('M_FILES_EXPIRATION_SECONDS', 3600), |
No description provided.