Don't allow calls from my blocked contacts

VERIFIED FIXED in Firefox 34

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: pauly, Assigned: mikedeboer)

Tracking

unspecified
mozilla36
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox33 unaffected, firefox34+ verified, firefox35+ verified, firefox36+ verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
I shouldn't be allowed to call a person who has me in its contacts list as 'blocked'
Mike -- Can you look into this?
Flags: needinfo?(mdeboer)
Assignee

Updated

5 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 1
Flags: qe-verify+
Flags: needinfo?(mdeboer)
Flags: firefox-backlog+
Assignee

Updated

5 years ago
Flags: needinfo?(mmucci)
Added to IT 35.3
Flags: needinfo?(mmucci)
Assignee

Comment 3

5 years ago
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'.
Flags: needinfo?(standard8)
Assignee

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee

Updated

5 years ago
Depends on: 1079950
Assignee

Updated

5 years ago
Flags: needinfo?(standard8)
Assignee

Updated

5 years ago
Depends on: 1082533
Assignee

Comment 4

5 years ago
Unassigning for now, due to too many dependencies.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Assignee

Updated

5 years ago
Iteration: 35.3 → ---
Assignee

Updated

5 years ago
No longer depends on: 1082533
Assignee

Updated

5 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Iteration: --- → 35.3
Assignee

Comment 5

5 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Adam, what do you think?
Attachment #8504804 - Flags: feedback?(adam)

Comment 6

5 years ago
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 "+12145551234".
Attachment #8504804 - Flags: feedback?(adam) → feedback+
Iteration: 35.3 → 36.1
Assignee

Comment 7

5 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
All comments addressed, I believe...
Attachment #8504804 - Attachment is obsolete: true
Attachment #8505430 - Flags: review?(adam)
Assignee

Comment 8

5 years ago
Posted patch Patch v2.1Splinter Review
Made the regex look a bit more the way you like it.
Attachment #8505430 - Attachment is obsolete: true
Attachment #8505430 - Flags: review?(adam)
Attachment #8505450 - Flags: review?(adam)
backlog: --- → Fx34+

Comment 9

5 years ago
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.
Attachment #8505450 - Flags: review?(adam) → review+
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).
Flags: needinfo?(mdeboer)
Assignee

Comment 11

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

Thanks for those, Adam!
Assignee

Updated

5 years ago
Flags: needinfo?(mdeboer)
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).
Flags: needinfo?(paul.silaghi)
sorry guys this was backed out in https://hg.mozilla.org/mozilla-central/rev/7fac025c8cdb for  mochitest-bc test failures
[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.)
Assignee

Comment 15

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

This got backed out yesterday. Need to reland.
https://hg.mozilla.org/mozilla-central/rev/e8a01f5feb55
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee

Comment 18

5 years ago
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.
Attachment #8505450 - Flags: approval-mozilla-beta?
Attachment #8505450 - Flags: approval-mozilla-aurora?
Tested on Windows and Linux nightly 10/18 (using dev server, see bug 1020449), works
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.
Attachment #8505450 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested and good on Aurora nightly build; windows and linux.
Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

Previously approved offline. Adding approval to the bug.
Attachment #8505450 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 25

5 years ago
(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
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Paul, can you please test this with the next beta?
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
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.
Reporter

Comment 28

5 years ago
Verified fixed FF 34b2 OS X 10.9.5
Flags: needinfo?(paul.silaghi)
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).
You need to log in before you can comment on or make changes to this bug.