Consult the host's mail policy on preview and attachment routes - #87
Open
MACscr wants to merge 1 commit into
Open
Consult the host's mail policy on preview and attachment routes#87MACscr wants to merge 1 commit into
MACscr wants to merge 1 commit into
Conversation
canManageMails() answers whether a user may use the mail log at all. Hosts that need a per-mail answer as well - a multi-tenant application where a user may only open mails belonging to their own tenant - have no hook: both controllers resolve the mail by id and serve it. When the host has registered a policy for the mail model, authorize the view ability against the requested mail before serving the preview or an attachment. Hosts without a policy are unaffected.
Contributor
|
What is the DenyOddMailPolicy class testing exactly? There is no Gate check for the $user. |
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.
Problem
#84 / #86 put the preview and attachment routes behind
AuthenticateandcanManageMails(). That answers "may this user use the mail log at all?", which is the same question every Resource, Page and Widget asks — but the controllers still resolve the mail by id and serve it:Hosts that need a second answer — "may this user see this mail?" — have no hook. In a multi-tenant application every user who may open the mail log can read any tenant's mail, and download its attachments, by changing the id in the URL. Wrapping the routes in extra middleware from the host is possible but means re-declaring
Mails::routes()and its middleware by hand, which is exactly the class of drift #84 was written to prevent.Fix
When the host has registered a policy for the mail model, both controllers authorize the
viewability against the requested mail before serving it:Gate::getPolicyFor()returnsnulland nothing changes.canManageMails()keeps its role as the coarse gate; this runs after it, inside the same middleware stack.The README section on
canManageMails()now documents the policy hook with a tenant example.Tests
Two new cases in
tests/MailRouteSecurityTest.phpusing aDenyOddMailPolicyfixture: the preview is forbidden for a denied mail and served for an allowed one; an attachment download is forbidden for a denied mail. Full suite: 16 passed (39 assertions).