Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
16ba05e
fix: authenticate cross-site user deletion requests
Kallyan01 Sep 22, 2026
a4da9cd
fix: defer brand admin hooks until pluggable.php is loaded — Profile_…
Kallyan01 Sep 22, 2026
109d974
fix: preserve passwords and single-word names when creating users
Kallyan01 Sep 22, 2026
955992d
fix: toast notification postion
Kallyan01 Sep 22, 2026
e7eb9a2
chore: minor refactoring of comments
Kallyan01 Sep 22, 2026
8d63dbe
fix: removed unused parameter
Kallyan01 Sep 22, 2026
8af9a3f
chore: formatting fix
Kallyan01 Sep 22, 2026
dd9b9db
Potential fix for pull request finding 'Preserve existing filter resu…
Kallyan01 Sep 22, 2026
1a55d3b
fix: code quality
Kallyan01 Oct 1, 2026
e373179
Merge branch 'main' into fix/user-deletion
Kallyan01 Oct 7, 2026
24d995e
fix: enhance API token validation and add site URL header for requests
Kallyan01 Oct 8, 2026
3145b01
fix: improve error handling for user creation and password validation
Kallyan01 Oct 8, 2026
6e60d43
fix: update inc/Modules/Rest/Governing_Site_Controller.php
Kallyan01 Oct 8, 2026
73969bd
fix: add site URL header to user deletion request
Kallyan01 Oct 8, 2026
a31ab49
fix: sanitize password input when creating a user
Kallyan01 Oct 8, 2026
e9fa5be
fix: refactor name splitting logic in user creation and update methods
Kallyan01 Oct 8, 2026
2563ccb
fix: enhance user deletion error handling and improve site name retri…
Kallyan01 Oct 8, 2026
016fd69
fix: linting
Kallyan01 Oct 8, 2026
26996c4
fix: refactor Snackbar handling to improve notice display logic
Kallyan01 Oct 8, 2026
f063eda
fix: streamline user retrieval and enhance error logging during delet…
Kallyan01 Oct 8, 2026
05fed9d
fix: improve site URL handling and enhance error logging for unknown …
Kallyan01 Oct 8, 2026
0b604b7
fix: remove unnecessary allow_oneaccess_host filter and related code …
Kallyan01 Oct 8, 2026
5ad2275
fix: update site URL normalization and improve shared site retrieval …
Kallyan01 Oct 8, 2026
6c87017
fix: add site URL handling and improve CORS headers for OneAccess req…
Kallyan01 Oct 8, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion assets/src/components/CreateUser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@
import { useState, useEffect, useCallback } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import {

Copy link
Copy Markdown
Member

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.

Card,

Check warning on line 7 in assets/src/components/CreateUser.tsx

View workflow job for this annotation

GitHub Actions / CSS/JS Lint / JS Lint & TypeScript

Use `Card.Root` from `@wordpress/ui` instead
CardHeader,

Check warning on line 8 in assets/src/components/CreateUser.tsx

View workflow job for this annotation

GitHub Actions / CSS/JS Lint / JS Lint & TypeScript

Use `Card.Header` (and optionally `Card.Title`) from `@wordpress/ui` instead
CardBody,

Check warning on line 9 in assets/src/components/CreateUser.tsx

View workflow job for this annotation

GitHub Actions / CSS/JS Lint / JS Lint & TypeScript

Use `Card.Content` from `@wordpress/ui` instead
TextControl,

Check warning on line 10 in assets/src/components/CreateUser.tsx

View workflow job for this annotation

GitHub Actions / CSS/JS Lint / JS Lint & TypeScript

Use `InputControl` from `@wordpress/ui` instead. See migration guide in the lint rule documentation
SelectControl,
Button,
Modal,
CheckboxControl,
Notice,
__experimentalGrid as Grid,

Check warning on line 16 in assets/src/components/CreateUser.tsx

View workflow job for this annotation

GitHub Actions / CSS/JS Lint / JS Lint & TypeScript

__experimentalGrid is planned for deprecation. Write your own CSS instead
__experimentalHStack as HStack,

Check warning on line 17 in assets/src/components/CreateUser.tsx

View workflow job for this annotation

GitHub Actions / CSS/JS Lint / JS Lint & TypeScript

Use `Stack` from `@wordpress/ui` instead
__experimentalVStack as VStack,

Check warning on line 18 in assets/src/components/CreateUser.tsx

View workflow job for this annotation

GitHub Actions / CSS/JS Lint / JS Lint & TypeScript

Use `Stack` from `@wordpress/ui` instead
Dashicon,
Snackbar,
SnackbarList,
Expand Down Expand Up @@ -201,6 +201,7 @@
message?: string;
data?: {
response_data?: CreateUserResult[];
error_log?: { site_name?: string; message?: string }[];
};
};
if ( ! data.success ) {
Expand All @@ -216,7 +217,18 @@
return;
}

