Conversation
…3507) Email ability permission_callbacks previously only enforced a flat use_tools/can_manage capability floor and left mailbox ownership entirely to execute()-time resolution inside EmailAuth::resolve_mailbox(). Any use_tools holder could pass an arbitrary auth_ref and only get denied deep inside IMAP connection logic — or succeed if that ref happened to be accessible (e.g. the shared default). The Abilities API permission_callback contract was answering "does this caller have a Data Machine capability" instead of "can this caller reach this specific mailbox". Add EmailMailboxPermission, a trait providing authorizeMailboxRef() that reuses EmailAuth's existing resolve_mailbox() authorization (named accounts scoped by site/user/agent ownership, plus per-agent operation delegations) as a side-effect-free preflight. It denies only on the specific email_mailbox_forbidden outcome — "this ref exists and you don't own it" — and lets execute() report every other resolution failure (not configured, malformed, ambiguous) with its own specific error, since those are configuration/state problems, not authorization decisions. Wire it into every email ability's permission_callback: fetch-email, email-reply, email-delete, email-move, email-flag, email-batch-move, email-batch-flag, email-batch-delete, email-unsubscribe, email-batch-unsubscribe, email-test-connection, send-email, and send-email-queued. EmailAbilities' shared callbacks live in a new EmailAbilitiesPermissions trait to keep EmailAbilities.php under the codebase's file-size threshold. send-email special-cases input carrying a `_mailbox_grant`: that shape is only ever produced by SendEmailQueuedAbility's Action Scheduler worker, whose dispatch context has no ambient PermissionHelper identity. The grant is independently re-verified by signature inside execute() against its own issuer identity, so checkPermission() defers to that instead of incorrectly denying a previously-authorized send. Decision on existing unowned refs: email_imap:default (site scope, user_id 0) stays a deliberately shared operational mailbox gated by EmailAuth::can_use_default()'s management-capability check — narrower than generic owns_resource() semantics for resource_user_id === 0, which would treat it as open to any use_tools holder. That's why this reuses EmailAuth's resolution instead of PermissionHelper::owns_resource(): the richer named-account/delegation model already encodes the right policy and owns_resource() would either duplicate or weaken it. Adds EmailMailboxPermissionTest covering: owner acting on their own named mailbox, a non-owner denied, an admin NOT bypassing another user's personal mailbox, an admin retaining access to the shared default, a use_tools-only caller denied the shared default, a missing ref falling back to the capability floor, an unconfigured ref not being treated as a denial, and the send-email grant bypass contract. Fixes #3507
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.
Fixes #3507
Where auth refs live and how ownership is resolved
email_imapmailbox refs are already a richer model than a flatuser_idcolumn —EmailAuth(extendingBaseAuthProvider) stores named accounts scoped bysite/user/agentownership (AUTH_SCOPE_SITE|USER|AGENT), plus per-agent operation delegations (grant_agent()/revoke_agent()).EmailAuth::resolve_mailbox()/resolve_mailbox_for_principal()is the single authorization choke point every execute() path already calls: it resolves the ref, checks the caller against the account's owner (or an agent's delegation grant), and returns aWP_Erroron denial.The actual gap #3507 reports isn't a missing owner model — it's that ability
permission_callbacks never consulted it. Every email ability'scheckPermission()was a flatuse_tools/can_managecapability floor. Ownership was enforced only deep insideexecute()'s IMAP connection logic, so the Abilities API'spermission_callbackcontract ("can this call proceed") never actually answered the resource-specific question — anyuse_toolsholder passed the gate for anyauth_ref.What changed
Added
EmailMailboxPermission(inc/Abilities/Email/EmailMailboxPermission.php), a trait providingauthorizeMailboxRef( $input, $operation ):use_tools/can_managefloor first (unchanged baseline).auth_ref, resolves it throughEmailAuth::resolve_mailbox()as a side-effect-free preflight (_skip_auditcontext flag added toEmailAuth::audit()so the preflight doesn't double-log every real operation).email_mailbox_forbidden— "this ref exists and you don't own it." Every other resolution failure (auth_ref_unresolved,auth_ref_invalid,email_mailbox_ambiguous) passes the permission gate and is left toexecute()to report with its specific error — those are configuration/state problems, not authorization decisions, and collapsing them into a generic 403 would be a UX regression (confirmed against the existingEmailAbilityRestTest::test_rest_visible_email_test_connection_resolves_to_abilitycontract).auth_refis present at all (e.g. a chat-tool availability probe called with no input), the capability floor alone governs —execute()still enforces whatever default/legacy-sender rule applies.Every ability gated
fetch-email,email-reply,email-delete,email-move,email-flag,email-batch-move,email-batch-flag,email-batch-delete,email-unsubscribe,email-batch-unsubscribe,email-test-connection,send-email,send-email-queued. TheEmailAbilitiesCRUD class's per-operation callbacks (each ability needs its own operation set — reply vs delete vs organize+delete for move, etc.) live in a newEmailAbilitiesPermissionstrait rather than inline inEmailAbilities.php, which was already close to the codebase's 1500-line file-size audit threshold (homeboy review audit --profile prflagged this on the first pass; splitting the trait out resolved it cleanly — verified green on the second pass).send-email's one special case: when input carries a_mailbox_grant— a shape onlySendEmailQueuedAbility's Action Scheduler worker ever produces —checkPermission()defers toexecute()'s independent HMAC signature verification against the grant's own issuer identity, instead of re-deriving ownership fromPermissionHelper's ambient context. That context is empty during real AS dispatch (no acting user/agent), so re-checking ownership there would incorrectly deny a send that was already authorized at queue time.send-email-queueditself needs no such case — queuing is always synchronous with live ambient context; the deferred send is dispatched directly against thedatamachine_send_email_workerhook, never throughwp_get_ability('datamachine/send-email-queued')->execute().Decision on the existing unowned/global ref
email_imap:default(site scope,owner_id0) stays a deliberately shared operational mailbox, gated byEmailAuth::can_use_default()'s management-capability check (manage_options/datamachine_manage_agents/datamachine_manage_flows/datamachine_manage_settings) — not by a bareuse_toolsfloor.This is why
authorizeMailboxRef()reusesEmailAuth::resolve_mailbox()instead of routing throughPermissionHelper::owns_resource()as the issue originally suggested:owns_resource()treatsresource_user_id === 0as "shared, accessible to anyone with the capability" (correct for jobs/agents in single-agent mode), which would have been a regression here — it would open the default mailbox to anyuse_toolsholder, exactly the hole this PR closes.EmailAuth's named-account/delegation model already encodes the correct, narrower policy; reusing it keeps one source of truth instead of introducing a second, weaker mechanism that disagrees with the first.Tests
New
tests/Unit/Abilities/EmailMailboxPermissionTest.php:AUTH_SCOPE_USER) mailbox → alloweduse_tools→ deniedEmailAuth::can_access()'s existing contract — no admin override for user-scoped accounts)email_imap:defaultmailboxuse_tools-only (non-management) caller is denied the shared defaultauth_reffalls back to the capability floorexecute()report it)send-emaildenies a direct call against an unowned ref, and defers to the signed grant when_mailbox_grantis presentsend-email-queuedgates ownership at queue time with no grant special casePlus updated
require_oncewiring in the existing pure-PHP smoke tests (send-email-template-smoke.php,email-reply-sent-copy-smoke.php,send-email-ability-lazy-definitions-smoke.php,abilities-send-email-load-order-smoke.php,lightweight-ability-manifest-smoke.php) thatrequirethese ability classes directly without an autoloader.Verification (real results)
named-mailbox-security-contract-smoke.php,named-mailbox-delegation-smoke.php,legacy-email-upgrade-auth-smoke.php.homeboy review audit --changed-since origin/main --profile pr: pass, 0 introduced findings (after splittingEmailAbilitiesPermissionsout — first pass flagged agod_fileline-count finding onEmailAbilities.php, resolved).homeboy review lint --changed-only: phpcs 0 findings, eslint 0 findings, phpstan 69 findings — all pre-existing, confirmed by re-running the identical lint against unmodifiedmain(viagit stash) with an isolated failing test: the same 69 findings reproduce with zero relation to this diff. They'reIMAP\Connectionvsresourcetype-signature drift from a PHP/phpstan-stub version bump, plus a fewright side of && is always true/empty() offsetfindings — all in pre-existingexecute()-path IMAP code untouched by this PR, none in the new permission-callback code. Reporting honestly rather than claiming or hiding this: this is pre-existing debt, not introduced here.homeboy review test -- --filter Email(real WordPress test runtime via WP Codebox): 19/20 passed. The 1 failure (EmailAbilityRestTest::test_rest_visible_get_orphaned_posts_round_trip) is unrelated to email — confirmed pre-existing by running the identical filtered test against unmodifiedmain, where it fails identically.Out of scope (per the issue)
data-machine-socials— the issue explicitly flags this as a sibling with a legitimately different answer (shared brand credentials vs. per-user inboxes) and out of scope for this PR.🤖 Generated with Claude Code