Handle reset credentials flow with logged in user

[graziang]

Avoid showing ResetCredentialChooseUser if SSO session is present.
Cookie re-authentication is skipped if the flow is forked, to show 'Email sent" message.

Closes #8887

[keycloak-github-bot]

Unreported flaky test detected, please review

[keycloak-github-bot]

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchangeCreateNewUserUsingMappers

Keycloak CI - Base IT

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchange

Keycloak CI - Base IT

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testAccountLinkNoTokenStore

Keycloak CI - Base IT

java.lang.AssertionError: Unexpected page. Current Page URL: http://localhost:8280/exchange-linking/link?realm=child&provider=parent-idp
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest.testAccountLinkNoTokenStore(BrokerLinkAndTokenExchangeTest.java:494)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

org.keycloak.testsuite.client.ClientTypesTest#testUpdateClientWithClientType

Keycloak CI - Base IT (4)

java.lang.AssertionError: Not expected to update client
	at org.junit.Assert.fail(Assert.java:89)
	at org.keycloak.testsuite.client.ClientTypesTest.testUpdateClientWithClientType(ClientTypesTest.java:112)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeWithDynamicScopesEnabled

Keycloak CI - Base IT (6)

org.keycloak.common.VerificationException: Token not set
	at org.keycloak.TokenVerifier.parse(TokenVerifier.java:401)
	at org.keycloak.testsuite.oauth.ClientTokenExchangeTest.testExchange(ClientTokenExchangeTest.java:309)
	at org.keycloak.testsuite.oauth.ClientTokenExchangeTest.testExchangeWithDynamicScopesEnabled(ClientTokenExchangeTest.java:1021)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testClientExchange

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testIntrospectTokenAfterImpersonation

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testPublicClientNotAllowed

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<403> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeUsingServiceAccount

Keycloak CI - Base IT (6)

org.keycloak.common.VerificationException: Token not set
	at org.keycloak.TokenVerifier.parse(TokenVerifier.java:401)
	at org.keycloak.testsuite.oauth.ClientTokenExchangeTest.testExchangeUsingServiceAccount(ClientTokenExchangeTest.java:347)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonation

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationUsingPublicClient

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

[mposolda]

Thanks for the fix. I appreciate that this fix can have good usability for end user. But at the same time, I afraid a bit of the security... As this change pretty much adds SSO logic to reset-credentials flow and allows to bypass the email checks (like ResetCredentialEmail authenticator). This means that users are able to reset their password and other credentials without any further restrictions... But for instance in the account console, we allow users to update their passwords (and other credentials) just if they authenticate sooner than 5 minutes ago. Otherwise they are require to re-authenticate. And they are even admins, who want to always re-authenticate users when they want to change their password (which was allowed by max-auth age password policy recently added). But the change in this PR would allow to update credentials without any re-authentication or further verifications...

My preference is rather more secure approach, which would still require email verification in the reset-credential flow (As email verification is the form of the authentication).

We can make sure that SSO authentication is ignored in the forked flow. Maybe some check can be added to OIDCLOginProtocol and SAMLProtocol method requireReauthentication to make sure that when the flow is forked (authentication session FORKED_FROM exists), the method returns true, so the cookie authenticator is ignored? So the email would be sent and the login page would be shown with the message that "Email was sent to your email address" like when user is not authenticated.

Also I guess that ResetCredentialChooseUser may not be shown (as we know the user from SSO session), but ResetCredentialsEmail authenticator would be still require for email verification. Maybe we need more authentication notes in the authSession instead of single ACTION_TOKEN_USER_ID note (but maybe we can just re-use the user from authenticationSession in ResetCredentialChooseUser ?)

WDYT?

[graziang]

@mposolda Thanks for the review. My idea was to avoid sending the email since there is an existing sso session, but I agree that sending the password reset email is certainly more secure.

Currently in the Cookie authenticator if a valid sso session exists but the requireReauthentication method returns true the form is shown requesting only the password with the message: Please re-authenticate to continue, so in this way the message box is busy. Should we replace it with Email was sent to your email address... in this case?

Yes, we could avoid showing ResetCredentialChooseUser if sso session is present, or just prepopulate the input with username.
I don't think we need other auth sessions as we can set the user or just set the AbstractUsernameFormAuthenticator.ATTEMPTED_USERNAME auth note reading the user from sso an keep the same behavior.

[mposolda]

@mposolda Thanks for the review. My idea was to avoid sending the email since there is an existing sso session, but I agree that sending the password reset email is certainly more secure.

Currently in the Cookie authenticator if a valid sso session exists but the requireReauthentication method returns true the form is shown requesting only the password with the message: Please re-authenticate to continue, so in this way the message box is busy. Should we replace it with Email was sent to your email address... in this case?

+1 to do something to make sure that Email was sent ... is displayed instead of Please re-authenticate to continue

Yes, we could avoid showing ResetCredentialChooseUser if sso session is present, or just prepopulate the input with username. I don't think we need other auth sessions as we can set the user or just set the AbstractUsernameFormAuthenticator.ATTEMPTED_USERNAME auth note reading the user from sso an keep the same behavior.

Yeah, sounds like a good idea. If we can use without introducing authNote, it is always better :-)

[keycloak-github-bot]

Unreported flaky test detected, please review

[keycloak-github-bot]

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.saml.ArtifactBindingWithResolutionServiceTest#testReceiveArtifactLoginFullWithRedirect

Keycloak CI - Base IT (6)

java.lang.AssertionError: 

Expected: response status code matches is <302>
     but: was <HttpResponseProxy{HTTP/1.1 400 Bad Request [Cache-Control: no-store, must-revalidate, max-age=0, content-length: 2913, Content-Language: en, Content-Security-Policy: frame-src 'self'; frame-ancestors 'self'; object-src 'none';, Content-Type: text/html;charset=utf-8, Referrer-Policy: no-referrer, Strict-Transport-Security: max-age=31536000; includeSubDomains, X-Content-Type-Options: nosniff, X-Frame-Options: SAMEORIGIN, X-Robots-Tag: none, X-XSS-Protection: 1; mode=block] ResponseEntityProxy{[Content-Type: text/html;charset=utf-8,Content-Length: 2913,Chunked: false]}}> with entity <!DOCTYPE html>
<html class="login-pf">
...

Report flaky test

[mposolda]

@graziang Thanks!