811624 - [Fugu][Buri][SETTINGS] "Lock Screen" should not be able to turn off if "Passcode Lock" is on

Status: RESOLVED FIXED

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

Description

[Walter Chen[:ypwalter][:wachen]]

*Environment:
Phone - Unagi
gaia master(git): 1549eb82367e0da38198cb3d458def15d829527a
gecko aurora(hg): 114166:c07b17e59971

*How to reproduce:
1. Go to "Settings" > "Phone Lock"
2. Enable "Lock Screen"
3. Enable "Passcode Lock"
4. Disable "Lock Screen"

*Expected Result:
   It should ask you to enter your passcode just like when you are trying to disable "Lock Screen"

*Actual Result:
   It disable the "Lock Screen" immediately

Comment 1

[Dylan Oliver [:doliver]]

blocking- 

We acknowledge that is a security hole but the exposure is small -- relies on someone grabbing your phone while it's unlocked, at which point they already have access to your data anyway.

Comment 2

[Walter Chen[:ypwalter][:wachen]]

Still sounds kind of strange though.
We designed if you turn off the passcode lock, you need to enter your passcode.
But, you don't need to enter you passcode when you turn off the whole lock?

Comment 3

[Walter Chen[:ypwalter][:wachen]]

This still happens.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

Traige: tef-

Comment 5

[Walter Chen[:ypwalter][:wachen]]

this reproduced in v101 and v110

Comment 6

[Neeraj]

Attachment: Proposed patch

Hello Arthur,

I have created a pull request(https://github.com/mozilla-b2g/gaia/pull/10987) for this bug. Please review it :) 

Regards,

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 775814 [details] [diff] [review]
Proposed patch

Neeraj, thank you for the effort. The patch works perfectly but I would like it to be implemented consistent to the original logic. Detail please refer to my comment in github. I would like to see you review request again once you complete it, thanks!

Comment 8

[Neeraj]

Comment on attachment 775814 [details] [diff] [review]
Proposed patch

Hello Arthur,

Sorry for the delayed response :)

I have updated PR. Please review it again and suggest the changes if required.

Regards,
Neeraj

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 775814 [details] [diff] [review]
Proposed patch

Neeraj, thanks for revising the patch. There is one thing needs to be improved. Instead of reverting the mozSetting value, we can simply use the preventDefault function to prevent the checkbox and mozSettings value from being changed. And there are also some minor things I would like you to address.

We are almost there! I would like to review the updated patch once you finished. :)

Comment 10

[Neeraj]

Comment on attachment 775814 [details] [diff] [review]
Proposed patch

Hello Arthur,

I have incorporated all the changes you have suggested. Please review it and let me know the changes required :)

Again sorry for the delayed response.

Regards,

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 775814 [details] [diff] [review]
Proposed patch

Thank you for the effort! It works really well. r=me with one more nit I would like you to address, please refer to github. And please remember to squash all of your commits into one before merging.

Comment 12

[Neeraj]

(In reply to Arthur Chen [:arthurcc] from comment #11)
> Comment on attachment 775814 [details] [diff] [review]
> Proposed patch
> 
> Thank you for the effort! It works really well. r=me with one more nit I
> would like you to address, please refer to github. And please remember to
> squash all of your commits into one before merging.

Hello Arthur,

Thanks a lot for your review!!! Your suggestions have been great help to me :)

I have addressed nit you mentioned and squashed all commits into one. But now I am seeing that  travis is failing for some reason,  perhaps this is not b/c of my change. 

I also think I cant merge directly. Please look if anything can be done :)

Regards,
Neeraj

Comment 13

[Neeraj]

(In reply to Arthur Chen [:arthurcc] from comment #11)
> Comment on attachment 775814 [details] [diff] [review]
> Proposed patch
> 
> Thank you for the effort! It works really well. r=me with one more nit I
> would like you to address, please refer to github. And please remember to
> squash all of your commits into one before merging.

Hello Arthur,

Thanks a lot for your review!!! Your suggestions have been great help to me :)

I have addressed nit you mentioned and squashed all commits into one. But now I am seeing that  travis is failing for some reason,  perhaps this is not b/c of my change. 

I also think I cant merge directly. Please look if anything can be done :)

