-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
do not warn when switching capabilities for resources with optional auth #61043
Conversation
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 for the fix! This check is consistent with what the security plugin does in its switcher today
edit: looking at failing test..
const isAnonymousRequest = !request.route.options.authRequired; | ||
|
||
if (isAnonymousRequest) { | ||
if (!request.auth.isAuthenticated) { |
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.
We may have to check if security is enabled before bailing here. Luckily (?), Spaces declares an optional dependency on security, so we can check if security is enabled, and if it is an authenticated request
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.
@legrego our tests verify another behavior.
const expected = mapValues(uiCapabilities.value!.catalogue, () => true); |
Seems that it doesn't rely on security plugin availability.
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.
Our goal here is to skip toggling capabilities if we don't have credentials, but would otherwise need them in order to perform the capabilities check. We used to silently fail in the past, but we made this more explicit in #57693 (comment).
So we need to figure out a way to determine if credentials are required in order to toggle capabilities. Prior to #58589, we had two distinct routes to retrieve capabilities: one which required auth, and another which didn't. This made the check easy for us here, but it's no longer sufficient now that we have a single route with optional
auth.
When security is disabled, then I believe request.auth.isAuthenticated
will always be false
, but in this case, we will still want to attempt to toggle capabilities.
When security is enabled, then we only want to attempt to toggle capabilities when request.auth.isAuthenticated
is true
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.
When security is disabled, then I believe request.auth.isAuthenticated will always be false, but in this case, we will still want to attempt to toggle capabilities.
It sounds like you want to know AuthState, which is available via auth api
And you need to retrieve capabilities for authenticated
& unknown
statuses. Is it right? Moreover, Spaces plugin will log the same warning. Then the current PR can wait or we even can keep the current behavior from the master.
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.
When security is disabled, then I believe request.auth.isAuthenticated will always be false, but in this case, we will still want to attempt to toggle capabilities.
It sounds like you want to know AuthState, which is available via auth api
And you need to retrieve capabilities forauthenticated
&unknown
statuses. Is it right? Moreover, Spaces plugin will log the same warning.
Yes, that sounds correct to me
Then the current PR can wait or we even can keep the current behavior from the master.
Perhaps it'd be worthwhile to reduce this message from warn
to debug
then, if this is a message we're going to expect in certain conditions. I don't want to pollute the logs unnecessarily.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…uth (elastic#61043) * do not switch capabilities for optional routes * downgrade message to debug
Thanks for setting up a PR to fix, I was seeing this message on master and 7.x -- when I would login or logout. I am seeing this error in the logs when I login or out of Kibana, is it expected or is it an issue? |
@liza-mae yes, as stated in the PR title:
|
Thanks @restrry ! |
Summary
You can see a warning in logs when landing on
/login
page:AFAIK It doesn't break any logic.
I'm not sure what it the right solution for this use case if we need to get capabilities for a resource with 'optional' auth. Any other plugin can use the Security plugin API to verify that a user is authenticated, but Spaces & Security cannot call each other due to circular dependencies.
UPDATED: based on a conversation with @legrego: it's expected behavior, and we should downgrade log level to avoid confusion.
Checklist
For maintainers