Skip to content
Open
Show file tree
Hide file tree
Changes from 19 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
83 changes: 52 additions & 31 deletions assets/src/components/CreateUser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,19 @@
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,
Icon,
} from '@wordpress/components';
Expand Down Expand Up @@ -60,6 +59,9 @@
role: 'subscriber',
};

// Id of the general (non per-site) notice within the snackbar list.
const GENERAL_NOTICE_ID = 'create-user-notice';

const CreateUser = ( {
availableSites,
}: {
Expand All @@ -82,12 +84,7 @@
StrengthLevel | 'default'
>( 'default' );
const [ userCreationNotices, setUserCreationNotices ] = useState<
Array<
Omit< React.ComponentProps< typeof Snackbar >, 'children' > & {
id: string;
content: string;
}
>
Array< { id: string; content: string; className: string } >
>( [] );

const fetchStrongPassword = useCallback( async () => {
Expand Down Expand Up @@ -186,12 +183,19 @@
);

if ( ! response.ok ) {
const errorData = ( await response
.json()
.catch( () => null ) ) as {
message?: string;
} | null;
setNotice( {
type: 'error',
message: __(
'Failed to create user. Please try again later.',
'oneaccess'
),
message:
errorData?.message ||
__(
'Failed to create user. Please try again later.',
'oneaccess'
),
} );
throw new Error( 'Failed to create user' );
Comment thread
Kallyan01 marked this conversation as resolved.
Outdated
}
Expand All @@ -201,6 +205,7 @@
message?: string;
data?: {
response_data?: CreateUserResult[];
error_log?: { site_name?: string; message?: string }[];
};
};
if ( ! data.success ) {
Expand All @@ -216,7 +221,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

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 Expand Up @@ -497,26 +512,32 @@
</Grid>
</form>

{ notice && notice.message && (
<Snackbar
className={
notice.type === 'error'
? 'oneaccess-error-notice'
: 'oneaccess-success-notice'
}
onRemove={ () => setNotice( null ) }
>
{ notice.message }
</Snackbar>
) }

{ userCreationNotices.length > 0 && (
{ ( notice?.message || userCreationNotices.length > 0 ) && (
<SnackbarList
notices={ userCreationNotices }
onRemove={ () => {
setTimeout( () => {
setUserCreationNotices( [] );
}, 3000 );
notices={ [
...( notice?.message
? [
{
id: GENERAL_NOTICE_ID,
content: notice.message,
className:
notice.type === 'error'
? 'oneaccess-error-notice'
: 'oneaccess-success-notice',
},
]
: [] ),
...userCreationNotices,
] }
onRemove={ ( id: string ) => {
if ( GENERAL_NOTICE_ID === id ) {
setNotice( null );
return;
}

setUserCreationNotices( ( current ) =>
current.filter( ( item ) => item.id !== id )
);
} }
/>
) }
Expand Down
56 changes: 44 additions & 12 deletions assets/src/components/SharedUsers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@ interface GenericApiResponse {
message?: string;
}

interface DeleteUserResponse extends GenericApiResponse {
data?: {
error_log?: Array< { site_name?: string; message?: string } >;
};
}

const SharedUsers = ( {
availableSites,
}: {
Expand Down Expand Up @@ -384,30 +390,56 @@ const SharedUsers = ( {
}
);

if ( ! response.ok ) {
throw new Error( 'Failed to delete user from sites' );
const data = ( await response
.json()
.catch( () => null ) ) as DeleteUserResponse | null;

if ( ! response.ok || ! data ) {
setNotice( {
type: 'error',
message:
data?.message ||
__( 'Failed to delete user.', 'oneaccess' ),
} );
return;
}

const data = ( await response.json() ) as GenericApiResponse;
if ( ! data.success ) {
throw new Error(
data.message || 'Failed to delete user from sites'
);
if ( data.success ) {
setNotice( {
type: 'success',
message: __( 'User deleted successfully.', 'oneaccess' ),
} );
return;
}

// Partial failure: list each site's error after the summary.
const siteErrors = ( data.data?.error_log || [] )
.filter( ( failure ) => !! failure.message )
.map( ( { site_name: siteName = '', message = '' } ) =>
! siteName || message.includes( siteName )
? message
: `${ siteName }: ${ message }`
);

setNotice( {
type: 'success',
message: __( 'User deleted successfully.', 'oneaccess' ),
type: 'error',
message: [
data.message ||
__(
'User could not be deleted from some sites.',
'oneaccess'
),
...siteErrors,
].join( ' ' ),
} );

// Refresh users list
await fetchUsers();
} catch {
setNotice( {
type: 'error',
message: __( 'Failed to delete user.', 'oneaccess' ),
} );
} finally {
// Refresh even on failure, since some sites may have been deleted.
await fetchUsers();
setIsDeletingUser( false );
setShowUserDeletionModal( false );
setSelectedUser( null );
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 @@ 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;
Expand All @@ -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 ) {

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 (bool) $is_external;
}
}
Loading
Loading