Repository navigation
fix: cross-site user management and auth hardening #195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
16ba05e
a4da9cd
109d974
955992d
e7eb9a2
8d63dbe
8af9a3f
dd9b9db
1a55d3b
e373179
24d995e
3145b01
6e60d43
73969bd
a31ab49
e9fa5be
2563ccb
016fd69
26996c4
f063eda
05fed9d
0b604b7
5ad2275
6c87017
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,35 +1,35 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useState, useEffect, useCallback } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { | ||
| Button, | ||
| Card, | ||
| CardHeader, | ||
| CardBody, | ||
| TextControl, | ||
| SelectControl, | ||
| Button, | ||
| Modal, | ||
| CardHeader, | ||
| CheckboxControl, | ||
| Notice, | ||
| Dashicon, | ||
| __experimentalGrid as Grid, | ||
| __experimentalHStack as HStack, | ||
| __experimentalVStack as VStack, | ||
| Dashicon, | ||
| Icon, | ||
| Modal, | ||
| Notice, | ||
| SelectControl, | ||
| Snackbar, | ||
| SnackbarList, | ||
| Icon, | ||
| TextControl, | ||
| __experimentalVStack as VStack, | ||
| } from '@wordpress/components'; | ||
| import { useCallback, useEffect, useState } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { | ||
| isValidEmail, | ||
| checkPasswordStrength, | ||
| strengthWidths, | ||
| getStrengthColor, | ||
| isValidEmail, | ||
| strengthWidths, | ||
| type StrengthLevel, | ||
| } from '../js/utils'; | ||
|
|
||
|
|
@@ -201,6 +201,7 @@ | |
| message?: string; | ||
| data?: { | ||
| response_data?: CreateUserResult[]; | ||
| error_log?: { site_name?: string; message?: string }[]; | ||
| }; | ||
| }; | ||
| if ( ! data.success ) { | ||
|
|
@@ -216,7 +217,17 @@ | |
| return; | ||
| } | ||
|
|
||
| const results = data?.data?.response_data || []; | ||
| const results: CreateUserResult[] = [ | ||
| ...( data?.data?.response_data || [] ), | ||
| ...( data?.data?.error_log || [] ).map( ( failure ) => ( { | ||
| status: 'error' as const, | ||
| site: failure.site_name ?? '', | ||
| message: | ||
| failure.message ?? | ||
| __( 'Failed to create user.', 'oneaccess' ), | ||
| } ) ), | ||
| ]; | ||
|
Comment on lines
+224
to
+233
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good: per-site failures are visible now. Two follow-ups:
The same per-site treatment is still missing in the delete flow ( |
||
|
|
||
| const newNotices = results.map( | ||
| ( result: CreateUserResult, index: number ) => ( { | ||
| id: `notice-${ Date.now() }-${ index }`, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,28 +3,26 @@ | |
| #oneaccess-settings-page, | ||
| #oneaccess-manage-user { | ||
|
|
||
| &:has(.components-snackbar-list) { | ||
|
|
||
| .components-snackbar-list { | ||
| position: fixed; | ||
| bottom: 20px; | ||
| right: 20px; | ||
| z-index: 1000000; | ||
| align-items: flex-end; | ||
| justify-content: flex-end; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
| .components-snackbar-list, | ||
| .components-snackbar { | ||
| position: fixed; | ||
| bottom: 20px; | ||
| right: 20px; | ||
| z-index: 1000000; | ||
| width: auto; | ||
| } | ||
|
Comment on lines
+6
to
13
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropping
The "several at once — toasts stack" test step only holds within a single A more robust approach:
Two smaller things:
|
||
|
|
||
| &:not(:has(.components-snackbar-list)) { | ||
| .components-snackbar-list { | ||
| align-items: flex-end; | ||
| justify-content: flex-end; | ||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| /* Snackbars in a list are laid out by the list itself. */ | ||
| .components-snackbar { | ||
| position: fixed; | ||
| bottom: 20px; | ||
| right: 20px; | ||
| z-index: 1000000; | ||
| width: auto; | ||
| position: static; | ||
| bottom: auto; | ||
| right: auto; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -41,8 +39,6 @@ | |
| background-color: #e11d1d; | ||
| color: #fff; | ||
| } | ||
|
|
||
|
|
||
| } | ||
|
|
||
| .toplevel_page_oneaccess { | ||
|
|
@@ -52,7 +48,6 @@ | |
| } | ||
| } | ||
|
|
||
|
|
||
| body { | ||
|
|
||
| &.oneaccess-missing-brand-sites, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -37,6 +37,9 @@ public function __construct() { | |||||
| * {@inheritDoc} | ||||||
| */ | ||||||
| public function register_hooks(): void { | ||||||
| // Needed on both site types, so it must come before the consumer check below. | ||||||
| add_filter( 'http_request_host_is_external', [ $this, 'allow_oneaccess_host' ], 10, 2 ); | ||||||
|
|
||||||
| // Early return if this is not a consumer site. | ||||||
| if ( ! Settings::is_consumer_site() ) { | ||||||
| return; | ||||||
|
|
@@ -63,4 +66,25 @@ public function register_hooks(): void { | |||||
| public function user_deduplication(): void { | ||||||
| $this->actions_controller->send_users_for_deduplication(); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Allow outbound requests to the configured OneAccess sites. | ||||||
| * | ||||||
| * @internal Hook callback | ||||||
| * | ||||||
| * @param bool $is_external Whether the host is considered external. | ||||||
| * @param string $host Host name of the request. | ||||||
| */ | ||||||
| public function allow_oneaccess_host( $is_external, $host ): bool { | ||||||
| $urls = array_column( Settings::get_shared_sites(), 'url' ); | ||||||
| $urls[] = (string) Settings::get_parent_site_url(); | ||||||
|
|
||||||
| foreach ( $urls as $url ) { | ||||||
| if ( ! empty( $url ) && wp_parse_url( $url, PHP_URL_HOST ) === $host ) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes a global, production-wide change to work around a local-environment problem. Context
If private-network deployments need to be supported, I'd suggest one of these:
Smaller points:
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (FWIW I don't get the purpose of this entire class. Assuming the existing coupling is a workaround for |
||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return (bool) $is_external; | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -112,8 +112,9 @@ public function check_api_permissions( $request ) { | |||||||||||||||
| return false; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // if token is valid and request is from different domain then check if it matches governing site url. | ||||||||||||||||
| return self::is_same_domain( $governing_site_url, $request_origin ) || false !== strpos( $user_agent, $governing_site_url ); | ||||||||||||||||
| $governing_host = (string) wp_parse_url( $governing_site_url, PHP_URL_HOST ); | ||||||||||||||||
|
|
||||||||||||||||
| return self::is_same_domain( $governing_site_url, $request_origin ) || ( '' !== $governing_host && false !== strpos( $user_agent, $governing_host ) ); | ||||||||||||||||
|
Copilot marked this conversation as resolved.
Outdated
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with Copilot's open thread here. A host substring in the User-Agent also matches lookalike hosts: for WordPress sends
Suggested change
This keeps the scheme-agnostic behaviour you were after, since Two related notes, both outside the diff:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do we do in Onesearch? Seems like we should just align on an single Abstract_REST_Controller implementation, instead of solving the same issue in different ways in each codebase.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In OneSearch, check_api_permissions() authenticates with the token first and doesn't use the User-Agent at all |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
|
|
||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete flow: issues outside the diff (the PR touches this flow, so worth fixing here):
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skipped 5 for now, the issue is unclear
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Role update (
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,11 @@ | |
| * Class Governing_Site_Controller | ||
| */ | ||
| class Governing_Site_Controller extends Abstract_REST_Controller { | ||
| /** | ||
| * Minimum number of characters accepted for a new user's password. | ||
| */ | ||
| public const MIN_PASSWORD_LENGTH = 8; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new minimum is fine, but:
|
||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
|
|
@@ -224,9 +229,8 @@ public function register_routes(): void { | |
| 'sanitize_callback' => 'sanitize_text_field', | ||
| ], | ||
| 'password' => [ | ||
| 'required' => true, | ||
| 'type' => 'string', | ||
| 'sanitize_callback' => 'sanitize_text_field', | ||
| 'required' => true, | ||
| 'type' => 'string', | ||
| ], | ||
| 'sites' => [ | ||
| 'required' => true, | ||
|
|
@@ -417,11 +421,10 @@ public function delete_user_from_sites( \WP_REST_Request $request ): \WP_REST_Re | |
| ); | ||
| } | ||
|
|
||
| $response_data = []; | ||
| $oneaccess_sites_info = Settings::get_shared_sites(); | ||
| $processed_sites = []; | ||
| $error_log = []; | ||
| $user_delete_results = []; | ||
| $response_data = []; | ||
| $processed_sites = []; | ||
| $error_log = []; | ||
| $user_delete_results = []; | ||
|
|
||
| foreach ( $sites as $site ) { | ||
|
|
||
|
|
@@ -433,8 +436,9 @@ public function delete_user_from_sites( \WP_REST_Request $request ): \WP_REST_Re | |
| continue; | ||
| } | ||
|
|
||
| $request_url = $site['site_url'] . '/wp-json/' . self::NAMESPACE . '/delete-user'; | ||
| $api_key = $oneaccess_sites_info[ $site['site_url'] ]['api_key'] ?? ''; | ||
| $site_info = Settings::get_shared_site_by_url( $site['site_url'] ); | ||
| $request_url = untrailingslashit( $site_info['url'] ?? $site['site_url'] ) . '/wp-json/' . self::NAMESPACE . '/delete-user'; | ||
| $api_key = $site_info['api_key'] ?? ''; | ||
|
Kallyan01 marked this conversation as resolved.
Outdated
|
||
| $response = wp_safe_remote_request( | ||
| $request_url, | ||
| [ | ||
|
|
@@ -445,6 +449,7 @@ public function delete_user_from_sites( \WP_REST_Request $request ): \WP_REST_Re | |
| ], | ||
| 'headers' => [ | ||
| 'X-OneAccess-Token' => $api_key, | ||
| 'Origin' => get_site_url(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Since the real fix for delete is the key lookup above, I'd either:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced Origin with X-OneAccess-Site-URL, which every outbound call to a token-protected route now sends. The check_api_permissions() still has the origin check to handel the in browser requests which by default adds origin in the request header. |
||
| ], | ||
| ] | ||
| ); | ||
|
|
@@ -692,7 +697,7 @@ public function add_user_to_sites( \WP_REST_Request $request ): \WP_REST_Respons | |
| $email = sanitize_email( $request->get_param( 'email' ) ); | ||
| $username = sanitize_text_field( $request->get_param( 'username' ) ); | ||
| $full_name = sanitize_text_field( $request->get_param( 'fullName' ) ); | ||
| $password = sanitize_text_field( $request->get_param( 'password' ) ); | ||
| $password = (string) $request->get_param( 'password' ); | ||
| $sites = $request->get_param( 'sites' ); | ||
|
|
||
| if ( empty( $email ) || empty( $username ) || empty( $full_name ) || empty( $password ) || empty( $sites ) ) { | ||
|
|
@@ -705,6 +710,20 @@ public function add_user_to_sites( \WP_REST_Request $request ): \WP_REST_Respons | |
| ); | ||
| } | ||
|
|
||
| if ( strlen( $password ) < self::MIN_PASSWORD_LENGTH ) { | ||
| return new \WP_REST_Response( | ||
| [ | ||
| 'success' => false, | ||
| 'message' => sprintf( | ||
| /* translators: %d is the minimum number of characters */ | ||
| __( 'Password must be at least %d characters long.', 'oneaccess' ), | ||
| self::MIN_PASSWORD_LENGTH | ||
| ), | ||
| ], | ||
| 400 | ||
| ); | ||
| } | ||
|
|
||
| // Validate sites. | ||
| if ( ! is_array( $sites ) ) { | ||
| return new \WP_REST_Response( | ||
|
|
@@ -1029,7 +1048,7 @@ public function update_user_roles_for_sites( \WP_REST_Request $request ): \WP_RE | |
| public function create_user( \WP_REST_Request $request ): \WP_REST_Response { | ||
| $username = sanitize_user( $request->get_param( 'username' ) ); | ||
| $email = sanitize_email( $request->get_param( 'email' ) ); | ||
| $password = sanitize_text_field( $request->get_param( 'password' ) ); | ||
| $password = (string) $request->get_param( 'password' ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropping
Suggested fix, mirroring core: if ( str_contains( $password, '\\' ) ) {
// 400: Passwords cannot contain the "\" character.
}
$user_id = wp_create_user( $username, wp_slash( $password ), $email );Could you also add a password with |
||
| $full_name = sanitize_text_field( $request->get_param( 'full_name' ) ); | ||
| $role = sanitize_text_field( $request->get_param( 'role' ) ); | ||
|
|
||
|
|
@@ -1043,6 +1062,20 @@ public function create_user( \WP_REST_Request $request ): \WP_REST_Response { | |
| ); | ||
| } | ||
|
|
||
| if ( strlen( $password ) < self::MIN_PASSWORD_LENGTH ) { | ||
| return new \WP_REST_Response( | ||
| [ | ||
| 'success' => false, | ||
| 'message' => sprintf( | ||
| /* translators: %d is the minimum number of characters */ | ||
| __( 'Password must be at least %d characters long.', 'oneaccess' ), | ||
| self::MIN_PASSWORD_LENGTH | ||
| ), | ||
| ], | ||
| 400 | ||
| ); | ||
| } | ||
|
|
||
| if ( ! is_email( $email ) ) { | ||
| return new \WP_REST_Response( | ||
| [ | ||
|
|
@@ -1093,14 +1126,16 @@ public function create_user( \WP_REST_Request $request ): \WP_REST_Response { | |
| $role = 'subscriber'; | ||
| } | ||
|
|
||
| $name_parts = preg_split( '/\s+/', trim( $full_name ), 2 ) ?: []; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice fix for single-word names.
|
||
|
|
||
| // Set the user's full name and role. | ||
| wp_update_user( | ||
| [ | ||
| 'ID' => $user_id, | ||
| 'display_name' => $full_name, | ||
| 'user_nicename' => sanitize_title( $full_name ), | ||
| 'first_name' => explode( ' ', $full_name )[0] ?: '', | ||
| 'last_name' => explode( ' ', $full_name )[1] ?: '', | ||
| 'first_name' => $name_parts[0] ?? '', | ||
| 'last_name' => $name_parts[1] ?? '', | ||
| 'role' => $role ?: 'subscriber', | ||
| ] | ||
| ); | ||
|
|
@@ -1364,6 +1399,19 @@ public function create_users_for_sites( \WP_REST_Request $request ): \WP_REST_Re | |
| 400 | ||
| ); | ||
| } | ||
| if ( strlen( (string) $userdata['password'] ) < self::MIN_PASSWORD_LENGTH ) { | ||
|
Kallyan01 marked this conversation as resolved.
Outdated
|
||
| return new \WP_REST_Response( | ||
| [ | ||
| 'success' => false, | ||
| 'message' => sprintf( | ||
| /* translators: %d is the minimum number of characters */ | ||
| __( 'Password must be at least %d characters long.', 'oneaccess' ), | ||
| self::MIN_PASSWORD_LENGTH | ||
| ), | ||
| ], | ||
| 400 | ||
| ); | ||
| } | ||
| if ( ! isset( $userdata['fullName'] ) || empty( $userdata['fullName'] ) ) { | ||
| return new \WP_REST_Response( | ||
| [ | ||
|
|
@@ -1433,7 +1481,22 @@ public function create_users_for_sites( \WP_REST_Request $request ): \WP_REST_Re | |
| continue; | ||
| } | ||
|
|
||
| $response_code = wp_remote_retrieve_response_code( $response ); | ||
| $response_body = json_decode( wp_remote_retrieve_body( $response ), true ); | ||
|
|
||
| if ( ! is_array( $response_body ) ) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good guard, and a helpful message. The same
Each of those does |
||
| $error_log[] = [ | ||
| 'site_name' => $site_name, | ||
| 'message' => sprintf( | ||
| /* translators: 1: site name, 2: HTTP status code */ | ||
| __( 'Unreadable response from site %1$s (HTTP %2$d). The user may have been created there; please verify before retrying.', 'oneaccess' ), | ||
| $site_name, | ||
| (int) $response_code | ||
| ), | ||
| ]; | ||
| continue; | ||
| } | ||
|
|
||
| if ( empty( $response_body['success'] ) ) { | ||
| $error_log[] = [ | ||
| 'site_name' => $site_name, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the import reordering here is unrelated to the fix. If it isn't enforced by the linter, consider dropping it to keep the diff focused.