-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add proxy support #149
Add proxy support #149
Conversation
@eeintech congrats on the release :D I just synced my branch so no problems on this side. The only reason I splitted this into a different merge request is, if the change to the parameter mapping conflicts with anything you meant to do with it or if you don't like my solution there, then we could include the proxy stuff independently. So do as you please :D |
@T0jan Thanks, I'll have a look this week 👍 |
Hello @T0jan I'm trying it out however for the proxy part, I don't really have a good way to test if it works. The settings view looks good to me: Does it needs only the two extra fields HTTP and HTTPS to work? I am able to connect to my instance when both fields are empty, then I tested:
[17/Apr/2023 11:52:00] "GET http://127.0.0.1:8000/api/ HTTP/1.1" 302 0
[17/Apr/2023 11:52:00] "GET http://127.0.0.1:8000/accounts/login/?next=/http:/127.0.0.1:8000/api/ HTTP/1.1" 302 0
[17/Apr/2023 11:52:00] "GET http://127.0.0.1:8000/accounts/login/?next=/http:/127.0.0.1:8000/accounts/login/ HTTP/1.1" 302 0
[17/Apr/2023 11:52:00] "GET http://127.0.0.1:8000/accounts/login/?next=/http:/127.0.0.1:8000/accounts/login/ HTTP/1.1" 302 0
[17/Apr/2023 11:52:00] "GET http://127.0.0.1:8000/accounts/login/?next=/http:/127.0.0.1:8000/accounts/login/ HTTP/1.1" 302 0 but eventually fails and gives me the error message: I have tried adding a random string and it keeps showing me the error message so that's good 👍
|
this is to be expected. basically the GUI now sends proxy request to your localhost server which this one can't interpret as it is not configured as a proxy server. if you want to test with a proxy server you can use any from a free proxy server, e.g. one from here
we could add a special error message based on the requests module error case
Yes, this is expected. if you want to connect to an HTTP-server it will use the HTTP-proxy and for a HTTPS-server the HTTPS-proxy. thinking about it probably would make sense here to give the user only one proxy input field here and letting the program decide (based on the server URL) if it is HTTP or HTTPS. do you think this would make things clearer for the user?
no. server communications just wouldn't work as the program then sends an HTTPS (proxy) request to an HTTP server which this one obviously couldn't decrypt. would throw the same |
Alright so I used one of the servers, put a random token (else it asks for username) and I'm getting this: Server connection error: <class 'requests.exceptions.SSLError'>
[TREE] Successfully connected to InvenTree server (ENV=DEVELOPMENT) So not sure if that actually works...
I guess for now the general message is clear, something is wrong with the InvenTree settings.
One field would definitely be clearer than two and lead to less confusion I believe. If you can use a generic name like |
Hello again @T0jan Regarding those two items:
I understand you'd like the Regarding the |
hi @eeintech
have you tried with different proxy servers and to which inventree server do you wanted to connect? The servers in the list often don"t work so you have to try a bit to find a working one. I managed to get a connection to the inventree-demo server with:
as well as:
I am not sure how reliable these proxy servers are. I usually use them only for immediate testing and have sometimes to search a while to find a working one. So we could often get errors in run_tests.py only because the proxy is not working.
Maybe it would be even a better idea to hide this field behind an additional switch, similar to the input fields for the IPN. 🤔
No you just have to construct the URL correctly like in my two examples above: protocol://address:port
Sounds like a lot cleaner approach then my try to press it into the current structure 😄 The only problem I see here is that you then get two config files (the supplier_parameters.yaml and the supplier_config.yaml) the user needs to modify to get the data actually into inventree. but maybe the supplier_config.yaml can hold the most common extra fields a user would need by default so only special cases would have to look into this file? |
Right both are working for me as well.
True... would be nice to find a permanently working proxy but we can keep it for a later stage.
I think that's a great idea to avoid basic users (like me) confusion. One switch, one field and proxy can be used.
The |
caefd31
to
87baafd
Compare
at some point I will learn to check linting before I push... @eeintech I just added the discussed hiding of the proxy setting. for the extra_fields do you have already an idea how you want to do it? then I would revert my commits related to the ECCN field and you can work cleanly on it in your repo. |
I sure can do that, if you don't mind reverting that change. About the proxy implementation, the settings look good: Without proxy it works great. I'm getting errors like: Unhandled server error: <class 'requests.exceptions.InvalidSchema'>
Server connection error: <class 'requests.exceptions.ProxyError'>
Server connection error: <class 'requests.exceptions.ConnectTimeout'> Also, how do you test connecting with a token only? Where do you get the token from? |
9790590
to
c0f12da
Compare
@eeintech ok the rebase went not as good as I hoped but it should be fine now. so the changes for the ECCN fields are gone.
unfortunately not, I tried with a few handful and had no luck. but at least with my own server I could connect via proxy so I know it works.
you can request the token directly from the server by sending a request to your server api with server_api_url/user/token/ and your credentials. with this command:
the gui basically also requests the token in the background, holds it as long as it is running and makes every api request to the database with this token, not the username and password. the inventree_python function to get the token is part of the InvenTreeAPI class: requestToken() |
Hello @T0jan Couple comments in your PR, could you please review? Other than that I'm happy to merge 😃 |
Could you also rebase on main? it created a lot of duplicate commits 😕 |
load_category_parameters no processes the whole category tree in reverse and picks the mapping for the lowest category in the tree. so if for a tree like 'Passive Components' - 'Resistors' mappings exist for Passive Components as well as for Resistors the mapping for resistors will be used.
d860d88
to
b20b70f
Compare
@eeintech 2nd rebase went better, should have a clean commit history now. I modified the README and cleaned up the Proxy settings and definitions in case no proxy is used as requested. On the token topic: I was thinking about adding a third button next to save to replace the user credentials with the token automatically as all requests in the background are anyway done with the token method. any opinions on this? |
Thanks, I'll take a look!
So a "Get Token" button? But this will overwrite the password/token field and then it won't be valid next time Ki-nTree is opened... am I understand this correctly? Would this button useful for proxy method only, because I don't see why using it when you have your username/password filed in? |
Everything looks good to me and I'm happy to merge as-is unless you really think adding the "Get Token" button is necessary to simplify the proxy workflow. |
exactly.
It would be valid. each user in InvenTree has a persistent token assigned to him which can be used for authentication as long as the user exists (and has the access rights to the specific information). it is not token which is connected to user sessions and therefore would needed to be updated each time you connect to the server. It is just a different kind of authentication method compared to user credentials and not connected to the proxy-topic. otherwise the addition of saving the token instead of the password wouldn't make much sense 😅 But I think we can skip the button for now. at least as long until my colleagues will start asking me about how they can get their token 🙃 |
@T0jan Thanks for the feedback. Let's go with what you proposed so far, we can discuss the extra button for another release, if really necessary. Thanks a lot for your contribution 👍 |
A small change concerning parameter handling. Consists of two parts:
also contains the commits of #146 because I am too stupid for correct branching 😅