Regards,
Neeraj

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Neeraj,

Sorry I totally missed the comment. It's buried in other mails. Please set a ni? flag if you need my help. :)
In the case that you think the travis failure is not related to your change, you can try to run the tests again. Login travis with your github account, and you will see a re-run button at the top-right corner.

Please rebase to the current master and re-run the tests. And ni? me for merging. Thanks a lot.

Comment 15

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

Reproduced on 
Firefox OS V1.2 Test build: 20131110004003
gecko: 8926376583fdd3711aaf75594fd4084306c5f02a
gaia: 3cc5e6ddec0656b3a6a197e989dabebee536c982

Comment 16

[Neeraj]

(In reply to Arthur Chen [:arthurcc] from comment #14)
> Hi Neeraj,
> 
> Sorry I totally missed the comment. It's buried in other mails. Please set a
> ni? flag if you need my help. :)
> In the case that you think the travis failure is not related to your change,
> you can try to run the tests again. Login travis with your github account,
> and you will see a re-run button at the top-right corner.
> 
> Please rebase to the current master and re-run the tests. And ni? me for
> merging. Thanks a lot.

Hello Arthur,

Thanks for your reply :)

I have rebased to current master and can see that travis is still failing. At the same time , I can't relate travis failure to my changes. Please help me by picking the commit and merge that in master if possible.

Once again thanks a lot :)

Regards,
Neeraj

Comment 17

[Bruce Huang [:bhuang] (inactive)]

Setting this to koi? as it has the potential to be a big deal.

Comment 18

[Jason Smith [:jsmith]]

(In reply to Bruce Huang (:bhuang) from comment #17)
> Setting this to koi? as it has the potential to be a big deal.

Why? What makes this a big deal if we've already shipped with this bug in past releases?

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Neeraj,

It seems that you pulled master directly to your branch. The pull request now contains changes more than your original patch. The steps to rebase to master are 1. Checkout and pull the latest master. 2. Checkout to your branch. 3. Rebase to master ($git rebase master) 4. If there are conflicts, resolve them and commit.
Feel free to let me know if you encounter problems.

Thanks!

Comment 20

[Bruce Huang [:bhuang] (inactive)]

(In reply to Jason Smith [:jsmith] from comment #18)
> (In reply to Bruce Huang (:bhuang) from comment #17)
> > Setting this to koi? as it has the potential to be a big deal.
> 
> Why? What makes this a big deal if we've already shipped with this bug in
> past releases?

There are some additional comments in bug 921013.  Safety and security bugs can get blown out of proportion even if they are minor.  It definitely should block 1.3 if we can manage it.

Comment 21

[Jason Smith [:jsmith]]

(In reply to Bruce Huang (:bhuang) from comment #20)
> (In reply to Jason Smith [:jsmith] from comment #18)
> > (In reply to Bruce Huang (:bhuang) from comment #17)
> > > Setting this to koi? as it has the potential to be a big deal.
> > 
> > Why? What makes this a big deal if we've already shipped with this bug in
> > past releases?
> 
> There are some additional comments in bug 921013.  Safety and security bugs
> can get blown out of proportion even if they are minor.  It definitely
> should block 1.3 if we can manage it.

Well in that case, shouldn't we evaluate this for 1.3 then instead of 1.2? We're kinda really late in the game right now for 1.2.

Comment 22

[Neeraj]

Attachment: New Proposed Patch

Thanks Authur for replying and correcting me :)

I can see that latest code has changed a lot. So in-lined to new code I have created new patch which is basically same as previous one. Please review it and let me know in case of any change is required.

Regards,

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 832386 [details] [diff] [review]
New Proposed Patch

Works well, thank you Neeraj!

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

master: 28db25f12838468e9e11afd5609e7a6513b74fc2

Comment 25

[Neeraj]

Thanks a lot Arthur for reviewing and landing to master :)

I was just curious to know that if we could land this to v1.2 branch?

Regards,
Neeraj

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

This will be uplifted to v1.2 branch if we have approval or this issue becomes koi+.

Comment 27

[Jason Smith [:jsmith]]

We have no plans to take this onto 1.2 per the above discussion - moving back to 1.3?

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

--- since the patch has already landed in 1.3.