1079941 - Don't allow calls from my blocked contacts

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Paul Silaghi, QA [:pauly]]

I shouldn't be allowed to call a person who has me in its contacts list as 'blocked'

Comment 1

[Maire Reavy [:mreavy]]

Mike -- Can you look into this?

Comment 2

[Marco Mucci [:MarcoM]]

Added to IT 35.3

Comment 3

[Mike de Boer [:mikedeboer]]

Mark, with direct calls, we identify callers by email address, right?
So, with an incoming direct call, when do we get more info about who's calling?

I *think* we're not getting that info atm. I would really like to have at least the email address be part of `callData` in `MozLoopService#_startCall()` for type 'incoming'.

Comment 4

[Mike de Boer [:mikedeboer]]

Unassigning for now, due to too many dependencies.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1

Adam, what do you think?

Comment 6

[Adam Roach [:abr]]

Comment on attachment 8504804 [details] [diff] [review]
Patch v1

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

Headed in the right direction. Please make sure we have good test coverage here, especially with regard to phone numbers.

::: browser/components/loop/LoopContacts.jsm
@@ +818,5 @@
> +    }
> +
> +    const checkForMatch = function(fieldValue) {
> +      if (typeof fieldValue == "string") {
> +        return fieldValue.startsWith(query.q);

For phone numbers, this needs to be "ends with" rather than "starts with". We also want to make sure we trim out anything that's not /[0-9\+]/ for phone numbers. Which is to say that "tel" might need special handling here. It's inelegant, but I don't think there's an easy way around it.

For non-phone-numbers, I think we want an exact match. If you want anything fancier than that, I suggest allowing q: to be a regex.

@@ +841,5 @@
> +        // Many fields are defined as Arrays.
> +        if (Array.isArray(matchWith)) {
> +          for (let fieldValue of matchWith) {
> +            if (checkForMatch(fieldValue)) {
> +              foundContacts.push(contact);

break;

(We don't want a contact to appear twice in the results on the off chance that two entries in the array match)

::: browser/components/loop/MozLoopService.jsm
@@ +801,5 @@
> +        null,
> +        // No title, let the page set that, to avoid flickering.
> +        "",
> +        "about:loopconversation#" + conversationType + "/" + callData.callId);
> +    }

I think you're missing a semicolon here?

@@ +806,5 @@
> +
> +    if (conversationType == "incoming" && ("callerId" in callData)) {
> +      LoopContacts.search({
> +        q: callData.callerId,
> +        field: callData.callerId.contains("@") ? "email" : "tel"

This should be a bit more sophisticated. We can get three formats here:

  ^\w+@\w+$ is an email address

  ^\+\d+$ is a phone number

  Anything else is a display name (from a link clicker) that should not be looked up in the database.

::: browser/components/loop/test/mochitest/browser_LoopContacts.js
@@ +424,5 @@
>  
>    LoopStorage.switchDatabase();
>    Assert.equal(LoopStorage.databaseName, "default", "The active partition should have changed");
>  
> +  let deferred = Promise.defer();

Promise.defer() is deprecated; see https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Deferred

@@ +469,5 @@
> +        deferred.resolve();
> +      });
> +    });
> +  });
> +  yield deferred.promise;

Please add a test searching for phone number, where, e.g., the query string is "5551234" and the entry to match is "[phone]".

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2

All comments addressed, I believe...

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.1

Made the regex look a bit more the way you like it.

Comment 9

[Adam Roach [:abr]]

Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

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

I have a number of suggestions for improvements that you may take or leave; however, you must check for error at in your call to getAll before landing.

r+ conditional on the error-check fix.

::: browser/components/loop/LoopContacts.jsm
@@ +819,5 @@
> +    }
> +    if (!("field" in query)) {
> +      query.field = "email";
> +    }
> +

For efficiency, I'd move the query.q.replace() for telephone numbers up here, so you're not repeating it each time you check a contact.

