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

Use HTTP GET for list requests #111

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

Conversation

orf
Copy link
Contributor

@orf orf commented Nov 8, 2024

Closes #109

This is the branch we are currently using, I’ll fix it up

@orf
Copy link
Contributor Author

orf commented Nov 8, 2024

I was not sure how to add default GET parameters to a request model - if there’s a better way let me know.

I’ll remove the extra commits later on

@orf orf force-pushed the list-request branch 2 times, most recently from af6add5 to b5f27da Compare November 13, 2024 14:12
@Haennetz
Copy link
Collaborator

Thanks for your PR. I would prefer to add an option that allows the developer to control if he would like to use the LIST or the GET method to accomplish this. It could also be that a reversed proxy only allows the LIST method to prevent that secret data are leaked.

@orf orf force-pushed the list-request branch 2 times, most recently from c2ceb44 to f7674c9 Compare November 16, 2024 22:05
@orf
Copy link
Contributor Author

orf commented Nov 16, 2024

Thanks @Haennetz - I've added kv2::list_with_http_get and kv1::list_with_http_get methods to let the developer control this, and I've added tests.

@Haennetz Haennetz self-requested a review November 16, 2024 22:23
@Haennetz
Copy link
Collaborator

Thanks @orf and nice work there are only some formatting issues. @stormshield-gt is it okay for you if we Marge this PR and you add the tests to #107?

@orf
Copy link
Contributor Author

orf commented Nov 16, 2024

Thanks! I've fixed the formatting 👍

@stormshield-gt
Copy link
Collaborator

#107 has just been merged.
@orf do you mind doing a rebase?

I want to take some time to evaluate if there are other alternatives to this. Maybe we should also generalize the passage of LIST to GET to all endpoints?

@orf orf force-pushed the list-request branch 2 times, most recently from 4dc0a53 to bebeb3f Compare November 20, 2024 15:51
@orf
Copy link
Contributor Author

orf commented Nov 20, 2024

I've rebased @stormshield-gt <3

It's a tricky issue: on one hand, switching from LIST to GET is supported by the API and is contractually the same. On the other hand it feels like a breaking change.

However perhaps that's over-thinking things: we don't need to be constrained by hyrum's law if we don't want to be, and it's unlikely that this would cause major issues for users given that the Vault API explicitly supports this as an alternative to LIST. The specific case would be someone restricting things to only LIST requests, but... that's unlikely.

Perhaps we should just switch things to be GET requests and remove LIST entirely?

@orf orf force-pushed the list-request branch 3 times, most recently from abe06d2 to a0b89f8 Compare November 20, 2024 17:28
@stormshield-gt
Copy link
Collaborator

Hi, sorry for the late response. I agree we shouldn't constrain ourselves to the hyrum's law. I think it mostly applies when you have a lot of users, and that's not the case currently.

Perhaps we should just switch things to be GET requests and remove LIST entirely?

That's the direction I want to go. What do you think @Haennetz?

I will be on vacation for a few weeks.

@Haennetz
Copy link
Collaborator

Sure we can switch completely to GET requests.

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.

Issues making LIST requests
3 participants