Skip to content

Refs #38124 - Add correct description for invalidating multiple users jwt #637

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

girijaasoni
Copy link
Contributor

No description provided.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this? On latest hammer-cli and hammer-cli-foreman I get:

Screenshot-1739795257755

UPD: We could rather treat this PR for #636 (comment)

@girijaasoni
Copy link
Contributor Author

Do we really need this?

Yes, because when passing the search params from CLI, we don't need to URL encode the search query. We need to do that only when using curl command

I can add tests too in a new ref PR

@ofedoren ofedoren changed the title Correct desciption Refs #38124 - Add correct description for invalidating multiple users jwt Feb 17, 2025
@ofedoren
Copy link
Member

Yes, because when passing the search params from CLI, we don't need to URL encode the search query. We need to do that only when using curl command

I'd say we can then remove URL-encoded from API description as well. curl can be used in a more user friendly way without a need to encode anything: curl -u user:password -H 'Content-Type:application/json' -X DELETE https://foreman.ofedoren.com/api/v2/registration_tokens -d '{ "search": "id ^ (5)" }'

I can add tests too in a new ref PR

👍

@girijaasoni
Copy link
Contributor Author

girijaasoni commented Feb 20, 2025

without a need to encode anything: curl -u user:password -H 'Content-Type:application/json' -X DELETE https://foreman.ofedoren.com/api/v2/registration_tokens -d '{ "search": "id ^ (5)" }'

We are sending search params as a part of url parameters. We need to URL encode it there
it is something like curl --user user:password -X DELETE http://localhost:3000/api/registration_tokens?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json

@ofedoren
Copy link
Member

We are sending search params as a part of url parameters. We need to URL encode it there it is something like curl --user user:password -X DELETE http://localhost:3000/api/registration_tokens?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json

Who are we and why do we do that? I mean, the API can be used that way, sure. But that's just a curl example, which can accept both ways (your and mine) to pass the parameters. Most tools/libs will just do that for you.

Wording URL-encoded even in API is kinda misleading since it forces one of the variants. Moreover, we don't do that for other search string params:

Screenshot-1740049729399
Screenshot-1740049743796

@girijaasoni
Copy link
Contributor Author

We are sending search params as a part of url parameters. We need to URL encode it there it is something like curl --user user:password -X DELETE http://localhost:3000/api/registration_tokens?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json

Who are we and why do we do that? I mean, the API can be used that way, sure. But that's just a curl example, which can accept both ways (your and mine) to pass the parameters. Most tools/libs will just do that for you.

@ShimShtein , what do you think about this?

@girijaasoni
Copy link
Contributor Author

converting this PR to draft. will be using it later for adding hammer tests.

@girijaasoni girijaasoni marked this pull request as draft February 27, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants