Closed Bug 926334 Opened 6 years ago Closed 6 years ago

[Gaia] To support WPA-EAP options(PEAP, TLS, TTLS) and manage certificate in WLAN setting.

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed

People

(Reporter: kchang, Assigned: iliu)

References

Details

(Whiteboard: [ft:ril])

Attachments

(1 file, 1 obsolete file)

As a user, I want to have an option to select "802.1x EAP-SIM" for WLAN authentication in the WLAN settings menu.
Arthur, are you able to handle this issue?
blocking-b2g: --- → 1.3+
Flags: needinfo?(arthur.chen)
Take the bug for further dispatching.
Assignee: nobody → arthur.chen
Flags: needinfo?(arthur.chen)
Assignee: arthur.chen → iliu
Blocks: 920936
Depends on: 926341
Summary: [Gaia] To support EAP-SIM option in WLAN setting. → [Gaia] To support EAP-SIM and WPA-EAP option in WLAN setting.
Ian, We should fix this bug before 11/22. If you think it isn't reasonable, please change it.
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Ken, just start to work on the feature. Do you have a evaluation schedule for landing EAP-SIM API? Thanks.
Whiteboard: [ft:ril]
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Hi, Kaze, and Arthur

I think both of you are able to review the new feature. If one of you have bandwidth in this channel, please give me some feedback for the patch. Since the API is still in reviewing status, I use WIP for Gaia reviewing work. The risk point should be hacking the "openDialog" to support a specific exit panel.


Hi Chuck, Hubert

If you want to test and work with the newest UX flow, please feel free to use the patch here. Before using it, check Gecko build is ready or not. Otherwise, you might not go into manage certificate page.

Thanks for all your help.
Attachment #8337496 - Flags: feedback?(kaze)
Attachment #8337496 - Flags: feedback?(chulee)
Attachment #8337496 - Flags: feedback?(arthur.chen)
Two minor changes to the object ID in wifi_helper.js to fit gecko, as commented in github.
Other parts looks good to me.  :)
If anyone like to test the whole function, the patch chain for gecko part is Bug 917102 -> Bug 917176 -> Bug 917175 -> Bug 745468 -> Bug 790056
Comment on attachment 8337496 [details] [review]
WIP since the API is in reviewing status

Thanks for the patch! Overall it looks good to me.
Attachment #8337496 - Flags: feedback?(arthur.chen) → feedback+
Summary: [Gaia] To support EAP-SIM and WPA-EAP option in WLAN setting. → [Gaia] To support WPA-EAP options(PEAP, TLS, TTLS) and manage certificate in WLAN setting.
For the issue here, we'll focus on security WPA-EAP options(PEAP, TLS, TTLS) with management certificate functionality. I will split WPA-EAP option(SIM) to bug 944232.
No longer blocks: 920936
No longer blocks: 920933
Blocks: 922930
No longer blocks: 920939
Comment on attachment 8337496 [details] [review]
WIP since the API is in reviewing status

Thanks for Chuck and Arthur's feedback first. I have updated the patch as following change. 

* Add error dialog for error case.(import/delete certificate file failed)
* Support auto-detection API.
  - If platform certificate manager API is not ready yet, we will hide all of the relative feature and layout. So, the landing process won't be blocking by platform side.

* Revise the API name according to @chuck-lee 's comment.
* Remove hack openDialog function to support that leave a dialog and go back to specific exit panel.
  - Use Settings.currentPanel instead of hacking it.
  - If import certificate file failed, it won't go back to #wifi-manageCertificates page. It will go back to previous page(#selectCertificateFile, sync UX flow).

Arthur,

Could you please help to review the pr? Thanks.
Attachment #8337496 - Flags: feedback?(kaze) → review?(arthur.chen)
Matej, could you help review the sentence? Thanks!

