Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin UI drag & drop in flow config seems to delete actions #28187

Closed
1 of 2 tasks
lf- opened this issue Mar 24, 2024 · 4 comments · Fixed by #28342
Closed
1 of 2 tasks

Admin UI drag & drop in flow config seems to delete actions #28187

lf- opened this issue Mar 24, 2024 · 4 comments · Fixed by #28342
Assignees

Comments

@lf-
Copy link

lf- commented Mar 24, 2024

Before reporting an issue

  • I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

admin/ui

Describe the bug

When I drag a subflow with subflows in it into another subflow, it deletes a bunch of steps!

Version

24.0.1

Regression

  • The issue is a regression

Expected behavior

I would like keycloak to not delete my authentication flows.

Actual behavior

Keycloak seems to delete bits of my authentication flows if I drag subflows around.

How to Reproduce?

Here is a 1 minute video of reproducing the issue, with flow names that do not dupe any others on the server:

Kooha-2024-03-23-19-24-47.webm

Anything else?

This appears to be a case of #25263 and #21960 striking again!

Overall, this issue recurring is a sign of major fragility in how the UI does this work, and that it is not atomic. I hit this issue on real flows, and it was frustrating, but at least it was in a test environment.

@lf- lf- added kind/bug Categorizes a PR related to a bug status/triage labels Mar 24, 2024
@lf-
Copy link
Author

lf- commented Mar 24, 2024

@edewit you seem to have worked on the previous two occurrences of this bug, so here's a ping

@edewit edewit changed the title Admin UI drag & drop in flow config deletion strikes again!! Admin UI drag & drop in flow config seems to delete actions Mar 25, 2024
@edewit
Copy link
Contributor

edewit commented Mar 25, 2024

The to issues that you have linked have nothing to do with this as they where about the config of the actions not the actions themselves. I get that issues in the UI can be frustrating, but I would like it if you wouldn't take out your frustration on me

@lf-
Copy link
Author

lf- commented Mar 25, 2024

The to issues that you have linked have nothing to do with this as they where about the config of the actions not the actions themselves. I get that issues in the UI can be frustrating, but I would like it if you wouldn't take out your frustration on me

Sorry, I realize I wrote this while frustrated and really should have done another pass over the wording, and I was primarily pinging you because you've worked on this area of the UI before. I will do better in the future.

Although I haven't reproduced #25263 personally, it looks like the same type of bug, since it seems to say the executions go missing due to what sounds like an API error in re-adding them after deleting them. It looks, admittedly without deeper inspection, to me like some kind of server API failure conspiring with the client first deleting then recreating the subflows non-atomically, such that the deletion succeeds, but then the recreation fails and the client drops the flows on the floor.

Is this a correct characterization of how it works? Would it be reasonable to at least give the flows back to the user in text form or something that they could restore on the server, in case a bug affecting flow recreation happens in the future, assuming this non-atomic design I inferred is indeed what is implemented? Or, could it be done in an atomic transaction on the server side?

My point with how it seems that this recurred is that it may be the case that the way that moving flows works in the UI is unsound with respect to API failure, but I have not dug into the code to find that out with certainty.

@edewit
Copy link
Contributor

edewit commented Mar 28, 2024

ATM the server doesn't support moving flows and adding this api is of low prio, so what we did is solve this on the client. It is true that it's not atomic and it would be better to do this on the server, but there shouldn't be any reason for a create or a delete would fail because the admin ui isn't used by a ton of users at the same time.

What the UI is trying to do is calculate what operations to do based on what items have been moved and in this logic there is a failure resulting in a create that is invalid therefore only deleting.

So the ultimate solution to this would be either solving it on the server so that moving would be something that is supported and can be done atomic or removing this moving logic completely and return to what it was in the old console where you had to delete and re-create yourself

I still think it's possible to get all the bugs out of this system and have it work

edewit added a commit to edewit/keycloak that referenced this issue Apr 2, 2024
fixes: keycloak#28187
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
edewit added a commit that referenced this issue Apr 2, 2024
fixes: #28187

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
edewit added a commit to edewit/keycloak that referenced this issue Apr 2, 2024
fixes: keycloak#28187
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
edewit added a commit that referenced this issue Apr 30, 2024
fixes: #28187

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants