1067802 - [dolphin][wappush] user can accept wappush message after input invalid userpin

Status: RESOLVED FIXED

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

Description

[Peipei Cheng (needinfo if you need my action)]

Attachment: 01001_cmccwap_wsp_nor.xml

STR
----------------------------------------------------------------------------
1. send a wap push message to device (the xml file is as attached)
2. User recieved wap push message on device
3. Tap on the message
4. Input correct user pin and then tap on accept
5. Cancel the operation, it will return to enter user pin page
6. Input a wrong user pin and tap on accept
   --> User can accept the message with invalid userpin.


Video and slog can be found: https://mega.co.nz/#F!g4s1BYAQ!79wFTBrL7AYFgbeCdihByQ

Comment 1

[Peipei Cheng (needinfo if you need my action)]

Please ignore above localtion.
Video and slog location: https://mega.co.nz/#F!wwMWVRQC!ReeFV6xgDCftIGeVBZxpqg

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] When the user cancels storing the message request authentication again

This looks like an easy fix, it seems that it's just a matter of requesting authentication again when the user cancels storing the configuration.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] When the user cancels storing the message request authentication again

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8489876 [details] [diff] [review]
[PATCH] When the user cancels storing the message request authentication again

Review of attachment 8489876 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for providing the fix. Read below why and how you should change the patch. Give a try please. I'll do it as well in the mean time.

::: apps/wappush/js/cp_screen_helper.js
@@ +324,5 @@
>     * from the client provisioning store confirm dialog.
>     */
>    function cpsh_onCancelStore(evt) {
>      evt.preventDefault();
> +    authenticated = false;

I'm afraid this fix won't be valid. Let say we are handling a message whose authentication mechanism is NETWPIN (no PIN requested as gecko already performed the authentication process) if we cancel the flow when we are about to store the APNs next time the user hits the accept button we will perform the authentication process on the app side, which will be wrong.

Instead of setting it as false, restore the value (authenticated = authInfo.checked;) as we do when populating the screen.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)

> ::: apps/wappush/js/cp_screen_helper.js
> @@ +324,5 @@
> >     * from the client provisioning store confirm dialog.
> >     */
> >    function cpsh_onCancelStore(evt) {
> >      evt.preventDefault();
> > +    authenticated = false;

Tested messages with both NETWPIN and USERPIN auth mechanisms and as I said restore the value (authenticated = authInfo.checked;) as we do when populating the screen.

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> Tested messages with both NETWPIN and USERPIN auth mechanisms and as I said
> restore the value (authenticated = authInfo.checked;) as we do when
> populating the screen.

Ah, yes! I hadn't paid attention to that, it makes perfect sense. I'll refresh the patch. BTW we should also try to cover this with unit tests at some stage because there aren't any covering the interaction (or maybe integration ones?). I'll open a follow up for that too.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] When the user cancels storing the message request authentication again

Updated patch, I've updated the PR too.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8490103 [details] [diff] [review]
[PATCH v2] When the user cancels storing the message request authentication again

Review of attachment 8490103 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. r=me

Thanks for taking care of it Gabriele!

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)

> BTW we should also try to cover this with unit tests at
> some stage because there aren't any covering the interaction (or maybe
> integration ones?). I'll open a follow up for that too.

Yes, add me to CC list on that new bug please.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Try is now green (I had to re-trigger Gij as usual): https://tbpl.mozilla.org/?rev=fb6be730c2bbf0342cc4e3542ca4c4d219d8d396&tree=Gaia-Try

Pushed to gaia/master 8e0d8c1cdd9087831f1a540f8c0b9fbf95a1f162

https://github.com/mozilla-b2g/gaia/commit/8e0d8c1cdd9087831f1a540f8c0b9fbf95a1f162

Comment 11

[jinghua.xing]

Hi Gabriele：

Can you help me uplift this change to v1.4?
Thank you

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

The patch in this bug applies cleanly to 1.4 so you can apply it yourself locally with the following commands:

git checkout v1.4
git cherry-pick -x 8e0d8c1cdd9087831f1a540f8c0b9fbf95a1f162

I'm not sure if we're still accepting uplifts for v1.4, no matter how small. needinfo'ing release management for comment 11.

Comment 13

[bhavana bajaj [:bajaj]]

(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> The patch in this bug applies cleanly to 1.4 so you can apply it yourself
> locally with the following commands:
> 
> git checkout v1.4
> git cherry-pick -x 8e0d8c1cdd9087831f1a540f8c0b9fbf95a1f162
> 
> I'm not sure if we're still accepting uplifts for v1.4, no matter how small.
> needinfo'ing release management for comment 11.

Its too late to land this in 1.4 at this point. If partner feels this is a blocker to ship the release please reach-out to the TAM(Vance, wayne) for a reason to considering blocking.

Gabriele, is this a 1.4 regression?

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to bhavana bajaj [:bajaj] from comment #13)
> Gabriele, is this a 1.4 regression?

No, the issue has been there since the original content provisioning support landed.

Comment 15

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Could you please try land this on 2.0M? Thanks!

Comment 16

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/4f6b19594b19c90f90f0d901ec71f551656fa429