Closed Bug 1067802 Opened 10 years ago Closed 10 years ago

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

Categories

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

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
blocking-b2g 2.0M+
Tracking Status
b2g-v1.4 --- affected
b2g-v2.0 --- affected
b2g-v2.0M --- fixed
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: angelc04, Assigned: gsvelto)

Details

(Whiteboard: [sprd346576])

Attachments

(3 files, 1 obsolete file)

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
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)
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.
(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.
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.
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
Closed: 10 years ago
Resolution: --- → FIXED
Hi Gabriele: Can you help me uplift this change to v1.4? Thank you
Flags: needinfo?(gsvelto)
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.
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)
(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)
blocking-b2g: --- → 2.0M?
Hi Kai-Zhen, Could you please try land this on 2.0M? Thanks!
blocking-b2g: 2.0M? → 2.0M+
Flags: needinfo?(kli)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: