Skip to content
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

[Workplace Search] Add Account Settings page imported from Security plugin #99791

Merged
merged 19 commits into from
Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4d7bb8a
Copy lazy_wrapper and suspense_error_boundary from Spaces plugin
yakhinvadim Jun 1, 2021
c416ab9
Create async versions of personal_info and change_password components
yakhinvadim Jun 1, 2021
f994ba5
Create ui_api that allows to load Security components asuncronously
yakhinvadim Jun 1, 2021
e09f42a
Make ui_api available through Security components's lifecycle methods
yakhinvadim Jun 1, 2021
897615a
Import Security plugin into Enterprise Search
yakhinvadim Jun 1, 2021
3080c1c
Add Security plugin and Notifications service to Kibana Logic file
yakhinvadim Jun 1, 2021
cfc1386
Export the required components from the Security plugin and
yakhinvadim Jun 1, 2021
efa7d68
Update link to the Account Settings page
yakhinvadim Jun 1, 2021
62aab12
Merge branch 'master' into account-settings
yakhinvadim Jun 8, 2021
4f8f832
Move getUiApi call to security start and pass core instead of getStar…
yakhinvadim Jun 8, 2021
9f18ea9
Simplify import of change_password_async component by providing...
yakhinvadim Jun 8, 2021
706ac2e
Remove UserAPIClient from ui_api
yakhinvadim Jun 8, 2021
ebc2753
Export ChangePasswordProps and PersonalInfoProps from account_managem…
yakhinvadim Jun 8, 2021
140cd91
Remove notifications service from kibana_logic
yakhinvadim Jun 8, 2021
e152365
Merge branch 'master' into account-settings
yakhinvadim Jun 10, 2021
8ec9379
Add UiApi to SecurityPluginStart interface
yakhinvadim Jun 10, 2021
9b932dc
Utilize index files for exporting Props types
yakhinvadim Jun 10, 2021
2425821
Replace Pick<...> with two separate interfaces as it doesn't work wel…
yakhinvadim Jun 10, 2021
1880755
Add a comment explaining why we're not loading async components throu…
yakhinvadim Jun 10, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import { mockHistory } from './react_router_history.mock';

import { chartPluginMock } from '../../../../../../src/plugins/charts/public/mocks';
import { securityMock } from '../../../../security/public/mocks';

