-
Notifications
You must be signed in to change notification settings - Fork 8
Fixes #38530 - proxy the flatpak index #54
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
Conversation
| query = "" | ||
| unless params.empty? | ||
| query_parts = params.map { |key, value| "#{key}=#{value}" } | ||
| query = "?#{query_parts.join('&')}" | ||
| end | ||
| uri = URI.parse("#{@pulp_endpoint}/pulpcore_registry/index/static#{query}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like the parameters to be escaped. I'd suggest this:
| query = "" | |
| unless params.empty? | |
| query_parts = params.map { |key, value| "#{key}=#{value}" } | |
| query = "?#{query_parts.join('&')}" | |
| end | |
| uri = URI.parse("#{@pulp_endpoint}/pulpcore_registry/index/static#{query}") | |
| uri = URI.parse("#{@pulp_endpoint}/pulpcore_registry/index/static") | |
| uri.query = params.map { |k, v| "#{URI.encode_uri_component(k)}=#{URI.encode_uri_component(v)}" }.join('&') if params.any? |
I really wish there was a method similar to URI.encode_www_form that uses URI.encode_uri_component and accepts a hash, but I can't find it.
And the tags method probably needs the same fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised too that there wasn't a cleaner way to build the URL with the query from a hash with escaping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swear I saw it used somewhere because I too thought there must be an easy way, but I couldn't find it. Feels so stupid because it's so extremely common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time to enhance encode_www_form with an escape arg...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh - encode_uri_component doesn't seem to exist before Ruby 3.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the + that encode_www_form uses for spaces is okay.
19292ee to
6867afd
Compare
6867afd to
e69ca68
Compare
| inject_attr :container_gateway_main_impl, :container_gateway_main | ||
|
|
||
| get '/index/static/?' do | ||
| # TODO: filter out repositories that are not tied to the (optional) authenticated host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the plan with this TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this merges we're going to implement filtering the index by host. Originally I was thinking of doing that at the same time but it depends on another PR, so I'm just saving my code for the next developer who comes by.
| # end | ||
|
|
||
| # pulp_index = JSON.parse(pulp_response.body) | ||
| # pulp_index["Results"] = pulp_index["Results"].select { |result| catalog.include?(result["Name"]) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's cheaper to inline the change
| # pulp_index["Results"] = pulp_index["Results"].select { |result| catalog.include?(result["Name"]) } | |
| # pulp_index["Results"].select! { |result| catalog.include?(result["Name"]) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that should be a bit better.
e69ca68 to
244ca24
Compare
| def flatpak_static_index(headers, params = {}) | ||
| uri = URI.parse("#{@pulp_endpoint}/pulpcore_registry/index/static") | ||
| unless params.empty? | ||
| uri.query = URI.encode_www_form(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'm going with encode_www_form since encode_uri_component isn't available in the versions of Ruby we use (3.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found https://bibwild.wordpress.com/2023/02/14/escaping-encoding-uri-components-in-ruby-3-2/ as a very good write up of he mess. encode_www_form is invalid because it encodes a space as + instead of %20. It points to the very unlikely ERB::Util.url_encode as a cross version compatible API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some research before and + subbed for seems to be regarded as safe in the query string, at least according to forums. This misguidance is probably what blog refers to in the "Why is this so confusing and weird?" section. Even then it wasn't super clear - I might've just stumbled upon some forums that were more misguided than others :)
Anyway, it's clear that %20 is most correct. I'd much prefer supporting getting away from using + chars in a strange way.
Including ERB for this does feel a little odd, but it sounds safe enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use ERB::Util.url_encode
|
Functionally this is working well..Tested adding proxy as the flatpak remote and able to install apps with basic auth. |
244ca24 to
3e6a1a4
Compare
3e6a1a4 to
e3a167f
Compare
sjha4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack..This has been working well in my testing..
|
Since there is an ACK and the previous comments have generally been addressed: merging. |
Serves the Flatpak index at /index/static.
Also prepares the code to filter the index based on the authenticated client. Includes sample code.
This will require a change to the Apache config: theforeman/puppet-foreman_proxy_content#522