Repository navigation
Always order required actions by priority (regardless of context) - #28956
Conversation
58d51bc to
16a9926
Compare
|
Draft PR for solving the linked issue. Currently draft only, because there are some (already) existing tests to be fixed - or maybe further existing code to be adjusted. |
|
@thomasdarimont Could you please review this PR? Is it aligned with your prototype in the PR #28419 ? |
There was a problem hiding this comment.
LGTM Just one minor nitpick about naming that we could change.
I think the rest is pretty solid! Great work @danielFesenmeyer!
I haven't tried it, but I just want to confirm: What happens when a required action is explicitly targeted via the kc_action parameter? Will the other actions still be ignored as before? Same question for executing an action via E-Mail link.
|
@thomasdarimont Thanks for your feedback. I'll have a deeper look at the |
16a9926 to
35b4085
Compare
35b4085 to
96dfcd8
Compare
19f0cf8 to
ce338aa
Compare
ce338aa to
abee8c2
Compare
|
My code still had a bug regarding the "cancel-AIA" behavior: the latest draft now considers that an application initiated action (AIA) cannot be cancelled. All tests - except one - are succeeding now without further adjustments. The only test to adjust was WebAuthnRegisterAndLoginTest#webAuthnPasswordlessAlternativeWithWebAuthnAndPassword: |
Afaik, the other actions were not ignored in case of the |
|
Awesome thanks @danielFesenmeyer for the great work! I'm unsure about the behavior change and I'd like to get the opinion of @mposolda for this. |
There was a problem hiding this comment.
@danielFesenmeyer @thomasdarimont Thanks for this PR and feedback! To me, the behaviour change is ok. The new behaviour is much better IMO.
|
@danielFesenmeyer The thing is, that this branch has conflicts. Can you please rebase and fix the conflicts? I hope to merge this PR afterwards. |
abee8c2 to
951f819
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.session.UserSessionPersisterProviderTest#testMigrateSessionKeycloak CI - Store Model Tests |
|
@mposolda Rebased - new flaky test is unrelated to the changes from my POV. |
- AuthenticationManager#actionRequired: make sure that the highest prioritized required action is performed first, possibly before the currently requested required action - AuthenticationManager#nextRequiredAction: make sure that the next action is requested via URL, also based on highest priority (-> requested URL will match actually performed action, unless required actions for the user are changed by a parallel operation) - add tests to RequiredActionPriorityTest, add helper method for priority setup to ApiUtil (for easier and more robust setup than up-to-now) - fix test WebAuthnRegisterAndLoginTest - which failed because WebAuthnRegisterFactory (prio 70) is now executed before WebAuthnPasswordlessRegisterFactory (prio 80) Closes #16873 Signed-off-by: Daniel Fesenmeyer <daniel.fesenmeyer@bosch.com>
951f819 to
071e90b
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.session.UserSessionPersisterProviderTest#testMigrateSessionKeycloak CI - Store Model Tests |
There was a problem hiding this comment.
@danielFesenmeyer Thanks!
Closes #16873