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

Adding check to show/hide Avatar form based on whether the user is a … #135743

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9291b07
Adding check to show/hide Avatar form based on whether the user is a …
kc13greiner Jul 5, 2022
044fae9
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kc13greiner Jul 5, 2022
60f7c0c
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kibanamachine Jul 5, 2022
b668d7f
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kc13greiner Jul 5, 2022
5457cc7
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kc13greiner Jul 5, 2022
8df287d
Adding tests to verify the avatar doesnt show up in the UserProfile i…
kc13greiner Jul 6, 2022
45d16fc
Changing the name of the link to 'Edit Profile' and making it availab…
kc13greiner Jul 7, 2022
af6af23
Adding/Updating unit tests and fixing translation files
kc13greiner Jul 7, 2022
b32c578
Removing unused values from FormattedText and related tests
kc13greiner Jul 7, 2022
af16260
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kc13greiner Jul 7, 2022
662482c
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kc13greiner Jul 8, 2022
bc54331
Updating unit test to work with merge from main
kc13greiner Jul 8, 2022
20adf25
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kc13greiner Jul 8, 2022
a3dd8c4
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kibanamachine Jul 8, 2022
1067020
Updating link to read Edit Profile to match wireframes per PR review
kc13greiner Jul 11, 2022
43a12fe
Reverting changes to translation files and changing the cloud Edit Pr…
kc13greiner Jul 12, 2022
6b12199
Changing capitalization of 'profile' so it follows the naming convention
kc13greiner Jul 12, 2022
9bfd590
Changing nav menu logic to only render the default Edit Profile link …
kc13greiner Jul 18, 2022
b327af7
Merge branch 'main' into feature/hide_avatar_form_for_cloud_user
kc13greiner Jul 18, 2022
e3afd79
Updating logic to display custom nav links if user is a cloud user an…
kc13greiner Jul 19, 2022
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
23 changes: 12 additions & 11 deletions x-pack/plugins/cloud/public/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ describe('Cloud Plugin', () => {
const securityStart = securityMock.createStart();
securityStart.authc.getCurrentUser.mockResolvedValue(
securityMock.createMockAuthenticatedUser({
roles: ['superuser'],
elastic_cloud_user: true,
})
);

Expand All @@ -446,14 +446,15 @@ describe('Cloud Plugin', () => {
expect(securityStart.authc.getCurrentUser).not.toHaveBeenCalled();
});

it('registers a custom nav link for superusers', async () => {
it('registers a custom nav link for cloud users', async () => {
const { plugin } = startPlugin();

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();

securityStart.authc.getCurrentUser.mockResolvedValue(
securityMock.createMockAuthenticatedUser({
roles: ['superuser'],
elastic_cloud_user: true,
})
);
plugin.start(coreStart, { security: securityStart });
Expand Down Expand Up @@ -494,14 +495,14 @@ describe('Cloud Plugin', () => {
`);
});

it('does not register a custom nav link for non-superusers', async () => {
it('does not register a custom nav link for non-cloud users', async () => {
const { plugin } = startPlugin();

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
securityStart.authc.getCurrentUser.mockResolvedValue(
securityMock.createMockAuthenticatedUser({
roles: ['not-a-superuser'],
elastic_cloud_user: false,
})
);
plugin.start(coreStart, { security: securityStart });
Expand All @@ -511,14 +512,14 @@ describe('Cloud Plugin', () => {
expect(coreStart.chrome.setCustomNavLink).not.toHaveBeenCalled();
});

it('registers user profile links for superusers', async () => {
it('registers user profile links for cloud users', async () => {
const { plugin } = startPlugin();

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
securityStart.authc.getCurrentUser.mockResolvedValue(
securityMock.createMockAuthenticatedUser({
roles: ['superuser'],
elastic_cloud_user: true,
})
);
plugin.start(coreStart, { security: securityStart });
Expand All @@ -532,7 +533,7 @@ describe('Cloud Plugin', () => {
Object {
"href": "https://cloud.elastic.co/profile/alice",
"iconType": "user",
"label": "Profile",
"label": "Edit profile",
"order": 100,
"setAsProfile": true,
},
Expand Down Expand Up @@ -564,7 +565,7 @@ describe('Cloud Plugin', () => {
Object {
"href": "https://cloud.elastic.co/profile/alice",
"iconType": "user",
"label": "Profile",
"label": "Edit profile",
"order": 100,
"setAsProfile": true,
},
Expand All @@ -579,14 +580,14 @@ describe('Cloud Plugin', () => {
`);
});

it('does not register profile links for non-superusers', async () => {
it('does not register profile links for non-cloud users', async () => {
const { plugin } = startPlugin();

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
securityStart.authc.getCurrentUser.mockResolvedValue(
securityMock.createMockAuthenticatedUser({
roles: ['not-a-superuser'],
elastic_cloud_user: false,
})
);
plugin.start(coreStart, { security: securityStart });
Expand Down
6 changes: 4 additions & 2 deletions x-pack/plugins/cloud/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,12 @@ export class CloudPlugin implements Plugin<CloudSetup> {
}
// Security plugin is disabled
if (!security) return true;
// Otherwise check roles. If user is not defined due to an unexpected error, then fail *open*.

// Otherwise check if user is a cloud user.
// If user is not defined due to an unexpected error, then fail *open*.
// Cloud admin console will always perform the actual authorization checks.
const user = await security.authc.getCurrentUser().catch(() => null);
return user?.roles.includes('superuser') ?? true;
return user?.elastic_cloud_user ?? true;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/cloud/public/user_menu_links.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export const createUserMenuLinks = (config: CloudConfigType): UserMenuLink[] =>
if (baseUrl && profileUrl) {
userMenuLinks.push({
label: i18n.translate('xpack.cloud.userMenuLinks.profileLinkText', {
defaultMessage: 'Profile',
defaultMessage: 'Edit profile',
}),
iconType: 'user',
href: getFullCloudUrl(baseUrl, profileUrl),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import { act, renderHook } from '@testing-library/react-hooks';
import { mount } from 'enzyme';
import type { FunctionComponent } from 'react';
import React from 'react';

Expand All @@ -14,10 +15,11 @@ import { coreMock, scopedHistoryMock, themeServiceMock } from '@kbn/core/public/
import { UserProfileAPIClient } from '..';
import type { UserData } from '../../../common';
import { mockAuthenticatedUser } from '../../../common/model/authenticated_user.mock';
import { UserAvatar } from '../../components';
import { UserAPIClient } from '../../management';
import { securityMock } from '../../mocks';
import { Providers } from '../account_management_app';
import { useUserProfileForm } from './user_profile';
import { UserProfile, useUserProfileForm } from './user_profile';

const user = mockAuthenticatedUser();
const coreStart = coreMock.createStart();
Expand Down Expand Up @@ -181,4 +183,52 @@ describe('useUserProfileForm', () => {

expect(result.current.initialValues.user.full_name).toEqual('Another Name');
});

describe('User Avatar Form', () => {
it('should display if the User is not a cloud user', () => {
const data: UserData = {};

const nonCloudUser = mockAuthenticatedUser({ elastic_cloud_user: false });

const testWrapper = mount(
<Providers
services={coreStart}
theme$={theme$}
history={history}
authc={authc}
securityApiClients={{
userProfiles: new UserProfileAPIClient(coreStart.http),
users: new UserAPIClient(coreStart.http),
}}
>
<UserProfile user={nonCloudUser} data={data} />
</Providers>
);

expect(testWrapper.exists(UserAvatar)).toBeTruthy();
});

it('should not display if the User is a cloud user', () => {
const data: UserData = {};

const cloudUser = mockAuthenticatedUser({ elastic_cloud_user: true });

const testWrapper = mount(
<Providers
services={coreStart}
theme$={theme$}
history={history}
authc={authc}
securityApiClients={{
userProfiles: new UserProfileAPIClient(coreStart.http),
users: new UserAPIClient(coreStart.http),
}}
>
<UserProfile user={cloudUser} data={data} />
</Providers>
);

expect(testWrapper.exists(UserAvatar)).toBeFalsy();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,8 @@ export const UserProfile: FunctionComponent<UserProfileProps> = ({ user, data })

const canChangeDetails = canUserChangeDetails(user, services.application.capabilities);

const isCloudUser = user.elastic_cloud_user;

const rightSideItems = [
{
title: (
Expand Down Expand Up @@ -559,7 +561,7 @@ export const UserProfile: FunctionComponent<UserProfileProps> = ({ user, data })
>
<Form aria-labelledby={titleId}>
<UserDetailsEditor user={user} />
<UserAvatarEditor user={user} formik={formik} />
{isCloudUser ? null : <UserAvatarEditor user={user} formik={formik} />}
<UserPasswordEditor
user={user}
onShowPasswordForm={() => setShowChangePasswordForm(true)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,10 @@ describe('SecurityNavControl', () => {
expect(wrapper.prop<boolean>('isOpen')).toEqual(false);
});

it('should render additional user menu links registered by other plugins', async () => {
it('should render additional user menu links registered by other plugins and should render the default Edit Profile link as the first link when no custom profile link is provided', async () => {
const wrapper = shallow(
<SecurityNavControl
editProfileUrl=""
editProfileUrl="edit-profile-link"
logoutUrl=""
userMenuLinks$={
new BehaviorSubject([
Expand All @@ -195,19 +195,15 @@ describe('SecurityNavControl', () => {
"items": Array [
Object {
"data-test-subj": "profileLink",
"href": "",
"href": "edit-profile-link",
"icon": <EuiIcon
size="m"
type="user"
/>,
"name": <FormattedMessage
defaultMessage="{profileOverridden, select, true{Preferences} other{Profile}}"
defaultMessage="Edit profile"
id="xpack.security.navControlComponent.editProfileLinkText"
values={
Object {
"profileOverridden": false,
}
}
values={Object {}}
/>,
"onClick": [Function],
},
Expand Down Expand Up @@ -258,10 +254,10 @@ describe('SecurityNavControl', () => {
`);
});

it('should render custom profile link registered by other plugins', async () => {
it('should render custom profile link registered by other plugins and not render default Edit Profile link', async () => {
const wrapper = shallow(
<SecurityNavControl
editProfileUrl=""
editProfileUrl="edit-profile-link"
logoutUrl=""
userMenuLinks$={
new BehaviorSubject([
Expand Down Expand Up @@ -311,24 +307,6 @@ describe('SecurityNavControl', () => {
/>,
"name": "link3",
},
Object {
"data-test-subj": "profileLink",
"href": "",
"icon": <EuiIcon
size="m"
type="controlsHorizontal"
/>,
"name": <FormattedMessage
defaultMessage="{profileOverridden, select, true{Preferences} other{Profile}}"
id="xpack.security.navControlComponent.editProfileLinkText"
values={
Object {
"profileOverridden": true,
}
}
/>,
"onClick": [Function],
},
Object {
"data-test-subj": "logoutLink",
"href": "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ export const SecurityNavControl: FunctionComponent<SecurityNavControlProps> = ({
</EuiHeaderSectionItemButton>
);

const isAnonymous = currentUser.value ? isUserAnonymous(currentUser.value) : false;
const items: EuiContextMenuPanelItemDescriptor[] = [];
if (userMenuLinks.length) {
const userMenuLinkMenuItems = userMenuLinks
Expand All @@ -93,17 +92,18 @@ export const SecurityNavControl: FunctionComponent<SecurityNavControlProps> = ({
items.push(...userMenuLinkMenuItems);
}

if (!isAnonymous) {
const hasCustomProfileLinks = userMenuLinks.some(({ setAsProfile }) => setAsProfile === true);
const isAnonymous = currentUser.value ? isUserAnonymous(currentUser.value) : false;
const hasCustomProfileLinks = userMenuLinks.some(({ setAsProfile }) => setAsProfile === true);

if (!isAnonymous && !hasCustomProfileLinks) {
Copy link
Member

Choose a reason for hiding this comment

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

issue: while testing this change I discovered that we rely on the user's roles (specifically superuser role) to determine whether we should show Cloud specific links or not (added in #97870). If we keep this Cloud plugin logic as is, after this PR non-Cloud managed superusers won't be able to edit their profiles in Kibana (we'll be rendering a single Edit profile link leading to Cloud UI instead of Profile and Preferences links we had previously).

@legrego do I understand correctly that we can now switch this code to rely on elastic_cloud_user user property instead or is there still a legitimate reason for non-Cloud managed superusers to see Cloud specific links?

Copy link
Member

Choose a reason for hiding this comment

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

@legrego do I understand correctly that we can now switch this code to rely on elastic_cloud_user user property instead or is there still a legitimate reason for non-Cloud managed superusers to see Cloud specific links?

That's correct, we should switch this code to rely on elastic_cloud_user instead of the role check. The superuser check was a hard-coded workaround because we didn't have a way to reliably determine which users were cloud users back then.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks for confirming, Larry.

const profileMenuItem: EuiContextMenuPanelItemDescriptor = {
name: (
<FormattedMessage
id="xpack.security.navControlComponent.editProfileLinkText"
defaultMessage="{profileOverridden, select, true{Preferences} other{Profile}}"
values={{ profileOverridden: hasCustomProfileLinks }}
defaultMessage="Edit profile"
/>
),
icon: <EuiIcon type={hasCustomProfileLinks ? 'controlsHorizontal' : 'user'} size="m" />,
icon: <EuiIcon type="user" size="m" />,
href: editProfileUrl,
onClick: () => {
setIsPopoverOpen(false);
Expand All @@ -112,11 +112,7 @@ export const SecurityNavControl: FunctionComponent<SecurityNavControlProps> = ({
};

// Set this as the first link if there is no user-defined profile link
if (!hasCustomProfileLinks) {
items.unshift(profileMenuItem);
} else {
items.push(profileMenuItem);
}
items.unshift(profileMenuItem);
}

items.push({
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/fr-FR.json
Original file line number Diff line number Diff line change
Expand Up @@ -24155,7 +24155,6 @@
"xpack.security.management.users.usersTitle": "Utilisateurs",
"xpack.security.management.usersTitle": "Utilisateurs",
"xpack.security.navControlComponent.accountMenuAriaLabel": "Menu Compte",
"xpack.security.navControlComponent.editProfileLinkText": "{profileOverridden, select, true{Préférences} other{Profil}}",
"xpack.security.navControlComponent.loginLinkText": "Connexion",
"xpack.security.navControlComponent.logoutLinkText": "Déconnexion",
"xpack.security.overwrittenSession.continueAsUserText": "Continuer en tant que {username}",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -24140,7 +24140,6 @@
"xpack.security.management.users.usersTitle": "ユーザー",
"xpack.security.management.usersTitle": "ユーザー",
"xpack.security.navControlComponent.accountMenuAriaLabel": "アカウントメニュー",
"xpack.security.navControlComponent.editProfileLinkText": "{profileOverridden, select, true{設定} other{プロファイル}}",
"xpack.security.navControlComponent.loginLinkText": "ログイン",
"xpack.security.navControlComponent.logoutLinkText": "ログアウト",
"xpack.security.overwrittenSession.continueAsUserText": "{username} として続行",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -24166,7 +24166,6 @@
"xpack.security.management.users.usersTitle": "用户",
"xpack.security.management.usersTitle": "用户",
"xpack.security.navControlComponent.accountMenuAriaLabel": "帐户菜单",
"xpack.security.navControlComponent.editProfileLinkText": "{profileOverridden, select, true{首选项} other{配置文件}}",
"xpack.security.navControlComponent.loginLinkText": "登录",
"xpack.security.navControlComponent.logoutLinkText": "注销",
"xpack.security.overwrittenSession.continueAsUserText": "作为 {username} 继续",
Expand Down