-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Removes deprecated platform security v1 routes #203915
base: main
Are you sure you want to change the base?
Conversation
💚 Build Succeeded
Metrics [docs]
History
cc @jeramysoucy |
Pinging @elastic/kibana-security (Team:Security) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Just a few suggestions.
@@ -23,79 +22,47 @@ export function defineSAMLRoutes({ | |||
buildFlavor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can remove basePath, logger, buildFlavor, docLinks
options: { | ||
access: 'public', | ||
excludeFromOAS: true, | ||
authRequired: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: while you're here, would you mind replacing deprecated authRequeired
with security.authc.enabled
?
@@ -27,58 +27,32 @@ export function defineOIDCRoutes({ | |||
basePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can remove logger, docLinks
path: '/api/security/oidc/implicit', | ||
validate: false, | ||
options: { | ||
authRequired: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: while you're here, would you mind replacing deprecated authRequired
with security.authc.enabled
in all routes in this file?
iss: schema.maybe(schema.uri({ scheme: ['https'] })), | ||
login_hint: schema.maybe(schema.string()), | ||
target_link_uri: schema.maybe(schema.uri()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we can remove these as it's only used for OP initiated login attempts and we deprecated usage of /api/security/oidc/callback
for this since 7.6.0 (#50695), users should rely on /api/security/oidc/initiate_login
instead
iss: schema.maybe(schema.uri({ scheme: ['https'] })), | |
login_hint: schema.maybe(schema.string()), | |
target_link_uri: schema.maybe(schema.uri()), |
} else if (request.query.iss) { | ||
// An HTTP GET request with a query parameter named `iss` as part of a 3rd party initiated authentication. | ||
// See more details at https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin | ||
loginAttempt = { | ||
type: OIDCLogin.LoginInitiatedBy3rdParty, | ||
iss: request.query.iss, | ||
loginHint: request.query.login_hint, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: same as above, this endpoint should no longer support OP initiated logins (that's why we logged deprecation notice for both deprecated and non-deprecated routes).
} else if (request.query.iss) { | |
// An HTTP GET request with a query parameter named `iss` as part of a 3rd party initiated authentication. | |
// See more details at https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin | |
loginAttempt = { | |
type: OIDCLogin.LoginInitiatedBy3rdParty, | |
iss: request.query.iss, | |
loginHint: request.query.login_hint, | |
}; | |
} | |
} |
@@ -36,124 +35,67 @@ export function defineCommonRoutes({ | |||
buildFlavor, | |||
docLinks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docLinks, |
options: { | ||
access: 'public', | ||
excludeFromOAS: true, | ||
authRequired: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: while you're here, would you mind replacing deprecated authRequired
with security.authc.enabled
in all routes in this file?
Summary
Removes the v1 routes deprecated in #199656
Part of Kibana 9.0.0 readiness https://github.com/elastic/kibana-team/issues/1190