const results = data?.data?.response_data || [];
// Per-site failures are only reported in `error_log`, so merge them in as well.
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: per-site failures are visible now. Two follow-ups:

  • Lost server message: L188, the ! response.ok branch, never reads the body. Any 4xx/5xx shows "Failed to create user. Please try again later.", including the new 400 password message. Parse the JSON and prefer data.message.
  • Empty site name: when the site isn't found, site_name is '', so the notice reads "API key not found for site .". The server could fall back to the URL.

The same per-site treatment is still missing in the delete flow (SharedUsers.tsx), which is this PR's main fix. See the review summary.


const newNotices = results.map(
( result: CreateUserResult, index: number ) => ( {
id: `notice-${ Date.now() }-${ index }`,
Expand Down
37 changes: 16 additions & 21 deletions assets/src/css/admin.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping :has() simplifies this. But every standalone .components-snackbar is now position: fixed in the same corner as the list, so they overlap rather than stack when more than one is on screen:

  • CreateUser renders a standalone notice and a SnackbarList.
  • SharedUsers and ProfileRequests render their own snackbars.

The "several at once — toasts stack" test step only holds within a single SnackbarList.

A more robust approach:

  1. Render one SnackbarList at the app root, fed by the @wordpress/notices store (createSuccessNotice( message, { type: 'snackbar' } )).
  2. Remove these overrides.

Two smaller things:

  • Root cause in the description: the plugin's snackbar CSS hasn't changed since chore: refactor code base according to psr4 plus OneDesign #12, so I suspect a core wp-components change, but I couldn't confirm without running it.
  • Outside the diff: CreateUser.tsx L527's onRemove clears all notices 3s after the first one is dismissed. It should remove by id.


&: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;
}
}

Expand All @@ -41,8 +39,6 @@
background-color: #e11d1d;
color: #fff;
}


}

.toplevel_page_oneaccess {
Expand All @@ -52,7 +48,6 @@
}
}


body {

&.oneaccess-missing-brand-sites,
Expand Down
24 changes: 24 additions & 0 deletions inc/Modules/Core/Hooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
* {@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;
Expand All @@ -63,4 +66,25 @@
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 {

Check failure on line 78 in inc/Modules/Core/Hooks.php

View workflow job for this annotation

GitHub Actions / PHPCS / PHPCS Coding Standards

Unused parameter $is_external.
$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 ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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

  • Brand sites on public hosts already pass wp_safe_remote_*(). The filter only matters when a configured host resolves to a private or loopback IP, e.g. Local's *.local.
  • The repo already ships tests/_data/plugins/localhost-helper.php for the wp-env case.
  • As a global filter, it also relaxes the check for any other plugin's (or core's) safe requests to these hosts.

If private-network deployments need to be supported, I'd suggest one of these:

  • make it opt-in (a filter or constant), or
  • scope it to OneAccess's own requests: verify the URL belongs to a configured site, then pass reject_unsafe_urls => false from one shared request helper.

Smaller points:

  • Ports: it doesn't cover non-standard ports, which http_allowed_safe_ports rejects separately. So it won't help wp-env-style localhost:8889 setups.
  • Placement: the class docblock describes consumer-site hooks, so this would fit better next to the HTTP code.
  • Case: host comparison should be case-insensitive:
Suggested change
if ( ! empty( $url ) && wp_parse_url( $url, PHP_URL_HOST ) === $host ) {
if ( ! empty( $url ) && strtolower( (string) wp_parse_url( $url, PHP_URL_HOST ) ) === strtolower( $host ) ) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 send_users_for_deduplication() being built-in to that class instead of the endpoint. But either way yah, seems like this specific diff is solved by localhost-helper.php and doesn't need to exist at all. )

return true;
}
}

return false;
Comment thread
Copilot marked this conversation as resolved.
Outdated
}
}
5 changes: 3 additions & 2 deletions inc/Modules/Rest/Abstract_REST_Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
Comment thread
Copilot marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 example.com, WordPress/6.9; https://example.com.attacker.test passes.

WordPress sends WordPress/<version>; <home_url>, so we can parse that URL and reuse the exact host comparison:

