1061055 - B2G NFC: move CheckPermissions into methods of NfcManager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=921453#c6

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 2

[Kamil Leszczuk [:kamituel]]

Sure, I will prepare the patch now.

Comment 3

[Kamil Leszczuk [:kamituel]]

Attachment: 0001-Bug-1061055-move-CheckPermissions-into-methods-of-Nf.patch

I've tested this on Flame, and it works as expected. Can you make a try run for me?

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 5

[Kamil Leszczuk [:kamituel]]

The CheckPermission and AvailableIn were removed two days ago in commit 4a56d8b3d613.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Run try with mochitest and ask r? from a DOM peer.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

[Blocking Requested - why for this release]:
Regression from Bug 952486

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: per comment 7 this is a regression

Comment 9

[Kamil Leszczuk [:kamituel]]

Yoshi, could you merge it for me? Thanks in advance!

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

From Sheriff's new policy, a Try-result needs to be run.

Comment 11

[Vincent Chang[:vchang][changyihsin]]

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/

Comment 12

[Kamil Leszczuk [: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?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1fe3d5f00254

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1fe3d5f00254

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 16

[Kamil Leszczuk [: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]:

Comment 17

[bhavana bajaj [:bajaj]]

(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!

Comment 18

[Kamil Leszczuk [: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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/294e6eb3cd56