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

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ypwalter, Assigned: Neeraj)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 1 obsolete attachment)

46 bytes, patch
arthurcc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
*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

Updated

6 years ago
Component: General → Gaia::System
(Reporter)

Updated

6 years ago
blocking-basecamp: --- → ?
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.
blocking-basecamp: ? → -
(Reporter)

Comment 2

6 years ago
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?

Updated

6 years ago
Component: Gaia::System → Gaia::Settings
(Reporter)

Comment 3

5 years ago
This still happens.
blocking-b2g: --- → tef?
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Traige: tef-
blocking-b2g: tef? → -
(Reporter)

Comment 5

5 years ago
this reproduced in v101 and v110
(Assignee)

Comment 6

5 years ago
Created attachment 775814 [details] [diff] [review]
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,
Attachment #775814 - Flags: review?(achen)
Attachment #775814 - Flags: review?(achen) → review?(arthur.chen)
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!
Attachment #775814 - Flags: review?(arthur.chen)
(Assignee)

Comment 8

5 years ago
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
Attachment #775814 - Flags: review?(arthur.chen)
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. :)
Attachment #775814 - Flags: review?(arthur.chen)
(Assignee)

Comment 10

5 years ago
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,
Attachment #775814 - Flags: review?(arthur.chen)
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.
Attachment #775814 - Flags: review?(arthur.chen) → review+
(Assignee)

Comment 12

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

Comment 13

5 years ago
(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
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.
Flags: needinfo?(neeraj_kumar)
Reproduced on 
Firefox OS V1.2 Test build: 20131110004003
gecko: 8926376583fdd3711aaf75594fd4084306c5f02a
gaia: 3cc5e6ddec0656b3a6a197e989dabebee536c982
Summary: [SETTINGS] "Lock Screen" should not be able to turn off if "Passcode Lock" is on → [Fugu][Buri][SETTINGS] "Lock Screen" should not be able to turn off if "Passcode Lock" is on
(Assignee)

Comment 16

5 years ago
(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
Flags: needinfo?(neeraj_kumar) → needinfo?(arthur.chen)
Setting this to koi? as it has the potential to be a big deal.
blocking-b2g: - → koi?
(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?
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!
Flags: needinfo?(arthur.chen) → needinfo?(neeraj_kumar)
(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.
(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.

Updated

5 years ago
blocking-b2g: koi? → 1.3?
(Assignee)

Comment 22

5 years ago
Created attachment 832386 [details] [diff] [review]
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,
Assignee: nobody → neeraj_kumar
Attachment #775814 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #832386 - Flags: review?(arthur.chen)
Flags: needinfo?(arthur.chen)
Comment on attachment 832386 [details] [diff] [review]
New Proposed Patch

Works well, thank you Neeraj!
Attachment #832386 - Flags: review?(arthur.chen) → review+
master: 28db25f12838468e9e11afd5609e7a6513b74fc2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(arthur.chen)
Resolution: --- → FIXED
(Assignee)

Comment 25

5 years ago
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
Flags: needinfo?(arthur.chen)
This will be uplifted to v1.2 branch if we have approval or this issue becomes koi+.
Flags: needinfo?(arthur.chen)
(Assignee)

Updated

5 years ago
Flags: needinfo?(neeraj_kumar)
blocking-b2g: 1.3? → koi?
We have no plans to take this onto 1.2 per the above discussion - moving back to 1.3?
blocking-b2g: koi? → 1.3?
--- since the patch has already landed in 1.3.
blocking-b2g: 1.3? → ---
You need to log in before you can comment on or make changes to this bug.