Suggested change
$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 ) );
// WordPress sends "WordPress/<version>; <home_url>" as the User-Agent.
$user_agent_url = preg_match( '#;\s*(https?://\S+)#i', $user_agent, $matches ) ? $matches[1] : '';
return self::is_same_domain( $governing_site_url, $request_origin ) || self::is_same_domain( $governing_site_url, $user_agent_url );

This keeps the scheme-agnostic behaviour you were after, since is_same_domain() compares hosts only.

Two related notes, both outside the diff:

  • Case: is_same_domain() compares hosts case-sensitively (L148); strtolower() both sides.
  • Governing side: the reverse direction, Actions_Controller::brand_site_to_governing_site_permission_check() (L199), still matches the full URL with strpos(). The same check now follows two different rules, so a shared helper would keep them in sync.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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

}

/**
Expand Down
99 changes: 82 additions & 17 deletions inc/Modules/Rest/Governing_Site_Controller.php

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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):

  1. Raw response leak (must fix). L479: $error_log[] = $response; pushes the raw wp_safe_remote_request() result (status, body, cookies) into the JSON returned to the browser. Remove it; the message pushed just above already covers the failure.
  2. Missing decode guard. L484 ! $response_body['success'] runs on a possibly-null decode; reuse the is_array() guard from L1487.
  3. Dead dedup check. L432: $processed_sites is only appended inside the skip branch, never after a site is processed, so the duplicate check never fires. Same at L758 in add_user_to_sites().
  4. Wrong-user deletion risk. delete_user() (L363) looks the user up by login first, then email. A different person with the same username on a brand site would be deleted. Email is the identity everywhere else (dedup table, update_user()), so match by email and verify the login.
  5. Orphaned posts. wp_delete_user( $user->ID, 0 ) (L382) reassigns the user's content to user ID 0. If keeping content is the intent, reassign to a real user, or make it a setting.
  6. Inconsistent error labels. Delete errors show the URL, while create errors show the site name.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped 5 for now, the issue is unclear

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Role update (update_user_roles_for_sites()): outside the diff, but it was broken on main by the same key change:

  1. Silent skip. L951–L959: an unknown site is skipped with continue and never added to error_log, so the response says success while nothing changed. Report it like add-to-sites does.
  2. Double slash. L953/L963 build https://brand.example/ + /wp-json/…, i.e. //wp-json. It works on most servers, but Abstract_REST_Controller::build_api_endpoint() has the same trailingslashit() . '/wp-json/' bug and isn't called anywhere. Fixing it and using it for every outbound URL would remove this class of issue.

Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new minimum is fine, but:

  • Duplicated: the same block is pasted three times (L713, L1065, L1402). A single helper keeps the rules in sync and is a natural place for core's \ rule (see the create_user() comment):

    private static function validate_password( string $password ): ?\WP_REST_Response {
    	if ( mb_strlen( $password ) < self::MIN_PASSWORD_LENGTH ) {
    		return new \WP_REST_Response(
    			[
    				'success' => false,
    				/* translators: %d is the minimum number of characters */
    				'message' => sprintf( __( 'Password must be at least %d characters long.', 'oneaccess' ), self::MIN_PASSWORD_LENGTH ),
    			],
    			400
    		);
    	}
    
    	if ( str_contains( $password, '\\' ) ) {
    		return new \WP_REST_Response(
    			[
    				'success' => false,
    				'message' => __( 'Passwords cannot contain the "\\" character.', 'oneaccess' ),
    			],
    			400
    		);
    	}
    
    	return null;
    }
  • Bytes vs characters: strlen() counts bytes, while the UI counts characters, so use mb_strlen().

  • Behaviour change: this is a new server-side policy, so please mention it in the PR description.

  • Message lost in the UI: CreateUser.tsx L188 never reads the body on ! response.ok, so this 400 shows up as "Failed to create user. Please try again later.". It should show data.message.


