[4x] Remove redundant universal route check from PreventAccess MW#1427
Conversation
The PreventAcessFromUnwantedDomains MW had the routeIsUniversal check either for returning early, or it was a leftover from some older implementation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1427 +/- ##
============================================
+ Coverage 86.03% 86.07% +0.03%
- Complexity 1156 1174 +18
============================================
Files 184 185 +1
Lines 3381 3448 +67
============================================
+ Hits 2909 2968 +59
- Misses 472 480 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| /** | ||
| * Prevents accessing central domains in the tenant context/tenant domains in the central context. | ||
| * The access isn't prevented if the request is trying to access a route flagged as 'universal', |
There was a problem hiding this comment.
Keeping the docblock like this may be a bit confusing because now, skipping this MW for universal routes is not as explicit. Instead of the explicit condition, this is implied (if ($this->accessingTenantRouteFromCentralDomain || $this->accessingCentralRouteFromTenantDomain) $abortRequest($request, $next) -- the abort only happens for central or tenant routes, never universal).
So maybe I'd rephrase this a bit
There was a problem hiding this comment.
Looking at this now, the current docblock makes sense, probably no reason to change it.
There was a problem hiding this comment.
This also seemed strange to me, so I'm marking this review unresolved. The docblock explicitly says that if the route is universal, the middleware will be skipped. Previously that was also implemented explicitly.
Now with this change it'd be implemented implicitly, making the docblock seem weird (you read it and think "where is this logic for skipping if it's universal?") and possibly too reliant on implementation details that aren't super explicit.
Unlike this PR's description, the original issue said the routeIsUniversal() check is redundant because shouldBeSkipped() is checking for the route being universal already (though that doesn't seem correct to me because the relevant code is actually more nuanced)
if (Tenancy::routeIsUniversal($route) && $this instanceof IdentificationMiddleware) {
return $this->determineUniversalRouteContextFromRequest(request()) === Context::CENTRAL;
}On the other hand this PR's desc talks about the universal case being "default fallthrough" by virtue of not passing the $this->accessingTenantRouteFromCentralDomain($request, $route) || $this->accessingCentralRouteFromTenantDomain($request, $route) condition.
Since we use route modes in a few different contexts, it wasn't immediately clear to me if the code used in those methods (tenancy()->getRouteMode($route)) even can be universal, or if it just determines the actual runtime context of the request. But I can see the docblock for that method says it has explicit high-priority logic for the universal case.
But if I change the code like this:
if ($this->accessingTenantRouteFromCentralDomain($request, $route) || $this->accessingCentralRouteFromTenantDomain($request, $route)) {
$abortRequest = static::$abortRequest ?? function () {
abort(404);
};
return $abortRequest($request, $next);
+} else {
+ dump(tenancy()->getRouteMode($route)->name);
}I only ever see CENTRAL and TENANT printed, never UNIVERSAL, so I'm not sure if the explanation in this PR's description is accurate.
That's when running PreventAccess tests, Universal tests do show UNIVERSAL here. So we just don't have tests for this case in the PreventAccess test file. Could be improved but not important.
Also confirmed that that branch is only ever reached if the patch from this PR is applied.
📝 WalkthroughWalkthroughThe middleware’s handle() now only returns early when shouldBeSkipped($route) is true; explicit Tenancy::routeIsUniversal($route) exclusion was removed so universal routes may be evaluated by the subsequent access checks. ChangesMiddleware Logic Simplification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
Removes a redundant universal-route short-circuit from PreventAccessFromUnwantedDomains, relying on the existing route-mode checks to ensure universal routes naturally pass through without triggering access prevention.
Changes:
- Removed
tenancy()->routeIsUniversal($route)from the early-return condition in the middleware. - Leaves skipping behavior solely to
shouldBeSkipped($route).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Middleware/PreventAccessFromUnwantedDomains.php`:
- Around line 34-39: Confirm that tenancy()->routeIsUniversal($route) and
tenancy()->getRouteMode($route) === RouteMode::UNIVERSAL are equivalent by
locating and comparing their implementations (look for the routeIsUniversal
function, the getRouteMode function, and the RouteMode enum values) and ensure
routeIsUniversal returns true exactly when getRouteMode(...) yields
RouteMode::UNIVERSAL; if they differ, restore the explicit
tenancy()->routeIsUniversal($route) check in PreventAccessFromUnwantedDomains
(the early-return at the top where shouldBeSkipped($route) is used) or modify
getRouteMode/routeIsUniversal so their logic matches, and add/update tests that
assert the equivalence to prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08a34a4b-0417-471a-9f46-bb18b1485fc9
📒 Files selected for processing (1)
src/Middleware/PreventAccessFromUnwantedDomains.php
stancl
changed the title
stancl
changed the title
The PreventAcessFromUnwantedDomains MW had the
tenancy()->routeIsUniversal($route)check either for returning early, or it was a leftover from some older implementation, so I removed it.The middleware aborts if the
$this->accessingTenantRouteFromCentralDomain($request, $route) || $this->accessingCentralRouteFromTenantDomain($request, $route)check passes. Meaning, for the middleware to abort, the route has to be either in central or tenant mode. When the route is in universal mode, the middleware will never reachreturn $abortRequest().return $next($request)will always get reached, even when the|| tenancy()->routeIsUniversal($route)check is deleted from the previous condition, so that check was basically useless.Since the docblock for the class does mention the behavior for universal routes explicitly, we've instead added a comment documenting that things work this way. That's probably the most reasonable way to have this explicit behavior for universal routes easily understandable in this fairly complex logic without redundant code.
Resolves #1418
Summary by CodeRabbit