| @@ -11,42 +11,27 @@ | ||
| 11 | 11 | use FluentSupport\App\Models\Ticket; |
| 12 | 12 | use FluentSupport\App\Services\Notifications\NotificationSettings; |
| 13 | 13 | |
| 14 | 14 | /** |
| 15 | - * Reconciles a customer record with the WordPress account it belongs to after | |
| 16 | - * that account's email address has moved. | |
| 15 | + * Moves a customer record onto its WordPress account's new email address once | |
| 16 | + * the customer proves they own it. | |
| 17 | 17 | * |
| 18 | - * ProfileInfoService deliberately refuses to follow an unverified email change, | |
| 19 | - * so an account whose address changes through REST, WP-CLI or a WooCommerce | |
| 20 | - * account form leaves its customer record pointing at the previous address. | |
| 21 | - * That is the safe outcome but not a finished one: support mail keeps going to | |
| 22 | - * an inbox the customer may have stopped reading, and any ticket they open from | |
| 23 | - * the new address lands on a second, unlinked record. | |
| 24 | - * | |
| 25 | - * This closes that gap without ever trusting the account address on its own. | |
| 26 | - * The divergence is surfaced in the portal, the customer asks for a | |
| 27 | - * confirmation link, and the link is mailed to the address being claimed. | |
| 28 | - * | |
| 29 | - * The security of the whole flow rests on which inbox that mail lands in. When | |
| 30 | - * somebody points their WordPress account at an address they do not own, the | |
| 31 | - * link goes to the real owner, who learns of the attempt, and the sender is | |
| 32 | - * left with nothing. Confirming also requires being signed in as the claiming | |
| 33 | - * account, so neither the inbox nor the account moves anything alone. | |
| 18 | + * ProfileInfoService won't follow an unverified email change, so the record | |
| 19 | + * keeps the old address. The confirmation link goes to the new address and | |
| 20 | + * only works for the signed-in account that requested it, so neither the inbox | |
| 21 | + * nor the account can move the record alone. | |
| 34 | 22 | */ |
| 35 | 23 | class EmailClaimService |
| 36 | 24 | { |
| 37 | 25 | /** |
| 38 | - * How long a claim link stays valid. | |
| 39 | - * | |
| 40 | - * WordPress's own email change confirmation never expires. Bounding it is | |
| 41 | - * cheap here because a fresh link can always be requested from the portal, | |
| 42 | - * and it limits how long a link left sitting in an inbox stays live. | |
| 26 | + * How long a claim link stays valid. A fresh one can always be requested | |
| 27 | + * from the portal. | |
| 43 | 28 | */ |
| 44 | 29 | const TTL_SECONDS = DAY_IN_SECONDS; |
| 45 | 30 | |
| 46 | 31 | /** |
| 47 | - * Domain separator mixed into the signing key so a token minted here can | |
| 48 | - * never be replayed against another feature that signs with the same salt. | |
| 32 | + * Mixed into the signature so these tokens can't be replayed against | |
| 33 | + * another feature that signs with the same salt. | |
| 49 | 34 | */ |
| 50 | 35 | const SIGNING_CONTEXT = 'fluent_support_email_claim_v1'; |
| 51 | 36 | |
| 52 | 37 | /** |
| @@ -54,12 +39,10 @@ | ||
| 54 | 39 | */ |
| 55 | 40 | public static function isEnabled() |
| 56 | 41 | { |
| 57 | 42 | /* |
| 58 | - * Filter whether customers may reconcile their support address with | |
| 59 | - * their WordPress account address from the portal. Turning this off | |
| 60 | - * leaves a diverged record pointing at its previous address until an | |
| 61 | - * agent moves it by hand. | |
| 43 | + * Filter whether customers may move their support address to their | |
| 44 | + * WordPress account address from the portal. | |
| 62 | 45 | * |
| 63 | 46 | * @since v2.4.1 |
| 64 | 47 | * @param bool $enabled |
| 65 | 48 | */ |
| @@ -66,21 +49,16 @@ | ||
| 66 | 49 | return (bool) apply_filters('fluent_support/enable_email_claim', true); |
| 67 | 50 | } |
| 68 | 51 | |
| 69 | 52 | /** |
| 70 | - * Describe the gap between the signed-in account's address and the address | |
| 71 | - * on its customer record, or null when there is nothing to reconcile. | |
| 53 | + * Describe how the signed-in account's address differs from its customer | |
| 54 | + * record, or null when there is nothing to reconcile. | |
| 72 | 55 | * |
| 73 | - * Returning null is the common case and covers rather more than "the | |
| 74 | - * addresses match": no signed-in account, no record linked to it, a record | |
| 75 | - * linked to somebody else, or an address already held by another linked | |
| 76 | - * record. That last one is a genuine conflict between two accounts and is | |
| 77 | - * left for an agent rather than resolved by whoever asks first. | |
| 56 | + * An address already held by another linked record also returns null. That | |
| 57 | + * is a conflict between two accounts and is left for an agent. | |
| 78 | 58 | * |
| 79 | - * Pass the customer when the caller has already resolved it. The portal | |
| 80 | - * renders this on every page load, and looking the same record up twice per | |
| 81 | - * render is the one cost this check has in the common case where the two | |
| 82 | - * addresses agree. | |
| 59 | + * Pass the customer if you already have it. The portal calls this on every | |
| 60 | + * page load. | |
| 83 | 61 | * |
| 84 | 62 | * @param \FluentSupport\App\Models\Customer|null $customer |
| 85 | 63 | * @return array|null |
| 86 | 64 | */ |
| @@ -115,12 +93,10 @@ | ||
| 115 | 93 | if (self::heldByLinkedCustomer($user->user_email, $customer->id)) { |
| 116 | 94 | return null; |
| 117 | 95 | } |
| 118 | 96 | |
| 119 | - // Deliberately does not count or load the stray records holding the new | |
| 120 | - // address. Nothing on the read path needs them -- the notice reports no | |
| 121 | - // count on purpose -- and apply() re-reads them at the moment it acts, | |
| 122 | - // where a list assembled pages earlier would be stale anyway. | |
| 97 | + // Stray records on the new address aren't loaded here. apply() reads | |
| 98 | + // them when it acts. | |
| 123 | 99 | return [ |
| 124 | 100 | 'customer' => $customer, |
| 125 | 101 | 'from' => $customer->email, |
| 126 | 102 | 'to' => $user->user_email |
| @@ -137,12 +113,10 @@ | ||
| 137 | 113 | { |
| 138 | 114 | $customer = $divergence['customer']; |
| 139 | 115 | $target = $divergence['to']; |
| 140 | 116 | |
| 141 | - // Two buckets, because the two abuses are different. The per-account | |
| 142 | - // bucket stops somebody cycling their own account address to mail-bomb | |
| 143 | - // a series of victims; the per-address bucket stops the same inbox | |
| 144 | - // being targeted repeatedly from several accounts. | |
| 117 | + // Per account, to stop one account mailing many inboxes. Per address, | |
| 118 | + // to stop many accounts mailing one inbox. | |
| 145 | 119 | $accountKey = 'fs_email_claim_user_' . (int) $customer->user_id; |
| 146 | 120 | $targetKey = 'fs_email_claim_to_' . wp_hash(self::normalize($target)); |
| 147 | 121 | |
| 148 | 122 | if (Helper::hitRateLimit($accountKey, 5, HOUR_IN_SECONDS) |
| @@ -170,10 +144,9 @@ | ||
| 170 | 144 | |
| 171 | 145 | $body = $pStart . sprintf( |
| 172 | 146 | // translators: %s is the customer's first name |
| 173 | 147 | __('Hello %s,', 'fluent-support'), |
| 174 | - // WordPress does not sanitize first_name on the way in, so this | |
| 175 | - // is arbitrary text landing in an HTML mail body. | |
| 148 | + // WordPress doesn't sanitize first_name on save. | |
| 176 | 149 | esc_html($customer->first_name) |
| 177 | 150 | ) . '</p>' . |
| 178 | 151 | $pStart . sprintf( |
| 179 | 152 | // translators: 1: site name, 2: the email address being confirmed |
| @@ -205,10 +178,8 @@ | ||
| 205 | 178 | |
| 206 | 179 | $sent = wp_mail($target, $subject, $message, ['Content-Type: text/html; charset=UTF-8']); |
| 207 | 180 | |
| 208 | 181 | if (!$sent) { |
| 209 | - // Telling the customer to go and read a mail that was never accepted | |
| 210 | - // for delivery wastes their time and both rate limit allowances. | |
| 211 | 182 | return 'send_failed'; |
| 212 | 183 | } |
| 213 | 184 | |
| 214 | 185 | return ''; |
| @@ -216,21 +187,12 @@ | ||
| 216 | 187 | |
| 217 | 188 | /** |
| 218 | 189 | * Validate a confirmation token against current state. |
| 219 | 190 | * |
| 220 | - * Every field in the token is re-checked rather than trusted, because the | |
| 221 | - * world moves between issuing a link and clicking it. Binding the record's | |
| 222 | - * address at issue time is what retires a used link: applying a claim moves | |
| 223 | - * that address, so replaying the same link finds a record that no longer | |
| 224 | - * matches. | |
| 191 | + * The token binds the record's current address, so applying a claim retires | |
| 192 | + * the link. It is not strictly single-use: if the record is moved back to | |
| 193 | + * that address before expiry, the link works again. | |
| 225 | 194 | * |
| 226 | - * That is a condition, not a stored single use. Nothing is written down, so | |
| 227 | - * a link works again if the record is put back on the address it was issued | |
| 228 | - * against before the link expires -- an agent correcting a mistake, say. | |
| 229 | - * The link is still in the inbox that proved the address in the first place, | |
| 230 | - * so this reconfirms what was already confirmed; it is recorded here because | |
| 231 | - * calling it single-use would overstate what the token does. | |
| 232 | - * | |
| 233 | 195 | * @param string $token |
| 234 | 196 | * @return array ['status' => slug, 'customer' => Customer|null, 'email' => string] |
| 235 | 197 | */ |
| 236 | 198 | public static function resolveClaim($token) |
| @@ -248,11 +210,9 @@ | ||
| 248 | 210 | if ($claim['expires'] < time()) { |
| 249 | 211 | return ['status' => 'expired']; |
| 250 | 212 | } |
| 251 | 213 | |
| 252 | - // Requiring the claiming account to be signed in is the second half of | |
| 253 | - // the proof. Reading the inbox is not enough on its own, and a link | |
| 254 | - // forwarded to somebody else does nothing in their hands. | |
| 214 | + // Inbox access alone isn't enough. The claiming account must be signed in. | |
| 255 | 215 | if ((int) get_current_user_id() !== $claim['user_id']) { |
| 256 | 216 | return ['status' => 'wrong_account']; |
| 257 | 217 | } |
| 258 | 218 | |
| @@ -305,11 +265,9 @@ | ||
| 305 | 265 | |
| 306 | 266 | $customer->email = $email; |
| 307 | 267 | $customer->save(); |
| 308 | 268 | |
| 309 | - // Rotates every ticket hash and writes the activity entry. Shared with | |
| 310 | - // the profile_update paths so a proven address move has exactly one | |
| 311 | - // set of consequences however it was proven. | |
| 269 | + // Shared with the profile_update path: rotates ticket hashes and logs activity. | |
| 312 | 270 | ProfileInfoService::onProvenEmailChange($customer, $previousEmail, 'claimed'); |
| 313 | 271 | |
| 314 | 272 | return ['merged' => $merged, 'tickets' => $moved]; |
| 315 | 273 | } |
| @@ -317,13 +275,10 @@ | ||
| 317 | 275 | /** |
| 318 | 276 | * Reparent everything hanging off one customer record onto another, then |
| 319 | 277 | * remove the record it came from. |
| 320 | 278 | * |
| 321 | - * The record is only deleted once it is provably empty. Reparenting is | |
| 322 | - * several statements over tables that may well be MyISAM, where a | |
| 323 | - * transaction would silently do nothing, so emptiness is re-read from the | |
| 324 | - * database rather than assumed from the writes having been attempted. A | |
| 325 | - * record that is not empty is left in place for an agent to look at. | |
| 279 | + * The source is deleted only if a re-read shows it empty. The tables may be | |
| 280 | + * MyISAM, so a transaction can't be relied on. | |
| 326 | 281 | * |
| 327 | 282 | * @param \FluentSupport\App\Models\Customer $source |
| 328 | 283 | * @param \FluentSupport\App\Models\Customer $target |
| 329 | 284 | * @return int Number of tickets moved |
| @@ -333,12 +288,10 @@ | ||
| 333 | 288 | $tickets = Ticket::where('customer_id', $source->id)->get(); |
| 334 | 289 | |
| 335 | 290 | foreach ($tickets as $ticket) { |
| 336 | 291 | $ticket->customer_id = $target->id; |
| 337 | - // Saved one at a time on purpose: Ticket's updating event rotates | |
| 338 | - // the hash when customer_id moves, and a mass update on the query | |
| 339 | - // builder would skip it, leaving every link already mailed for | |
| 340 | - // these tickets working under their new owner. | |
| 292 | + // Saved one by one so Ticket's updating event rotates the hash. | |
| 293 | + // A query-builder update would skip it. | |
| 341 | 294 | $ticket->save(); |
| 342 | 295 | } |
| 343 | 296 | |
| 344 | 297 | Conversation::where('person_id', $source->id)->update(['person_id' => $target->id]); |
| @@ -351,16 +304,10 @@ | ||
| 351 | 304 | Activity::where('object_type', 'customer') |
| 352 | 305 | ->where('object_id', $source->id) |
| 353 | 306 | ->update(['object_id' => $target->id]); |
| 354 | 307 | |
| 355 | - // Internal notifications name the person who caused them, and a customer | |
| 356 | - // reply is one of the things that raises one. Left behind, they point at | |
| 357 | - // a person row that is about to be deleted. | |
| 358 | - // | |
| 359 | - // The tables are opt-in: NotificationSettings creates them the first | |
| 360 | - // time internal notifications are switched on, and the default is off. | |
| 361 | - // Querying them unguarded is a fatal on every site that never enabled | |
| 362 | - // the feature. | |
| 308 | + // Notification tables only exist once internal notifications have been | |
| 309 | + // enabled, so check before querying them. | |
| 363 | 310 | $hasNotifications = (new NotificationSettings())->notificationTablesExist(); |
| 364 | 311 | |
| 365 | 312 | if ($hasNotifications) { |
| 366 | 313 | Notification::where('actor_id', $source->id) |
| @@ -369,15 +316,12 @@ | ||
| 369 | 316 | |
| 370 | 317 | self::mergeMeta($source, $target); |
| 371 | 318 | |
| 372 | 319 | /* |
| 373 | - * Fires while one customer record is being folded into another, after | |
| 374 | - * core has reparented tickets, conversations, attachments, activity and | |
| 375 | - * person meta, and before the emptied record is removed. | |
| 320 | + * Fires while one customer record is merged into another, after core | |
| 321 | + * rows have moved and before the source record is removed. Move any | |
| 322 | + * rows keyed by person id here. Pro moves time tracking on this hook. | |
| 376 | 323 | * |
| 377 | - * Anything holding its own rows against a person id must move them here | |
| 378 | - * -- Fluent Support Pro moves time tracking on this hook. | |
| 379 | - * | |
| 380 | 324 | * @since v2.4.1 |
| 381 | 325 | * @param \FluentSupport\App\Models\Customer $source Record being absorbed |
| 382 | 326 | * @param \FluentSupport\App\Models\Customer $target Record it is folded into |
| 383 | 327 | */ |
| @@ -408,9 +352,14 @@ | ||
| 408 | 352 | 'object_type' => 'customer', |
| 409 | 353 | 'description' => $remaining === 0 |
| 410 | 354 | ? sprintf( |
| 411 | 355 | // translators: 1: merged customer record id, 2: that record's email address, 3: number of tickets moved |
| 412 | - __('Merged customer record #%1$s (%2$s) into this one after the address was confirmed. %3$s ticket(s) moved and their shared links were reissued.', 'fluent-support'), | |
| 356 | + _n( | |
| 357 | + 'Merged customer record #%1$s (%2$s) into this one after the address was confirmed. %3$s ticket moved and its shared link was reissued.', | |
| 358 | + 'Merged customer record #%1$s (%2$s) into this one after the address was confirmed. %3$s tickets moved and their shared links were reissued.', | |
| 359 | + $ticketsMoved, | |
| 360 | + 'fluent-support' | |
| 361 | + ), | |
| 413 | 362 | $sourceId, |
| 414 | 363 | $sourceEmail, |
| 415 | 364 | $ticketsMoved |
| 416 | 365 | ) |
| @@ -415,9 +364,14 @@ | ||
| 415 | 364 | $ticketsMoved |
| 416 | 365 | ) |
| 417 | 366 | : sprintf( |
| 418 | 367 | // translators: 1: customer record id, 2: that record's email address, 3: number of tickets moved |
| 419 | - __('Moved %3$s ticket(s) from customer record #%1$s (%2$s) into this one after the address was confirmed. That record still holds other data and was kept.', 'fluent-support'), | |
| 368 | + _n( | |
| 369 | + 'Moved %3$s ticket from customer record #%1$s (%2$s) into this one after the address was confirmed. That record still holds other data and was kept.', | |
| 370 | + 'Moved %3$s tickets from customer record #%1$s (%2$s) into this one after the address was confirmed. That record still holds other data and was kept.', | |
| 371 | + $ticketsMoved, | |
| 372 | + 'fluent-support' | |
| 373 | + ), | |
| 420 | 374 | $sourceId, |
| 421 | 375 | $sourceEmail, |
| 422 | 376 | $ticketsMoved |
| 423 | 377 | ) |
| @@ -426,16 +380,10 @@ | ||
| 426 | 380 | return $ticketsMoved; |
| 427 | 381 | } |
| 428 | 382 | |
| 429 | 383 | /** |
| 430 | - * Carry across person meta the surviving record does not already have. | |
| 384 | + * Move person meta the target doesn't already have. The target's own keys win. | |
| 431 | 385 | * |
| 432 | - * Keys the target already holds are left alone: the record being kept is | |
| 433 | - * the one the customer has been using, and a stray record assembled from an | |
| 434 | - * inbound email is not a better source of truth than it. Whatever is left | |
| 435 | - * goes with the row, rather than sitting in fs_meta pointing at a person id | |
| 436 | - * that no longer exists. | |
| 437 | - * | |
| 438 | 386 | * @param \FluentSupport\App\Models\Customer $source |
| 439 | 387 | * @param \FluentSupport\App\Models\Customer $target |
| 440 | 388 | * @return void |
| 441 | 389 | */ |
| @@ -494,10 +442,9 @@ | ||
| 494 | 442 | |
| 495 | 443 | /** |
| 496 | 444 | * URL the portal notice points at to request a confirmation email. |
| 497 | 445 | * |
| 498 | - * Nonced because following it sends mail, and a GET that has an effect is | |
| 499 | - * otherwise a link an attacker can put in front of a signed-in customer. | |
| 446 | + * Nonced because following it sends mail. | |
| 500 | 447 | * |
| 501 | 448 | * @return string |
| 502 | 449 | */ |
| 503 | 450 | public static function buildRequestUrl() |
| @@ -524,14 +471,9 @@ | ||
| 524 | 471 | * @return string |
| 525 | 472 | */ |
| 526 | 473 | public static function buildToken($customerId, $userId, $from, $to, $expires) |
| 527 | 474 | { |
| 528 | - // The addresses are percent-encoded before they are joined, because the | |
| 529 | - // separator is legal inside one: WordPress's is_email() accepts '|' in | |
| 530 | - // the local part, so a@b|[email protected] is a real address somebody can | |
| 531 | - // hold. Unencoded it splits the payload into seven fields and the exact | |
| 532 | - // field count below rejects the token, which fails closed but leaves | |
| 533 | - // that customer unable to use the flow at all. | |
| 475 | + // Encoded because is_email() accepts '|' in the local part. | |
| 534 | 476 | $payload = implode('|', [ |
| 535 | 477 | (int) $customerId, |
| 536 | 478 | (int) $userId, |
| 537 | 479 | rawurlencode(self::normalize($from)), |
| @@ -563,10 +505,8 @@ | ||
| 563 | 505 | } |
| 564 | 506 | |
| 565 | 507 | $parts = explode('|', $raw); |
| 566 | 508 | |
| 567 | - // Exact: the addresses were percent-encoded before joining, so no field | |
| 568 | - // can carry the separator. | |
| 569 | 509 | if (count($parts) !== 6) { |
| 570 | 510 | return null; |
| 571 | 511 | } |
| 572 | 512 | |
| @@ -577,10 +517,9 @@ | ||
| 577 | 517 | if (!hash_equals(self::sign($payload), $signature)) { |
| 578 | 518 | return null; |
| 579 | 519 | } |
| 580 | 520 | |
| 581 | - // Decoded only after the signature has been checked, so what is verified | |
| 582 | - // is exactly the string that was signed. | |
| 521 | + // Decode only after verifying the signature over the encoded string. | |
| 583 | 522 | return [ |
| 584 | 523 | 'customer_id' => (int) $customerId, |
| 585 | 524 | 'user_id' => (int) $userId, |
| 586 | 525 | 'from' => rawurldecode($from), |
| @@ -598,11 +537,10 @@ | ||
| 598 | 537 | return hash_hmac('sha256', self::SIGNING_CONTEXT . '|' . $payload, wp_salt('auth')); |
| 599 | 538 | } |
| 600 | 539 | |
| 601 | 540 | /** |
| 602 | - * Customer records holding this address that no WordPress account has | |
| 603 | - * claimed. These are the rows an address change stranded, and they are the | |
| 604 | - * only ones a confirmed claim is allowed to absorb. | |
| 541 | + * Records on this address with no WordPress account. Only these may be | |
| 542 | + * absorbed by a claim. | |
| 605 | 543 | * |
| 606 | 544 | * @param string $email |
| 607 | 545 | * @param int $excludeId |
| 608 | 546 | * @return \FluentSupport\Framework\Support\Collection |
| @@ -616,11 +554,9 @@ | ||
| 616 | 554 | ->get(); |
| 617 | 555 | } |
| 618 | 556 | |
| 619 | 557 | /** |
| 620 | - * Whether another WordPress account's customer record already holds this | |
| 621 | - * address. That is a conflict between two accounts, not something a | |
| 622 | - * confirmation can settle, so nothing is offered and nothing is moved. | |
| 558 | + * Whether another account's customer record already holds this address. | |
| 623 | 559 | * |
| 624 | 560 | * @param string $email |
| 625 | 561 | * @param int $excludeId |
| 626 | 562 | * @return bool |
