Repository navigation
Conversation
Unreported flaky test detectedIf 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.SAMLLoginResponseHandlingTest#testAttributesKeycloak CI - Adapter IT Strict Cookies |
2c3c43a to
6927d2a
Compare
64cf932 to
b2ea215
Compare
Unreported flaky test detectedIf 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.model.user.UserModelTest#testAddRemoveUserKeycloak CI - Store Model Tests |
3d081eb to
0d1663e
Compare
Unreported flaky test detectedIf 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.forms.BruteForceTest#testNoFailureResetForPermanentLockout |
8143546 to
bf8cdad
Compare
| event.detail(Details.CLIENT_POLICY_ERROR, cpe.getError()); | ||
| event.detail(Details.CLIENT_POLICY_ERROR_DETAIL, cpe.getErrorDetail()); | ||
| event.error(cpe.getError()); | ||
| throw new ErrorPageException(session, authenticationSession, cpe.getErrorStatus(), cpe.getErrorDetail()); |
There was a problem hiding this comment.
cpe.getError() gets lost here, so a client policy raising a different OAuth error code than invalid_request will always be overridden by invalid_request now?
There was a problem hiding this comment.
Yes, this is how the translation from ClientPolicyException => ErrorPageException works. Perhaps the html error page does not show the error property from the CPE. This code path has not changed.
There was a problem hiding this comment.
This has been fixed - the error code coming from ErrorPageException and AuthorizationCheckException are now passed on correctly.
b40195a to
0707c36
Compare
| return handleRedirectErrorToClient(epex.getError(), errorMessage); | ||
| } | ||
|
|
||
| // [TODO >>>] Should we throw an ErrorResponseException instead? |
There was a problem hiding this comment.
@tdiesler Would you happen to know if there is already an existing issue tracking the use of ErrorResponseException instead? If not, do you think it would be worthwhile creating one for future tracking?
Also, I noticed there are 14 failing checks. Do you think these are related to test flakiness, or does this PR require some changes in order for the tests to pass?
There was a problem hiding this comment.
This needs to be discussed. The spec does not say that there should be a json error response at all. It also does not prohibit it AFAIK.
Unreported flaky test detectedIf 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.SAMLServletAdapterTest#salesMetadataTestKeycloak CI - Adapter IT Strict Cookies |
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
There was a problem hiding this comment.
In the error scenarios, we already redirect to client whenever we can safely do that. But we can do that just if combination of parameters redirect_uri, response_mode or response_type is valid and aligned with the client configuration. Otherwise the error page needs to be displayed.
-
We had security issues in the past due the issues with unsafe redirects. The redirects are OK just if we can be 100% sure they are safe. Otherwise the error page needs to be displayed.
-
Keycloak is already certified as the OIDC and FAPI provider and also pass the FAPI 2 testsuite. In some tests, it is expected that error page is displayed instead of redirect to the client. In such cases, it is expected to take a screenshot and attach it to the particular conformance test. This is perfectly fine to pass the certification.
-
Introducing the realm-wide option is not good IMO as the only use of such an option is the conformance testsuite. As in production, developers will figure that they use incorrect parameters in their OAuth/OIDC requests and they would fix it in their client applications to send correct requests. I don't think we need to introduce an option just because of the conformance testsuite (especially since conformance testsuite does not need it as there is a solution with the attachments)
closes #48542
This PR tries to be as unintrusive as possible. While processing an authorization request there are potential errors coming from
most of these errors map to an unchecked ErrorPageException, which then displays an html error page.
When the request passes the above, errors tend to be reported back to the caller via redirect_uri already.
This PR proposes to first catch these ErrorPageExceptions and then (when configured to do so) make a "best effort" to use the redirect_uri path - otherwise, the error is returned to the caller as json object.
Without the
authorization.preferErrorOnRedirectattribute on the realm, this PR changes nothing. As a result, no test cases needed to be touched. That was in fact the hard bit: change nothing, unless the headless error path is wanted.Automated conformance tests are headless - they prefer errors to be delivered on the redirect_uri when possible.
If this approach gets accepted, we would likely want to expose that switch on the admin UI, which is not (yet) part of this PR.
This PR is at the bottom of all other HAIP conformance work.