Apply cache prefix, drop vendored Folder, tighten route-slot matchers#169
Open
dereuromark wants to merge 2 commits intomasterfrom
Open
Apply cache prefix, drop vendored Folder, tighten route-slot matchers#169dereuromark wants to merge 2 commits intomasterfrom
dereuromark wants to merge 2 commits intomasterfrom
Conversation
Cache::key() never applied the configured cachePrefix, so two TinyAuth
installs sharing a Cake cache pool collided on the bare `acl` / `allow`
keys. Multi-tenant or test environments where Cache pools survive across
plugin boots could read another install's permissions data. The prefix
is now applied, yielding `tiny_auth_acl` / `tiny_auth_allow` by default.
Apps that set a custom or empty prefix work as written. Existing caches
under the old bare keys go cold once and repopulate on next request.
Removed src/Filesystem/Folder.php — a vendored 946-line copy of the
legacy CakePHP Folder class that the rest of the framework deleted long
ago. Internally it was used only to scan controller directories from
Syncer and Adder; replaced with a local _listDirectory(...) using
scandir() that returns the same [folders, files] shape, so the
$folderContent[0] / $folderContent[1] callsites work unchanged. Two
test files used Folder::copy() to seed /tmp; replaced with a
RecursiveDirectoryIterator-based copyDirectory() helper in each test.
AclTrait::_isPublic and AllowTrait::_getAllowRule mixed isset() with
!empty() to compare plugin/prefix slots between request and rule. The
mismatch swallowed PHP-falsy values ('0', empty string) where it
shouldn't and treated them inconsistently between the two sides.
Extracted a _matchesRouteSlot / _matchesAllowSlot helper that treats
null and empty-string as the "no plugin / no prefix" sentinel and
requires exact match otherwise. Also replaced two strpos(..., $needle)
=== 0 calls with str_starts_with for clarity (PHP 8 has it natively).
3 new tests cover the cache-prefix application: default prefix, custom
prefix, empty prefix yields bare key. All 118 tests, phpstan, and phpcs
pass.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #169 +/- ##
============================================
+ Coverage 67.98% 77.65% +9.66%
+ Complexity 639 494 -145
============================================
Files 33 32 -1
Lines 1646 1329 -317
============================================
- Hits 1119 1032 -87
+ Misses 527 297 -230 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request updates TinyAuth’s caching and route-matching behavior to avoid cross-install collisions, removes a vendored legacy filesystem dependency, and makes route slot matching semantics consistent.
Changes:
- Apply
TinyAuth.cachePrefixwhen computing cache keys (defaulting totiny_auth_) and add tests covering default/custom/empty prefixes. - Remove the vendored legacy
Folderimplementation and replace controller-directory scanning with a localscandir()-based helper; update affected command tests with a new directory copy helper. - Normalize plugin/prefix “route slot” comparisons in
AclTrait/AllowTraitand replacestrpos(... ) === 0withstr_starts_with().
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/TestCase/Utility/CacheTest.php | Adds tests validating cache key prefix behavior (default/custom/empty prefix). |
| tests/TestCase/Command/SyncCommandTest.php | Replaces Folder::copy() usage with a local recursive directory copy helper. |
| tests/TestCase/Command/AddCommandTest.php | Replaces Folder::copy() usage with a local recursive directory copy helper. |
| src/Utility/Cache.php | Updates Cache::key() to prepend the configured cache prefix. |
| src/Sync/Syncer.php | Replaces Folder::read() with a local _listDirectory() helper for controller discovery. |
| src/Sync/Adder.php | Replaces Folder::read() with a local _listDirectory() helper for controller discovery. |
| src/Filesystem/Folder.php | Removes the vendored legacy CakePHP Folder class implementation. |
| src/Auth/AllowTrait.php | Introduces consistent “empty slot” matching helper for plugin/prefix and uses str_starts_with(). |
| src/Auth/AclTrait.php | Introduces consistent “empty slot” matching helper for plugin/prefix and uses str_starts_with(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses two PR-review concerns:
1. _listDirectory() in Syncer/Adder must skip hidden entries (.git, .svn,
.DS_Store, ...) the way the previously-vendored Folder::read did.
Adding 'is the first char a dot' to the existing '.'/'..' filter
restores the prior behavior — without this, the scan would recurse
into editor/VCS metadata directories and potentially pick up
controller-shaped files inside them.
2. Codecov flagged Adder.php at 0% patch coverage and AclTrait at 61%.
Adder's controller-discovery loop was previously only reachable via
the AddCommand integration test, and the trait's new _matchesRouteSlot
helper had no direct coverage. Added:
- SyncCommandTest::testSyncSkipsHiddenDirectories and
AddCommandTest::testAddSkipsHiddenDirectories — drop a .git /
.svn directory with a controller-shaped file in tmp, run the
command, assert the controller name doesn't surface in output.
This exercises Adder._listDirectory and Syncer._listDirectory
with hidden-entry rejection in the same path.
- AclTraitRouteSlotMatcherTest — anonymous classes hosting AclTrait
and AllowTrait expose the protected matchers via a thin wrapper,
plus a 11-row dataProvider covers all (null, '', value) crossings
including the 'distinct types must not collapse' case the previous
!empty() implementation got wrong.
phpunit (145/145), phpstan, and phpcs all pass.
Owner
Author
|
Addressed Copilot's feedback on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cache::key()never applied the configuredcachePrefix, so two TinyAuth installs sharing a Cake cache pool collided on the bareacl/allowkeys. Multi-tenant or test environments where Cache pools survive across plugin boots could read another install's permissions data. The prefix is now applied, yieldingtiny_auth_acl/tiny_auth_allowby default. Apps that set a custom or empty prefix work as written. Existing caches under the old bare keys go cold once and repopulate on next request.src/Filesystem/Folder.php— a vendored 946-line copy of the legacy CakePHPFolderclass that the rest of the framework deleted long ago. Internally it was used only to scan controller directories fromSyncerandAdder; replaced with a local_listDirectory()usingscandir()that returns the same[folders, files]shape, so the$folderContent[0]/$folderContent[1]callsites work unchanged. Two test files usedFolder::copy()to seed/tmp; replaced with aRecursiveDirectoryIterator-basedcopyDirectory()helper.AclTrait::_isPublicandAllowTrait::_getAllowRulemixedisset()with!empty()to compare plugin/prefix slots between request and rule. The mismatch swallowed PHP-falsy values ('0', empty string) where it should not and treated them inconsistently between the two sides. Extracted a_matchesRouteSlot/_matchesAllowSlothelper that treats null and empty-string as the "no plugin / no prefix" sentinel and requires exact match otherwise.strpos(..., $needle) === 0calls withstr_starts_withfor clarity (PHP 8 has it natively).3 new tests cover the cache-prefix application: default prefix, custom prefix, empty prefix yields bare key. All 118 tests, phpstan, and phpcs pass.