Exposing whether or not security should authenticate #24616
Conversation
This will allow Canvas to determine whether or not they should be authenticating their socket messages. Additionally, this consolidates the logic to within the checkLicenseResultGenerator since we are making this determination based on the xpack info and the license itself.
kobelb
added
Team:Security
|
Just out of curiosity, is this only targeted for 7.0, or is this also being backported to 6.x & 6.5? |
|
6.x would be great. 6.5 would be nice, but isn't required. |
💚 Build Succeeded |
x-pack/plugins/security/server/lib/authentication/__tests__/authenticator.js
Outdated
Show resolved
Hide resolved
| @@ -27,6 +27,7 @@ export function checkLicense(xPackInfo) { | |||
| // assume worst-case and lock user at login screen. | |||
| if (!xPackInfo.isAvailable()) { | |||
| return { | |||
| authenticate: true, | |||
There was a problem hiding this comment.
What are your thoughts on naming this shouldAuthenticate? I'm not opposed to leaving it as-is, but shouldAuthenticate would match the name that we are exposing on the security plugin, and I feel it would be more consistent with the rest of the flags we are presenting in this license check response.
Other options: isAuthenticated / allowAuthentication (but this feels similar to allowLogin)
edit: If I'm reading this correctly, the authenticate flag value mirrors that of showLogin. Do you think it makes sense to consolidate these flags?
There was a problem hiding this comment.
Switching everything to use allowAuthentication from the license check results seems reasonable to me. I'm torn on whether or not we should expose server.plugins.security.allowAuthentication though or we should continue using server.plugins.security.shouldAuthenticate.
There was a problem hiding this comment.
I don't have a strong opinion on allowAuthentication vs shouldAuthenticate as the exposed function. Whatever you choose there is fine with me
Co-Authored-By: kobelb <brandon.kobel@gmail.com>
…thenticator.js Co-Authored-By: kobelb <brandon.kobel@gmail.com>
💚 Build Succeeded |
💔 Build Failed |
|
retest |
💔 Build Failed |
kobelb
force-pushed
the
security-should-authenticate
branch
from
f48f5f8
to
9d0f506
Compare
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
|
@kobelb I think so. There's code in Canvas to do this already, but it would be nice if it was moved into Security instead. I don't know what this PR was waiting on either, maybe just a review label and some assignees? |
I think it was just waiting on me not forgetting about it, I'll get it updated and merged. |
💔 Build Failed |
|
@kobelb wondering if we can close this PR? (cleaning up review dashboard 🙂 ) |
|
This will allow Canvas to determine whether or not they should be
authenticating their socket messages. Additionally, this consolidates
the logic to within the checkLicenseResultGenerator since we are making
this determination based on the xpack info and the license itself.