Skip to content

Commit

Permalink
Review#1: do not perform license check for the /logout endpoint, ke…
Browse files Browse the repository at this point in the history
…ep `/api/security/v1/me` in 7.x, validate OIDC `authenticationResponseURI` as a proper URI, explain the need for `allowUnknowns` in OIDC routes validation, localize error message, update APM dev script with new `users` internal routes.
  • Loading branch information
azasypkin committed Dec 10, 2019
1 parent 43bb372 commit 9763717
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ async function createOrUpdateUser(newUser: User) {
async function createUser(newUser: User) {
const user = await callKibana<User>({
method: 'POST',
url: `/api/security/v1/users/${newUser.username}`,
url: `/internal/security/users/${newUser.username}`,
data: {
...newUser,
enabled: true,
Expand All @@ -209,7 +209,7 @@ async function updateUser(existingUser: User, newUser: User) {
// assign role to user
await callKibana({
method: 'POST',
url: `/api/security/v1/users/${username}`,
url: `/internal/security/users/${username}`,
data: { ...existingUser, roles: allRoles }
});

Expand All @@ -219,7 +219,7 @@ async function updateUser(existingUser: User, newUser: User) {
async function getUser(username: string) {
try {
return await callKibana<User>({
url: `/api/security/v1/users/${username}`
url: `/internal/security/users/${username}`
});
} catch (e) {
const err = e as AxiosError;
Expand Down
34 changes: 22 additions & 12 deletions x-pack/plugins/security/server/routes/authentication/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef
validate: { query: schema.object({}, { allowUnknowns: true }) },
options: { authRequired: false },
},
createLicensedRouteHandler(async (context, request, response) => {
async (context, request, response) => {
const serverBasePath = basePath.serverBasePath;
if (path === '/api/security/v1/logout') {
logger.warn(
Expand All @@ -51,18 +51,28 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef
} catch (error) {
return response.customError(wrapIntoCustomErrorResponse(error));
}
})
}
);
}

router.get(
{ path: '/internal/security/me', validate: false },
createLicensedRouteHandler(async (context, request, response) => {
try {
return response.ok({ body: (await authc.getCurrentUser(request)) as any });
} catch (error) {
return response.customError(wrapIntoCustomErrorResponse(error));
}
})
);
// Generate two identical routes with new and deprecated URL and issue a warning if route with deprecated URL is ever used.
for (const path of ['/internal/security/me', '/api/security/v1/me']) {
router.get(
{ path, validate: false },
createLicensedRouteHandler(async (context, request, response) => {
if (path === '/api/security/v1/me') {
logger.warn(
`The "${basePath.serverBasePath}${path}" endpoint is deprecated and will be removed in the next major version.`,
{ tags: ['deprecation'] }
);
}

try {
return response.ok({ body: (await authc.getCurrentUser(request)) as any });
} catch (error) {
return response.customError(wrapIntoCustomErrorResponse(error));
}
})
);
}
}
20 changes: 15 additions & 5 deletions x-pack/plugins/security/server/routes/authentication/oidc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { schema } from '@kbn/config-schema';
import { i18n } from '@kbn/i18n';
import { KibanaRequest, KibanaResponseFactory } from '../../../../../../src/core/server';
import { OIDCAuthenticationFlow } from '../../authentication';
import { createCustomResourceResponse } from '.';
Expand Down Expand Up @@ -95,7 +96,7 @@ export function defineOIDCRoutes({
validate: {
query: schema.object(
{
authenticationResponseURI: schema.maybe(schema.string({ minLength: 1 })),
authenticationResponseURI: schema.maybe(schema.uri()),
code: schema.maybe(schema.string()),
error: schema.maybe(schema.string()),
error_description: schema.maybe(schema.string()),
Expand All @@ -105,6 +106,9 @@ export function defineOIDCRoutes({
target_link_uri: schema.maybe(schema.uri()),
state: schema.maybe(schema.string()),
},
// The client MUST ignore unrecognized response parameters according to
// https://openid.net/specs/openid-connect-core-1_0.html#AuthResponseValidation and
// https://tools.ietf.org/html/rfc6749#section-4.1.2.
{ allowUnknowns: true }
),
},
Expand Down Expand Up @@ -178,6 +182,8 @@ export function defineOIDCRoutes({
login_hint: schema.maybe(schema.string()),
target_link_uri: schema.maybe(schema.uri()),
},
// Other parameters MAY be sent, if defined by extensions. Any parameters used that are not understood MUST
// be ignored by the Client according to https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin.
{ allowUnknowns: true }
),
},
Expand Down Expand Up @@ -215,6 +221,8 @@ export function defineOIDCRoutes({
login_hint: schema.maybe(schema.string()),
target_link_uri: schema.maybe(schema.uri()),
},
// Other parameters MAY be sent, if defined by extensions. Any parameters used that are not understood MUST
// be ignored by the Client according to https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin.
{ allowUnknowns: true }
),
},
Expand All @@ -235,7 +243,7 @@ export function defineOIDCRoutes({
loginAttempt: ProviderLoginAttempt
) {
try {
// We handle the fact that the user might get redirected to Kibana while already having an session
// We handle the fact that the user might get redirected to Kibana while already having a session
// Return an error notifying the user they are already logged in.
const authenticationResult = await authc.login(request, {
provider: 'oidc',
Expand All @@ -244,9 +252,11 @@ export function defineOIDCRoutes({

if (authenticationResult.succeeded()) {
return response.forbidden({
body:
'Sorry, you already have an active Kibana session. ' +
'If you want to start a new one, please logout from the existing session first.',
body: i18n.translate('xpack.security.conflictingSessionError', {
defaultMessage:
'Sorry, you already have an active Kibana session. ' +
'If you want to start a new one, please logout from the existing session first.',
}),
});
}

Expand Down

0 comments on commit 9763717

Please sign in to comment.