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

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: angelc04, Assigned: gsvelto)

Tracking

unspecified
Other
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0M+, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.0M fixed, b2g-v2.1 affected, b2g-v2.2 fixed)

Details

(Whiteboard: [sprd346576])

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 8489852 [details]
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
Whiteboard: [sprd346576]
Please ignore above localtion.
Video and slog location: https://mega.co.nz/#F!wwMWVRQC!ReeFV6xgDCftIGeVBZxpqg
(Assignee)

Comment 2

4 years ago
Created attachment 8489876 [details] [diff] [review]
[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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8489876 - Flags: review?(josea.olivera)
(Assignee)

Comment 3

4 years ago
Created attachment 8489877 [details] [review]
[PULL REQUEST] When the user cancels storing the message request authentication again
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.
Attachment #8489876 - Flags: review?(josea.olivera)
(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.
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 8490103 [details] [diff] [review]
[PATCH v2] When the user cancels storing the message request authentication again

Updated patch, I've updated the PR too.
Attachment #8489876 - Attachment is obsolete: true
Attachment #8490103 - Flags: review?(josea.olivera)
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!
Attachment #8490103 - Flags: review?(josea.olivera) → review+
(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.
(Assignee)

Comment 10

4 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 11

4 years ago
Hi Gabriele:

Can you help me uplift this change to v1.4?
Thank you
Flags: needinfo?(gsvelto)
(Assignee)

Comment 12

4 years ago
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.
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Flags: needinfo?(gsvelto) → needinfo?(release-mgmt)
(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?
Flags: needinfo?(release-mgmt) → needinfo?(gsvelto)
(Assignee)

Comment 14

4 years ago
(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.
Flags: needinfo?(gsvelto)

Updated

4 years ago
blocking-b2g: --- → 2.0M?

Comment 15

4 years ago
Hi Kai-Zhen,
Could you please try land this on 2.0M? Thanks!
blocking-b2g: 2.0M? → 2.0M+
Flags: needinfo?(kli)
v2.0m: https://github.com/mozilla-b2g/gaia/commit/4f6b19594b19c90f90f0d901ec71f551656fa429
status-b2g-v2.0M: affected → fixed
Flags: needinfo?(kli)

Updated

4 years ago
Blocks: 1080481
You need to log in before you can comment on or make changes to this bug.