/**
* {@inheritDoc}
*/
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 ) {

Expand All @@ -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'] ?? '';
Comment thread
Kallyan01 marked this conversation as resolved.
Outdated
$response = wp_safe_remote_request(
$request_url,
[
Expand All @@ -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(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Origin is now sent only on this request; the other outbound calls (create, add to sites, role update, profile requests, dedup) don't send it.

Since the real fix for delete is the key lookup above, I'd either:

  • drop this, or
  • send it from one shared request helper for every call, so the brand-side check sees the same headers everywhere.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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.

],
]
);
Expand Down Expand Up @@ -692,8 +697,9 @@ 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' ) );
$sites = $request->get_param( 'sites' );
// Passwords are hashed, never rendered or queried, so they must reach wp_create_user() verbatim.
$password = (string) $request->get_param( 'password' );
$sites = $request->get_param( 'sites' );

if ( empty( $email ) || empty( $username ) || empty( $full_name ) || empty( $password ) || empty( $sites ) ) {
return new \WP_REST_Response(
Expand All @@ -705,6 +711,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(
Expand Down Expand Up @@ -1027,9 +1047,10 @@ public function update_user_roles_for_sites( \WP_REST_Request $request ): \WP_RE
* @param \WP_REST_Request $request The REST request object.
*/
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' ) );
$username = sanitize_user( $request->get_param( 'username' ) );
$email = sanitize_email( $request->get_param( 'email' ) );
// Passwords are hashed, never rendered or queried, so they must reach wp_create_user() verbatim.
$password = (string) $request->get_param( 'password' );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping sanitize_text_field() is right. Passwords containing ', " or \ still won't work on the brand site's wp-login.php, though, because of WordPress's slashing convention:

  • Create: wp_create_user() (L1112) doesn't slash the password, so the stored hash is of the raw value.
  • Login: wp_signon() checks $_POST['pwd'] without wp_unslash(), i.e. the slashed value. It only matches if the hash was also made from the slashed form.
  • How core handles it: the REST users controller calls wp_insert_user( wp_slash( (array) $user ) ) and rejects \ in check_user_password().

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 ' and " to the testing instructions? </> alone doesn't exercise this.

$full_name = sanitize_text_field( $request->get_param( 'full_name' ) );
$role = sanitize_text_field( $request->get_param( 'role' ) );

Expand All @@ -1043,6 +1064,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(
[
Expand Down Expand Up @@ -1093,14 +1128,16 @@ public function create_user( \WP_REST_Request $request ): \WP_REST_Response {
$role = 'subscriber';
}

$name_parts = preg_split( '/\s+/', trim( $full_name ), 2 ) ?: [];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix for single-word names.

add_user_to_sites() (L832) still splits with explode( ' ', … ) for the dedup table. A small shared helper would keep both paths producing the same first/last name, e.g. [ $first, $last ] = self::split_full_name( $full_name );.


// 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',
]
);
Expand Down Expand Up @@ -1364,6 +1401,19 @@ public function create_users_for_sites( \WP_REST_Request $request ): \WP_REST_Re
400
);
}
if ( strlen( (string) $userdata['password'] ) < self::MIN_PASSWORD_LENGTH ) {
Comment thread
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(
[
Expand Down Expand Up @@ -1433,7 +1483,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 ) ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good guard, and a helpful message.

The same json_decode() result is still dereferenced without a check elsewhere:

  • delete (L484)
  • add to sites (L814)
  • role update (L1008)

Each of those does $response_body['success'] on a possible null. Worth applying the same guard there. Better still, decode once in a shared request helper.

$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,
Expand Down
27 changes: 22 additions & 5 deletions inc/Modules/Settings/Settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ public static function sanitize_shared_sites( $input ): array {
*/

/**
* Get brand sites configured for this governing site, keyed by the (trailing-slash) URL.
* Get brand sites configured for this governing site, keyed by the site URL.
*
* @return array<string,array{
* api_key: string,
Expand All @@ -240,8 +240,7 @@ public static function get_shared_sites(): array {
continue;
}

// Always use a trailing-slash URL.
$url = trailingslashit( $brand['url'] );
$url = untrailingslashit( $brand['url'] );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This revert is the fix that matters most in the PR, and it's bigger than the description suggests.

#124 switched these keys to trailingslashit(), while create_users_for_sites(), add_user_to_sites() and update_user_roles_for_sites() all look sites up with untrailingslashit() keys. On main, that means:

  • create: fails with "API key not found" for every site (silently, because the UI ignored error_log)
  • add to sites: fails with "Site not found"
  • role update: skips every site but still reports success

Please list those in the PR description. A unit test pinning the key format would stop the next refactor from flipping it silently.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A unit test pinning the key format would stop the next refactor from flipping it silently.

Codebase unification can't come soon enough, but FWIW it wasn't flipped "silently", but to align shared code from the repos that were audited. The problem is that without internal unit tests the rest of this codebase that didn't update started failing.

We need to decide whether all onepress repos are going to deal store brand urls as trailing-slashed (onesearch) or untrailingslashed, and then be consistent.

Personally I don't care which. The one that allows us to need to explicitly (un)trailingslashit() the least amount.


$brands_to_return[ $url ] = [
'api_key' => $brand['api_key'] ?? '',
Expand Down Expand Up @@ -269,9 +268,27 @@ public static function get_shared_sites(): array {
public static function get_shared_site_by_url( string $site_url ): ?array {
$brand_sites = self::get_shared_sites();

$normalized_url = trailingslashit( $site_url );
$normalized_url = self::normalize_site_url( $site_url );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: this lookup ignores scheme and trailing slashes. It's only used by delete, though. Create (L1425–L1453), add to sites (L739–L767) and role update (L945–L952) still index $oneaccess_sites_info[ $url ] directly, so an http/https or slash mismatch still breaks them.

Suggest routing every lookup through this helper. It may also be worth lowercasing in normalize_site_url(), since hosts are case-insensitive.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#195 (comment) . I agree it makes sense on a public static getting since we don't know where it will be used, but we shouldn't just normalize "every lookup" unless we have an explicit reason to believe it's not going to be normalized.


return $brand_sites[ $normalized_url ] ?? null;
foreach ( $brand_sites as $site ) {
if ( self::normalize_site_url( $site['url'] ) === $normalized_url ) {
return $site;
}
}

return null;
}

/**
* Normalize a site URL for comparison.
*
* A site reports itself via `get_site_url()`, whose scheme and trailing slash can differ
* from the URL configured on the governing site, so strip both before comparing.
*
* @param string $site_url The site URL.
*/
private static function normalize_site_url( string $site_url ): string {
return (string) preg_replace( '#^https?://#i', '', untrailingslashit( $site_url ) );

@justlevine justlevine Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of the preg_replace() seems unnecessarily heavy for what we need to be doing here. Feel's like (un)trailingslashit( trim( $site_url ) ) should be more than enough, similar to Onesearch\Utils::normalize_url() without the Utils baggage.

@up1512001 thoughts?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed normalize_site_url()

}

/**
Expand Down
8 changes: 8 additions & 0 deletions inc/Modules/User/Profile_Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ public function register_hooks(): void {
add_action( 'personal_options_update', [ $this, 'store_profile_update_request' ] );
add_action( 'edit_user_profile_update', [ $this, 'store_profile_update_request' ] );

// the current user is not available until pluggable.php has loaded, so defer.
add_action( 'init', [ $this, 'register_brand_admin_hooks' ] );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring to init works, and it's the right hook for wp_get_current_user(). The root cause is the bootstrap, though:

  • feat!: Rescaffold and update dependencies. #124 replaced add_action( 'plugins_loaded', '\OneAccess\load_plugin' ) in oneaccess.php with a direct \OneAccess\Main::instance().
  • So every module's register_hooks() now runs while the plugin file is being included, before pluggable.php exists.

Restoring the deferral would prevent the next current_user_can() / wp_get_current_user() in any register_hooks() from taking brand sites down again:

// oneaccess.php
add_action( 'plugins_loaded', [ \OneAccess\Main::class, 'instance' ] );

One of the two fixes is enough, but I'd go with the bootstrap one. An E2E smoke test (set oneaccess_site_type = brand-site, load /wp-admin/) would have caught this. The current E2E only activates and deactivates the plugin.

@justlevine justlevine Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoring the deferral would prevent the next current_user_can() / wp_get_current_user() in any register_hooks() from taking brand sites down again:

This was an intentional change, and IMO we should not revert loading Main::instance(), on plugins_loaded, and instead use the correct lifecycle hook when needed.

(In the future, we'll have tests to cover these sort of edge cases).

Reason why it was changed is that there are numerous occasions where we don't want to hook things on plugins_loaded (e.g. our Async task runner), and it's a lot easier to scope things to run where they're supposed to than it is to carve out edge cases that we then need to throw onto one*.php.


That said, if these specific hooks only need to run on the dashboard, then lets use admin_init instead of init() or plugins_loaded

Suggested change
// the current user is not available until pluggable.php has loaded, so defer.
add_action( 'init', [ $this, 'register_brand_admin_hooks' ] );
// the current user is not available until pluggable.php has loaded, so defer.
add_action( 'admin_init', [ $this, 'register_brand_admin_hooks' ] );

}

/**
* Register the hooks that only apply to brand admins.
*/
public function register_brand_admin_hooks(): void {
// get current user.
$current_user = wp_get_current_user();

Expand Down
Loading