Skip to content

Commit

Permalink
Review#2: remove role related methods from UsersAPIClient, use more…
Browse files Browse the repository at this point in the history 
… appropriate naming format for the management app object names, fix feature catalogue link, properly handle role mapping name change in Edit Role Mapping page, properly encode parameters in breadcrumbs links.
  • Loading branch information
@azasypkin
azasypkin committed Jan 16, 2020
1 parent c352d13 commit 093b223
Show file tree
Hide file tree
Showing 20 changed files with 150 additions and 107 deletions.
20 changes: 10 additions & 10 deletions x-pack/plugins/security/public/management/api_keys/api_keys_management_app.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ jest.mock('./api_keys_grid', () => ({
APIKeysGridPage: (props: any) => `Page: ${JSON.stringify(props)}`,
}));

import { APIKeysManagementApp } from './api_keys_management_app';
import { apiKeysManagementApp } from './api_keys_management_app';
import { coreMock } from '../../../../../../src/core/public/mocks';

describe('APIKeysManagementApp', () => {
describe('apiKeysManagementApp', () => {
it('create() returns proper management app descriptor', () => {
const { getStartServices } = coreMock.createSetup();

expect(APIKeysManagementApp.create({ getStartServices: getStartServices as any }))
expect(apiKeysManagementApp.create({ getStartServices: getStartServices as any }))
.toMatchInlineSnapshot(`
Object {
"id": "api_keys",
Expand All @@ -31,13 +31,13 @@ describe('APIKeysManagementApp', () => {
const container = document.createElement('div');

const setBreadcrumbs = jest.fn();
const unmount = await APIKeysManagementApp.create({
getStartServices: getStartServices as any,
}).mount({
basePath: '/some-base-path',
element: container,
setBreadcrumbs,
});
const unmount = await apiKeysManagementApp
.create({ getStartServices: getStartServices as any })
.mount({
basePath: '/some-base-path',
element: container,
setBreadcrumbs,
});

expect(setBreadcrumbs).toHaveBeenCalledTimes(1);
expect(setBreadcrumbs).toHaveBeenCalledWith([{ href: '#/some-base-path', text: 'API Keys' }]);
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/public/management/api_keys/api_keys_management_app.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ interface CreateParams {
getStartServices: CoreSetup<PluginStartDependencies>['getStartServices'];
}

export const APIKeysManagementApp = Object.freeze({
export const apiKeysManagementApp = Object.freeze({
id: 'api_keys',
create({ getStartServices }: CreateParams) {
return {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/public/management/api_keys/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

export { APIKeysManagementApp } from './api_keys_management_app';
export { apiKeysManagementApp } from './api_keys_management_app';
20 changes: 10 additions & 10 deletions x-pack/plugins/security/public/management/management_service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import { BehaviorSubject } from 'rxjs';
import { ManagementApp } from '../../../../../src/plugins/management/public';
import { SecurityLicenseFeatures } from '../../common/licensing/license_features';
import { ManagementService } from './management_service';
import { UsersManagementApp } from './users';
import { usersManagementApp } from './users';

import { coreMock } from '../../../../../src/core/public/mocks';
import { licenseMock } from '../../common/licensing/index.mock';
import { securityMock } from '../mocks';
import { RolesManagementApp } from './roles';
import { APIKeysManagementApp } from './api_keys';
import { RoleMappingsManagementApp } from './role_mappings';
import { rolesManagementApp } from './roles';
import { apiKeysManagementApp } from './api_keys';
import { roleMappingsManagementApp } from './role_mappings';

describe('ManagementService', () => {
describe('setup()', () => {
Expand Down Expand Up @@ -119,10 +119,10 @@ describe('ManagementService', () => {
} as unknown) as jest.Mocked<ManagementApp>;
};
const mockApps = new Map<string, jest.Mocked<ManagementApp>>([
[UsersManagementApp.id, getMockedApp()],
[RolesManagementApp.id, getMockedApp()],
[APIKeysManagementApp.id, getMockedApp()],
[RoleMappingsManagementApp.id, getMockedApp()],
[usersManagementApp.id, getMockedApp()],
[rolesManagementApp.id, getMockedApp()],
[apiKeysManagementApp.id, getMockedApp()],
[roleMappingsManagementApp.id, getMockedApp()],
] as Array<[string, jest.Mocked<ManagementApp>]>);

service.start({
Expand Down Expand Up @@ -165,7 +165,7 @@ describe('ManagementService', () => {
it('disables only Role Mappings app if `showLinks` is `true`, but `showRoleMappingsManagement` is `false` at `start`', () => {
const { mockApps } = startService({ showLinks: true, showRoleMappingsManagement: false });
for (const [appId, mockApp] of mockApps) {
expect(mockApp.enabled).toBe(appId !== RoleMappingsManagementApp.id);
expect(mockApp.enabled).toBe(appId !== roleMappingsManagementApp.id);
}
});

Expand Down Expand Up @@ -197,7 +197,7 @@ describe('ManagementService', () => {
updateFeatures({ showLinks: true, showRoleMappingsManagement: false });

for (const [appId, mockApp] of mockApps) {
expect(mockApp.enabled).toBe(appId !== RoleMappingsManagementApp.id);
expect(mockApp.enabled).toBe(appId !== roleMappingsManagementApp.id);
}
});

Expand Down
39 changes: 12 additions & 27 deletions x-pack/plugins/security/public/management/management_service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ import {
import { SecurityLicense } from '../../common/licensing';
import { AuthenticationServiceSetup } from '../authentication';
import { PluginStartDependencies } from '../plugin';
import { APIKeysManagementApp } from './api_keys';
import { RoleMappingsManagementApp } from './role_mappings';
import { RolesManagementApp } from './roles';
import { UsersManagementApp } from './users';
import { apiKeysManagementApp } from './api_keys';
import { roleMappingsManagementApp } from './role_mappings';
import { rolesManagementApp } from './roles';
import { usersManagementApp } from './users';

interface SetupParams {
management: ManagementSetup;
Expand Down Expand Up @@ -48,24 +48,24 @@ export class ManagementService {
euiIconType: 'securityApp',
});

securitySection.registerApp(UsersManagementApp.create({ authc, getStartServices }));
securitySection.registerApp(usersManagementApp.create({ authc, getStartServices }));
securitySection.registerApp(
RolesManagementApp.create({ fatalErrors, license, getStartServices })
rolesManagementApp.create({ fatalErrors, license, getStartServices })
);
securitySection.registerApp(APIKeysManagementApp.create({ getStartServices }));
securitySection.registerApp(RoleMappingsManagementApp.create({ getStartServices }));
securitySection.registerApp(apiKeysManagementApp.create({ getStartServices }));
securitySection.registerApp(roleMappingsManagementApp.create({ getStartServices }));
}

start({ management }: StartParams) {
this.licenseFeaturesSubscription = this.license.features$.subscribe(async features => {
const securitySection = management.sections.getSection('security')!;

const securityManagementAppsStatuses: Array<[ManagementApp, boolean]> = [
[securitySection.getApp(UsersManagementApp.id)!, features.showLinks],
[securitySection.getApp(RolesManagementApp.id)!, features.showLinks],
[securitySection.getApp(APIKeysManagementApp.id)!, features.showLinks],
[securitySection.getApp(usersManagementApp.id)!, features.showLinks],
[securitySection.getApp(rolesManagementApp.id)!, features.showLinks],
[securitySection.getApp(apiKeysManagementApp.id)!, features.showLinks],
[
securitySection.getApp(RoleMappingsManagementApp.id)!,
securitySection.getApp(roleMappingsManagementApp.id)!,
features.showLinks && features.showRoleMappingsManagement,
],
];
Expand All @@ -92,19 +92,4 @@ export class ManagementService {
this.licenseFeaturesSubscription = undefined;
}
}

// TODO: DO WE STILL NEED THIS?
/* private checkLicense({
management,
notifications,
}: Pick<StartParams, 'management' | 'notifications'>) {
const { showLinks, linksMessage } = this.license.getFeatures();
if (!showLinks) {
notifications.toasts.addDanger({ title: linksMessage });
management.sections.navigateToApp('management');
return false;
}
return true;
}*/
}
6 changes: 6 additions & 0 deletions ...ins/security/public/management/role_mappings/edit_role_mapping/edit_role_mapping_page.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ export class EditRoleMappingPage extends Component<Props, State> {
this.loadAppData();
}

public async componentDidUpdate(prevProps: Props) {
if (prevProps.name !== this.props.name) {
await this.loadAppData();
}
}

public render() {
const { loadState } = this.state;

Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/public/management/role_mappings/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

export { RoleMappingsManagementApp } from './role_mappings_management_app';
export { roleMappingsManagementApp } from './role_mappings_management_app';
30 changes: 24 additions & 6 deletions ...ck/plugins/security/public/management/role_mappings/role_mappings_management_app.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,25 @@ jest.mock('./edit_role_mapping', () => ({
EditRoleMappingPage: (props: any) => `Role Mapping Edit Page: ${JSON.stringify(props)}`,
}));

import { RoleMappingsManagementApp } from './role_mappings_management_app';
import { roleMappingsManagementApp } from './role_mappings_management_app';

import { coreMock } from '../../../../../../src/core/public/mocks';

async function mountApp(basePath: string) {
const container = document.createElement('div');
const setBreadcrumbs = jest.fn();

const unmount = await RoleMappingsManagementApp.create({
getStartServices: coreMock.createSetup().getStartServices as any,
}).mount({ basePath, element: container, setBreadcrumbs });
const unmount = await roleMappingsManagementApp
.create({ getStartServices: coreMock.createSetup().getStartServices as any })
.mount({ basePath, element: container, setBreadcrumbs });

return { unmount, container, setBreadcrumbs };
}

describe('RoleMappingsManagementApp', () => {
describe('roleMappingsManagementApp', () => {
it('create() returns proper management app descriptor', () => {
expect(
RoleMappingsManagementApp.create({
roleMappingsManagementApp.create({
getStartServices: coreMock.createSetup().getStartServices as any,
})
).toMatchInlineSnapshot(`
Expand Down Expand Up @@ -106,4 +106,22 @@ describe('RoleMappingsManagementApp', () => {

expect(container).toMatchInlineSnapshot(`<div />`);
});

it('mount() properly encodes role mapping name in `edit role mapping` page link in breadcrumbs', async () => {
const basePath = '/some-base-path/role_mappings';
const roleMappingName = 'some 安全性 role mapping';
window.location.hash = `${basePath}/edit/${roleMappingName}`;

const { setBreadcrumbs } = await mountApp(basePath);

expect(setBreadcrumbs).toHaveBeenCalledTimes(1);
expect(setBreadcrumbs).toHaveBeenCalledWith([
{ href: `#${basePath}`, text: 'Role Mappings' },
{
href:
'#/some-base-path/role_mappings/edit/some%20%E5%AE%89%E5%85%A8%E6%80%A7%20role%20mapping',
text: roleMappingName,
},
]);
});
});
4 changes: 2 additions & 2 deletions x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ interface CreateParams {
getStartServices: CoreSetup<PluginStartDependencies>['getStartServices'];
}

export const RoleMappingsManagementApp = Object.freeze({
export const roleMappingsManagementApp = Object.freeze({
id: 'role_mappings',
create({ getStartServices }: CreateParams) {
return {
Expand Down Expand Up @@ -60,7 +60,7 @@ export const RoleMappingsManagementApp = Object.freeze({
setBreadcrumbs([
...roleMappingsBreadcrumbs,
name
? { text: name, href: `#${basePath}/edit/${name}` }
? { text: name, href: `#${basePath}/edit/${encodeURIComponent(name)}` }
: {
text: i18n.translate('xpack.security.roleMappings.createBreadcrumb', {
defaultMessage: 'Create',
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/public/management/roles/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
* you may not use this file except in compliance with the Elastic License.
*/

export { RolesManagementApp } from './roles_management_app';
export { rolesManagementApp } from './roles_management_app';
export { RolesAPIClient } from './roles_api_client';
35 changes: 27 additions & 8 deletions x-pack/plugins/security/public/management/roles/roles_management_app.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jest.mock('./edit_role', () => ({
EditRolePage: (props: any) => `Role Edit Page: ${JSON.stringify(props)}`,
}));

import { RolesManagementApp } from './roles_management_app';
import { rolesManagementApp } from './roles_management_app';

import { coreMock } from '../../../../../../src/core/public/mocks';

Expand All @@ -23,21 +23,23 @@ async function mountApp(basePath: string) {
const container = document.createElement('div');
const setBreadcrumbs = jest.fn();

const unmount = await RolesManagementApp.create({
license: licenseMock.create(),
fatalErrors,
getStartServices: jest.fn().mockResolvedValue([coreMock.createStart(), { data: {} }]),
}).mount({ basePath, element: container, setBreadcrumbs });
const unmount = await rolesManagementApp
.create({
license: licenseMock.create(),
fatalErrors,
getStartServices: jest.fn().mockResolvedValue([coreMock.createStart(), { data: {} }]),
})
.mount({ basePath, element: container, setBreadcrumbs });

return { unmount, container, setBreadcrumbs };
}

describe('RolesManagementApp', () => {
describe('rolesManagementApp', () => {
it('create() returns proper management app descriptor', () => {
const { fatalErrors, getStartServices } = coreMock.createSetup();

expect(
RolesManagementApp.create({
rolesManagementApp.create({
license: licenseMock.create(),
fatalErrors,
getStartServices: getStartServices as any,
Expand Down Expand Up @@ -138,4 +140,21 @@ describe('RolesManagementApp', () => {

expect(container).toMatchInlineSnapshot(`<div />`);
});

it('mount() properly encodes role name in `edit role` page link in breadcrumbs', async () => {
const basePath = '/some-base-path/roles';
const roleName = 'some 安全性 role';
window.location.hash = `${basePath}/edit/${roleName}`;

const { setBreadcrumbs } = await mountApp(basePath);

expect(setBreadcrumbs).toHaveBeenCalledTimes(1);
expect(setBreadcrumbs).toHaveBeenCalledWith([
{ href: `#${basePath}`, text: 'Roles' },
{
href: '#/some-base-path/roles/edit/some%20%E5%AE%89%E5%85%A8%E6%80%A7%20role',
text: roleName,
},
]);
});
});
4 changes: 2 additions & 2 deletions x-pack/plugins/security/public/management/roles/roles_management_app.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ interface CreateParams {
getStartServices: CoreSetup<PluginStartDependencies>['getStartServices'];
}

export const RolesManagementApp = Object.freeze({
export const rolesManagementApp = Object.freeze({
id: 'roles',
create({ license, fatalErrors, getStartServices }: CreateParams) {
return {
Expand Down Expand Up @@ -58,7 +58,7 @@ export const RolesManagementApp = Object.freeze({
setBreadcrumbs([
...rolesBreadcrumbs,
action === 'edit' && roleName
? { text: roleName, href: `#${basePath}/edit/${roleName}` }
? { text: roleName, href: `#${basePath}/edit/${encodeURIComponent(roleName)}` }
: {
text: i18n.translate('xpack.security.roles.createBreadcrumb', {
defaultMessage: 'Create',
Expand Down
Loading

0 comments on commit 093b223

Please sign in to comment.