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

[Sascha Silbe]

[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]

> (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.

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).


[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".


[...]
> @@ -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.


[...]
> +        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).

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.

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.


[...]
> +        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.


> +            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.


[...]
> +            self._chooser_button = gtk.Button(_('Choose..'))

This should probably tell the user exactly what to choose (and have
three dots rather than two).


[...]
> +    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".

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?

> +        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).


> +    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)

(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?

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).


[...]
> +            # 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.


> +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.


> +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.

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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/dextrose/attachments/20111219/7a72e48a/attachment.pgp>