Merge lp:~joergberroth/ubuntu-system-settings/wifi-802-1x-configurations into lp:ubuntu-system-settings
| Status: | Superseded |
|---|---|
| Proposed branch: | lp:~joergberroth/ubuntu-system-settings/wifi-802-1x-configurations |
| Merge into: | lp:ubuntu-system-settings |
| Diff against target: |
931 lines (+626/-169) 3 files modified
plugins/wifi/OtherNetwork.qml (+495/-163) plugins/wifi/wifidbushelper.cpp (+129/-5) plugins/wifi/wifidbushelper.h (+2/-1) |
| To merge this branch: | bzr merge lp:~joergberroth/ubuntu-system-settings/wifi-802-1x-configurations |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Marcus Tomlinson (community) | Needs Fixing on 2015-05-01 | ||
| Matthew Paul Thomas | design | 2015-04-25 | Abstain on 2015-04-28 |
| Jonas G. Drange | 2015-04-25 | Pending | |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2015-04-18.
This proposal has been superseded by a proposal from 2015-04-27.
Description of the Change
Added support for 802-1x wireless network configurations.
| JkB (joergberroth) wrote : | # |
> This is really great. Thanks for proposing this.
>
> I've added some comments, but they're mostly about your decision to make the
> Dialog an ItemPage. If this is not crucial to the implementation of support
> for 802-1x, we need to revert it.
>
> Also, if cert picking is not implemented, please remove all components that
> are unused. Please leave the UI bits in, but hide it with "visible:
> showAllUI". This way we can get strings translated.
>
> Thanks again.
Hey, thanks a lot for for reviewing.
First I had the concerns about pushing it to an ItemPage, too. Then, after dealing with the variety of different configurations, I found there would be an advantage if one changes to ItemPage. There is no need for the ItemPage from implementation point of view but there are configurations for which the user has to be asked up to two different certificates and an additional private key (TLS) and there can be different types of path2 authentication types for the different WPA Enterprise configurations.
So if we want the user to be able two choose all the different types of configurations, it seems to me that a dialog would get quite overloaded with ui inputs. And correct me if I'm wrong but scrolling, which certainly would be needed, seems not to be provided for the dialogs.
At the moment there are some pages in the cellular settings (e.g. APN) where one switched to ItemPages, too. So from design point of view it would keep consistent.
Maybe, based on these points, we can quickly discuss this again before I revert the change.
All other comments are clear. I will consider for the next commit.
I think the file picker would make selecting certificates even more comfortable but I will remove it as proposed, so one can work on that later.
Thanks again.
Joerg
| Matthew Paul Thomas (mpt) wrote : | # |
Thanks so much for working on this, Joerg!
Jonas is right, though, that the authentication UI does need to remain a dialog. The reason is that System Settings is just one of three -- and will eventually be just one of six -- different places that might launch the UI for entering Wi-Fi details. The first-run setup "Connect to Wi-Fi" screen, and the network indicator menu, should let you connect to all the same Wi-Fi networks that System Settings does, by putting up the same dialog. <https:/
When dialog contents are bigger than the available screen space, the body is supposed to scroll automatically, and apparently this has been implemented already (bug 1376763). If it's not working, please report a bug.
| JkB (joergberroth) wrote : | # |
Thanks for the review and the hints.
It was not clear from documentation for me. But the bug post made it.
I just reverted the page back to dialog.
Am 2015-04-27 um 13:04 schrieb Matthew Paul Thomas:
> Review: Needs Fixing design
>
> Thanks so much for working on this, Joerg!
>
> Jonas is right, though, that the authentication UI does need to remain a dialog. The reason is that System Settings is just one of three -- and will eventually be just one of six -- different places that might launch the UI for entering Wi-Fi details. The first-run setup "Connect to Wi-Fi" screen, and the network indicator menu, should let you connect to all the same Wi-Fi networks that System Settings does, by putting up the same dialog. <https:/
>
> When dialog contents are bigger than the available screen space, the body is supposed to scroll automatically, and apparently this has been implemented already (bug 1376763). If it's not working, please report a bug.
>
| Marcus Tomlinson (marcustomlinson) wrote : | # |
Your code isn't compiling. There are some weird "!==" occurrences in 2 of your .cpp files. Rather hard to test your changes work when the project doesn't compile no? :P
- 1392. By JkB on 2015-05-05
-
Fixed weird "!==". Added basic FilePicker functionality.
- 1393. By JkB on 2015-05-12
-
Added CertificateHandling to list installed and add new certificates from OtherNetwork
Now uses ItemSelectors and dataModels. Certs can be added by calling ContentHub.ToDos:
*improve list update for ItemSelectors
*Add handling for private keys and pac files.
*add password-flags
*add pac-Provisioning
*change? from blob to path scheme for nm configurations no that certs can be stored in a central place.--- in wifi settings:
*Page (from PageComponents) to manage all installed certs and keys. - 1394. By JkB on 2015-05-25
-
Improved certificate and key handling. List installed and add new certificates from OtherNetwork Dialog.
Uses ItemSelectors and dataModels to select certs and keys. Certs and keys can be added by calling ContentHub.ToDos:
*improve list update for ItemSelectors
*Improve handling pac files.
*change? from blob to path scheme for nm configurations no that certs can be stored in a central place.--- in wifi settings:
*Page (from PageComponents) to manage all "installed" certs and keys. - 1395. By JkB on 2015-05-26
-
Merge with trunk
- 1396. By JkB on 2015-05-27
-
merge with trunk.
Fixed bugs. - 1397. By JkB on 2015-05-27
-
removed some bugs,
finally tested on my BQ Aquaris. At least for WPA and a WPA Enterprise TTLS/MCHAP2 connection.
Had no other networks around.final changes:
*changed to path scheme for cert handling with networkmanager
*removed some further small bugs that made it finally work.----
ToDos:
*Improve handling of pac Files.
*As suggested, cert and key managment for "installed" ones will be needed.
*Improve CertDialog not to show raw content of cert.
*Implement network encryption detection to use dialog for visible networks as well. - 1398. By JkB on 2015-05-31
-
merge with trunk
----
*added checks against bad cert/key content
*improved data update. - 1399. By JkB on 2015-06-03
-
OtherNetwork extensions mature now.
---
*improved Pac fIle handling. - 1400. By JkB on 2015-06-03
-
*
- 1401. By JkB on 2015-06-04
-
formatting
- 1402. By JkB on 2015-06-08
-
* ItemSelectors: select back to "None" as standard for cases when filePicker is canceled.
* formatting
----
ToDo (later on):
- change selectedIndex to Item that has been added via ContentHub. - 1403. By JkB on 2015-06-08
-
merge with trunk
- 1404. By JkB on 2015-06-15
-
* improve config handling
* fixed bug: added missing nul termination to successfully store certs with path scheme. - 1405. By JkB on 2015-06-15
-
* added QStandardPaths for cert handling
* improved getting secrets in Previous network to get wpa-eap passwords as well.
* restored .bzrignore - 1406. By JkB on 2015-06-16
-
merge with trunk
- 1407. By JkB on 2015-06-24
-
merged with jgdx merge proposal. Thanks a lot Jonas for the extensive format fixes!
+Also, considered some quickly managable FIXMEs from Jonas. Some stay. - 1408. By JkB on 2015-06-25
-
* fixed wpa-eap/peap configurations.
* added "certificates recommended" security hint. - 1409. By JkB on 2015-06-25
-
*merged with trunk
- 1410. By JkB on 2015-06-26
-
merge jonas branch
- 1411. By JkB on 2015-06-26
-
merge with trunk
- 1412. By JkB on 2015-06-27
-
* set feedback back to original implementation
- 1413. By JkB on 2015-06-29
-
merge Jonas fixmes
- 1414. By JkB on 2015-06-29
-
clear spaces
- 1415. By JkB on 2015-06-29
-
merge with trunk
- 1416. By JkB on 2015-07-01
-
* multiple %1 args in i18n.tr all fall back to the first arg() provided.
Is this an individuell problem of my build?
If not we should consider this last commit. - 1417. By JkB on 2015-07-01
-
* only show "using certificate.." hint if selected one == "None"


This is really great. Thanks for proposing this.
I've added some comments, but they're mostly about your decision to make the Dialog an ItemPage. If this is not crucial to the implementation of support for 802-1x, we need to revert it.
Also, if cert picking is not implemented, please remove all components that are unused. Please leave the UI bits in, but hide it with "visible: showAllUI". This way we can get strings translated.
Thanks again.