Salvador de la Puente González [:salva] Reporter
	Description • 10 years ago

In order to fulfill bug 987155 we need SMS app to be quiet when Cost Control is sending its specific SMS.

	Marina Rodríguez [:mai] Assignee
	Comment 1 • 10 years ago

Hi,
do you mind reviewing the patch?
Regards

	Julien Wajsberg [:julienw]
	Comment 2 • 10 years ago

I'd like some more information about what the patch is actually doing, what is the used algorithm and the rationale.

Thanks!

	Marina Rodríguez [:mai] Assignee
	Comment 3 • 10 years ago

Hi Julien, 

This patch tries to improve the current SMS app blacklist feature. That feature avoids the vibration, sound and notification for the sent/received SMS when the sender is part of that list which is generated from "blacklist.json" file, at build time (so it can't be modified/activated/deactivated)

This feature is currently used by Cost Control app for managing balance and Top-up requests in one country. We would like to use it in a global way, all the countries. The problem is that there is no dedicated/exclusive numbers for balance/Top-up services for each country, it means, the same number can be used for another services, not just balance/Top-up in a specific country, moreover, the same number in one country could be used by another one for other services (eg. promotions). For those reasons, we'd need a dynamic list that can be activated/deactivated as needed during a period of time.

This patch has been implemented taking into account the following workflow:

  1. CC app will ask SMS app to silence a list of senders during a period of time via IAC communication
  2. When SMS app confirms via IAC that the senders have been silent , CC app will send the corresponding balance/Top-up message
  3. If an answer is received during the silent period:
       3.1 The SMS app won't send any type of notification or sound.
       3.2 In case CC app detects the answer before finishing the silent period it will immediately ask SMS app to disable the silent mode for that list
  4. If no answer is received during this period: The SMS app automatically deactivates the silence mode for those numbers (To implement this, we set a timeout alarm on the SMS app)


The IAC communication can only be established between certified apps, but if you are concerned about the use of this feature by another app, it's possible to check the origin of the  connection to guarantee which app uses it.

I don't know if I miss something, I hope this clarify your questions.
Regards

	Marina Rodríguez [:mai] Assignee
	Comment 4 • 10 years ago

I've updated the PR with the Julien comments.

	Salvador de la Puente González [:salva] Reporter
	Comment 5 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

I like it. Address the comments on GitHub but wait for Borja's review.

	Marina Rodríguez [:mai] Assignee
	Comment 6 • 10 years ago

Updated the PR with the comments of Salva.

	Julien Wajsberg [:julienw]
	Comment 7 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

I want to review this once more before this goes in.
Thanks for the explanation and sorry for the delay

	Julien Wajsberg [:julienw]
	Comment 8 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

Yuren, can you please have a quick look to the changes in the build files? Should be easy but I prefer that you have a look.

	Julien Wajsberg [:julienw]
	Comment 9 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

I finally reviewed this, sorry for the delay.

I really think SilentSMS should use Promises for the various asynchronous steps, that will lead to clearer and cleaner code.

Also, maybe we should have a safety net: keeping the blacklist.json file to have a list of numbers that we can actually blacklist (even temporarily). I don't feel comfortable that any number could be made silent...

I admit I don't have the big picture though, so if you think it should stay like this, I'd like to ask the security peers.

	Julien Wajsberg [:julienw]
	Comment 10 • 10 years ago

Also, please ask review from :steveck if you need a review before the end of this week as I'm in holidays (in your country ;) ) for some days.

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)
	Comment 11 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

r+++++++

	Marina Rodríguez [:mai] Assignee
	Comment 12 • 10 years ago

updated The pr with the last comments of Salva

	Marina Rodríguez [:mai] Assignee
	Comment 13 • 10 years ago

Updated the PR with the comments of Borja

	Julien Wajsberg [:julienw]
	Comment 14 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

(adding r- just because there is a r+ already)

added more comments.

It just occurred to me that we can have issues with simultaneous requests :( I'm afraid we need to do a task runner...

	Julien Wajsberg [:julienw]
	Comment 15 • 10 years ago

NI Stéphanie to check the function from the security point of view.

Especially, I'd like to know if we should keep a list of phone numbers that can be silenced, or if the current code and architecture is safe enough already.

	Julien Wajsberg [:julienw]
	Comment 16 • 10 years ago

> if we should keep a list of phone numbers that can be silenced

Maybe, we can already have a mapping of "type -> numbers" in the sms app, and the IAC would only enable/disable the type.

	Marcelino Veiga Tuimil [:sonmarce]
	Comment 17 • 10 years ago

Those numbers are already included in cost control configuration files, that is more accurate as they are defined per country (MCC/MNC) for the final purpose (check ballance & top up), and it would be the only app able to call SMS app

	Marina Rodríguez [:mai] Assignee
	Comment 18 • 10 years ago

Hi Stéphanie, 
The patch implement the following security rules:
Only the costcontrol app could make a request to silent sms numbers (a rule form the manifiest field)
The costcontrol app generates the list of sms numbers from its configuration files (filtering by mcc/mnc).
The IAC communication is allowed only for certified apps.
Regards

	Stephanie Ouillon [:arroway] (needinfo me)
	Comment 19 • 10 years ago

My comment collided with Marina's and Marcelino's at the same time, anyway here it is, it's globally saying the same thing:

From a security perspective, I see two main potential threats: 

1) Somebody can control the list of senders to be silenced in the cost control app. 

The lists of senders to be silenced are hardcoded in costcontrol/js/config/* in the cost control certified app, so from that point of view it looks safe.

2) Another app can communicate via IAC with the sms app and send it a non-legitimate list to be silenced.

The manifest settings related to the IAC API allow to specify the sms app is to talk to the cost control app only. Thus it shouldn't be tricked to received a list of senders from another source. The code have already addressed the issue of checking that it is dealing with the expected request.

So from a high level point of view, I don't see any particular security issue as far as I understand.

	Borja Salguero [:borjasalguero]
	Comment 20 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

Im gonna leave the review to Julien here, due to he started the review before and there are some comments to address.

My main concern is how the tests are created, it's quite tough to read and probably we need a better level of granularity in order to check small things, instead of end-to-end ones.

	Julien Wajsberg [:julienw]
	Comment 21 • 10 years ago

(In reply to Borja Salguero (this week part-time in Gaia) [:borjasalguero] from comment #20)

> My main concern is how the tests are created, it's quite tough to read and
> probably we need a better level of granularity in order to check small
> things, instead of end-to-end ones.

yes, I agree with this, but I think we'll revisit this later. It's already good to have tests :)

	Julien Wajsberg [:julienw]
	Comment 22 • 10 years ago

I worked a lot with Marina today, and the patch is going really well. I've seen issues in the costcontrol patch that I reported in Bug 1007588 comment 4.

	Julien Wajsberg [:julienw]
	Comment 23 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

added some more comments

I think the next round will be the good one !

Thanks for this good work !

	Julien Wajsberg [:julienw]
	Comment 24 • 10 years ago

Comment on attachment 8426286 [details] [review]
patch v1.0

r=me

please land with a green travis

Thanks !

	Marina Rodríguez [:mai] Assignee
	Comment 25 • 10 years ago

Master: 857129928b6e56a809cee9d5445effb8fa9f1c2c