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 23 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 |
|---|---|---|
|
|
@@ -4,20 +4,19 @@ | |
| import { useState, useEffect, useCallback } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { | ||
| Card, | ||
| CardHeader, | ||
| CardBody, | ||
| TextControl, | ||
| SelectControl, | ||
| Button, | ||
| Modal, | ||
| CheckboxControl, | ||
| Notice, | ||
| __experimentalGrid as Grid, | ||
| __experimentalHStack as HStack, | ||
| __experimentalVStack as VStack, | ||
| Dashicon, | ||
| Snackbar, | ||
| SnackbarList, | ||
| Icon, | ||
| } from '@wordpress/components'; | ||
|
|
@@ -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, | ||
| }: { | ||
|
|
@@ -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 () => { | ||
|
|
@@ -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' ); | ||
|
Kallyan01 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
@@ -201,6 +205,7 @@ | |
| message?: string; | ||
| data?: { | ||
| response_data?: CreateUserResult[]; | ||
| error_log?: { site_name?: string; message?: string }[]; | ||
| }; | ||
| }; | ||
| if ( ! data.success ) { | ||
|
|
@@ -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
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 }`, | ||
|
|
@@ -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 ) | ||
| ); | ||
| } } | ||
| /> | ||
| ) } | ||
|
|
||
| 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, | ||
|
|
||
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.