Skip to content

Commit

Permalink
Render user friendly UI for unhandled auth errors (elastic#173959)
Browse files Browse the repository at this point in the history
## Summary

Closes elastic#171040 


## Fixes

- Added a specific check for 500 errors when auth fails with OIDC/SAML
- Renders unauthenticated page with the redirect set for `/`

## Steps to test

There are multiple ways to test the scenarios for rendering the
unauthenticated use case.


### Use existing testing configs to run Kibana with different
configurations:
Changes added to the test suite here:
[x-pack/test/security_functional/plugins/test_endpoints/server/init_routes.ts](https://github.com/elastic/kibana/pull/173959/files#diff-d4d10bb4dd30278eac5887d8be2ce2a9638d7741209be2ece7c0600e422175fd)
assist in the testing of the scenarios.


1. Login selector is enabled

```
node scripts/functional_tests_server.js --config x-pack/test/security_functional/login_selector.config.ts
```
2. Login selector is disabled, login page is available, but not default
== we ignore the existence of /login UI
```
node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/saml.config.ts
```

3. Login selector is not enabled, but login page is available and
default
```
node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/http_bearer.config.ts
```

Now, when you run Kibana in any of these test configurations you must
first navigate directly to http://localhost:5620/login to log in as
elastic user and then access the following URLs and see how the code
behaves

```
http://localhost:5620/authentication/app/not_auth_flow
http://localhost:5620/authentication/app/not_auth_flow?statusCode=400
http://localhost:5620/authentication/app/not_auth_flow?statusCode=401
http://localhost:5620/authentication/app/not_auth_flow?statusCode=500

# Auth flow routes - part of the auth flow
http://localhost:5620/authentication/app/auth_flow
http://localhost:5620/authentication/app/auth_flow?statusCode=400
http://localhost:5620/authentication/app/auth_flow?statusCode=401
http://localhost:5620/authentication/app/auth_flow?statusCode=500
```

### Run kibana as usual for testing

To test this by running kibana locally, you'll first need to set up your
local instance to run with SAML

- Login to https://oktanaut.app.elastic.dev/
- Fill in your local endpoint
- Set Stack version 8.0+ and platform to Other

Then with the cofigs given to you from there, run ES as follows:
```
yarn es snapshot --license trial \ 
... your config
```

and then run KBN as 

```
yarn start --no-base-path \
   -xpack.security.authc.selector.enabled=false
   ... rest of the config from Oktanaut goes here
```

This will start up your local stack pointing to SAML. At this point, we
need to force an error to test this flow. There are a few ways to do
that:

#### Option A
- Go to the SAML provider in
[`/x-pack/plugins/security/server/authentication/providers/saml.ts`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/server/authentication/providers/saml.ts)
- Replace [`ids: !isIdPInitiatedLogin ? [stateRequestId] : [],` with
`ids:
[]`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/server/authentication/providers/saml.ts#L372)
- Comment out the line [`...(providerRealm ? { realm: providerRealm } :
{})`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/server/authentication/providers/saml.ts#L374)


#### Option B
You could also do the following to reproduce the error on main:

- Open kibana in a private browser window. 
- Then start the login flow which redirects you to okta. 
- Before entering your password, switch off the ES instance running
locally.
- Then login which should give you a redirect back to the saml/callback
page with an error JSON. In the PR, this should give you the
unauthenticated screen

#### Option C

Force one (or both) of the Kibana (SAML/OIDC) routes to throw a custom
error

For example, in
`x-pack/plugins/security/server/routes/authentication/saml.ts:L64`, add
the following code snippet

```ts
return response.customError({ statusCode: 500 });
```
Now running through the SAML login flow on main should show a JSON error
string whereas this PR should show an unauthenticated error page.

This sends a bad request to the SAML provider which results in the error
which should now show you an Unauthenticated page.




## Release Notes:

- Renders a user-friendly UI for unhandled login failures.
elastic#171040

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
SiddharthMantri and kibanamachine authored Mar 28, 2024
1 parent 737f8f7 commit 0d46e05
Show file tree
Hide file tree
Showing 7 changed files with 299 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -966,7 +966,6 @@ describe('AuthenticationService', () => {
const { authenticator, onPreResponseHandler } = getService();
authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some');
mockCanRedirectRequest.mockReturnValue(true);

await expect(
onPreResponseHandler(
httpServerMock.createKibanaRequest({ path: '/app/some', query: { param: 'one two' } }),
Expand Down Expand Up @@ -1056,5 +1055,123 @@ describe('AuthenticationService', () => {
});
});
});

describe('handles unexpected post-authentication failures', () => {
let mockReturnedValue: { type: any; body: string };
let mockOnPreResponseToolkit: jest.Mocked<OnPreResponseToolkit>;
beforeEach(() => {
mockReturnedValue = { type: 'render' as any, body: 'body' };
mockOnPreResponseToolkit = httpServiceMock.createOnPreResponseToolkit();
mockOnPreResponseToolkit.render.mockReturnValue(mockReturnedValue);
});

it('when login page is unavailable', async () => {
mockSetupAuthenticationParams.config = createConfig(
ConfigSchema.validate({
authc: {
providers: { saml: { saml1: { order: 150, realm: 'saml1' } } },
},
}),
loggingSystemMock.create().get(),
{ isTLSEnabled: false }
);
const mockRenderUnauthorizedPage = jest
.requireMock('./unauthenticated_page')
.renderUnauthenticatedPage.mockReturnValue('rendered-view');

const { authenticator, onPreResponseHandler } = getService();
authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some');
mockCanRedirectRequest.mockReturnValue(true);

await expect(
onPreResponseHandler(
httpServerMock.createKibanaRequest({
path: '/app/some',
query: { param: 'one two' },
routeTags: [ROUTE_TAG_AUTH_FLOW],
}),
{ statusCode: 500 },
mockOnPreResponseToolkit
)
).resolves.toBe(mockReturnedValue);

expect(mockOnPreResponseToolkit.render).toHaveBeenCalledWith({
body: 'rendered-view',
headers: {
'Content-Security-Policy': CspConfig.DEFAULT.header,
},
});

expect(mockRenderUnauthorizedPage).toHaveBeenCalledWith({
basePath: mockSetupAuthenticationParams.http.basePath,
staticAssets: expect.any(Object),
originalURL: '/mock-server-basepath/',
customBranding: undefined,
});
});

it('when login page is available', async () => {
mockSetupAuthenticationParams.config = createConfig(
ConfigSchema.validate({
authc: {
selector: { enabled: true },
},
}),
loggingSystemMock.create().get(),
{ isTLSEnabled: false }
);
const { authenticator, onPreResponseHandler } = getService();
authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some');
mockCanRedirectRequest.mockReturnValue(true);

await expect(
onPreResponseHandler(
httpServerMock.createKibanaRequest({
path: '/app/some',
query: { param: 'one two' },
routeTags: [ROUTE_TAG_AUTH_FLOW],
}),
{ statusCode: 500 },
mockOnPreResponseToolkit
)
).resolves.toBe(mockReturnedValue);

expect(mockOnPreResponseToolkit.render).toHaveBeenCalledWith({
body: '<div/>',
headers: {
'Content-Security-Policy': CspConfig.DEFAULT.header,
Refresh:
'0;url=/mock-server-basepath/login?msg=UNAUTHENTICATED&next=%2Fmock-server-basepath%2F',
},
});
});

it('when logout redirect fails', async () => {
const { authenticator, onPreResponseHandler } = getService();
authenticator.getRequestOriginalURL.mockReturnValue('/mock-server-basepath/app/some');
mockCanRedirectRequest.mockReturnValue(true);
const mockRenderUnauthorizedPage = jest
.requireMock('./unauthenticated_page')
.renderUnauthenticatedPage.mockReturnValue('rendered-view');

await expect(
onPreResponseHandler(
httpServerMock.createKibanaRequest({
path: '/api/security/logout',
routeTags: [ROUTE_TAG_AUTH_FLOW],
}),
{ statusCode: 500 },
mockOnPreResponseToolkit
)
).resolves.toBe(mockReturnedValue);

expect(mockRenderUnauthorizedPage).toHaveBeenCalledWith({
basePath: mockSetupAuthenticationParams.http.basePath,
staticAssets: expect.any(Object),
originalURL: '/mock-server-basepath/',
customBranding: undefined,
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -187,30 +187,44 @@ export class AuthenticationService {
});

http.registerOnPreResponse(async (request, preResponse, toolkit) => {
if (preResponse.statusCode !== 401 || !canRedirectRequest(request)) {
return toolkit.next();
}

if (!this.authenticator) {
// Core doesn't allow returning error here.
this.logger.error('Authentication sub-system is not fully initialized yet.');
return toolkit.next();
}

const isAuthRoute = request.route.options.tags.includes(ROUTE_TAG_AUTH_FLOW);
const isLogoutRoute =
request.route.path === '/api/security/logout' ||
request.route.path === '/api/v1/security/logout';

// If users can eventually re-login we want to redirect them directly to the page they tried
// to access initially, but we only want to do that for routes that aren't part of the various
// authentication flows that wouldn't make any sense after successful authentication.
const originalURL = !request.route.options.tags.includes(ROUTE_TAG_AUTH_FLOW)
? this.authenticator.getRequestOriginalURL(request)
: `${http.basePath.get(request)}/`;
if (!isLoginPageAvailable) {
const originalURL = isAuthRoute
? `${http.basePath.get(request)}/`
: this.authenticator.getRequestOriginalURL(request);

// Let API responses or <400 responses pass through as we can let their handlers deal with them.
if (preResponse.statusCode < 400 || !canRedirectRequest(request)) {
return toolkit.next();
}

if (preResponse.statusCode !== 401 && !isAuthRoute) {
return toolkit.next();
}

// Now we are only dealing with authentication flow errors or 401 errors in non-authentication routes.
// Additionally, if logout fails for any reason, we also want to show an error page.
// At this point we redirect users to the login page if it's available, or render a dedicated unauthenticated error page.
if (!isLoginPageAvailable || isLogoutRoute) {
const customBrandingValue = await customBranding.getBrandingFor(request, {
unauthenticated: true,
});
return toolkit.render({
body: renderUnauthenticatedPage({
basePath: http.basePath,
staticAssets: http.staticAssets,
basePath: http.basePath,
originalURL,
customBranding: customBrandingValue,
}),
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export { HTTPAuthorizationHeader } from './authentication';
export type { CasesSupportedOperations } from './authorization';
export type { SecurityPluginSetup, SecurityPluginStart };
export type { AuthenticatedUser } from '../common';
export { ROUTE_TAG_CAN_REDIRECT } from './routes/tags';
export { ROUTE_TAG_CAN_REDIRECT, ROUTE_TAG_AUTH_FLOW } from './routes/tags';

// Re-export types from the plugin directly to enhance the developer experience for consumers of the Security plugin.
export type {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,55 @@ export default function ({ getService }: FtrProviderContext) {
.set('authorization', `Bearer ${accessToken}`)
.expect(401);
});

describe('Post-authentication', () => {
it('correctly handles unexpected post-authentication errors', async () => {
const { accessToken } = await createToken();

await supertest
.get('/authentication/app/not_auth_flow')
.set('authorization', `Bearer ${accessToken}`)
.expect(200);

await supertest
.get('/authentication/app/not_auth_flow?statusCode=400')
.set('authorization', `Bearer ${accessToken}`)
.expect(400);

const { text: nonauthFlow500ResponseText } = await supertest
.get('/authentication/app/not_auth_flow?statusCode=500')
.set('authorization', `Bearer ${accessToken}`)
.expect(500);
expect(nonauthFlow500ResponseText).to.eql(
'{"statusCode":500,"error":"Internal Server Error","message":"500 response"}'
);

// Auth-flow routes
await supertest
.get('/authentication/app/auth_flow')
.set('authorization', `Bearer ${accessToken}`)
.expect(200);

const {
text: authFlow401ResponseText,
headers: { refresh: refresh401Header },
} = await supertest
.get('/authentication/app/auth_flow?statusCode=401')
.set('authorization', `Bearer ${accessToken}`)
.expect(401);
expect(authFlow401ResponseText).to.contain('<div/>');
expect(refresh401Header).to.contain('url=/login');

const {
text: authFlow500ResponseText,
headers: { refresh: refresh500Header },
} = await supertest
.get('/authentication/app/auth_flow?statusCode=500')
.set('authorization', `Bearer ${accessToken}`)
.expect(500);
expect(authFlow500ResponseText).to.contain('<div/>');
expect(refresh500Header).to.contain('url=/login');
});
});
});
}
56 changes: 56 additions & 0 deletions x-pack/test/security_api_integration/tests/saml/saml_login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { FileWrapper } from '../audit/file_wrapper';
export default function ({ getService }: FtrProviderContext) {
const randomness = getService('randomness');
const supertest = getService('supertestWithoutAuth');

const config = getService('config');
const retry = getService('retry');

Expand Down Expand Up @@ -892,5 +893,60 @@ export default function ({ getService }: FtrProviderContext) {
expect(auditEvents[0].kibana.authentication_provider).to.be('saml');
});
});

describe('Post-authentication failures', () => {
it('correctly handles unexpected post-authentication errors', async () => {
const samlAuthenticationResponse = await supertest
.post('/api/security/saml/callback')
.send({ SAMLResponse: await createSAMLResponse() })
.expect(302);

// User should be redirected to the base URL.
expect(samlAuthenticationResponse.headers.location).to.be('/');

const cookies = samlAuthenticationResponse.headers['set-cookie'];
expect(cookies).to.have.length(1);

await checkSessionCookie(parseCookie(cookies[0])!);

const sessionCookie = parseCookie(cookies[0])!.cookieString();
// Non-auth flow routes
await supertest
.get('/authentication/app/not_auth_flow')
.set('Cookie', sessionCookie)
.expect(200);

await supertest
.get('/authentication/app/not_auth_flow?statusCode=400')
.set('Cookie', sessionCookie)
.expect(400);

const { text: nonauthFlow500ResponseText } = await supertest
.get('/authentication/app/not_auth_flow?statusCode=500')
.set('Cookie', sessionCookie)
.expect(500);
expect(nonauthFlow500ResponseText).to.eql(
'{"statusCode":500,"error":"Internal Server Error","message":"500 response"}'
);

// Auth-flow routes
await supertest
.get('/authentication/app/auth_flow')
.set('Cookie', sessionCookie)
.expect(200);

const { text: authFlow401ResponseText } = await supertest
.get('/authentication/app/auth_flow?statusCode=401')
.set('Cookie', sessionCookie)
.expect(401);
expect(authFlow401ResponseText).to.contain('We hit an authentication error');

const { text: authFlow500ResponseText } = await supertest
.get('/authentication/app/auth_flow?statusCode=500')
.set('Cookie', sessionCookie)
.expect(500);
expect(authFlow500ResponseText).to.contain('We hit an authentication error');
});
});
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
BulkUpdateTaskResult,
} from '@kbn/task-manager-plugin/server';
import { restApiKeySchema } from '@kbn/security-plugin-types-server';
import { ROUTE_TAG_AUTH_FLOW } from '@kbn/security-plugin/server';
import { PluginStartDependencies } from '.';

export const SESSION_INDEX_CLEANUP_TASK_NAME = 'session_cleanup';
Expand All @@ -39,6 +40,32 @@ export function initRoutes(
);

const router = core.http.createRouter();

for (const isAuthFlow of [true, false]) {
router.get(
{
path: `/authentication/app/${isAuthFlow ? 'auth_flow' : 'not_auth_flow'}`,
validate: {
query: schema.object({
statusCode: schema.maybe(schema.number()),
message: schema.maybe(schema.string()),
}),
},
options: { tags: isAuthFlow ? [ROUTE_TAG_AUTH_FLOW] : [], authRequired: !isAuthFlow },
},
(context, request, response) => {
if (request.query.statusCode) {
return response.customError({
statusCode: request.query.statusCode,
body: request.query.message ?? `${request.query.statusCode} response`,
});
}

return response.ok({ body: isAuthFlow ? 'Auth flow complete' : 'Not auth flow complete' });
}
);
}

router.post(
{
path: '/authentication/app/setup',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,5 +209,29 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await testSubjects.click('loginBackToLoginLink');
await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible();
});

it('correctly handles unexpected post-authentication errors', async () => {
const supertest = getService('supertest');
await PageObjects.security.loginSelector.login('basic', 'basic1');

await supertest.get('/authentication/app/not_auth_flow').expect(200);
await supertest.get('/authentication/app/not_auth_flow?statusCode=400').expect(400);
await supertest.get('/authentication/app/not_auth_flow?statusCode=500').expect(500);
await browser.navigateTo(
`${deployment.getHostPort()}/authentication/app/not_auth_flow?statusCode=401`
);
await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible();

await supertest.get('/authentication/app/auth_flow').expect(200);
await browser.navigateTo(
`${deployment.getHostPort()}/authentication/app/auth_flow?statusCode=400`
);
await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible();

await browser.navigateTo(
`${deployment.getHostPort()}/authentication/app/auth_flow?statusCode=500`
);
await PageObjects.security.loginSelector.verifyLoginSelectorIsVisible();
});
});
}

0 comments on commit 0d46e05

Please sign in to comment.