[Dextrose] [PATCH v5][sucrose-0.94][RFC] Add capability to connect to WPA/WPA2-Enterprise Networks.

[Ajay Garg]

Hi Sascha,

I submitted the revised patch.
Please find my comments inline.

On Mon, Dec 19, 2011 at 7:44 PM, Sascha Silbe <silbe at activitycentral.com> wrote:
> [Dropped sugar-devel from CC as the patch is based on a stable branch
>  rather than master]
>
>
> Excerpts from Ajay Garg's message of 2011-12-15 10:55:13 +0100:
>
>> [PATCH v5][sucrose-0.94][RFC] Add capability to connect to WPA/WPA2-Enterprise Networks.
>
> The tags should all go inside the (same) brackets:
> [PATCH RFC sugar sucrose-0.94 v5]

Done.


>
>> (Note that this is a consolidated patch, to be applied in full;
>> and NOT OVER version-4, version-3, version-2, and version-1 patches).
> [...]
>
> Please keep the changelog below the marker ("---") line.

Done.


>
> What I'm still missing is an explanation of what this patch does. What
> exactly can a user do now that wasn't possible before (e.g. connecting
> to WLANs using TTLS or PEAP for authentication - if possible include
> links to explanations what these are)? What still isn't possible /
> wasn't changed for some reason? How was it achieved technically? In what
> way was the UI changed?  What issues are there? What was tested
> (successfully?), what wasn't? What alternative approaches were
> considered and why was this one chosen (but only include information
> relevant to Sugar (Labs), not internal business decisions of AC, OLPC
> and / or whoever took part in the design and implementation of this
> patch)?
>
> Some of this information is already spread out over the changelog and
> needs to be summarised (in a coherent way) in the description; other
> parts are missing entirely.
>
> The patch description is an important resource for reviewers (What is
> the patch _supposed_ to do? Why was it done this way?), release note
> authors (What changed since the last version and what does the user need
> to know about it?) and developers hunting down bugs in the code you
> touched (Why does the code work like this? How is it supposed to work?)
>
> Writing good patch descriptions is hard, but very important for
> maintainability and long-term sustainability of a project.
>
>> ---
>
> This is the marker line. Changelogs should follow it immediately (before
> the statistics part).
>

Done.


>
> [src/jarabe/desktop/keydialog.py]
>> @@ -20,9 +20,13 @@ from gettext import gettext as _
>>
>>  import gtk
>>  import dbus
>> +import os
>> +import shutil
>
> Both os and shutil are part of the standard Python library, so they
> should be imported in the previous block. See PEP 8 [1], section
> "Imports".

Done.


>
>
> [...]
>> @@ -32,6 +36,11 @@ WEP_PASSPHRASE = 1
>>  WEP_HEX = 2
>>  WEP_ASCII = 3
>>
>> +SETTING_TYPE_STRING = 1
>> +SETTING_TYPE_LIST = 2
>> +SETTING_TYPE_CHOOSER = 3
>
> See comment below (for src/jarabe/desktop/networkviews.py).
>
>
>> +
>> +
>>
>
> Please use exactly two blank lines to separate top-level definitions.
> See PEP 8, section "Code layout", "Blank Lines". You can use the tool
> pep8 to check for [2] this kind of mistake; your patch should not
> trigger any output from the tool.
>
> This may seem a bit picky (and to some extent it is), but consistent
> style is important for readability.
>
>
>> +class NetworkParameters(gtk.HBox):
>> +    def __init__(self, auth_param):
>
> What is this class for? Please add a docstring and consider choosing a
> more expressive name.
>

Done.


>
> [...]
>> +        self._key = auth_param._key_name
>
> Please don't access private parts ("subclass API" in PEP 8 terms) of
> "third-party" classes (i.e. those without an inheritance relationship to
> your class). See PEP 8, section "Naming Conventions", "Prescriptive:
> Naming Conventions", "Designing for inheritance". pylint [2] would have
> complained about this (W0212).

Done.


>
> The purpose of the distinction between "public" and "private" (or
> "protected" in some languages) is to allow changing the implementation
> (the "private" parts) without breaking API for the callers (the "public"
> part).
>
> pylint outputs some false positives for the Sugar source code alongside
> some actual mistakes that need cleaning up, making it a bit harder to
> see what messages your patch introduced. It's nevertheless a very useful
> tool and you should run it directly prior to submitting your patch. If
> you trigger additional pylint messages and don't think they should apply
> to your code, please mention this in the patch comment (i.e. after the
> marker line).
>
>
> [...]
>> +        self._label = gtk.Label(_(auth_param._key_label))
>
> This isn't going to work as-is. _() not only looks up a translation, but
> also marks a string in the source as needing translation, so it needs to
> be a literal string rather than a variable or attribute.

Done (actually the most important FIX) ;P



