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

[yakhinvadim]

This description is outdated, skip to #99791 (comment) for latest update.

Summary

This PR adds the Account Settings page to Workplace Search personal dashboard. The components for this page were imported from the Security plugin.

@azasypkin
I'm looking for feedback on the general approach. Please let me know what you think and if you see any areas for improvement. Commits 632589f and 3a17681 should be most relevant to you.

I will request a review from my team if the general approach looks good to you.

---

Screenshots

Workplace Search Account Settings page:

Screen.Cast.2021-05-11.at.12.29.38.PM.mp4

Current Account page in Security plugin for the reference:

---

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios

[yakhinvadim]

Thanks again for your help with this, Oleg!

I have several questions:

1. Page load bundle increase for security plugin worries me a bit. Is this something we can keep at the previous level, or is the increase unavoidable?
2. The test errors are coming from the ui_capabilities/spaces_only test suite, but I didn't touch it. Do you have any insights into why the tests are failing?
3. This is the first time I see Public APIs missing comments and Public APIs missing exports warnings. Should I run the recommended commands and commit the diff in this PR? Based on this PR, they are not required for merging the PR.

Questions 2 and 3 are not directly related to the security plugin, so please let me know if I should seek help elsewhere!

[azasypkin]

The approach looks good to me. We all will probably need to sync again in the future when we decide to update how personal info screen looks like (once we have user personal settings, notifications and whatnot), but that sounds like a reasonable price for reusable components.

@legrego do see any problems with sharing ChangePassword and PersonalInfo components with other plugins that I'm missing? Here is some context: the personal dashboards for Workplace Search will be part of the Workplace Search Kibana plugin, or more precisely part of the a chromeless app in the Kibana plugin. Currently such dashboards contain a form to change the user password that Workplace Search users are already familiar with. Ideally we'd integrate Kibana own account management view into this flow somehow, but it doesn't seem like we have a good (architecturally and UX-wise) way of doing this now. That's how the idea to reuse only certain security UI components came up.

1. Page load bundle increase for security plugin worries me a bit. Is this something we can keep at the previous level, or is the increase unavoidable?

Yeah, that worries me a bit too, but I'm not sure if we can do anything about it unless we do something like this in Security plugin:

import type { ChangePassword } from './account_management/change_password';
import type { PersonalInfo } from './account_management/personal_info';
export type { ChangePassword, PersonalInfo };

export function loadUserComponents(): Promise<{
  ChangePassword: typeof ChangePassword;
  PersonalInfo: typeof PersonalInfo;
}> {
  return Promise.all([
    import('./account_management/change_password'),
    import('./account_management/personal_info'),
  ]).then(([comp1, comp2]) => ({
    ChangePassword: comp1.ChangePassword,
    PersonalInfo: comp2.PersonalInfo,
  }));
}

@elastic/kibana-operations is there any recommended way to export public components from plugin A, so that plugin B can import them, but these components aren't included into main bundle of the plugin A?

2. The test errors are coming from the ui_capabilities/spaces_only test suite, but I didn't touch it. Do you have any insights into why the tests are failing?

Yeah, as you see only spaces_only tests are failing. These tests are run with disabled security, and since you made security required dependency for EntSearch, your plugins are disabled in these tests and don't register their specific UI capabilities anymore. As I asked you in another comment, I'm not sure if it's what you really want. If it's intentional, then just remove capabilities your app registers from the test expected results.

3. This is the first time I see Public APIs missing comments and Public APIs missing exports warnings. Should I run the recommended commands and commit the diff in this PR?

It's the first time I see this as well, but if running these commands produces changes that make CI happy, please do that. If not, let me know and I'll figure this out.

[azasypkin]

question: security can potentially be disabled, are you okay with EntSearch be disabled in this case as well because of disabled required dependency?

[yakhinvadim]

