Closed Bug 1061055 Opened 5 years ago Closed 5 years ago

B2G NFC: move CheckPermissions into methods of NfcManager

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S5 (26sep)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: allstars.chh, Assigned: kamituel)

References

Details

Attachments

(1 file)

Since the patch is Bug 1054139 is already r+ so I move the problem into this new bug.

Kamil, do you have time to fix this *TODAY* ? Since it will be branched soon and hope that we could make it in-time for landing in v2.1 branch.

Otherwise I will take it.

Thanks
Flags: needinfo?(kamituel)
Sure, I will prepare the patch now.
Flags: needinfo?(kamituel)
Assignee: nobody → kamituel
I've tested this on Flame, and it works as expected. Can you make a try run for me?
Attachment #8482259 - Flags: review?(allstars.chh)
Comment on attachment 8482259 [details] [diff] [review]
0001-Bug-1061055-move-CheckPermissions-into-methods-of-Nf.patch

Review of attachment 8482259 [details] [diff] [review]:
-----------------------------------------------------------------

The CheckPermission and perhaps AvailableIn should be removed
Attachment #8482259 - Flags: review?(allstars.chh) → review+
The CheckPermission and AvailableIn were removed two days ago in commit 4a56d8b3d613.
Keywords: checkin-needed
Run try with mochitest and ask r? from a DOM peer.
Keywords: checkin-needed
[Blocking Requested - why for this release]:
Regression from Bug 952486
blocking-b2g: --- → 2.1?
Attachment #8482259 - Flags: review+ → review?(bugs)
triage: per comment 7 this is a regression
blocking-b2g: 2.1? → 2.1+
Attachment #8482259 - Flags: review?(bugs) → review+
Yoshi, could you merge it for me? Thanks in advance!
Flags: needinfo?(allstars.chh)
From Sheriff's new policy, a Try-result needs to be run.
Flags: needinfo?(allstars.chh)
Hi Kamil, can you help to provide the try-server[1][2] result so that we can commit the patch ? 

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer
[2] http://trychooser.pub.build.mozilla.org/
Flags: needinfo?(kamituel)
Yes, I actually ran the try run yesterday, and I think I have the green build: https://tbpl.mozilla.org/?tree=Try&rev=366722cc6e07 (first time doing that, so forgive me if I got something wrong).

Can we check-in it now?
Flags: needinfo?(kamituel)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1fe3d5f00254
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(kamituel)
Comment on attachment 8482259 [details] [diff] [review]
0001-Bug-1061055-move-CheckPermissions-into-methods-of-Nf.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TBPL]: TBPL tests passing
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8482259 - Flags: approval-mozilla-aurora?
Flags: needinfo?(kamituel)
(In reply to Kamil Leszczuk [:kamituel] from comment #16)
> Comment on attachment 8482259 [details] [diff] [review]
> 0001-Bug-1061055-move-CheckPermissions-into-methods-of-Nf.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]:
> [User impact if declined]:
> [Describe test coverage new/current, TBPL]: TBPL tests passing
> [Risks and why]: 
> [String/UUID change made/needed]:

:kamil can you please fill in the approval request form for uplift with more information ? Please check the guildelines documented here that can be helpful : https://wiki.mozilla.org/Release_Management/Uplift_rules#Guidelines_on_approval_comments to answer all the questions that are being asked. thanks!
Flags: needinfo?(kamituel)
Sure :bajaj, I can try (hopefully I know all the answers ;)):

Approval Request Comment

[Feature/regressing bug #]: n/a

[User impact if declined]: methods of NfcManager would be exposed to the apps even if those apps have no permission to access them. Those apps would probably still not be able to actually use those methods, because additional security checks are done in their implementations, but it's still undesired to expose them at all.

[Describe test coverage new/current, TBPL]: TBPL tests passing. In addition to that, change tested using test app to see whether methods are no longer exposed.

[Risks and why]: I don't see any major risks - so risk is low.

[String/UUID change made/needed]: none
Flags: needinfo?(kamituel)
Attachment #8482259 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.