-
Notifications
You must be signed in to change notification settings - Fork 241
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 manual proxy settings manager in network settings #665
Conversation
Hi @ManashRaja, thanks for working on this, it looks like a very useful feature for schools that require proxies!! I am yet to review this in depth, but just a few quick comments:
Anyway, looks great. Hopefully we an work this up to inclusion in the next release! |
Hello @samdroid-apps,
|
Hello @samdroid-apps, Thanks |
Please also review godiard@ebfd1ed. |
|
|
|
@quozl : Thanks for the suggestions to use /jarabe/main.py .
Can you help me figuring out why is the sugar.in included in this commit even if it is same with the sugarlabs/sugar repository? |
You changed tabs to spaces. You have an editor which does not show this properly. You might do this to fix;
Then amend your commit and force push. Medium term, before commit make sure you understand the "git diff" output fully, and revert any changes you don't want. I use Magit in Emacs, which shows me tiny changes that I can revert by keyboard shortcut. Long term, use an editor that shows the difference between tabs and spaces. My preference is emacs with some custom Lisp code. The Emacs Wiki has an article on visible tabs. https://www.emacswiki.org/emacs/ShowWhiteSpace Also stackoverflow.com has several questions on the same issue. Do some research. |
":" + str(g_socks.get_int('port')) + "/" | ||
os.environ['socks_proxy'] = text_to_set | ||
|
||
elif g_mode == 'none': |
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.
Here you erase the environment variables if the setting is set to none. I'm surprised. What if the system administrator has set a default proxy in /etc/profile.d or other scripts? The configuration would be lost.
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.
The final setting in the environment variable will be set by the script that runs last. I just tested setting the proxy at both /etc/environment file and a custom /etc/profile.d/proxy.sh file and I found that the file at /etc/profile.d is getting called after the execution of /jarabe/main.py. In fact the sequence seems to be /etc/environment => jarabe/main.py => profile.d/prosh.sh
Hence, it system administrator may set the proxy settings at /etc/profile.d
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 disagree. I think you tested with Terminal activity, which is a new shell process, which therefore executes /etc/profile.d again. Test instead with an activity such as Browse, which is not a shell interpreter.
Besides what you say; /etc/environment may be the file a system administrator sets proxy, and if so your proposed code persists in losing the configuration.
Why do you want to lose the system-wide configuration?
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 tested with browser and found as you indicated that the proxy settings are as per the /jarabe/main.py. What do you suggest how a system administrator should be able to set default proxy?
@quozl How do you expect the proxy settings be set in case of a system administrator settings default proxy settings? |
@ManashRaja, in the systems I've administered for the past 30 years, there has been no clear standard for where and when to set environment variables. Some systems use shell initialisation files, all of which depend on which shell the user has selected using chsh. Some systems use graphical shell initialisation files, like .xsession or xinitrc. Some set the environment variable in the process that starts the shell, e.g. sshd, lightdm, getty; although this has tended to become unsupported so that environments are pristine and not inherited. You will see in Firefox, Preferences, Advanced, Network; use system proxy settings. So perhaps you should have that as an option in the UI? |
Ya, so as you mentioned that a system administrator need to set up default proxy settings sometimes. Also that there haven't been a clear standard for the settings. I wonder after this feature is implemented, why would a system admin not set the proxy through the "Network" control panel itself rather than using any of his own script? |
|
[03:25] <ManashRaja> Ya, so I am adding a new option "Use system proxy". To implement it, I was thinking of writing a custom schema. Because this option can't be accomodated in "org.gnome.system.proxy mode" as it only accepts "none", "manual", and "auto". [03:47] <Quozl`_> ManashRaja: my guess is that "none" means not to set the environment variables. [03:47] <Quozl`_> ... or "auto". Have you found code that uses org.gnome.system.proxy to be able to check? [03:50] <ManashRaja> "manual" and "auto' are types of settings. Then I think "none" is for no proxy settings at all and direct connection, where I beleive the proxy settings should be cleared and hence for adding the feature of letting user set system proxy, we may have another option "use system proxy" [03:51] <Quozl`_> ManashRaja: have you found the GNOME equivalent code for using the setting? [03:53] <ManashRaja> Quozl`_: I do have the one that uses the standard shema "org.gnome.system.proxy" as I implemented in my PR so far. But to add another option, I am thinking of creating a new custom schema "org.sugarlabs.systemproxy" [03:54] <Quozl`_> ManashRaja: you mean "GNOME schema", not "standard". GNOME isn't a standard, it's a desktop environment. [03:54] <ManashRaja> for that I found /sugar/data/org.sugarlabs.gschema.xml where we can add custom schemas [03:54] <Quozl`_> (and there are other desktop environments). [03:57] <ManashRaja> Quozl`_: Oh, sorry for that. I used the gnome based schemas as they were used in https://github.com/godiard/sugar/commit/ebfd1ed0f941dca982b90d0d08efda7c2ae75a84 you sent to me to review [03:58] <Quozl`_> ManashRaja: I've no idea why those were used, I was not part of the development. [03:58] <ManashRaja> We might instead use sugarlabs based schemas instead "org.sugarlabs.system.proxy" which I guess will be more apt and good for customization as well as logical in terms of sugar as a desktop environment having its own schemas [03:59] <Quozl`_> Yes, we can alsop make the options mean what we mean instead of being unable to explain what they mean. [04:00] <ManashRaja> Quozl`_: Yes, customizability will be an advantage. So, I will create new schemas then by modifying /sugar/data/org.sugarlabs.gschema.xml file [04:00] <Quozl`_> Ok. [04:01] <ManashRaja> and sugar/data/sugar-schemas.convert file also. [04:01] <ManashRaja> Cool |
@quozl The option has been included with custom schemas :) |
g_http_host = g_http.get_string('host') | ||
g_http_port = str(g_http.get_int('port')) | ||
if(g_http.get_boolean('use-authentication')): | ||
text_to_set = "http://%s:%s@%s:%s/" % ( |
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.
Wouldn't it make sense to also have the authentication with https proxies?
Disclaimer: never used a proxy in my life.
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 wrote the settings on the basis of how it has been shown in archlinux [1]
export http_proxy="http://$pre$server:$port/" #$pre = "$username:$password@" used only for http
export https_proxy=$http_proxy
export ftp_proxy=$http_proxy
export rsync_proxy=$http_proxy
export HTTP_PROXY=$http_proxy
export HTTPS_PROXY=$http_proxy
export FTP_PROXY=$http_proxy
export RSYNC_PROXY=$http_proxy
And proxy setting implementation in Gnome [2]. The following are the settings available in Gnome that are related to proxy server.
org.gnome.system.proxy autoconfig-url ''
org.gnome.system.proxy ignore-hosts ['localhost', '127.0.0.0/8']
org.gnome.system.proxy mode 'none'
org.gnome.system.proxy use-same-proxy true
org.gnome.system.proxy.ftp host ''
org.gnome.system.proxy.ftp port 0
org.gnome.system.proxy.http authentication-password ''
org.gnome.system.proxy.http authentication-user ''
org.gnome.system.proxy.http enabled false
org.gnome.system.proxy.http host ''
org.gnome.system.proxy.http port 8080
org.gnome.system.proxy.http use-authentication false
org.gnome.system.proxy.https host ''
org.gnome.system.proxy.https port 0
org.gnome.system.proxy.socks host ''
org.gnome.system.proxy.socks port 0
So, I seems that authentication is required for only http_proxy.
Note: I do use proxy server which is http but does not require authentication.
[1] https://wiki.archlinux.org/index.php/proxy_settings
[2] https://developer.gnome.org/ProxyConfiguration/
You don't seem to do anything with the Have you tested |
Also, how do I test? I did:
Expected: Browse could not go to http sites Actual: Browse loaded http://example.com just fine |
@samdroid-apps ,
[1] https://wiki.archlinux.org/index.php/proxy_settings |
Shall I add ignore-hosts option? I almost forgot it as the option was not there in Ubuntu proxy setting page. |
@samdroid-apps I have added ignore hosts option so that user can define hosts for which proxy settings are not required. I added this option to 'manual' type proxy alone as in case of gnome Network Manager. Here is the screen-shot of how it looks now: So, with this, I guess all the work have been done and the feature is fully functional as it is in gnome desktop environment. |
Squashed multiple commits to one. |
What is the risk of misconfiguration? Many years ago, one of the reasons put for why this feature should not be merged is that it would be too easy for a very early learner to enter bad data into the fields and stop all internet work, and it would take a long time for a teacher or peer to identify the problem. What happens when garbage text or incorrect data is entered into the new control panel widgets? |
If garbage text or incorrect data is entered in the settings, then the internet connection will suffer. But, I don't think this feature will bring on serious risks as:
Hence, my point is that, even though it might be easy for someone to mess with the settings as is the case with all settings, but still it would be very easy for a teacher or peer to identify the problem as they have to look at only one place which is the ultimate place that dictates proxy settings. :) |
Can you ping the proxy before you set it? Eg, make the flow like:
|
@quozl Browse does not report that the proxy is invalid. It shows the following error in any case when it is not able to access the internet. It does not even mention about troubleshooting the proxy settings. |
@samdroid-apps Suggest edits please.
|
When the dialog with the 'break my internet' button is shown, is it still
possible to click the "restart now" or "later" buttons at the top?
|
@davelab6 Yes, it is possible to click the buttons on the top as I am not able to figure out a way to hide them or disable the buttons. The top alert bar is created by "jarabe/controlpanel/gui.py". I am not able to understand the inheritance and how the different classes (ControlPanel, SectionView, Newtork) are being set up and hence am not able to call the methods present in the above file from the "cpsections/network/view.py" file. What I find is that at present there isn't a framework to modify the flow of how alerts are displayed and methods are called to suit our needs other than the existing restart alert which we can only enable by calling "self.needs_restart = True" and can't disable afterwards. At present we can only modify the alert title and msg. |
@ManashRaja, thanks for the Browse screenshot; confirms that Browse does not explain the problem, and supports our need for checking the proxy settings in another way. Yes, jarabe/controlpanel/gui.py is quite complex, but with a careful review you should be able to work out what is needed. Follow the control flow to see how a cpsection module is imported. It is unconventional, because we have the flexibility to add cpsection modules from outside Sugar. Examine other cpsection modules; I seem to recall there may be one that calls back into the gui.py class. The double alert looks like it might be confusing. Other sections use a triangle icon to identify problems. The + and - buttons on the port numbers are really not required; there's almost never a situation where a user will need to use them, and so they could be the cause of accidental misconfiguration. |
@quozl Hence I can't find a way to actually modify the alert created by gui.py from view.py due to encapsulation without actually passing the instance of gui.py to view.py through the |
Thanks, nice summary. Yes, include gui.py in your edits and let us see what you suggest. See if you can make your changes to gui.py in a cautious and re-usable way that provides an API to section instance, rather than only expose data. |
@quozl , Thanks
|
You wrote:
You could make that an optional named argument? That way you will not need to change any of the other modules. Reference found in a hasty search; http://www.diveintopython.net/power_of_introspection/optional_arguments.html |
@quozl , no Sir, But the function I am dealing with accepts lesser arguments than I am passing it. |
Well, I'm surprised, but you're right, I had things the wrong way around. So you are proposing to add an argument to each class. The reason this is bad is that we have external and third-party control panel sections, not just at OLPC, but also at deployments. You can't edit them. Breaking the API like this must be avoided, if we can. I'm looking for an alternative way to implement. How about a new signal request-alert sent by the section view (self.emit), and a connect in ControlPanel.show_section_view to redirect to a callback method? (Others will also reject this patch because you are using private variables in the ControlPanel class from outside the class. Private variables have an underscore (_) prefix.) |
Please take over the feature page; https://wiki.sugarlabs.org/go/Features/Proxy_Settings ... it hasn't changed from 2013. Delete any old information. |
Oh cool. 👍 I will try the signal and callback method and report back. |
Sorry, I don't know the process for inclusion in next release, but it looks like you just add a link to the feature page in one of the next release subpages; https://wiki.sugarlabs.org/go/0.110/Feature_List |
Thanks. I am learning new things everyday and is so exciting. :)
|
Quick review:
More review later. |
Edits done. Will squash all commits to one after final review and edit. |
Feature to set proxy settings for both manual and automatic (PAC) types are added to the 'Network' section of 'My Settings' control panel. The settings also support authentication for http proxy servers. The /jarabe/ main.py file where the proxy settings get exported finally is run after any other standard scripts like /etc/environment and those inside /etc/profile.d/, hence any proxy setting exported from such files gets overwritten by the main.py. Therefore a new option ('Use system proxy') has been added to the settings options list along with the standard 'none', 'manual', and 'auto', where in the main.py does not export the proxy settings to environment variables. Once the settings are changed and the user clicks on toolbar to save it, then the settings are verified. For manual proxy, the host address that is provided is pinged and for auto proxy, the availability of the pac file is checked. If the vefification fails, then an alert is shown with options to either revert the changes or apply them anyway. GObject signals are emitted by the view.py to perform the following: - 'add-alert' : adds the restart alert so that it can be added only when required. - 'set-toolbar-sensitivity' : Set the sensitivity of top toolbar buttons. The above two features were added to handle the gui so that there is a straightforward user flow through the setting options. Implementation: It is based on a patch by Manuel Quiñones [1]. The GUI is based on the Gnome 3 proxy settings to maintain consistency. The settings are stored at two standard locations for diverse accessibility: - schemas : custom defined schemas (eg 'org.sugarlabs.system.proxy') - environment variables : (eg 'http_proxy') Custom schemas (eg 'org.sugarlabs.system.proxy') are used based on gnome schemas (eg 'org.gnome.system.proxy') [2]. It provides the advantage of customizing the schemas. Hence the schema 'org.sugarlabs.system.proxy mode' was customized to include a new mode 'system' corresponding to the system proxy option as above. Six files are modified for this feature: - /sugar/data/org.sugarlabs.gschema.xml : It defines the custom schemas - /sugar/extensions/cpsection/network/view.py : It sets up the GUI and stores the proxy settings to the schemas. A restart is required for the proxy settings to take effect. Hence, this file also provides restart options. - /jarabe/main.py : It reads the proxy settings from schemas and exports them to environment variables according to the options, when it is run at the time of log in. - /jarabe/controlpanel/gui.py : It handles the toolbar and restart alert. Hence signal callbacks are added. - /jarabe/controlpanel/sectionview.py : Added definition for the new signals and a boolean 'show_restart_alert'. - /sugar/extensions/cpsection/network/__init__.py : Add 'proxy' keyword to the list to include it in search options. Here is the related feature page [3] to be updated along with it. [1] godiard@ebfd1ed [2] https://developer.gnome.org/ProxyConfiguration/ [3] https://wiki.sugarlabs.org/go/Features/Proxy_Settings
Updated feature page. https://wiki.sugarlabs.org/go/Features/Proxy_Settings |
<default>true</default> | ||
<summary>Unused; ignore</summary> | ||
<description> | ||
This key is not used, and should not be read or modified. |
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.
can we remove it if it is not used?
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.
use-same-proxy is a short way of saying use the same proxy for all other
protocals like https ftp and should auto populate the remaining proxy boxes
for those services.
On Sun, May 8, 2016 at 5:37 AM, Sam notifications@github.com wrote:
In data/org.sugarlabs.gschema.xml
#665 (comment):
<key name="ignore-hosts" type="as">
<default>[ 'localhost', '127.0.0.0/8', '::1' ]</default>
<summary>
Non-proxy hosts
<description>
This key contains a list of hosts which are connected to directly,
rather than via the proxy (if it is active). The values can be
hostnames, domains (using an initial wildcard like *.foo.com), IP host
addresses (both IPv4 and IPv6) and network addresses with a netmask
(something like 192.168.0.0/24).
</description>
</key>
<key name="use-same-proxy" type="b">
<default>true</default>
<summary>
Unused; ignore
<description>
This key is not used, and should not be read or modified.
can we remove it if it is not used?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/sugarlabs/sugar/pull/665/files/4bbac9afae7e02b854e02be492f163200a3f16b1#r62429966
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.
The "use-same-proxy" and "enabled" keys are not used now. But I left them, if it might be required in the future. Should I remove them?
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.
We can always add new settings later. Please remove them now; they are a little confusing having unused settings.
This is amazing. I love how it warns me when I try to set invalid settings. It's so close to me merging it. I just posted 2 small comments about the code. Also, when I set the proxy to be empty, I feel like it should have no effect. Eg. I set the FTP proxy to "", I don't get warned about it being an invalid server and it doesn't export the FTP_proxy environment variable. Is that that is the behaviour that other systems use? (note, I know nothing about proxy settings, CC @quozl) But once you fix those little things I will merge. It's magic and beautiful. |
Thanks @samdroid-apps ,
|
- I removed the unused gsetting keys - Refactored the list part of 'main.py' to make it compact. - Modified the behavior of alert so that if a user set blank '' host name to some type of proxy in manual settings, then it does not give you the alert. It might happen sometime that proxy settings are required for only some type like 'http, https' instead of all. But error is shown when user eaves all the hostnames blank.
Looks great. I merged it. It probably has some bugs. But we are so far away from a realease, that we can fix them between now and then (hopefully). I can make a testing release if anybody is interested? (CC Fedora people) |
Add GUI based proxy manager in the Network section of 'My Settings' to
enable management of manual proxy servers.
The proxy settings are stored in /etc/profile.d/proxy.sh as exports to
environment variables and requires re-login to let the changes take effect.
/extensions/cpsection/network/set_proxy.py file writes the proxy settings,
while two polkit policy files org.sugar.proxy.policy and org.sugar.logout.policy
located at /usr/share/polkit-1/actions provide root permissions required
for file handling and logout command. These two polkit files are placed at
data/ directory of this repository.