"Notice: If you need import certificates into NSS list, please go to “Manage certificates” to import new CA keys."
Flags: needinfo?(Mnovak)
(In reply to Arthur Chen [:arthurcc] from comment #11)
> Matej, could you help review the sentence? Thanks!
> 
> "Notice: If you need import certificates into NSS list, please go to “Manage
> certificates” to import new CA keys."

I think "NSS list" might be confusing to user, maybe just "import certificates" is enough?
The user story bug this blocks (bug 922930) has a target milestone of future, so this is not a committed feature for 1.3. Non-committed features for 1.3 should not block the release.
blocking-b2g: 1.3+ → 1.3?
Comment on attachment 8337496 [details] [review]
WIP since the API is in reviewing status

Ian, thank you for the effort! Could you address my comments in github and request a review again?
Attachment #8337496 - Flags: review?(arthur.chen)
(In reply to Chuck Lee [:chucklee] from comment #12)
> (In reply to Arthur Chen [:arthurcc] from comment #11)
> > Matej, could you help review the sentence? Thanks!
> > 
> > "Notice: If you need import certificates into NSS list, please go to “Manage
> > certificates” to import new CA keys."
> 
> I think "NSS list" might be confusing to user, maybe just "import
> certificates" is enough?

I'm not sure I understand what this is saying well enough to know how much it can be simplified and still retain it's proper meaning, but here's what I would recommend:

Notice: If you need to import new certificates, please go to “Manage certificates."
Flags: needinfo?(Mnovak)
(In reply to Matej Novak [:matej] from comment #15)
> (In reply to Chuck Lee [:chucklee] from comment #12)
> > (In reply to Arthur Chen [:arthurcc] from comment #11)
> > > Matej, could you help review the sentence? Thanks!
> > > 
> > > "Notice: If you need import certificates into NSS list, please go to “Manage
> > > certificates” to import new CA keys."
> > 
> > I think "NSS list" might be confusing to user, maybe just "import
> > certificates" is enough?
> 
> I'm not sure I understand what this is saying well enough to know how much
> it can be simplified and still retain it's proper meaning, but here's what I
> would recommend:
> 
> Notice: If you need to import new certificates, please go to “Manage
> certificates."

Already updated in my patch!
Comment on attachment 8337496 [details] [review]
WIP since the API is in reviewing status

Close the WIP patch since API dev and app owner have gave some feedback. Thank you all:)
Attachment #8337496 - Attachment is obsolete: true
Attached file pull request 13989
Arthur,

Thanks for your reviewing effort. I have revised the patch according your comment on GitHub. Please help to review it again.
Attachment #8342227 - Flags: review?(arthur.chen)
Comment on attachment 8342227 [details] [review]
pull request 13989

r=me. Good work, thanks!!
Attachment #8342227 - Flags: review?(arthur.chen) → review+
Arthur, thanks really. Let's wait UX's finial comment for improving string. https://bugzilla.mozilla.org/show_bug.cgi?id=926341#c21
Since UX commented the string refinement(https://bugzilla.mozilla.org/show_bug.cgi?id=926341#c23), I have updated it on Github.
The patch is landed. We can close the issue now.

Gaia/master:  7c568287adbf97e3b1454512f0028ec273467bd1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Note:

The UI will be effected and showing in Settings::Wifi section while platform API "navigator.mozWifiManager.getImportedCerts()" is ready.
I've had to revert this patch because it is breaking Gaia UI test.

https://github.com/mozilla-b2g/gaia/commit/a96de870bb6a028d10b48c39d855a7038a42a1d1

Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette_test.py", line 143, in run
testMethod()
File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_do_not_track.py", line 26, in test_enable_do_not_track_via_settings_app
do_not_track_settings.tap_disallow_tracking()
File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/apps/settings/regions/do_not_track.py", line 22, in tap_disallow_tracking
el.tap()
File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 83, in tap
return self.marionette._send_message('singleTap', 'ok', id=self.id, x=x, y=y)
File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 577, in _send_message
self._handle_error(response)
File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 604, in _handle_error
raise ElementNotVisibleException(message=message, status=status, stacktrace=stacktrace)

TEST-UNEXPECTED-FAIL | test_settings_do_not_track.py test_settings_do_not_track.TestSettingsDoNotTrack.test_enable_do_not_track_via_settings_app | ElementNotVisibleException: Element is not currently visible and may not be manipulated
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hi Anthony,

Thanks for your reminder. I have fixed the CSS side effect. The class "description" is hiding other element in settings::Do Not Track section.
Since the patch is passed Travis build test and landed, we can close the issue now.

Gaia/master: eaf1e57b731914122fc9a139fc3538b522024de9
Gaia/v1.3:   cbd29217ea37209449774b0c31cbfbdd2da6ee68
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Triage: This bug includes feature work on RIL team v1.3, and receives a post-landing 1.3+ since it's landed already.
blocking-b2g: 1.3? → 1.3+
Is there a reason for not supporting PKCS#12 files ?
Flags: needinfo?(iliu)
(In reply to Alexandre LISSY :gerard-majax from comment #29)
> Is there a reason for not supporting PKCS#12 files ?

I can't read private key out of NSS, and reviews has concerns about doing this.
It's depended on API ability. These files format('cer', 'crt', 'pem', 'der') which are supported and browsed from SD card.(https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/wifi_select_certificate_file.js#L42) We defined supported format for certificate files according to Chuck's suggestion as comment 30.
Flags: needinfo?(iliu)
You need to log in before you can comment on or make changes to this bug.