export const mockKibanaValues = {
config: { host: 'http://localhost:3002' },
Expand All @@ -18,6 +19,7 @@ export const mockKibanaValues = {
},
history: mockHistory,
navigateToUrl: jest.fn(),
security: securityMock.createStart(),
setBreadcrumbs: jest.fn(),
setChromeIsVisible: jest.fn(),
setDocTitle: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { getContext } from 'kea';
import { coreMock } from '../../../../../src/core/public/mocks';
import { chartPluginMock } from '../../../../../src/plugins/charts/public/mocks';
import { licensingMock } from '../../../licensing/public/mocks';
import { securityMock } from '../../../security/public/mocks';

import { AppSearch } from './app_search';
import { EnterpriseSearch } from './enterprise_search';
Expand All @@ -27,6 +28,7 @@ describe('renderApp', () => {
plugins: {
licensing: licensingMock.createStart(),
charts: chartPluginMock.createStartContract(),
security: securityMock.createStart(),
},
} as any;
const pluginData = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const renderApp = (
cloud: plugins.cloud || {},
history: params.history,
navigateToUrl: core.application.navigateToUrl,
security: plugins.security || {},
setBreadcrumbs: core.chrome.setBreadcrumbs,
setChromeIsVisible: core.chrome.setIsVisible,
setDocTitle: core.chrome.docTitle.change,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { kea, MakeLogicType } from 'kea';
import { ApplicationStart, ChromeBreadcrumb } from '../../../../../../../src/core/public';
import { ChartsPluginStart } from '../../../../../../../src/plugins/charts/public';
import { CloudSetup } from '../../../../../cloud/public';
import { SecurityPluginStart } from '../../../../../security/public';

import { HttpLogic } from '../http';
import { createHref, CreateHrefOptions } from '../react_router_helpers';
Expand All @@ -23,6 +24,7 @@ interface KibanaLogicProps {
cloud: Partial<CloudSetup>;
charts: ChartsPluginStart;
navigateToUrl: ApplicationStart['navigateToUrl'];
security: Partial<SecurityPluginStart>;
setBreadcrumbs(crumbs: ChromeBreadcrumb[]): void;
setChromeIsVisible(isVisible: boolean): void;
setDocTitle(title: string): void;
Expand All @@ -47,6 +49,7 @@ export const KibanaLogic = kea<MakeLogicType<KibanaValues>>({
},
{},
],
security: [props.security || {}, {}],
setBreadcrumbs: [props.setBreadcrumbs, {}],
setChromeIsVisible: [props.setChromeIsVisible, {}],
setDocTitle: [props.setDocTitle, {}],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { getWorkplaceSearchUrl } from '../../../../shared/enterprise_search_url'
import { EuiButtonEmptyTo } from '../../../../shared/react_router_helpers';
import { AppLogic } from '../../../app_logic';
import { WORKPLACE_SEARCH_TITLE, ACCOUNT_NAV } from '../../../constants';
import { PERSONAL_SOURCES_PATH, LOGOUT_ROUTE, KIBANA_ACCOUNT_ROUTE } from '../../../routes';
import { PERSONAL_SOURCES_PATH, LOGOUT_ROUTE, PERSONAL_SETTINGS_PATH } from '../../../routes';

export const AccountHeader: React.FC = () => {
const [isPopoverOpen, setPopover] = useState(false);
Expand All @@ -44,8 +44,7 @@ export const AccountHeader: React.FC = () => {

const accountNavItems = [
<EuiContextMenuItem key="accountSettings">
{/* TODO: Once auth is completed, we need to have non-admins redirect to the self-hosted form */}
<EuiButtonEmpty href={KIBANA_ACCOUNT_ROUTE}>{ACCOUNT_NAV.SETTINGS}</EuiButtonEmpty>
<EuiButtonEmptyTo to={PERSONAL_SETTINGS_PATH}>{ACCOUNT_NAV.SETTINGS}</EuiButtonEmptyTo>
</EuiContextMenuItem>,
<EuiContextMenuItem key="logout">
<EuiButtonEmpty href={LOGOUT_ROUTE}>{ACCOUNT_NAV.LOGOUT}</EuiButtonEmpty>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ import {
ORG_SETTINGS_PATH,
ROLE_MAPPINGS_PATH,
SECURITY_PATH,
PERSONAL_SETTINGS_PATH,
} from './routes';
import { AccountSettings } from './views/account_settings';
import { SourcesRouter } from './views/content_sources';
import { SourceAdded } from './views/content_sources/components/source_added';
import { SourceSubNav } from './views/content_sources/components/source_sub_nav';
Expand Down Expand Up @@ -103,6 +105,11 @@ export const WorkplaceSearchConfigured: React.FC<InitialAppData> = (props) => {
<SourcesRouter />
</PrivateSourcesLayout>
</Route>
<Route path={PERSONAL_SETTINGS_PATH}>
<PrivateSourcesLayout restrictWidth readOnlyMode={readOnlyMode}>
<AccountSettings />
</PrivateSourcesLayout>
</Route>
<Route path={SOURCES_PATH}>
<Layout
navigation={<WorkplaceSearchNav sourcesSubNav={showSourcesSubnav && <SourceSubNav />} />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export const SETUP_GUIDE_PATH = '/setup_guide';

export const NOT_FOUND_PATH = '/404';
export const LOGOUT_ROUTE = '/logout';
export const KIBANA_ACCOUNT_ROUTE = '/security/account';

export const LEAVE_FEEDBACK_EMAIL = '[email protected]';
export const LEAVE_FEEDBACK_URL = `mailto:${LEAVE_FEEDBACK_EMAIL}?Subject=Elastic%20Workplace%20Search%20Feedback`;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React, { useState, useEffect, useMemo } from 'react';

import { useValues } from 'kea';

import type { AuthenticatedUser } from '../../../../../../security/public';
import { KibanaLogic } from '../../../shared/kibana/kibana_logic';

export const AccountSettings: React.FC = () => {
const { security } = useValues(KibanaLogic);

const [currentUser, setCurrentUser] = useState<AuthenticatedUser | null>(null);

useEffect(() => {
security!.authc!.getCurrentUser().then(setCurrentUser);
Copy link
Member

Choose a reason for hiding this comment

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

question: won't we have an unhandled exception here in case security plugin is disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking our plugin code as well! Good question.

No, if this component loads, we are guaranteed to have the security plugin enabled.
One of the requirements for being able to run Workplace Search is to have security enabled (see Step 2 in our installation docs). If security is disabled, Workplace Search won't run and this page could not be opened.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for clarifying! Was just confused by these ! and was wondering if security and its properties can really be undefined or not (by the way it seems ! isn't strictly needed for security itself and is only needed to access its properties).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I guess I overdid it here. 😄
I plan to do a follow-up PR with some cleanup in the enterprise_search plugin only.
I'll update this line there if you don't mind.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍

}, [security.authc]);

const PersonalInfo = useMemo(() => security!.uiApi!.components.getPersonalInfo, [security.uiApi]);
const ChangePassword = useMemo(() => security!.uiApi!.components.getChangePassword, [
security.uiApi,
]);

if (!currentUser) {
return null;
}

return (
<>
<PersonalInfo user={currentUser} />
<ChangePassword user={currentUser} />
</>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export { AccountSettings } from './account_settings';
3 changes: 3 additions & 0 deletions x-pack/plugins/enterprise_search/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
} from '../../../../src/plugins/home/public';
import { CloudSetup } from '../../cloud/public';
import { LicensingPluginStart } from '../../licensing/public';
import { SecurityPluginSetup, SecurityPluginStart } from '../../security/public';

import {
APP_SEARCH_PLUGIN,
Expand All @@ -42,11 +43,13 @@ export interface ClientData extends InitialAppData {
interface PluginsSetup {
cloud?: CloudSetup;
home?: HomePublicPluginSetup;
security?: SecurityPluginSetup;
}
export interface PluginsStart {
cloud?: CloudSetup;
licensing: LicensingPluginStart;
charts: ChartsPluginStart;
security?: SecurityPluginStart;
}

export class EnterpriseSearchPlugin implements Plugin {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ import { canUserChangePassword } from '../../../common/model';
import type { UserAPIClient } from '../../management/users';
import { ChangePasswordForm } from '../../management/users/components/change_password_form';

interface Props {
export interface ChangePasswordProps {
user: AuthenticatedUser;
userAPIClient: PublicMethodsOf<UserAPIClient>;
notifications: NotificationsSetup;
}

export class ChangePassword extends Component<Props, {}> {
export class ChangePassword extends Component<ChangePasswordProps, {}> {
public render() {
const canChangePassword = canUserChangePassword(this.props.user);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';

import type { CoreStart } from 'src/core/public';

import { UserAPIClient } from '../../management/users';
import type { ChangePasswordProps } from './change_password';

export const getChangePasswordComponent = async (
core: CoreStart
): Promise<React.FC<Pick<ChangePasswordProps, 'user'>>> => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I learnt yesterday that Pick doesn't play well with our dev docs. Would you mind creating two separate interfaces inside of change_password instead and replacing any Pick-based types with dedicated interface?

export interface ChangePasswordProps {
  user: AuthenticatedUser;
}

export interface ChangePasswordPropsInternal extends ChangePasswordProps {
  userAPIClient: PublicMethodsOf<UserAPIClient>;
  notifications: NotificationsSetup;
}

(and you also need to merge the latest master and likely to update latest version of SecurityPluginStart)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good to know, and thanks for the link! I replaced it here: 2425821

const { ChangePassword } = await import('./change_password');

return (props: Pick<ChangePasswordProps, 'user'>) => {
return (
<ChangePassword
notifications={core.notifications}
userAPIClient={new UserAPIClient(core.http)}
{...props}
/>
);
};
};
3 changes: 3 additions & 0 deletions x-pack/plugins/security/public/account_management/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@
*/

export { accountManagementApp } from './account_management_app';

export type { ChangePasswordProps } from './change_password/change_password';
export type { PersonalInfoProps } from './personal_info/personal_info';
Copy link
Member

Choose a reason for hiding this comment

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

nit: please export these types from ./change_password | personal_info/index.ts. We use index.ts as a grouping mechanism for all exports where feasible.

Suggested change
export type { ChangePasswordProps } from './change_password/change_password';
export type { PersonalInfoProps } from './personal_info/personal_info';
export type { ChangePasswordProps } from './change_password';
export type { PersonalInfoProps } from './personal_info';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks! Updated here: 9b932dc

Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import { FormattedMessage } from '@kbn/i18n/react';

import type { AuthenticatedUser } from '../../../common/model';

interface Props {
export interface PersonalInfoProps {
user: AuthenticatedUser;
}

export const PersonalInfo = (props: Props) => {
export const PersonalInfo = (props: PersonalInfoProps) => {
return (
<EuiDescribedFormGroup
fullWidth
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';

import type { PersonalInfoProps } from './personal_info';

export const getPersonalInfoComponent = async (): Promise<React.FC<PersonalInfoProps>> => {
const { PersonalInfo } = await import('./personal_info');
return (props: PersonalInfoProps) => {
return <PersonalInfo {...props} />;
};
};
2 changes: 2 additions & 0 deletions x-pack/plugins/security/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { mockAuthenticatedUser } from '../common/model/authenticated_user.mock';
import { authenticationMock } from './authentication/index.mock';
import { navControlServiceMock } from './nav_control/index.mock';
import { createSessionTimeoutMock } from './session/session_timeout.mock';
import { getUiApiMock } from './ui_api/index.mock';

function createSetupMock() {
return {
Expand All @@ -23,6 +24,7 @@ function createStartMock() {
return {
authc: authenticationMock.createStart(),
navControlService: navControlServiceMock.createStart(),
uiApi: getUiApiMock.createStart(),
};
}

Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/security/public/plugin.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@ describe('Security Plugin', () => {
features: {} as FeaturesPluginStart,
})
).toEqual({
uiApi: {
components: {
getChangePassword: expect.any(Function),
getPersonalInfo: expect.any(Function),
},
},
authc: {
getCurrentUser: expect.any(Function),
areAPIKeysEnabled: expect.any(Function),
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { ManagementService } from './management';
import { SecurityNavControlService } from './nav_control';
import { SecurityCheckupService } from './security_checkup';
import { SessionExpired, SessionTimeout, UnauthorizedResponseHttpInterceptor } from './session';
import { getUiApi } from './ui_api';

export interface PluginSetupDependencies {
licensing: LicensingPluginSetup;
Expand Down Expand Up @@ -147,6 +148,7 @@ export class SecurityPlugin
}

return {
uiApi: getUiApi({ core }),
navControlService: this.navControlService.start({ core }),
authc: this.authc as AuthenticationServiceStart,
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export { SuspenseErrorBoundary } from './suspense_error_boundary';
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { EuiLoadingSpinner } from '@elastic/eui';
import type { PropsWithChildren } from 'react';
import React, { Component, Suspense } from 'react';

import { i18n } from '@kbn/i18n';
import type { NotificationsStart } from 'src/core/public';

interface Props {
notifications: NotificationsStart;
}

interface State {
error: Error | null;
}

export class SuspenseErrorBoundary extends Component<PropsWithChildren<Props>, State> {
state: State = {
error: null,
};

static getDerivedStateFromError(error: Error) {
// Update state so next render shows fallback UI.
return { error };
}

public componentDidCatch(error: Error) {
const { notifications } = this.props;
if (notifications) {
const title = i18n.translate('xpack.security.uiApi.errorBoundaryToastTitle', {
defaultMessage: 'Failed to load Kibana asset',
});
const toastMessage = i18n.translate('xpack.security.uiApi.errorBoundaryToastMessage', {
defaultMessage: 'Reload page to continue.',
});
notifications.toasts.addError(error, { title, toastMessage });
}
}

render() {
const { children, notifications } = this.props;
const { error } = this.state;
if (!notifications || error) {
return null;
}
return <Suspense fallback={<EuiLoadingSpinner />}>{children}</Suspense>;
}
}
Loading