[v23.2.x] admin: fix path param parsing in admin api #16813
Conversation
pgellert
requested review from
BenPope,
oleiman,
graphcareful and
michael-redpanda
|
Looks like this got sort of mangled (or I'm reading it wrong)
Given (1) maybe we should ditch this backport? Or also cherry-pick the original |
This adds a utility method to parse/unescape path parameters in urls. This was motivated by an issue where the admin API was using query parameter unescaping to parse the username path parameter in ACL delete and update requests, in which case the + sign was incorrectly replaced with a ' '. This prevented admins from deleting users with a + in their username. (cherry picked from commit 27fe9c6)
This uses the new path parameter parsing method to correctly decode path parameters in the admin API. This was motivated by an issue where the admin API was using query parameter unescaping to parse the username path parameter in ACL delete and update requests, in which case the + sign was incorrectly replaced with a ' '. This prevented admins from deleting users with a + in their username. (cherry picked from commit 02bb44e)
This adds a regression test to test that we can both create and delete a user with a complex username (containing symbols that are considered to be special symbols in the url syntax). This is tested through a ducktape test using rpk to validate that the url encoding and decoding works well throughout the interaction of rpk and the admin API. This adds a regression test to test that we can both create and delete a user with a complex username (containing symbols that are considered to be special symbols in the url syntax). This is tested through a ducktape test using rpk to validate that the url encoding and decoding works well throughout the interaction of rpk and the admin API. (cherry picked from commit c55f603)
(cherry picked from commit bf6c0f6)
pgellert
force-pushed
the
vbotbuildovich/backport-16694-v23.2.x-541
branch
from
89f3433 to
5a9169a
Compare
Yes, it's made a bit more complex because the admin API code was only separated out into the
It is getting called but as I was cherry picking and resolving conflicts the change to exercise the new functions got ahead to the first commit. I've fixed this now in the force-push to moves this back to the second (now non-empty) commit to align with the original PR. Based on the Slack discussion, I think this should be good to merge as is, but let me know what you think. |
Ah, yeah, my diffs were noisy from the admin/util changes, so I missed it. Sounds good. For the admin/util changes themselves, I think doing it this way would make backporting the original admin/util change more difficult, but presumably there's no plan to do that. If you're unsure, you might solicit a 3rd set of eyes, but it seems fine to me. |
Backport of PR #16694