From 3c1b888a2ef342455131433c7be53eb8e9366503 Mon Sep 17 00:00:00 2001 From: leogermani Date: Wed, 7 Oct 2026 16:20:51 -0300 Subject: [PATCH 1/5] feat(slack): credit Slack entries to users by member ID or handle Co-Authored-By: Claude Opus 5.5 --- includes/class-slack.php | 4 +- .../slack/class-slack-author-resolver.php | 186 ++++++++++++ .../slack/class-slack-webhook-controller.php | 9 +- src/admin/DEVELOPMENT.md | 13 + tests/test-slack-author-resolver.php | 276 ++++++++++++++++++ tests/test-slack-webhook.php | 48 +++ 6 files changed, 531 insertions(+), 5 deletions(-) create mode 100644 tests/test-slack-author-resolver.php diff --git a/includes/class-slack.php b/includes/class-slack.php index 7ea3d062..58743753 100644 --- a/includes/class-slack.php +++ b/includes/class-slack.php @@ -33,10 +33,12 @@ public static function init(): void { } /** - * Register always-on admin REST routes and the bot user's avatar. + * Register always-on admin REST routes, the bot user's avatar and the + * Slack handle field on user profiles. */ public static function register_hooks(): void { add_action( 'rest_api_init', [ self::get_webhook_controller(), 'register_admin_routes' ] ); + Slack_Author_Resolver::init(); // After Simple Local Avatars, so the bundled avatar replaces its fallback but not an uploaded avatar. add_filter( 'pre_get_avatar_data', [ Slack_Config::class, 'filter_bot_avatar' ], 20, 2 ); } diff --git a/includes/slack/class-slack-author-resolver.php b/includes/slack/class-slack-author-resolver.php index 31ef3fa5..b6127a3c 100644 --- a/includes/slack/class-slack-author-resolver.php +++ b/includes/slack/class-slack-author-resolver.php @@ -11,9 +11,33 @@ /** * Resolves Slack users to WordPress authors and filters entry author display. + * + * A WordPress user can enter their Slack handle or member ID on their + * profile. Entries from Slack messages that person posts are then credited + * to them rather than to the generic Slack bot user. */ class Slack_Author_Resolver { + /** + * User meta holding the Slack handle or member ID mapped to a WordPress user. + */ + const META_SLACK_HANDLE = 'rolling_coverage_slack_handle'; + + /** + * Name of the Slack handle or member ID field on the profile form. + */ + const FIELD_SLACK_HANDLE = 'rolling_coverage_slack_handle'; + + /** + * Register the Slack handle field on the profile and user edit screens. + */ + public static function init(): void { + add_action( 'show_user_profile', [ __CLASS__, 'render_profile_section' ] ); + add_action( 'edit_user_profile', [ __CLASS__, 'render_profile_section' ] ); + add_action( 'personal_options_update', [ __CLASS__, 'save_profile_section' ] ); + add_action( 'edit_user_profile_update', [ __CLASS__, 'save_profile_section' ] ); + } + /** * Get or create the Slack bot WordPress user ID. * @@ -22,4 +46,166 @@ class Slack_Author_Resolver { public static function get_slack_bot_user_id(): int { return Slack_Config::get_or_create_bot_user_id(); } + + /** + * The WordPress user to credit with a Slack message: the user whose + * stored Slack member ID or handle matches the message's author, or the + * Slack bot user when nobody does. + * + * The member ID is tried first. It comes with the message, so it matches + * even when the author's profile could not be read, and unlike a handle + * it can't be changed or shared by someone else. Slack shows a person's + * display name as their handle, and falls back to their username when the + * display name is empty, so both are tried next, in that order. Only + * users who can write entries are credited. + * + * @param string $slack_user_id The author's Slack member ID. + * @param array|\WP_Error $user_info The author's `users.info` profile. + * @return int WordPress user ID, or 0 when the bot user is unavailable. + */ + public static function resolve_author_id( string $slack_user_id, $user_info ): int { + $candidates = [ $slack_user_id ]; + + if ( is_array( $user_info ) ) { + $candidates[] = $user_info['profile']['display_name'] ?? ''; + $candidates[] = $user_info['name'] ?? ''; + } + + foreach ( $candidates as $candidate ) { + $candidate = is_string( $candidate ) ? self::normalize_handle( $candidate ) : ''; + + if ( '' === $candidate ) { + continue; + } + + $user_ids = self::find_user_ids_by_handle( $candidate, 'edit_posts' ); + + if ( ! empty( $user_ids ) ) { + return (int) $user_ids[0]; + } + } + + return self::get_slack_bot_user_id(); + } + + /** + * A handle as stored and compared: trimmed, without the leading `@` + * people tend to type. + * + * @param string $handle Slack handle. + * @return string + */ + public static function normalize_handle( string $handle ): string { + return trim( ltrim( trim( $handle ), '@' ) ); + } + + /** + * Whether a stored value is a Slack member ID rather than a handle. + * Member IDs are uppercase letters and digits starting with `U`, or `W` + * on Enterprise Grid, as Slack's "Copy member ID" gives them. + * + * @param string $value Normalized handle or member ID. + * @return bool + */ + public static function is_member_id( string $value ): bool { + return 1 === preg_match( '/^[UW][A-Z0-9]{8,}$/', $value ); + } + + /** + * IDs of the users mapped to a Slack handle or member ID. A handle + * matches ignoring case, as the database collation does; a member ID + * matches only exactly. + * + * @param string $handle Normalized Slack handle or member ID. + * @param string $capability Capability the users must have, or '' for any user. + * @return int[] + */ + private static function find_user_ids_by_handle( string $handle, string $capability = '' ): array { + $args = [ + 'meta_key' => self::META_SLACK_HANDLE, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key + 'meta_value' => $handle, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_value + 'fields' => 'ID', + 'orderby' => 'ID', + 'order' => 'ASC', + ]; + + if ( '' !== $capability ) { + $args['capability'] = $capability; + } + + $user_ids = array_map( 'intval', get_users( $args ) ); + + if ( self::is_member_id( $handle ) ) { + $user_ids = array_values( array_filter( $user_ids, fn( $user_id ) => get_user_meta( $user_id, self::META_SLACK_HANDLE, true ) === $handle ) ); + } + + return $user_ids; + } + + /** + * Render the Rolling Coverage section on the profile and user edit screens. + * + * @param \WP_User $user The user being edited. + */ + public static function render_profile_section( $user ): void { + $handle = (string) get_user_meta( $user->ID, self::META_SLACK_HANDLE, true ); + ?> +

+ + + + + + + add( + 'rolling_coverage_slack_handle_taken', + sprintf( + /* translators: %s: Slack handle. */ + __( 'Error: The Slack handle or member ID %s is already assigned to another user.', 'newspack-rolling-coverage' ), + '' . esc_html( $handle ) . '' + ) + ); + } + ); + return; + } + + update_user_meta( $user_id, self::META_SLACK_HANDLE, $handle ); + } } diff --git a/includes/slack/class-slack-webhook-controller.php b/includes/slack/class-slack-webhook-controller.php index 523261f7..49f386e6 100644 --- a/includes/slack/class-slack-webhook-controller.php +++ b/includes/slack/class-slack-webhook-controller.php @@ -1552,8 +1552,9 @@ static function ( string $mentioned_user_id ) use ( $api_client, $can_call_api, // 2. Content processing. $content = $content_processor->process( $text, is_array( $event['blocks'] ?? null ) ? $event['blocks'] : [] ); - // 3. Bot user resolution. - $bot_user_id = Slack_Author_Resolver::get_slack_bot_user_id(); + // 3. The entry's author: the WordPress user mapped to the message + // author's Slack member ID or handle, or the bot user. + $author_id = Slack_Author_Resolver::resolve_author_id( $user_id, $user_info ); // 4. Build the normalized payload. $source_payload = new Source_Event_Payload( @@ -1580,13 +1581,13 @@ static function ( string $mentioned_user_id ) use ( $api_client, $can_call_api, // 6. Call the generic ingestion service. Uploaded images are imported // from inside it, once the message is known not to be a redelivery. $files = is_array( $event['files'] ?? null ) ? $event['files'] : []; - $media_importer = new Slack_Media_Importer( $api_client, $bot_user_id ); + $media_importer = new Slack_Media_Importer( $api_client, $author_id ); $post_id = Entry_Ingestion_Service::ingest( $source_payload, $term_id, $auto_publish, - $bot_user_id, + $author_id, $provenance_meta, static fn( callable $keep_lock ): string => $media_importer->import( $files, $keep_lock ) ); diff --git a/src/admin/DEVELOPMENT.md b/src/admin/DEVELOPMENT.md index 6231c9da..8dda217d 100644 --- a/src/admin/DEVELOPMENT.md +++ b/src/admin/DEVELOPMENT.md @@ -180,6 +180,19 @@ The entries list has no Source column. The source shows as a marker before the t The `source` field stays defined, with no `render` and `enableHiding: false`. DataViews builds filters from every field, whether or not it's shown, so Source is offered under **Add filter** but never as a column. +## Slack authors + +An entry ingested from Slack is credited to the WordPress user whose Slack member ID or handle matches the message's author, and to the Slack bot user (`Slack_Config::get_or_create_bot_user_id()`) when nobody's does. The value is set in the **Rolling Coverage** section of the profile and user edit screens, in its **Slack handle** field, and stored in the `rolling_coverage_slack_handle` user meta (`Slack_Author_Resolver::META_SLACK_HANDLE`). The field's description says how to find a member ID in Slack (Profile, then the More menu, then **Copy member ID**). + +- A value that is uppercase letters and digits starting with `U` or `W` (`Slack_Author_Resolver::is_member_id()`) is a member ID; anything else is a handle. Values are stored trimmed and without a leading `@`. Saving a value another user already has is refused with an error on the profile screen, so a message never has two people it could be credited to. +- At ingest, `Slack_Author_Resolver::resolve_author_id()` tries the author's member ID, which comes with the message, then their Slack display name, then their Slack username. A member ID matches only exactly; a handle matches ignoring case, as the database collation does. Only users who can `edit_posts` are credited. +- Handles need Slack's `users.info` lookup (1s budget on the webhook); when it fails, only a member ID can match, and otherwise the entry goes to the bot user. +- Images uploaded with the message are owned by the same user as the entry. + +Slack display names are not unique, and anyone in the workspace can change theirs to match someone's handle, so a handle tells who posted only as far as the channel's members can be trusted to use their own names. A member ID can't be taken this way, which is why it is tried first and why the field recommends it. + +Tests: `tests/test-slack-author-resolver.php`, and the end-to-end cases in `tests/test-slack-webhook.php`. + ## Adding a source The taxonomy already anticipates other chat platforms (Beeper, WhatsApp, Telegram). A new source needs each of these: diff --git a/tests/test-slack-author-resolver.php b/tests/test-slack-author-resolver.php new file mode 100644 index 00000000..e9bb0944 --- /dev/null +++ b/tests/test-slack-author-resolver.php @@ -0,0 +1,276 @@ + $name, + 'profile' => [ 'display_name' => $display_name ], + ]; + } + + /** + * Create an author mapped to a Slack handle. + * + * @param string $handle Slack handle. + * @param string $role Role. + * @return int User ID. + */ + private static function mapped_user( string $handle, string $role = 'author' ): int { + $user_id = self::factory()->user->create( [ 'role' => $role ] ); + update_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, $handle ); + return $user_id; + } + + /** + * Submit the profile form for a user, as someone allowed to edit them. + * + * @param int $user_id User being saved. + * @param string $handle Submitted handle. + */ + private static function save_profile( int $user_id, string $handle ): void { + $_POST[ Slack_Author_Resolver::FIELD_SLACK_HANDLE ] = wp_slash( $handle ); + $_REQUEST['_wpnonce'] = wp_create_nonce( 'update-user_' . $user_id ); + Slack_Author_Resolver::save_profile_section( $user_id ); + } + + /** + * A message from a mapped display name is credited to that user. + */ + public function test_credits_the_user_with_the_matching_display_name() { + $user_id = self::mapped_user( 'Riley Sample' ); + + $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + } + + /** + * Case and a typed `@` don't stop a match. + */ + public function test_match_ignores_case_and_the_at_sign() { + $user_id = self::mapped_user( 'riley.sample' ); + + $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( '@Riley.Sample' ) ) ); + } + + /** + * With no display name, Slack shows the username as the handle. + */ + public function test_falls_back_to_the_slack_username() { + $user_id = self::mapped_user( 'rsample' ); + + $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( '', 'rsample' ) ) ); + } + + /** + * The display name wins over the username when both are mapped. + */ + public function test_display_name_wins_over_the_username() { + self::mapped_user( 'rsample' ); + $by_display_name = self::mapped_user( 'Riley Sample' ); + + $this->assertSame( $by_display_name, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample', 'rsample' ) ) ); + } + + /** + * A message from a mapped member ID is credited to that user, even when + * Slack can't say who the author is. + */ + public function test_credits_the_user_with_the_matching_member_id() { + $user_id = self::mapped_user( self::MEMBER_ID ); + + $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, new WP_Error( 'http_request_failed' ) ) ); + } + + /** + * The member ID wins over a handle, which anyone can take. + */ + public function test_member_id_wins_over_a_handle() { + self::mapped_user( 'Riley Sample' ); + $by_member_id = self::mapped_user( self::MEMBER_ID ); + + $this->assertSame( $by_member_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + } + + /** + * A handle that differs from a member ID only in case is a handle, and + * doesn't claim that member's messages. + */ + public function test_member_id_matches_only_exactly() { + self::mapped_user( strtolower( self::MEMBER_ID ) ); + + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + } + + /** + * Member IDs are told from handles by their prefix and form. + * + * @dataProvider data_member_ids + * + * @param string $value Stored value. + * @param bool $is_member Whether it is a member ID. + */ + public function test_tells_member_ids_from_handles( string $value, bool $is_member ) { + $this->assertSame( $is_member, Slack_Author_Resolver::is_member_id( $value ) ); + } + + /** + * Values and whether each is a member ID. + * + * @return array + */ + public function data_member_ids() { + return [ + 'user ID' => [ 'U012AB3CDE', true ], + 'Enterprise Grid ID' => [ 'W012AB3CDE', true ], + 'lowercase' => [ 'u012ab3cde', false ], + 'other prefix' => [ 'C012AB3CDE', false ], + 'too short' => [ 'U012', false ], + 'handle' => [ 'Umberto', false ], + ]; + } + + /** + * Nobody mapped means the bot user. + */ + public function test_unmapped_handle_goes_to_the_bot_user() { + self::mapped_user( 'Someone Else' ); + + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + } + + /** + * A failed Slack lookup means the bot user. + */ + public function test_failed_lookup_goes_to_the_bot_user() { + self::mapped_user( 'Riley Sample' ); + + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, new WP_Error( 'http_request_failed' ) ) ); + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, [] ) ); + } + + /** + * Someone who can't write entries is never credited with one. + */ + public function test_user_who_cannot_write_entries_is_not_credited() { + self::mapped_user( 'Riley Sample', 'subscriber' ); + + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + } + + /** + * The profile form stores the handle without a typed `@`. + */ + public function test_profile_saves_the_handle() { + $user_id = self::log_in_as( 'author' ); + + self::save_profile( $user_id, ' @Riley Sample ' ); + + $this->assertSame( 'Riley Sample', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ) ); + } + + /** + * Emptying the field removes the mapping. + */ + public function test_profile_clears_the_handle() { + $user_id = self::log_in_as( 'author' ); + update_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, 'Riley Sample' ); + + self::save_profile( $user_id, '' ); + + $this->assertSame( '', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ) ); + } + + /** + * A handle another user has is refused, with an error on the profile. + */ + public function test_profile_refuses_a_handle_another_user_has() { + $owner = self::mapped_user( 'Riley Sample' ); + $user_id = self::log_in_as( 'author' ); + + self::save_profile( $user_id, 'riley sample' ); + + $errors = new WP_Error(); + do_action_ref_array( 'user_profile_update_errors', [ &$errors, true, get_userdata( $user_id ) ] ); + + $this->assertSame( '', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ), 'The handle should not be saved.' ); + $this->assertContains( 'rolling_coverage_slack_handle_taken', $errors->get_error_codes(), 'The profile should show an error.' ); + $this->assertSame( 'Riley Sample', get_user_meta( $owner, Slack_Author_Resolver::META_SLACK_HANDLE, true ), 'The other user should keep the handle.' ); + } + + /** + * Saving a profile again with its own handle is not a conflict. + */ + public function test_profile_keeps_its_own_handle() { + $user_id = self::log_in_as( 'author' ); + update_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, 'Riley Sample' ); + + self::save_profile( $user_id, 'Riley Sample' ); + + $errors = new WP_Error(); + do_action_ref_array( 'user_profile_update_errors', [ &$errors, true, get_userdata( $user_id ) ] ); + + $this->assertFalse( $errors->has_errors() ); + $this->assertSame( 'Riley Sample', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ) ); + } + + /** + * Someone who can't edit the user can't set their handle. + */ + public function test_profile_ignores_users_who_cannot_edit_the_user() { + $other = self::factory()->user->create( [ 'role' => 'author' ] ); + self::log_in_as( 'author' ); + + self::save_profile( $other, 'Riley Sample' ); + + $this->assertSame( '', get_user_meta( $other, Slack_Author_Resolver::META_SLACK_HANDLE, true ) ); + } + + /** + * The profile screens show the section with the stored handle. + */ + public function test_profile_shows_the_section() { + $user_id = self::mapped_user( 'Riley Sample' ); + + ob_start(); + Slack_Author_Resolver::render_profile_section( get_userdata( $user_id ) ); + $html = ob_get_clean(); + + $this->assertStringContainsString( '

Rolling Coverage

', $html ); + $this->assertStringContainsString( 'Slack handle', $html ); + $this->assertStringContainsString( 'value="Riley Sample"', $html ); + $this->assertStringContainsString( 'Copy member ID', $html, 'The section should say how to find a member ID.' ); + } +} diff --git a/tests/test-slack-webhook.php b/tests/test-slack-webhook.php index 019a0b61..5e7cf628 100644 --- a/tests/test-slack-webhook.php +++ b/tests/test-slack-webhook.php @@ -10,6 +10,7 @@ use Newspack_Rolling_Coverage\Post_Type; use Newspack_Rolling_Coverage\Slack; use Newspack_Rolling_Coverage\Slack_API_Client; +use Newspack_Rolling_Coverage\Slack_Author_Resolver; use Newspack_Rolling_Coverage\Slack_Config; use Newspack_Rolling_Coverage\Slack_Ingestion_Service; use Newspack_Rolling_Coverage\Slack_Media_Importer; @@ -656,6 +657,53 @@ public function test_message_in_a_linked_channel_becomes_a_draft_entry() { $this->assertSame( '1767225600.000100', Slack_Config::get_channel_settings( self::CHANNEL_ID )['last_sync_ts'], 'The channel should remember the last message it ingested.' ); } + /** + * A message from a Slack handle mapped to a WordPress user is credited + * to that user, images included, instead of the bot user. + */ + public function test_message_from_a_mapped_handle_is_credited_to_that_user() { + self::configure_slack(); + $coverage_id = self::create_coverage(); + Slack_Config::update_channel( self::CHANNEL_ID, [ 'term_id' => $coverage_id ] ); + $author_id = self::factory()->user->create( [ 'role' => 'author' ] ); + update_user_meta( $author_id, Slack_Author_Resolver::META_SLACK_HANDLE, 'Riley Sample' ); + + self::controller()->handle_event( self::webhook_request( self::message_event_body( [ 'files' => [ self::slack_file() ] ] ) ) ); + $entries = self::get_coverage_entries( $coverage_id ); + + $this->assertCount( 1, $entries, 'The message should create one entry.' ); + $this->assertSame( $author_id, (int) $entries[0]->post_author, 'The mapped user should own the entry.' ); + + $images = get_posts( + [ + 'post_type' => 'attachment', + 'post_parent' => $entries[0]->ID, + 'post_status' => 'inherit', + ] + ); + + $this->assertNotEmpty( $images, 'The image should be attached to the entry.' ); + $this->assertSame( $author_id, (int) $images[0]->post_author, 'The mapped user should own the image.' ); + } + + /** + * A message from a member ID mapped to a WordPress user is credited to + * that user. + */ + public function test_message_from_a_mapped_member_id_is_credited_to_that_user() { + self::configure_slack(); + $coverage_id = self::create_coverage(); + Slack_Config::update_channel( self::CHANNEL_ID, [ 'term_id' => $coverage_id ] ); + $author_id = self::factory()->user->create( [ 'role' => 'author' ] ); + update_user_meta( $author_id, Slack_Author_Resolver::META_SLACK_HANDLE, 'U0REPORTER' ); + + self::controller()->handle_event( self::webhook_request( self::message_event_body() ) ); + $entries = self::get_coverage_entries( $coverage_id ); + + $this->assertCount( 1, $entries, 'The message should create one entry.' ); + $this->assertSame( $author_id, (int) $entries[0]->post_author, 'The mapped user should own the entry.' ); + } + /** * A formatted message keeps its formatting in the entry, mentions show * the person's name, and the entry has no generated title. From 25fa0eac6e430ba69aadebc97486fd94f0a8537d Mon Sep 17 00:00:00 2001 From: leogermani Date: Wed, 7 Oct 2026 16:35:33 -0300 Subject: [PATCH 2/5] fix(slack): stop Slack names from matching member-ID mappings Co-Authored-By: Claude Opus 5.5 --- .../slack/class-slack-author-resolver.php | 153 +++++++++++------- .../slack/class-slack-webhook-controller.php | 4 +- .../sources/class-entry-ingestion-service.php | 8 +- src/admin/DEVELOPMENT.md | 16 +- tests/test-slack-author-resolver.php | 105 ++++++++++-- 5 files changed, 199 insertions(+), 87 deletions(-) diff --git a/includes/slack/class-slack-author-resolver.php b/includes/slack/class-slack-author-resolver.php index b6127a3c..55f8e9e1 100644 --- a/includes/slack/class-slack-author-resolver.php +++ b/includes/slack/class-slack-author-resolver.php @@ -1,6 +1,6 @@ $slack_user_id ]; if ( is_array( $user_info ) ) { - $candidates[] = $user_info['profile']['display_name'] ?? ''; - $candidates[] = $user_info['name'] ?? ''; + $candidates['display_name'] = $user_info['profile']['display_name'] ?? ''; + $candidates['real_name'] = $user_info['profile']['real_name'] ?? ''; + $candidates['name'] = $user_info['name'] ?? ''; } - foreach ( $candidates as $candidate ) { + foreach ( $candidates as $matched_by => $candidate ) { $candidate = is_string( $candidate ) ? self::normalize_handle( $candidate ) : ''; if ( '' === $candidate ) { continue; } - $user_ids = self::find_user_ids_by_handle( $candidate, 'edit_posts' ); + $user_ids = array_filter( + self::find_user_ids( $candidate, 'member_id' === $matched_by ), + fn( $user_id ) => user_can( $user_id, 'edit_posts' ) + ); if ( ! empty( $user_ids ) ) { - return (int) $user_ids[0]; + return [ + 'user_id' => (int) reset( $user_ids ), + 'matched_by' => $matched_by, + ]; } } - return self::get_slack_bot_user_id(); + return [ + 'user_id' => self::get_slack_bot_user_id(), + 'matched_by' => 'bot', + ]; } /** @@ -100,64 +117,73 @@ public static function normalize_handle( string $handle ): string { } /** - * Whether a stored value is a Slack member ID rather than a handle. - * Member IDs are uppercase letters and digits starting with `U`, or `W` - * on Enterprise Grid, as Slack's "Copy member ID" gives them. + * Whether a stored value is a Slack member ID rather than a handle: + * uppercase letters and at least one digit, starting with `U`, or `W` on + * Enterprise Grid, as Slack's "Copy member ID" gives them. The digit keeps + * an all-caps handle such as `WALTERWHITE` a handle. * * @param string $value Normalized handle or member ID. * @return bool */ public static function is_member_id( string $value ): bool { - return 1 === preg_match( '/^[UW][A-Z0-9]{8,}$/', $value ); + return 1 === preg_match( '/^[UW](?=[A-Z]*[0-9])[A-Z0-9]{8,}$/', $value ); } /** - * IDs of the users mapped to a Slack handle or member ID. A handle - * matches ignoring case, as the database collation does; a member ID - * matches only exactly. + * IDs of the users whose stored value matches, lowest first. A member ID + * matches stored member IDs exactly; a handle matches stored handles, + * ignoring case as the database collation does. * - * @param string $handle Normalized Slack handle or member ID. - * @param string $capability Capability the users must have, or '' for any user. + * @param string $value Normalized handle or member ID. + * @param bool $member_id Whether `$value` is a member ID. * @return int[] */ - private static function find_user_ids_by_handle( string $handle, string $capability = '' ): array { - $args = [ - 'meta_key' => self::META_SLACK_HANDLE, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key - 'meta_value' => $handle, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_value - 'fields' => 'ID', - 'orderby' => 'ID', - 'order' => 'ASC', - ]; - - if ( '' !== $capability ) { - $args['capability'] = $capability; - } - - $user_ids = array_map( 'intval', get_users( $args ) ); - - if ( self::is_member_id( $handle ) ) { - $user_ids = array_values( array_filter( $user_ids, fn( $user_id ) => get_user_meta( $user_id, self::META_SLACK_HANDLE, true ) === $handle ) ); - } - - return $user_ids; + private static function find_user_ids( string $value, bool $member_id ): array { + $user_ids = get_users( + [ + 'meta_key' => self::META_SLACK_HANDLE, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key + 'meta_value' => $value, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_value + 'fields' => 'ID', + 'orderby' => 'ID', + 'order' => 'ASC', + ] + ); + + return array_values( + array_filter( + array_map( 'intval', $user_ids ), + static function ( int $user_id ) use ( $value, $member_id ): bool { + $stored = (string) get_user_meta( $user_id, self::META_SLACK_HANDLE, true ); + return $member_id ? $stored === $value : ! self::is_member_id( $stored ); + } + ) + ); } /** - * Render the Rolling Coverage section on the profile and user edit screens. + * Render the Rolling Coverage section on the profile and user edit + * screens, for users who can write entries and so can be credited. * * @param \WP_User $user The user being edited. */ - public static function render_profile_section( $user ): void { - $handle = (string) get_user_meta( $user->ID, self::META_SLACK_HANDLE, true ); + public static function render_profile_section( \WP_User $user ): void { + if ( ! user_can( $user, 'edit_posts' ) ) { + return; + } + + $handle = (string) get_user_meta( $user->ID, self::META_SLACK_HANDLE, true ); + $description_id = self::FIELD_SLACK_HANDLE . '_description'; ?>

@@ -166,13 +192,13 @@ public static function render_profile_section( $user ): void { /** * Save the Slack handle or member ID from the profile and user edit - * screens. A value already mapped to another user is refused, so a Slack - * message never has two people it could be credited to. + * screens. A value already mapped to another user who can be credited is + * refused, so a Slack message never has two people it could go to. * * @param int $user_id ID of the user being saved. */ - public static function save_profile_section( $user_id ): void { - if ( ! current_user_can( 'edit_user', $user_id ) ) { + public static function save_profile_section( int $user_id ): void { + if ( ! current_user_can( 'edit_user', $user_id ) || ! user_can( $user_id, 'edit_posts' ) ) { return; } @@ -189,14 +215,19 @@ public static function save_profile_section( $user_id ): void { return; } - if ( ! empty( array_diff( self::find_user_ids_by_handle( $handle ), [ (int) $user_id ] ) ) ) { + $holders = array_filter( + self::find_user_ids( $handle, self::is_member_id( $handle ) ), + fn( $holder_id ) => $holder_id !== $user_id && user_can( $holder_id, 'edit_posts' ) + ); + + if ( ! empty( $holders ) ) { add_action( 'user_profile_update_errors', static function ( \WP_Error $errors ) use ( $handle ) { $errors->add( 'rolling_coverage_slack_handle_taken', sprintf( - /* translators: %s: Slack handle. */ + /* translators: %s: Slack handle or member ID. */ __( 'Error: The Slack handle or member ID %s is already assigned to another user.', 'newspack-rolling-coverage' ), '' . esc_html( $handle ) . '' ) diff --git a/includes/slack/class-slack-webhook-controller.php b/includes/slack/class-slack-webhook-controller.php index 49f386e6..c2691e77 100644 --- a/includes/slack/class-slack-webhook-controller.php +++ b/includes/slack/class-slack-webhook-controller.php @@ -1554,7 +1554,8 @@ static function ( string $mentioned_user_id ) use ( $api_client, $can_call_api, // 3. The entry's author: the WordPress user mapped to the message // author's Slack member ID or handle, or the bot user. - $author_id = Slack_Author_Resolver::resolve_author_id( $user_id, $user_info ); + $author = Slack_Author_Resolver::resolve_author( $user_id, $user_info ); + $author_id = $author['user_id']; // 4. Build the normalized payload. $source_payload = new Source_Event_Payload( @@ -1629,6 +1630,7 @@ static function ( string $mentioned_user_id ) use ( $api_client, $can_call_api, 'post_id' => (int) $post_id, 'channel' => $channel_id, 'status' => $auto_publish ? 'publish' : 'draft', + 'author' => $author['matched_by'], ] ); } diff --git a/includes/sources/class-entry-ingestion-service.php b/includes/sources/class-entry-ingestion-service.php index 3a88a710..0448dbcc 100644 --- a/includes/sources/class-entry-ingestion-service.php +++ b/includes/sources/class-entry-ingestion-service.php @@ -45,7 +45,7 @@ class Entry_Ingestion_Service { * @param Source_Event_Payload $payload Normalized event. * @param int $term_id Resolved rolling coverage term id. * @param bool $auto_publish Whether to insert as 'publish' or 'draft'. - * @param int $bot_user_id WP user id to assign as post_author. + * @param int $author_id WP user id to assign as post_author. * @param array $provenance_meta Platform-specific meta keyed by meta_key. * @param callable|null $render_media Returns block markup for the event's * media, added after the content. Only @@ -64,7 +64,7 @@ public static function ingest( Source_Event_Payload $payload, int $term_id, bool $auto_publish, - int $bot_user_id, + int $author_id, array $provenance_meta, ?callable $render_media = null ) { @@ -93,7 +93,7 @@ public static function ingest( return 0; } - if ( $bot_user_id <= 0 ) { + if ( $author_id <= 0 ) { // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log error_log( 'Source ingestion: bot user unavailable, skipping entry.' ); return 0; @@ -121,7 +121,7 @@ public static function ingest( 'post_type' => Post_Type::CPT_SLUG, 'post_title' => '', 'post_content' => wp_slash( $content ), - 'post_author' => $bot_user_id, + 'post_author' => $author_id, 'post_status' => $auto_publish ? 'publish' : 'draft', ]; diff --git a/src/admin/DEVELOPMENT.md b/src/admin/DEVELOPMENT.md index 8dda217d..7aab8d86 100644 --- a/src/admin/DEVELOPMENT.md +++ b/src/admin/DEVELOPMENT.md @@ -182,14 +182,20 @@ The `source` field stays defined, with no `render` and `enableHiding: false`. Da ## Slack authors -An entry ingested from Slack is credited to the WordPress user whose Slack member ID or handle matches the message's author, and to the Slack bot user (`Slack_Config::get_or_create_bot_user_id()`) when nobody's does. The value is set in the **Rolling Coverage** section of the profile and user edit screens, in its **Slack handle** field, and stored in the `rolling_coverage_slack_handle` user meta (`Slack_Author_Resolver::META_SLACK_HANDLE`). The field's description says how to find a member ID in Slack (Profile, then the More menu, then **Copy member ID**). +An entry ingested from Slack is credited to the WordPress user whose Slack member ID or handle matches the message's author, and to the Slack bot user (`Slack_Config::get_or_create_bot_user_id()`) when nobody's does. The value is set in the **Slack handle** field of the **Rolling Coverage** section on the profile and user edit screens, and stored in the `rolling_coverage_slack_handle` user meta (`Slack_Author_Resolver::META_SLACK_HANDLE`). The section shows only for users who can `edit_posts`, since nobody else can be credited. -- A value that is uppercase letters and digits starting with `U` or `W` (`Slack_Author_Resolver::is_member_id()`) is a member ID; anything else is a handle. Values are stored trimmed and without a leading `@`. Saving a value another user already has is refused with an error on the profile screen, so a message never has two people it could be credited to. -- At ingest, `Slack_Author_Resolver::resolve_author_id()` tries the author's member ID, which comes with the message, then their Slack display name, then their Slack username. A member ID matches only exactly; a handle matches ignoring case, as the database collation does. Only users who can `edit_posts` are credited. -- Handles need Slack's `users.info` lookup (1s budget on the webhook); when it fails, only a member ID can match, and otherwise the entry goes to the bot user. +- A value that starts with `U` or `W` and is uppercase letters with at least one digit (`Slack_Author_Resolver::is_member_id()`) is a member ID; anything else is a handle. Values are stored trimmed and without a leading `@`. +- Saving a value that another user who can `edit_posts` already has is refused with an error on the profile screen, so a message never has two people it could be credited to. +- At ingest, `Slack_Author_Resolver::resolve_author()` compares the message's member ID with stored member IDs, exactly. It then compares the author's Slack display name, full name and username, in that order, with stored handles only, ignoring case as the database collation does. Names are free text their owner can change, so a name shaped like a member ID never reaches a member-ID mapping. Only users who can `edit_posts` are credited. +- Names need Slack's `users.info` lookup (1s budget on the webhook). When it fails, only a member ID can match. - Images uploaded with the message are owned by the same user as the entry. +- The ingest log's `author` field records what matched (`member_id`, `display_name`, `real_name`, `name` or `bot`), so entries credited through a name can be found if one turns out to be wrong. -Slack display names are not unique, and anyone in the workspace can change theirs to match someone's handle, so a handle tells who posted only as far as the channel's members can be trusted to use their own names. A member ID can't be taken this way, which is why it is tried first and why the field recommends it. +Trust model: + +- Anyone in the Slack workspace can change their name to match someone's handle. A handle therefore tells who posted only as far as the channel's members can be trusted to use their own names. Slack never lets anyone change a member ID, which is why the field recommends one. +- The field is self-service: anyone who can `edit_posts` can enter any value on their own profile, including a colleague's member ID, and nothing checks it against Slack. A value is first come, first served, and an administrator settles a dispute by clearing it on the other profile. +- On multisite, the meta is shared across the network, but the duplicate check only sees users of the current site. Tests: `tests/test-slack-author-resolver.php`, and the end-to-end cases in `tests/test-slack-webhook.php`. diff --git a/tests/test-slack-author-resolver.php b/tests/test-slack-author-resolver.php index e9bb0944..4209c80a 100644 --- a/tests/test-slack-author-resolver.php +++ b/tests/test-slack-author-resolver.php @@ -33,15 +33,29 @@ public function tear_down() { * * @param string $display_name Display name. * @param string $name Username. + * @param string $real_name Full name. * @return array */ - private static function slack_user( string $display_name, string $name = 'someone' ): array { + private static function slack_user( string $display_name, string $name = 'someone', string $real_name = '' ): array { return [ 'name' => $name, - 'profile' => [ 'display_name' => $display_name ], + 'profile' => [ + 'display_name' => $display_name, + 'real_name' => $real_name, + ], ]; } + /** + * The user credited with a message from the test member ID. + * + * @param array|WP_Error $user_info The author's `users.info` profile. + * @return int User ID. + */ + private static function resolve( $user_info ): int { + return Slack_Author_Resolver::resolve_author( self::MEMBER_ID, $user_info )['user_id']; + } + /** * Create an author mapped to a Slack handle. * @@ -73,7 +87,7 @@ private static function save_profile( int $user_id, string $handle ): void { public function test_credits_the_user_with_the_matching_display_name() { $user_id = self::mapped_user( 'Riley Sample' ); - $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + $this->assertSame( $user_id, self::resolve( self::slack_user( 'Riley Sample' ) ) ); } /** @@ -82,16 +96,19 @@ public function test_credits_the_user_with_the_matching_display_name() { public function test_match_ignores_case_and_the_at_sign() { $user_id = self::mapped_user( 'riley.sample' ); - $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( '@Riley.Sample' ) ) ); + $this->assertSame( $user_id, self::resolve( self::slack_user( '@Riley.Sample' ) ) ); } /** - * With no display name, Slack shows the username as the handle. + * With no display name, Slack shows the full name, so it is tried next, + * and the legacy username after it. */ - public function test_falls_back_to_the_slack_username() { - $user_id = self::mapped_user( 'rsample' ); + public function test_falls_back_to_the_full_name_then_the_username() { + $by_username = self::mapped_user( 'rsample' ); + $by_full_name = self::mapped_user( 'Riley Sample' ); - $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( '', 'rsample' ) ) ); + $this->assertSame( $by_full_name, self::resolve( self::slack_user( '', 'rsample', 'Riley Sample' ) ), 'The full name should win over the username.' ); + $this->assertSame( $by_username, self::resolve( self::slack_user( '', 'rsample' ) ), 'The username should match when nothing else does.' ); } /** @@ -101,7 +118,7 @@ public function test_display_name_wins_over_the_username() { self::mapped_user( 'rsample' ); $by_display_name = self::mapped_user( 'Riley Sample' ); - $this->assertSame( $by_display_name, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample', 'rsample' ) ) ); + $this->assertSame( $by_display_name, self::resolve( self::slack_user( 'Riley Sample', 'rsample' ) ) ); } /** @@ -111,7 +128,7 @@ public function test_display_name_wins_over_the_username() { public function test_credits_the_user_with_the_matching_member_id() { $user_id = self::mapped_user( self::MEMBER_ID ); - $this->assertSame( $user_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, new WP_Error( 'http_request_failed' ) ) ); + $this->assertSame( $user_id, self::resolve( new WP_Error( 'http_request_failed' ) ) ); } /** @@ -121,7 +138,34 @@ public function test_member_id_wins_over_a_handle() { self::mapped_user( 'Riley Sample' ); $by_member_id = self::mapped_user( self::MEMBER_ID ); - $this->assertSame( $by_member_id, Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + $this->assertSame( $by_member_id, self::resolve( self::slack_user( 'Riley Sample' ) ) ); + } + + /** + * A Slack name is free text, so one set to a colleague's member ID must + * not credit the message to that colleague. + * + * @dataProvider data_names_shaped_like_a_member_id + * + * @param array $user_info The impersonator's `users.info` profile. + */ + public function test_name_shaped_like_a_member_id_does_not_match_it( array $user_info ) { + self::mapped_user( 'U0COLLEAGUE1' ); + + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), self::resolve( $user_info ) ); + } + + /** + * Profiles whose names copy a colleague's member ID. + * + * @return array + */ + public function data_names_shaped_like_a_member_id() { + return [ + 'display name' => [ self::slack_user( 'U0COLLEAGUE1' ) ], + 'full name' => [ self::slack_user( '', 'someone', 'U0COLLEAGUE1' ) ], + 'username' => [ self::slack_user( '', 'U0COLLEAGUE1' ) ], + ]; } /** @@ -131,7 +175,7 @@ public function test_member_id_wins_over_a_handle() { public function test_member_id_matches_only_exactly() { self::mapped_user( strtolower( self::MEMBER_ID ) ); - $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), self::resolve( self::slack_user( 'Riley Sample' ) ) ); } /** @@ -158,6 +202,7 @@ public function data_member_ids() { 'lowercase' => [ 'u012ab3cde', false ], 'other prefix' => [ 'C012AB3CDE', false ], 'too short' => [ 'U012', false ], + 'no digits' => [ 'WALTERWHITE', false ], 'handle' => [ 'Umberto', false ], ]; } @@ -168,7 +213,7 @@ public function data_member_ids() { public function test_unmapped_handle_goes_to_the_bot_user() { self::mapped_user( 'Someone Else' ); - $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), self::resolve( self::slack_user( 'Riley Sample' ) ) ); } /** @@ -177,8 +222,8 @@ public function test_unmapped_handle_goes_to_the_bot_user() { public function test_failed_lookup_goes_to_the_bot_user() { self::mapped_user( 'Riley Sample' ); - $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, new WP_Error( 'http_request_failed' ) ) ); - $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, [] ) ); + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), self::resolve( new WP_Error( 'http_request_failed' ) ) ); + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), self::resolve( [] ) ); } /** @@ -187,7 +232,7 @@ public function test_failed_lookup_goes_to_the_bot_user() { public function test_user_who_cannot_write_entries_is_not_credited() { self::mapped_user( 'Riley Sample', 'subscriber' ); - $this->assertSame( Slack_Config::get_or_create_bot_user_id(), Slack_Author_Resolver::resolve_author_id( self::MEMBER_ID, self::slack_user( 'Riley Sample' ) ) ); + $this->assertSame( Slack_Config::get_or_create_bot_user_id(), self::resolve( self::slack_user( 'Riley Sample' ) ) ); } /** @@ -230,6 +275,18 @@ public function test_profile_refuses_a_handle_another_user_has() { $this->assertSame( 'Riley Sample', get_user_meta( $owner, Slack_Author_Resolver::META_SLACK_HANDLE, true ), 'The other user should keep the handle.' ); } + /** + * A value held by someone who can't be credited doesn't block anyone. + */ + public function test_profile_ignores_a_value_held_by_someone_who_cannot_be_credited() { + self::mapped_user( 'Riley Sample', 'subscriber' ); + $user_id = self::log_in_as( 'author' ); + + self::save_profile( $user_id, 'Riley Sample' ); + + $this->assertSame( 'Riley Sample', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ) ); + } + /** * Saving a profile again with its own handle is not a conflict. */ @@ -273,4 +330,20 @@ public function test_profile_shows_the_section() { $this->assertStringContainsString( 'value="Riley Sample"', $html ); $this->assertStringContainsString( 'Copy member ID', $html, 'The section should say how to find a member ID.' ); } + + /** + * Users who can't write entries can't be credited, so they don't get the + * section, and a submitted value is ignored. + */ + public function test_profile_leaves_out_users_who_cannot_write_entries() { + $user_id = self::log_in_as( 'subscriber' ); + + ob_start(); + Slack_Author_Resolver::render_profile_section( get_userdata( $user_id ) ); + $html = ob_get_clean(); + self::save_profile( $user_id, 'Riley Sample' ); + + $this->assertSame( '', $html, 'The section should not show.' ); + $this->assertSame( '', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ), 'The value should not be saved.' ); + } } From 60f11a587494e8b5d732e8416089de8d1e2fce44 Mon Sep 17 00:00:00 2001 From: leogermani Date: Wed, 7 Oct 2026 16:56:22 -0300 Subject: [PATCH 3/5] fix(slack): verify the profile nonce only when the Slack field is sent Co-Authored-By: Claude Opus 5.5 --- .../slack/class-slack-author-resolver.php | 11 +++--- src/admin/DEVELOPMENT.md | 3 +- tests/test-slack-author-resolver.php | 35 +++++++++++++++++-- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/includes/slack/class-slack-author-resolver.php b/includes/slack/class-slack-author-resolver.php index 55f8e9e1..1b5c9816 100644 --- a/includes/slack/class-slack-author-resolver.php +++ b/includes/slack/class-slack-author-resolver.php @@ -181,7 +181,7 @@ public static function render_profile_section( \WP_User $user ): void {
-

+

@@ -193,7 +193,9 @@ public static function render_profile_section( \WP_User $user ): void { /** * Save the Slack handle or member ID from the profile and user edit * screens. A value already mapped to another user who can be credited is - * refused, so a Slack message never has two people it could go to. + * refused, so a Slack message has one person it could go to. A user who + * later regains `edit_posts` can bring back a value someone else has + * since saved; the lower user ID is then credited. * * @param int $user_id ID of the user being saved. */ @@ -202,12 +204,13 @@ public static function save_profile_section( int $user_id ): void { return; } - check_admin_referer( 'update-user_' . $user_id ); - + // phpcs:ignore WordPress.Security.NonceVerification.Missing -- Only checks the field was submitted; the nonce is verified next. if ( ! isset( $_POST[ self::FIELD_SLACK_HANDLE ] ) ) { return; } + check_admin_referer( 'update-user_' . $user_id ); + $handle = self::normalize_handle( sanitize_text_field( wp_unslash( $_POST[ self::FIELD_SLACK_HANDLE ] ) ) ); if ( '' === $handle ) { diff --git a/src/admin/DEVELOPMENT.md b/src/admin/DEVELOPMENT.md index 7aab8d86..a45e2796 100644 --- a/src/admin/DEVELOPMENT.md +++ b/src/admin/DEVELOPMENT.md @@ -185,7 +185,8 @@ The `source` field stays defined, with no `render` and `enableHiding: false`. Da An entry ingested from Slack is credited to the WordPress user whose Slack member ID or handle matches the message's author, and to the Slack bot user (`Slack_Config::get_or_create_bot_user_id()`) when nobody's does. The value is set in the **Slack handle** field of the **Rolling Coverage** section on the profile and user edit screens, and stored in the `rolling_coverage_slack_handle` user meta (`Slack_Author_Resolver::META_SLACK_HANDLE`). The section shows only for users who can `edit_posts`, since nobody else can be credited. - A value that starts with `U` or `W` and is uppercase letters with at least one digit (`Slack_Author_Resolver::is_member_id()`) is a member ID; anything else is a handle. Values are stored trimmed and without a leading `@`. -- Saving a value that another user who can `edit_posts` already has is refused with an error on the profile screen, so a message never has two people it could be credited to. +- Saving a value that another user who can `edit_posts` already has is refused with an error on the profile screen, so a message has one person it could be credited to. Users without `edit_posts` are ignored, so a value left on a demoted user's profile can come back alongside someone else's when that user regains `edit_posts`; the lower user ID is then credited. +- A member ID typed in lowercase is a handle, so it never matches its owner's messages. Slack's **Copy member ID** gives it in uppercase. - At ingest, `Slack_Author_Resolver::resolve_author()` compares the message's member ID with stored member IDs, exactly. It then compares the author's Slack display name, full name and username, in that order, with stored handles only, ignoring case as the database collation does. Names are free text their owner can change, so a name shaped like a member ID never reaches a member-ID mapping. Only users who can `edit_posts` are credited. - Names need Slack's `users.info` lookup (1s budget on the webhook). When it fails, only a member ID can match. - Images uploaded with the message are owned by the same user as the entry. diff --git a/tests/test-slack-author-resolver.php b/tests/test-slack-author-resolver.php index 4209c80a..10718805 100644 --- a/tests/test-slack-author-resolver.php +++ b/tests/test-slack-author-resolver.php @@ -107,8 +107,22 @@ public function test_falls_back_to_the_full_name_then_the_username() { $by_username = self::mapped_user( 'rsample' ); $by_full_name = self::mapped_user( 'Riley Sample' ); - $this->assertSame( $by_full_name, self::resolve( self::slack_user( '', 'rsample', 'Riley Sample' ) ), 'The full name should win over the username.' ); - $this->assertSame( $by_username, self::resolve( self::slack_user( '', 'rsample' ) ), 'The username should match when nothing else does.' ); + $this->assertSame( + [ + 'user_id' => $by_full_name, + 'matched_by' => 'real_name', + ], + Slack_Author_Resolver::resolve_author( self::MEMBER_ID, self::slack_user( '', 'rsample', 'Riley Sample' ) ), + 'The full name should win over the username.' + ); + $this->assertSame( + [ + 'user_id' => $by_username, + 'matched_by' => 'name', + ], + Slack_Author_Resolver::resolve_author( self::MEMBER_ID, self::slack_user( '', 'rsample' ) ), + 'The username should match when nothing else does.' + ); } /** @@ -275,6 +289,23 @@ public function test_profile_refuses_a_handle_another_user_has() { $this->assertSame( 'Riley Sample', get_user_meta( $owner, Slack_Author_Resolver::META_SLACK_HANDLE, true ), 'The other user should keep the handle.' ); } + /** + * A member ID another user has is refused, like a handle. + */ + public function test_profile_refuses_a_member_id_another_user_has() { + $owner = self::mapped_user( self::MEMBER_ID ); + $user_id = self::log_in_as( 'author' ); + + self::save_profile( $user_id, self::MEMBER_ID ); + + $errors = new WP_Error(); + do_action_ref_array( 'user_profile_update_errors', [ &$errors, true, get_userdata( $user_id ) ] ); + + $this->assertSame( '', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ), 'The member ID should not be saved.' ); + $this->assertContains( 'rolling_coverage_slack_handle_taken', $errors->get_error_codes(), 'The profile should show an error.' ); + $this->assertSame( self::MEMBER_ID, get_user_meta( $owner, Slack_Author_Resolver::META_SLACK_HANDLE, true ), 'The other user should keep the member ID.' ); + } + /** * A value held by someone who can't be credited doesn't block anyone. */ From 0b1717d3e7cf7f5a2aba6c242ccccce99eeeed60 Mon Sep 17 00:00:00 2001 From: leogermani Date: Wed, 7 Oct 2026 17:43:07 -0300 Subject: [PATCH 4/5] fix(slack): say when Slack entries go to the bot user in settings Co-Authored-By: Claude Opus 5.5 --- src/admin/components/slack/settings/ingestion-settings-tab.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/admin/components/slack/settings/ingestion-settings-tab.tsx b/src/admin/components/slack/settings/ingestion-settings-tab.tsx index de3c0325..13fbdc11 100644 --- a/src/admin/components/slack/settings/ingestion-settings-tab.tsx +++ b/src/admin/components/slack/settings/ingestion-settings-tab.tsx @@ -129,7 +129,7 @@ function IngestionSettingsTab( { heading={ 2 } title={ __( 'Bot User', 'newspack-rolling-coverage' ) } description={ __( - 'This WordPress user is created automatically and is the author of every entry ingested from Slack. Change its display name or avatar from its WordPress profile.', + "This WordPress user is created automatically. Entries ingested from Slack are credited to it, unless the person who posted them has added their Slack handle or member ID to their WordPress profile. Change the bot user's display name or avatar from its WordPress profile.", 'newspack-rolling-coverage' ) } /> From 0dc9c7de70912efe830cde37382f501b05fb92d7 Mon Sep 17 00:00:00 2001 From: leogermani Date: Thu, 8 Oct 2026 09:45:28 -0300 Subject: [PATCH 5/5] fix(slack): store a Slack handle with a backslash as entered Co-Authored-By: Claude Opus 5.5 --- includes/slack/class-slack-author-resolver.php | 4 +++- tests/test-slack-author-resolver.php | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/includes/slack/class-slack-author-resolver.php b/includes/slack/class-slack-author-resolver.php index 1b5c9816..3b8902ae 100644 --- a/includes/slack/class-slack-author-resolver.php +++ b/includes/slack/class-slack-author-resolver.php @@ -240,6 +240,8 @@ static function ( \WP_Error $errors ) use ( $handle ) { return; } - update_user_meta( $user_id, self::META_SLACK_HANDLE, $handle ); + // update_user_meta() unslashes what it stores; slash the value so a + // backslash in it is stored as checked above. + update_user_meta( $user_id, self::META_SLACK_HANDLE, wp_slash( $handle ) ); } } diff --git a/tests/test-slack-author-resolver.php b/tests/test-slack-author-resolver.php index 10718805..366e89b0 100644 --- a/tests/test-slack-author-resolver.php +++ b/tests/test-slack-author-resolver.php @@ -260,6 +260,18 @@ public function test_profile_saves_the_handle() { $this->assertSame( 'Riley Sample', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ) ); } + /** + * A backslash in the value is stored as typed, so the stored value is + * the one the duplicate check compared. + */ + public function test_profile_keeps_a_backslash() { + $user_id = self::log_in_as( 'author' ); + + self::save_profile( $user_id, 'Riley\\ Sample' ); + + $this->assertSame( 'Riley\\ Sample', get_user_meta( $user_id, Slack_Author_Resolver::META_SLACK_HANDLE, true ) ); + } + /** * Emptying the field removes the mapping. */