Great catch, I haven't considered this case. I'll look into it.
(Currently, I think I'll make it an optional dependency again).

[legrego]

@azasypkin thanks for the ping and the context.

Page load bundle increase for security plugin worries me a bit. Is this something we can keep at the previous level, or is the increase unavoidable?

I'm not a fan of the increased bundle size either -- while we are technically under the 100kb recommended limit, we spent a lot of time optimizing this bundle recently, so I'd like to keep this as small as possible.

We took a different approach with the Spaces plugin recently (#91976), in order to share reusable components between plugins. Essentially the components are available through an async function on the plugin's lifecycle methods, which allows us to lazy-load them, which in turn keeps them out of the page load bundle. It's a bit more work, and less elegant...but it's the best we've come up with so far. I'd suggest taking a similar approach here if we really need these components to be sharable.

do see any problems with sharing ChangePassword and PersonalInfo components with other plugins

I have some questions first -- they are probably silly questions, but I want to make sure I understand the problem we're trying to solve here.

1. Perhaps this is better suited for the product team, but why does this chromeless application lack the global Elastic header, which already includes the account nav? I was under the impression that this was more or less a permanent fixture, allowing folks to navigate throughout Elastic (Cloud Console, support portal, etc). Is the goal of this chromeless app to hide the rest of Kibana, or to hide the rest of Elastic?
2. What is the expected experience for our Cloud users? Our cloud users will be using SSO more and more in the future, and these components aren't all that useful unless you happen to authenticate via the native realm. Cloud effectively replaces our default "Profile" link to send users to their Cloud profile instead, since that is arguably more useful to them. Our components are still available to Cloud users, just under a different link. See Only add cloud-specific links for superusers #97870 for sample screenshots.

[yakhinvadim]

@legrego These are great questions, thanks for asking them!

Question 1

Perhaps this is better suited for the product team, but why does this chromeless application lack the global Elastic header, which already includes the account nav? I was under the impression that this was more or less a permanent fixture, allowing folks to navigate throughout Elastic (Cloud Console, support portal, etc). Is the goal of this chromeless app to hide the rest of Kibana, or to hide the rest of Elastic?

I'll copy the content from a product document that has a good explanation:

---

Summary

• Enterprise Search is dropping support for its standalone UIs with 8.0, and will rely exclusively on Kibana as its user interface for experience management.
• Workplace Search’s search user persona represents a new type of user for the Kibana ecosystem, and certain needs specific to this new user need to be addressed.
• A pared down version of Kibana is most appropriate for search users.

Defining the Search User persona

Workplace Search brings a unique persona to the Elastic ecosystem: the search (end-)user persona. Search users are non-operators, and their experience relies on the Stack and its operational functionality being entirely obfuscated and abstracted. Here are a few key concepts to understand:

• Search users are first-class users in the ecosystem, represented as Elasticsearch users, with specific roles (or role mappings) associated.
• Search users are not operators or administrators.
• Search users are assumed to be non-technical users, with no explicit knowledge of a work tool’s underlying technology layers.
• Search users are cognitively trained to use a particular product (in this case Workplace Search) and not a platform or technology (i.e. Kibana, Elastic or Elasticsearch).
• Search users require some level of personal account management from a distinct management area that is non-technical in nature. This includes consuming personal analytics and connecting private data sources.

EXAMPLE
Amanda, who contributes to the Customer Success team at Acme Co., has access to the Workplace Search experience where she can easily query the data she has access to using a standalone search interface. She uses the Workplace Search credentials that were provided to her by her IT department.

Amanda may also want to add Slack as a Content Source for her search experience, for which she requires a simple management experience. This management experience is powered by Kibana, but does not present the traditional operator-specific characteristics usually represented in Kibana.

Visually and aspirationally speaking, the experience for Amanda should look as follows at a high-level:

… whereas the experience for Catherine (who’s an admin) is aligned with the traditional Kibana experience:

---

To summarize: the Workplace Search personal dashboard application hides the global Elastic header because the search user most likely doesn't know about Elastic or Kibana. The goal of this chromeless app is to hide the rest of Elastic.

Hope this clarifies things a bit. Let me know if I could provide more detail.

Note: We might bring the Elastic header back somewhere in 8.x+, but for 7.x the decision was to hide it completely.

Question 2

What is the expected experience for our Cloud users? Our cloud users will be using SSO more and more in the future, and these components aren't all that useful unless you happen to authenticate via the native realm. Cloud effectively replaces our default "Profile" link to send users to their Cloud profile instead, since that is arguably more useful to them. Our components are still available to Cloud users, just under a different link. See #97870 for sample screenshots.

Good question. I don't have a definitive answer right now. I expect that we will either:

1. hide Workplace Search Account Settings and link to it completely for such users, or
2. show them a not that useful version of this page.

This will require a discussion with the product team.

For now not having a polished experience is expected and ok, because the Workplace Search Kibana plugin will be in beta until 8.0.

Bundle size increase

Thank you for the link! I'll look into loading components through an async function.

[legrego]

@yakhinvadim thanks so much for the context, that's very helpful. Overall I don't have any objections to the approach you've outlined above for reusing our components, so long as we can address the async loading to keep the bundle sizes small.

I'm satisfied with your answers to my first question, and I agree that we need a discussion with the product team about the second question.

Ping @alexfrancoeur since we worked together recently on the profile links for cloud users 🙂. Workplace Search is adding an inlined version of Kibana's account settings page within their personal dashboard view. This will work well for users in the native realm, but we don't know what the experience should look like for cloud users who come in via SSO. We can show these same controls for now, but they won't be all that useful. Also worth noting that this interface is beta for the remainder of 7.x, so we have some time to decide if we wish to leave it as-is and defer the decision.

[spalger]

@elastic/kibana-operations is there any recommended way to export public components from plugin A, so that plugin B can import them, but these components aren't included into main bundle of the plugin A?

The only real option is to export an async function from the root of plugin A and then use a return async import() in that async function, which you will need to call from B.

We could probably add the ability to specify an extraAsyncPluginDirs or something in kibana.json which would only be accessible with async import() statements, but it will still need to run the import in an async context somewhere so a loadDependencyFromPluginA() function works just as well in my eyes.

The holy grail would be to allow standard import statements in an async chunk of plugin B to load an async chunk from plugin A, but implementing that would take a good deal of time and doesn't seem to have enough demand to justify it right now.

[alexfrancoeur]

Ping @alexfrancoeur since we worked together recently on the profile links for cloud users 🙂. Workplace Search is adding an inlined version of Kibana's account settings page within their personal dashboard view. This will work well for users in the native realm, but we don't know what the experience should look like for cloud users who come in via SSO. We can show these same controls for now, but they won't be all that useful. Also worth noting that this interface is beta for the remainder of 7.x, so we have some time to decide if we wish to leave it as-is and defer the decision.

Thanks for the ping @legrego! For the time being, until we have a better user model for the Cloud / Stack, I'm wondering if we should be providing a similar experience that we do for Kibana. Do we have a sense of how many users will be coming from Cloud SSO? @jonasll @yakhinvadim Let me know if it'd be helpful to connect for 15 minutes and talk through what that experience is today.

[jonasll]

@alexfrancoeur It would be helpful.

To answer the question: I expect few users (not admins) to come from Cloud SSO or native. We expect most to come from organizational SSO experiences powered by AD, Okta and the likes.

[yakhinvadim]

@azasypkin This should be ready for re-review.

The first 4 commits in this PR only touch Security plugin files. Last 4 - only Enterprise Search plugin files. I will request a review from my team for Enterprise Search changes after you approve the changes in the Security plugin.

Major changes since last time

• Components PersonalInfo and ChangePassword are now available via the uiApi property in Security plugin lifecycle methods. The effect on the Security plugin's page load bundle size is minor: +2.8KB.
• The Security plugin is now optional in the Enterprise Search solution (consists of 2 products: App Search and Workplace Search):
  • If the Security plugin is enabled, the Workplace Search's Account Settings page asynchronously loads and shows the PersonalInfo and ChangePassword components.
  • If the Security plugin is disabled, only the App Search product is available. Workplace Search and the Account Settings page become unavailable. PersonalInfo and ChangePassword components are not loaded.

Implementation details

• I mostly copied the patterns from the Spaces plugin (see [Workplace Search] Add Account Settings page imported from Security plugin #99791 (comment)) with minimal changes for two reasons: 1) this works and has been approved before and 2) it should be easier to DRY out in the future if needed.

Screencast

Throttled to Fast 3G. Can be skipped to 0:15.

Screen.Cast.2021-06-02.at.11.43.11.AM.mp4

[azasypkin]

ACK: will review on Monday, Jun 7th at the latest.

[azasypkin]

Looks good, just left a few suggestions. I know you tried to replicate the lazy-loading code from the Spaces as much as possible, but I believe there is no reason to reproduce all the complexity we needed there, but may never need here.

What do you think?

[azasypkin]

note: It looks like we don't use uiApi in the setup yet, would you mind just calling getUiApi in the start instead? This way we don't need to persist it anywhere.

Also since we'll call it in the start, we can pass core: CoreStart directly instead of getStartServices.

[yakhinvadim]

Great suggestion! Updated here: 4f8f832

[azasypkin]

nit: would you mind exporting these props types from '../account_management/index.ts so that we can import them both from '../account_management' directly?

[yakhinvadim]

Update here: ebc2753. Could you confirm that this is what we want? I'm still a bit confused about what should be exported from components and what from index files.

[azasypkin]

suggestion: Since we switched to returning components from the start contract, it seems we don't need to export UserAPIClient anymore. We have everything to be able to instantiate it internally, see my comment in change_password_async.tsx.

Suggested change

	export interface UiApi {
	components: {
	getPersonalInfo: LazyComponentFn<PersonalInfoProps>;
	getChangePassword: LazyComponentFn<ChangePasswordProps>;
	};
	UserAPIClient: typeof UserAPIClient;
	}
	export interface UiApi {
	components: {
	getPersonalInfo: LazyComponentFn<PersonalInfoProps>;
	getChangePassword: LazyComponentFn<Pick<ChangePasswordProps, 'user'>>;
	};
	}

[yakhinvadim]

Removed it here: 706ac2e

[azasypkin]

nit: LazyWrapper has access to the CoreStart already, so we should be able to leverage it here. This would simplify API for our consumers since they'll only need to provide user and won't need to care about internal implementtion details like notifications and userAPIClient.

Suggested change

	export const getChangePasswordComponent = async (): Promise<React.FC<ChangePasswordProps>> => {
	const { ChangePassword } = await import('./change_password');
	return (props: ChangePasswordProps) => {
	return <ChangePassword {...props} />;
	};
	};
	export const getChangePasswordComponent = async (
	core: CoreStart
	): Promise<React.FC<Pick<ChangePasswordProps, 'user'>>> => {
	const { ChangePassword } = await import('./change_password');
	return (props: Pick<ChangePasswordProps, 'user'>) => {
	return (
	<ChangePassword
	notifications={core.notifications}
	userAPIClient={new UserAPIClient(core.http)}
	{...props}
	/>
	);
	};
	};

[yakhinvadim]

Was really happy it worked! See this commit: 9f18ea9

[yakhinvadim]

Thanks for the review, @azasypkin!
I agree with all the suggestions. I'll update the PR and ping you when it's ready for re-review.

[yakhinvadim]

@azasypkin This should be ready for re-review!

[azasypkin]

LGTM, just few more nits, thanks 👍

[azasypkin]

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)

[yakhinvadim]

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

[azasypkin]

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';

[yakhinvadim]

Got it, thanks! Updated here: 9b932dc

[azasypkin]

nit: (once you split this one in two interfaces)

Suggested change

	getChangePassword: LazyComponentFn<Pick<ChangePasswordProps, 'user'>>;
	getChangePassword: LazyComponentFn<ChangePasswordProps>;

[yakhinvadim]

Updated here: 2425821

[azasypkin]

nit: (once you propagate all required exports through relevant index.ts files)

Suggested change

	import { getChangePasswordComponent } from '../account_management/change_password/change_password_async';
	import { getPersonalInfoComponent } from '../account_management/personal_info/personal_info_async';
	import { getChangePasswordComponent, getPersonalInfoComponent } from '../account_management';

[yakhinvadim]

I tried to did it before, but it resulted in an increased bundle size.

I took the following info from security/target/public/metrics.json after running the first command from CI Metrics doc.

Before:

{
  "group": "page load bundle size",
  "id": "security",
  "value": 87166,
  "limit": 95864,
  "limitConfigPath": "packages/kbn-optimizer/limits.yml"
}

After:

{
  "group": "page load bundle size",
  "id": "security",
  "value": 101392,
  "limit": 95864,
  "limitConfigPath": "packages/kbn-optimizer/limits.yml"
}

I'm guessing that once async components are exported through the index file and we use that index file somewhere that also brings all the sync components' dependencies and increases bundle size.

I kept the imports as is, but let me know if you have more thoughts.

[azasypkin]

Oh, okay, let's keep it as is then. Can you please leave a brief comment for these imports so that readers can quickly understand why it's done this way?

[yakhinvadim]

Good idea, absolutely.

[yakhinvadim]

Added a comment here: 1880755

[azasypkin]

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

[yakhinvadim]

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.

[azasypkin]

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

[yakhinvadim]

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.

[azasypkin]

Sounds good to me 👍

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 1880755
• Storybooks Preview

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
enterpriseSearch	1418	1420	+2
security	475	482	+7
total			+9

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
enterpriseSearch	2.1MB	2.1MB	+1.7KB
security	766.7KB	782.3KB	+15.6KB
total			+17.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
security	5	6	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
security	82.5KB	85.3KB	+2.8KB

Unknown metric groups

API count

id	before	after	diff
security	109	110	+1

async chunk count

id	before	after	diff
security	21	23	+2

History

• 💚 Build #129846 succeeded 140cd91
• 💚 Build #128501 succeeded efa7d68
• 💔 Build #125561 failed 2c350dc
• 💔 Build #125333 failed 48d7bfa568a395f9d405d347d40c1fffdeaf623d
• 💔 Build #125314 failed 3c87b4b4a44a14c2478323c4e52aa00776e46b26

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[scottybollinger]

This is great work Vadim! Thanks for knocking it out 👍

[yakhinvadim]

Thanks to everyone and especially @azasypkin for helping with this! 🙌

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.