>
> There are two ways to solve this (or rather three ways, as it turns
> out later on in this review):
>
> 1. Use subclassing and pass the translated label to the superclass.
>
> 2. Pull a trick like sugar.util does for translating timestamps.
>
>
> In this particular case, subclassing would make more sense: you can
> introduce intermediary classes for the various parameter types
> (single-valued, multi-valued and file) and get rid of the _is_entry(),
> _is_liststore() and _is_chooser() methods.
>
> [...]
>> +        self._label.show()
>
> Unless you're going to hide some of the widgets, please use show_all()
> on the container rather than littering the code with show() for the
> individual widgets. GTK already is much too verbose, making it hard to
> get a high-level view on the code.

Will keep this in mind for future coding. Did not want to touch the
code too much (UI code is anyways, the biggest cluster of code) :|



>
>
> [...]
>> +        elif self._is_liststore():
>> +            self._option_store = gtk.ListStore(str, str)
>> +            for option in auth_param._options:
>> +                self._option_store.append(option)
>
> This might be correct, but please take it as an opportunity to stop,
> lean back and think about your interfaces. Where do the values come
> from, where do they do? What encoding are the strings in? Are they / do
> they need to be escaped in any way?
>
> GTK uses (read: both expects and gives you) UTF-8 and has a few
> interfaces that scan the string for control sequences, requiring
> escaping of strings we get from "the outside". IIRC NetworkManager uses
> UTF-8 for most interfaces, but there are some that are binary data (e.g.
> SSID). The NetworkManager D-Bus API docs should tell you more.
>
> We've had [3] resp. still have [4] issues with escaping and encoding in
> the NetworkManager client code (which your patch touches), so we need to
> make sure we don't introduce additional issues.



If any wrong entries are made, the connection will not be made.
I did go through [3] and [4]; here, the settings once entered, don't
need to be displayed anywhere on the UI ever again, only to be used by
the Networkmanager backend.




>
>
>> +            self._option_combo = gtk.ComboBox(self._option_store)
>> +            cell = gtk.CellRendererText()
>> +            self._option_combo.pack_start(cell, True)
>> +            self._option_combo.add_attribute(cell, 'text', 0)
>> +            self._option_combo.set_active(0)
>> +            self._option_combo.connect('changed',
>> +                    self._option_combo_changed_cb)
>> +            self.pack_start(self._option_combo)
>
> Without taking a closer look, I guess this could be simplified. Check
> out the "demo/menu.py" example from PyGTK
> (/usr/share/doc/python-gtk2-doc/examples/demo/menu.py on Debian if you
> have python-gtk2-doc installed; Fedora probably ships it in a similar
> package):
>
>        combo_box = gtk.combo_box_new_text()
>        combo_box.set_wrap_width(2)
>        for i in range(50):
>            combo_box.append_text('item - %d' % i)
>        combo_box.set_active(0)
>        vbox2.pack_start(combo_box)
>
>
> No CellRenderer instantiation, no attribute declaration, just
> gtk.combo_box_new_text() as constructor.


Did not change this, because I wanted to keep both the
"identifier-key" and "value" attached to the Liststore entries.



>
>
> [...]
>> +            self._chooser_button = gtk.Button(_('Choose..'))
>
> This should probably tell the user exactly what to choose (and have
> three dots rather than two).

Done.



>
>
> [...]
>> +    def _is_entry(self):
>> +        return ( not self._is_chooser() ) and \
>> +               ( len(self._auth_param._options) == 0 )
>
> Please remove extra whitespace immediately inside parentheses. See
> PEP 8, section "Whitespace in Expressions and Statements", "Pet Peeves".

Done.


>
> You don't need explicit line continuation (the lone backslash at the end
> of the line) as there's still a parenthesis open, triggering implicit
> line continuation. See also PEP 8, section "Code lay-out", "Maximum Line
> Length".
>
> Testing (non-)emptiness should use the fact that empty sequences are
> false, rather than invoking len(). See PEP 8, section "Programming
> Recommendations". Besides making the code easier to read (less noise),
> it can be faster in some cases (as the exact count doesn't need to be
> calculated; this only applies to special objects and not to the built-in
> lists, though).
>
>
> [...]
>> +    def _object_chooser_cb(self, chooser_button):
>> +        self._want_document = True
>> +        self._show_picker_cb()
>> +
>> +    def _show_picker_cb(self):
>> +        if not self._want_document:
>> +            return
>
> What's the purpose of _want_document?

Just followed the semantics of the ImageViewer code :)



>
>> +        self._chooser = ObjectChooser()
>> +        self._chooser._set_callback(self.__process_selected_journal_object)
>
> Having to access the private API of
> jarabe.journal.objectchooser.ObjectChooser suggests you should use
> sugar.graphics.objectchooser.ObjectChooser (which is public API) rather
> than jarabe.journal.objectchooser.ObjectChooser (which is an
> implementation detail of the Journal).


