Ignore intermediate unauthenticated session during repeated authentication attempt. #79300
Conversation
azasypkin
added
release_note:fix
Team:Security
| @@ -31,16 +31,16 @@ const onlyNotInCoverageTests = [ | |||
| require.resolve('../test/plugin_api_integration/config.ts'), | |||
| require.resolve('../test/kerberos_api_integration/config.ts'), | |||
| require.resolve('../test/kerberos_api_integration/anonymous_access.config.ts'), | |||
| require.resolve('../test/saml_api_integration/config.ts'), | |||
| require.resolve('../test/security_api_integration/saml.config.ts'), | |||
There was a problem hiding this comment.
note: slowly moving security related integration tests to a single folder.
| await supertest | ||
| .get('/login') | ||
| .ca(CA_CERT) | ||
| .set('Cookie', intermediateAuthCookie.cookieString()) | ||
| .expect(200); | ||
|
|
||
| // When user tries to access any other page in Kibana. |
There was a problem hiding this comment.
note: that is the main test in this PR, I made sure it fails without my change.
azasypkin
force-pushed
the
issue-75338-ignore-intermediate-cookie
branch
from
5790f55 to
899f8e1
Compare
| @@ -30,7 +30,9 @@ export default function ({ getService }: FtrProviderContext) { | |||
| } | |||
|
|
|||
| async function getNumberOfSessionDocuments() { | |||
| return (await es.search({ index: '.kibana_security_session*' })).hits.total.value; | |||
| return (((await es.search({ index: '.kibana_security_session*' })).hits.total as unknown) as { | |||
There was a problem hiding this comment.
note: typechecker suddenly started to complain about these return types (types are invalid indeed).
|
@kibanamachine merge upstream |
|
Pinging @elastic/kibana-security (Team:Security) |
|
note: the referenced issue is marked as |
Thanks for calling this out! I've relabeled issue to |
|
Hey Team/@elastic/kibana-security, PR is ready for review whenever you have time. Essentially it's just a one-liner, the rest are just test changes. I'm also planning to backport this fix to 7.9.3+. Thanks! |
| @@ -637,7 +637,7 @@ export class Authenticator { | |||
| // 4. Request isn't attributed with HTTP Authorization header | |||
| return ( | |||
| canRedirectRequest(request) && | |||
| !sessionValue && | |||
| (!sessionValue || !sessionValue.username) && | |||
There was a problem hiding this comment.
nit: what do you think about introducing a isSessionAuthenticated function (or similar) for this? I know it's a very basic check, but since this will be the second place for such a check (
There was a problem hiding this comment.
Yeah, I think it makes sense, will do, thanks!
There was a problem hiding this comment.
While fixing this I realized that I cannot backport it to 7.9.x since we started to store username in the session for all providers only in 7.10 (with server-side sessions) that means we don't have an easy way to distinguish authenticated session from unauthenticated in 7.9 😢
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
|
7.x/7.10.0: 9beebb0 |
Basically what title says + a bunch of test-only changes, see more details and reasoning in #75338.
The current behavior is quite annoying when you have multiple providers, start SSO, but then change your mind (or realize that you don't have proper credentials for IdP) and want to login with a different provider.
Fixes: #75338
Release note: previously when user started SAML or OpenID authentication handshake, but didn't or couldn't finish it they weren't able to access Login Selector easily (e.g. to log in with another authentication provider) unless they used
/loginURL directly or manually cleared the session cookies. That was a very confusing user experience. The reason was that unauthenticated intermediate session that was created to support handshake forced Kibana to automatically restart the same handshake whenever user accessed Kibana. We fixed that and now in certain cases we ignore unauthenticated intermediate session allowing user to easily access Login Selector whenever they need it.