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 14 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'; | ||
|
|
||
|
|
@@ -186,12 +186,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' ); | ||
|
Kallyan01 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
@@ -201,6 +208,7 @@ | |
| message?: string; | ||
| data?: { | ||
| response_data?: CreateUserResult[]; | ||
| error_log?: { site_name?: string; message?: string }[]; | ||
| }; | ||
| }; | ||
| if ( ! data.success ) { | ||
|
|
@@ -216,7 +224,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; | ||||||
| } | ||||||
| } | ||||||
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.