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)
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+ |
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
Reporter | ||
Updated•10 years ago
|
Whiteboard: [sprd346576]
Reporter | ||
Comment 1•10 years ago
|
||
Please ignore above localtion.
Video and slog location: https://mega.co.nz/#F!wwMWVRQC!ReeFV6xgDCftIGeVBZxpqg
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
(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•10 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•10 years ago
|
||
Updated patch, I've updated the PR too.
Attachment #8489876 -
Attachment is obsolete: true
Attachment #8490103 -
Flags: review?(josea.olivera)
Comment 8•10 years ago
|
||
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+
Comment 9•10 years ago
|
||
(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•10 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
Closed: 10 years ago
Resolution: --- → FIXED
Comment 11•10 years ago
|
||
Hi Gabriele:
Can you help me uplift this change to v1.4?
Thank you
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 12•10 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)
Comment 13•10 years ago
|
||
(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•10 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•10 years ago
|
blocking-b2g: --- → 2.0M?
Comment 15•10 years ago
|
||
Hi Kai-Zhen,
Could you please try land this on 2.0M? Thanks!
blocking-b2g: 2.0M? → 2.0M+
Flags: needinfo?(kli)
Comment 16•10 years ago
|
||
Flags: needinfo?(kli)
You need to log in
before you can comment on or make changes to this bug.
Description
•