@@ +828,5 @@
> +        }
> +        return fieldValue == query.q;
> +      }
> +      if (typeof fieldValue == "number" || typeof fieldValue == "boolean") {
> +        return fieldValue === query.q;

I would think we want coercion of the query.q to a boolean if the field is boolean. Perhaps "==" is more appropriate here?

@@ +837,5 @@
> +      return false;
> +    };
> +
> +    let foundContacts = [];
> +    this.getAll((err, contacts) => {

Check for err here.

::: browser/components/loop/test/mochitest/browser_LoopContacts.js
@@ +451,5 @@
> +add_task(function* () {
> +  yield promiseLoadContacts();
> +
> +  yield new Promise(resolve => {
> +    LoopContacts.search({

This sequence would be much cleaner, shorter, and easier to read if you used LoopContacts.promise("search",...) rather than repeated callbacks.

Comment 10

[Maire Reavy [:mreavy]]

Hi Mike -- Can you update the patch and land it today?  I'm hoping to get this into tonight's Nightly so QA can verify it tomorrow (against the dev server).

Comment 11

[Mike de Boer [:mikedeboer]]

Pushed with all comments addressed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/b0843f9cb541

Thanks for those, Adam!

Comment 12

[Maire Reavy [:mreavy]]

Pauly -- Can you verify this in tomorrow's Nightly if it gets merged in time?  You will need to use the Loop dev-server like you did for caller ID (bug 1020449).

Comment 13

[Carsten Book [:Tomcat]]

sorry guys this was backed out in https://hg.mozilla.org/mozilla-central/rev/7fac025c8cdb for  mochitest-bc test failures

Comment 14

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:

We need to handling blocking calls from blocked contacts.  It's a privacy requirement.  (This also requires a Loop server change which is currently being deployed to production.)

Comment 15

[Mike de Boer [:mikedeboer]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fe9325c1befa

This got backed out yesterday. Need to reland.

Comment 16

[Mike de Boer [:mikedeboer]]

Pushed to fx-team - again - as https://hg.mozilla.org/integration/fx-team/rev/e8a01f5feb55

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8a01f5feb55

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

Approval Request Comment
[Feature/regressing bug #]: Loop
[User impact if declined]: When a user blocks a contact, currently that contact is still able to call that user, because the logic to block incoming calls from blocked contacts was simply not implemented. This patch fixes that.
[Describe test coverage new/current, TBPL]: Landed on m-c, the changes in the LoopContacts class has plenty unit test coverage.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Tested on Windows and Linux nightly 10/18 (using dev server, see bug 1020449), works

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

Thanks for testing jesup. Approved for Aurora. If everything goes well with your testing on Aurora on Sunday, we'll get this uplifted for beta2.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81358b1794d1

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Tested and good on Aurora nightly build; windows and linux.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/bda95894a692

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

Previously approved offline. Adding approval to the bug.

Comment 25

[Paul Silaghi, QA [:pauly]]

(In reply to Randell Jesup [:jesup] from comment #22)
> Tested and good on Aurora nightly build; windows and linux.
Verified fixed 36.0a1 (2014-10-20) Win 7

Comment 26

[u279076]

Paul, can you please test this with the next beta?

Comment 27

[Maire Reavy [:mreavy]]

Just a reminder: we need server version 0.12.5 to verify that this works.  Services is deploying server version 0.12.5 to production today and tomorrow.  If it's not available when you do this verification, you would need to use the dev server (as described in bug 1020449:https://bugzilla.mozilla.org/show_bug.cgi?id=1020449#c32).   Basically, we need caller ID from the server in order to test call blocking.

Comment 28

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b2 OS X 10.9.5

Comment 29

[Mark Banner (:standard8)]

Note to testers (on production): for a callee to receive the caller id, the caller must have last signed into FxA after the deployment time of 0.12.5 (Mon 27 Oct).