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.

Sign up for GitHub

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

Migrate the rest of the API endpoints to the New Platform plugin #50695

Merged
merged 4 commits into from
Dec 11, 2019
Merged

Migrate the rest of the API endpoints to the New Platform plugin #50695

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4189b08
Move files.
azasypkin Nov 12, 2019
94018c1
Change files.
azasypkin Nov 12, 2019
43bb372
Merge branch 'master' into issue-xxx-migrate-np-api
azasypkin Dec 10, 2019
9763717
Review#1: do not perform license check for the `/logout` endpoint, ke…
azasypkin Dec 10, 2019
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
6 changes: 3 additions & 3 deletions x-pack/legacy/plugins/apm/scripts/kibana-security/setup-custom-kibana-user-role.ts
View file
Open in desktop
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
View file
Open in desktop
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(
Copy link
Member

@legrego legrego Dec 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about integrating this with the upgrade assistant in a followup PR? I think it'd be helpful for folks when upgrading to the next major version if we make it known that something in their environment is relying on this deprecated endpoint.

To say this another way, I'd personally be annoyed if I ran the upgrade assistant, and it didn't warn me that this would stop working if I was relying on it 😄

Comment applies to the other deprecated endpoints as well

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great idea! I'll try to figure how hard/easy it would be. I tried it once in the distant past and UA wasn't ready for things like this back then, but I believe it's much more advanced now.

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(
Copy link
Member Author

Choose a reason for hiding this comment

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

note: Assuming we make this route internal I don't mention the name of the internal route as we don't provide any guarantees regarding it.

`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
View file
Open in desktop
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 }
Copy link
Member

Choose a reason for hiding this comment

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

nit: unrelated to the migration, but can we add a comment explaining why we want to allow unknown object properties here? We're trying to reduce the number of places this is required in code, so documenting legitimate uses will be helpful going forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, added comments in all 3 places in this file.

),
},
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