Well, I did use the public API originally, but it was taking too much
time to load (a good 30 seconds) with some dbus-connection-exceptions.
I actually wasted a good 1 day, banging my head for this. I had no
resort but to use this; after originally trying the ImageViewer
journal-chooser code, which does use the public API.




>
>
>> +    def __process_selected_journal_object(self, object_id):
>> +        jobject = self._chooser.get_selected_object()
>> +        if jobject and jobject.file_path:
> [a lot of code follows; only self._chooser.destroy() is called if the
>  above test fails]
>
> Please use "guardian style" instead:
>
>        if not jobject or not jobject.file_path:
>            self._chooser.destroy()
>            return
>
> This makes it clear that in the "empty" case nothing else happens
> (immediate return) and gets rid of one level of indentation, packing
> more source code on the screen and helping to get an overview of the
> code.
>
>
>> +            file_basename = \
>> +                    os.path.basename(jobject._metadata._properties['title'])
>
> If you need continuations because of accessing nested objects, using a
> temporary variable can increase readability without any expense in
> terseness (as you'd need two lines anyway):
>
>            object_title = jobject._metadata._properties['title']
>            file_basename = os.path.basename(object_title)
Done.



>
> (OK, in this case it didn't really help a lot ;) )
>
> There are two issues that worry me more, though:
>
> 1. Accessing private data of sugar.datastore.datastore.DSObject (which
>   isn't even in the same module as your code). What does the private
>   API (_metadata) give you that the public API (metadata) doesn't
>   offer?

Answered previously.


>
> 2. Using random user data for file names and widget labels after partial
>   sanitation. The data store API supports storing NUL bytes, but POSIX
>   disallows them in file names and GTK works with C-style (i.e.
>   NUL-delimited) strings. Can this cause your code to misbehave in an
>   exploitable way after the user was tricked into choosing a specially
>   crafted Journal entry?
>
>   Why do we need to turn the title into a file name in the first place?
>   It's not a unique identifier (only the object_id, 'uid' for the
>   current version of sugar-datastore, is unique) and NetworkManager
>   shouldn't care about the file name (if it does, we should take care
>   of that ourselves rather than forcing the user to handle it) - if it
>   even accesses the file, rather than us passing the content down via
>   D-Bus (haven't checked that part of your code yet).


Here, is did not originally do the "DISCOVERY-CUM-REQUIREMENT" thing,
and was simply using the journal way to access files.
However, I could only use the setting-files once, and not upon reboot
(as it should be for network auto-connect).
The reason, was that the entry name/uid/ kept on changing. (I did not
delve into the details :( )
So, used this hack.



>
>
> [...]
>> +            # The chooser entries are chosen from the journal.
>> +            # Now, the journal entries misbehave, and the entries keep
>> +            # changing their names. Thus, in order to have the same
>> +            # entry available at later times, we need to store the
>> +            # selected entries at a 'fixed-name' place.
>
> What exactly do you consider misbehaviour and why?
>
>
>> +            # DISCOVERY-CUM-REQUIREMENT:
>> +            # -------------------------
>
> What is this supposed to mean? It doesn't make any kind of sense to me
> (except for "REQUIREMENT").
>
>
>> +            profile_path = env.get_profile_path()
>> +            self._entry = os.path.join(profile_path, 'nm',
>> +                                       file_basename)
>> +
>> +            # Remove (older) file, if it exists.
>> +            if os.path.exists(self._entry):
>> +                os.remove(self._entry)
>> +
>> +            # Copy the file.
>> +            shutil.copy2(jobject.file_path, self._entry)
>
> This looks like it would "leak" files when the user chooses entries with
> different titles. Using a constant name (see above) would fix this.
>
>
>> +    def _option_combo_changed_cb(self, widget):
>> +        it = self._option_combo.get_active_iter()
>
> Please don't abbreviate where it's not necessary and especially not when
> abbreviating makes it hard to understand what the intention is. In this
> case, "it" could mean "the object" (raising the question "what object");
> "iterator" only comes to mind after taking a closer look at the code in
> question.
>
>
> [...]
>> +    def _get_key(self):
>> +        return self._key
>
> What do you need a private accessor for? Either it's private API and you
> don't need an accessor (because your code knows it can access the
> attribute directly) or it's public API. Even for public API, consider
> whether it's likely that you need to do expensive calculations in order
> to generate this value in the future. Apart from being local to the
> sugar module (making it easy to adapt all callers), in Python you can
> easily turn this into a property later on, so you only need an accessor
> to indicate expensive accesses or side effects. See also PEP 8, section
> "Naming Conventions", "Prescriptive: Naming Conventions", "Designing for
> inheritance".
>
>
> [...]
>> +class KeyValuesDialog(gtk.Dialog):
>> +    def __init__(self, auth_lists, final_callback, uuid, settings):
>> +        # This must not be "modal", else the "chooser" widgets won't
>> +        # accept anything !!
>
> I'll also bite you if you introduce something modal. :-P
>
> IMO (Gary in particular will disagree) using a modal dialog always
> implies that you made a design mistake somewhere. Why do you _need_ to
> prevent the user from interacting with other parts of the system? How
> can you be sure that preventing this interaction doesn't render the user
> unable to complete the dialog successfully (e.g. by looking up the
> passphrase somewhere)?
>
> In addition to being a design failure, modal dialogs are hard to get
> right in practice. See e.g. SL#453 [5], SL#1138 [6]. I'm quite sure we
> still haven't sorted out all issues with the existing modal dialogs.
>
> I also strongly dislike pop-up dialogs that the user hasn't explicitly
> requested, but that's a topic for another day. :)
>
>
> [src/jarabe/desktop/networkviews.py]
>> @@ -56,6 +57,192 @@ _OLPC_MESH_ICON_NAME = 'network-mesh'
>>
>>  _FILTERED_ALPHA = 0.33
>>
>> +SETTING_TYPE_STRING = 1
>> +SETTING_TYPE_LIST = 2
>> +SETTING_TYPE_CHOOSER = 3
>
> If these values need to be consistent across the different source files,
> they should go into a single file. jarabe.model.network should be a good
> fit.

Did not do this, since it would have cluttered the
WPA_AUTHENTICATION_METHODS list a lot (i could have use the
static-object-idiom, containing the settings, but it would have
cluttered the WPA_AUTHENTICATION_METHODS_LIST.


>
>
>> +class AuthenticationType:
>> +    def __init__(self, auth_label, auth_type, params_list):
>
> What is this all about? (include docstring, better class name, etc.)
>
>> +        self._auth_label = auth_label
>> +        self._auth_type = auth_type
>> +        self._params_list = params_list
>
> Why does it have only private API, but apparently no subclass?
>
>
>> +class AuthenticationParameter:
>> +    def __init__(self, key_name, key_label, key_type,
>> +                       options):
> [...]
>
> Ditto.

Done.



>
>
>> +AUTHENTICATION_LIST = \
>> +        [
>> +          AuthenticationType('TLS',
>> +                             'tls',
>> +                               [
>> +                                 AuthenticationParameter(
>> +                                 'eap',
>> +                                 'Authentication',
>> +                                  SETTING_TYPE_LIST,
>> +                                  [['TLS', 'tls']]
>> +                                  ),
> [...]
>
> Still very hard to read and doesn't follow the PEP 8 indentation rules.
>
> Let's give it a try:
>
> WPA_AUTHENTICATION_METHODS = [
>    AuthenticationType(_('TLS'), 'tls',
>                       [AuthenticationParameter('eap', _('Authentication'),
>                                                SETTING_TYPE_LIST,
>                                                [['TLS', 'tls']]),
>                        AuthenticationParameter('identity', _('Identity'),
>                                                SETTING_TYPE_STRING, []),
>                        AuthenticationParameter('client-cert',
>                                                _('User certificate'),
>                                                SETTING_TYPE_CHOOSER, []),
>                        AuthenticationParameter('ca-cert', _('CA certificate'),
>                                                SETTING_TYPE_CHOOSER, []),
>                        AuthenticationParameter('private-key',
>                                                _('Private key'),
>                                                SETTING_TYPE_CHOOSER, []),
>                        AuthenticationParameter('private-key-password',
>                                                _('Private Key password'),
>                                                SETTING_TYPE_STRING, []),
>                       ]),
>    AuthenticationType(_('LEAP'), 'leap',
>                       [AuthenticationParameter('eap', _('Authentication'),
>                                                SETTING_TYPE_LIST,
>                                                [['LEAP', 'leap']]),
>
> and so on.
>
> Not perfect yet, but already much more compact. And it includes the
> translated labels, solving your dynamic string translation issue from
> above.


Done.



>
> With proper subclassing for the different value types it'll probably get
> even better.
>
> I'm stopping the review at this point; the above should be enough to
> chew on already.
>
> Sascha
>
> [1] http://www.python.org/dev/peps/pep-0008/
> [2] https://wiki.sugarlabs.org/go/Development_Team/Code_Guidelines#Tools
> [3] https://bugs.sugarlabs.org/ticket/2099
> [4] https://bugs.sugarlabs.org/ticket/2023
> [5] https://bugs.sugarlabs.org/ticket/453
> [6] https://bugs.sugarlabs.org/ticket/1138
> --
> http://sascha.silbe.org/
> http://www.infra-silbe.de/


Thanks a ton !! :)

Regards,
Ajay