[Dextrose] [PATCH v5][sucrose-0.94][RFC] Add capability to connect to WPA/WPA2-Enterprise Networks.
David Farning
dfarning at activitycentral.com
Mon Dec 19 18:58:36 EST 2011
On Mon, Dec 19, 2011 at 8:14 AM, Sascha Silbe
<sascha-ml-reply-to-2011-4 at silbe.org> 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]
>
>> (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.
Sascha, Ajay.
Thanks for slogging through this process reviewing and revising a
major patch. While initially painful, the quality of the peer review
is the single indicator of the success of a project.
david
>> ---
>
> 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/
>
> _______________________________________________
> Dextrose mailing list
> Dextrose at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/dextrose
>
More information about the Dextrose
mailing list