Skip to content

Commit

Permalink
Do not use slashes in app IDs (these are forbidden) and remove /app
Browse files Browse the repository at this point in the history
… prefix for view route (with `appRoute`) that are rendered by dedicated server routes.
  • Loading branch information
azasypkin committed Mar 3, 2020
1 parent db9c2cd commit 321a4cf
Show file tree
Hide file tree
Showing 29 changed files with 67 additions and 75 deletions.
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/security/public/hacks/legacy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const securityPluginSetup = (npSetup.plugins as any).security as SecurityPluginS
if (securityPluginSetup) {
routes.when('/account', {
template: '<div />',
controller: () => npStart.core.application.navigateToApp('security/account'),
controller: () => npStart.core.application.navigateToApp('security_account'),
});

const getNextParameter = () => {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/xpack_main/public/services/path.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export const Path = {
return (
path === '/login' ||
path === '/logout' ||
path === '/app/security/logged_out' ||
path === '/security/logged_out' ||
path === '/status'
);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ describe('accountManagementApp', () => {

const [[appRegistration]] = coreSetupMock.application.register.mock.calls;
expect(appRegistration).toEqual({
id: 'security/account',
appRoute: '/app/security/account',
id: 'security_account',
appRoute: '/security/account',
navLinkStatus: AppNavLinkStatus.hidden,
title: 'Account Management',
mount: expect.any(Function),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ interface CreateDeps {
}

export const accountManagementApp = Object.freeze({
id: 'security/account',
id: 'security_account',
create({ application, authc, getStartServices }: CreateDeps) {
const title = i18n.translate('xpack.security.account.breadcrumb', {
defaultMessage: 'Account Management',
Expand All @@ -26,7 +26,7 @@ export const accountManagementApp = Object.freeze({
title,
// TODO: switch to proper enum once https://github.com/elastic/kibana/issues/58327 is resolved.
navLinkStatus: 3,
appRoute: '/app/security/account',
appRoute: '/security/account',
async mount({ element }: AppMountParameters) {
const [[coreStart], { renderAccountManagementPage }] = await Promise.all([
getStartServices(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,15 @@ describe('loggedOutApp', () => {
loggedOutApp.create(coreSetupMock);

expect(coreSetupMock.http.anonymousPaths.register).toHaveBeenCalledTimes(1);
expect(coreSetupMock.http.anonymousPaths.register).toHaveBeenCalledWith(
'/app/security/logged_out'
);
expect(coreSetupMock.http.anonymousPaths.register).toHaveBeenCalledWith('/security/logged_out');

expect(coreSetupMock.application.register).toHaveBeenCalledTimes(1);

const [[appRegistration]] = coreSetupMock.application.register.mock.calls;
expect(appRegistration).toEqual({
id: 'security/logged_out',
id: 'security_logged_out',
chromeless: true,
appRoute: '/app/security/logged_out',
appRoute: '/security/logged_out',
title: 'Logged out',
mount: expect.any(Function),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ interface CreateDeps {
}

export const loggedOutApp = Object.freeze({
id: 'security/logged_out',
id: 'security_logged_out',
create({ application, http, getStartServices }: CreateDeps) {
http.anonymousPaths.register('/app/security/logged_out');
http.anonymousPaths.register('/security/logged_out');
application.register({
id: this.id,
title: i18n.translate('xpack.security.loggedOutAppTitle', { defaultMessage: 'Logged out' }),
chromeless: true,
appRoute: '/app/security/logged_out',
appRoute: '/security/logged_out',
async mount({ element }: AppMountParameters) {
const [[coreStart], { renderLoggedOutPage }] = await Promise.all([
getStartServices(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('loginApp', () => {

const [[appRegistration]] = coreSetupMock.application.register.mock.calls;
expect(appRegistration).toEqual({
id: 'security/login',
id: 'security_login',
chromeless: true,
appRoute: '/login',
title: 'Login',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ interface CreateDeps {
}

export const loginApp = Object.freeze({
id: 'security/login',
id: 'security_login',
create({ application, http, getStartServices, config }: CreateDeps) {
http.anonymousPaths.register('/login');
application.register({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('logoutApp', () => {

const [[appRegistration]] = coreSetupMock.application.register.mock.calls;
expect(appRegistration).toEqual({
id: 'security/logout',
id: 'security_logout',
chromeless: true,
appRoute: '/logout',
title: 'Logout',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ interface CreateDeps {
}

export const logoutApp = Object.freeze({
id: 'security/logout',
id: 'security_logout',
create({ application, http }: CreateDeps) {
http.anonymousPaths.register('/logout');
application.register({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ describe('overwrittenSessionApp', () => {

const [[appRegistration]] = coreSetupMock.application.register.mock.calls;
expect(appRegistration).toEqual({
id: 'security/overwritten_session',
id: 'security_overwritten_session',
title: 'Overwritten Session',
chromeless: true,
appRoute: '/app/security/overwritten_session',
appRoute: '/security/overwritten_session',
mount: expect.any(Function),
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ interface CreateDeps {
}

export const overwrittenSessionApp = Object.freeze({
id: 'security/overwritten_session',
id: 'security_overwritten_session',
create({ application, authc, getStartServices }: CreateDeps) {
application.register({
id: this.id,
title: i18n.translate('xpack.security.overwrittenSessionAppTitle', {
defaultMessage: 'Overwritten Session',
}),
chromeless: true,
appRoute: '/app/security/overwritten_session',
appRoute: '/security/overwritten_session',
async mount({ element }: AppMountParameters) {
const [[coreStart], { renderOverwrittenSessionPage }] = await Promise.all([
getStartServices(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export class SecurityNavControlService {

const props = {
user: currentUserPromise,
editProfileUrl: core.http.basePath.prepend('/app/security/account'),
editProfileUrl: core.http.basePath.prepend('/security/account'),
logoutUrl: this.logoutUrl,
};
ReactDOM.render(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ describe('KerberosAuthenticationProvider', () => {
mockOptions.tokens.invalidate.mockResolvedValue(undefined);

await expect(provider.logout(request, tokenPair)).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
}

return DeauthenticationResult.redirectTo(
`${this.options.basePath.serverBasePath}/app/security/logged_out`
`${this.options.basePath.serverBasePath}/security/logged_out`
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ describe('OIDCAuthenticationProvider', () => {
mockOptions.client.callAsInternalUser.mockResolvedValue({ redirect: null });

await expect(provider.logout(request, { accessToken, refreshToken })).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider {
}

return DeauthenticationResult.redirectTo(
`${this.options.basePath.serverBasePath}/app/security/logged_out`
`${this.options.basePath.serverBasePath}/security/logged_out`
);
} catch (err) {
this.logger.debug(`Failed to deauthenticate user: ${err.message}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ describe('PKIAuthenticationProvider', () => {
mockOptions.tokens.invalidate.mockResolvedValue(undefined);

await expect(provider.logout(request, state)).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class PKIAuthenticationProvider extends BaseAuthenticationProvider {
}

return DeauthenticationResult.redirectTo(
`${this.options.basePath.serverBasePath}/app/security/logged_out`
`${this.options.basePath.serverBasePath}/security/logged_out`
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,16 +365,13 @@ describe('SAMLAuthenticationProvider', () => {
state
)
).resolves.toEqual(
AuthenticationResult.redirectTo(
'/mock-server-basepath/app/security/overwritten_session',
{
state: {
username: 'new-user',
accessToken: 'new-valid-token',
refreshToken: 'new-valid-refresh-token',
},
}
)
AuthenticationResult.redirectTo('/mock-server-basepath/security/overwritten_session', {
state: {
username: 'new-user',
accessToken: 'new-valid-token',
refreshToken: 'new-valid-refresh-token',
},
})
);

expectAuthenticateCall(mockOptions.client, { headers: { authorization } });
Expand Down Expand Up @@ -962,7 +959,7 @@ describe('SAMLAuthenticationProvider', () => {
});
});

it('redirects to /app/security/logged_out if `redirect` field in SAML logout response is null.', async () => {
it('redirects to /security/logged_out if `redirect` field in SAML logout response is null.', async () => {
const request = httpServerMock.createKibanaRequest();
const accessToken = 'x-saml-token';
const refreshToken = 'x-saml-refresh-token';
Expand All @@ -972,7 +969,7 @@ describe('SAMLAuthenticationProvider', () => {
await expect(
provider.logout(request, { username: 'user', accessToken, refreshToken })
).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
Expand All @@ -981,7 +978,7 @@ describe('SAMLAuthenticationProvider', () => {
});
});

it('redirects to /app/security/logged_out if `redirect` field in SAML logout response is not defined.', async () => {
it('redirects to /security/logged_out if `redirect` field in SAML logout response is not defined.', async () => {
const request = httpServerMock.createKibanaRequest();
const accessToken = 'x-saml-token';
const refreshToken = 'x-saml-refresh-token';
Expand All @@ -991,7 +988,7 @@ describe('SAMLAuthenticationProvider', () => {
await expect(
provider.logout(request, { username: 'user', accessToken, refreshToken })
).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
Expand All @@ -1012,7 +1009,7 @@ describe('SAMLAuthenticationProvider', () => {
await expect(
provider.logout(request, { username: 'user', accessToken, refreshToken })
).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
Expand All @@ -1033,7 +1030,7 @@ describe('SAMLAuthenticationProvider', () => {
refreshToken: 'x-saml-refresh-token',
})
).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
Expand All @@ -1042,13 +1039,13 @@ describe('SAMLAuthenticationProvider', () => {
});
});

it('redirects to /app/security/logged_out if `redirect` field in SAML invalidate response is null.', async () => {
it('redirects to /security/logged_out if `redirect` field in SAML invalidate response is null.', async () => {
const request = httpServerMock.createKibanaRequest({ query: { SAMLRequest: 'xxx yyy' } });

mockOptions.client.callAsInternalUser.mockResolvedValue({ redirect: null });

await expect(provider.logout(request)).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
Expand All @@ -1057,13 +1054,13 @@ describe('SAMLAuthenticationProvider', () => {
});
});

it('redirects to /app/security/logged_out if `redirect` field in SAML invalidate response is not defined.', async () => {
it('redirects to /security/logged_out if `redirect` field in SAML invalidate response is not defined.', async () => {
const request = httpServerMock.createKibanaRequest({ query: { SAMLRequest: 'xxx yyy' } });

mockOptions.client.callAsInternalUser.mockResolvedValue({ redirect: undefined });

await expect(provider.logout(request)).resolves.toEqual(
DeauthenticationResult.redirectTo('/mock-server-basepath/app/security/logged_out')
DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
);

expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider {
}

return DeauthenticationResult.redirectTo(
`${this.options.basePath.serverBasePath}/app/security/logged_out`
`${this.options.basePath.serverBasePath}/security/logged_out`
);
} catch (err) {
this.logger.debug(`Failed to deauthenticate user: ${err.message}`);
Expand Down Expand Up @@ -366,7 +366,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider {
'Login initiated by Identity Provider is for a different user than currently authenticated.'
);
return AuthenticationResult.redirectTo(
`${this.options.basePath.serverBasePath}/app/security/overwritten_session`,
`${this.options.basePath.serverBasePath}/security/overwritten_session`,
{ state: newState }
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,10 @@ import { RouteDefinitionParams } from '..';
* Defines routes required for the Account Management view.
*/
export function defineAccountManagementRoutes({ router, csp }: RouteDefinitionParams) {
router.get(
{ path: '/app/security/account', validate: false },
async (context, request, response) => {
return response.ok({
body: await context.core.rendering.render({ includeUserSettings: true }),
headers: { 'content-security-policy': csp.header },
});
}
);
router.get({ path: '/security/account', validate: false }, async (context, request, response) => {
return response.ok({
body: await context.core.rendering.render({ includeUserSettings: true }),
headers: { 'content-security-policy': csp.header },
});
});
}
Loading

0 comments on commit 321a4cf

Please sign in to comment.